nanogui: Thread: Fonts offset leads to wrong memory location -- genfont.c bug??

Hello everyone,
 
I'm running microwindows-0.91 in an Altera Nios II processor (which happens
to be a soft-core 32-bit processor implemented inside an FPGA). I was able
to compile and run it in my prototype system which has a PS2 mouse and a VGA
monitor.
 
When I tried to replace one of the built-in fonts, weird things started to
show up on the screen. Obviously, one of the routines was reading the font
bitmaps from the wrong memory location.
 
I realized that the C language files created with the convfnt32.exe tool
generated a font structure like:

MWCFONT font_winArial8x22 = {
	"winArial8x22", 19, 22, 18, 30, 226,
	winArial8x22_bits,
	winArial8x22_offset,
	winArial8x22_width,
};

while the files distributed with microwindows had a structure like:

MWCFONT font_winFreeSansSerif11x13 = {
	"winFreeSansSerif11x13", 11, 13, 11, 32, 224,
	winFreeSansSerif11x13_bits,
	0 /*winFreeSansSerif11x13_offset*/,          //  <-- note this zero
here!
	winFreeSansSerif11x13_width,
};

My first attempt was replacing 'winArial8x22_offset' with the value 0. It
indeed solved the problem, but it only worked for monospaced (fixed width)
fonts.

Single-stepping through the code, I located the source of the problem (line
171 in genfont.c):

	/* get font bitmap depending on fixed pitch or not*/
	bits = pf->bits + (pf->offset? pf->offset[ch]: (pf->height * ch));

So, when offset is 0, the actual offset value is not uset to calculate the
bitmap address in memory, and everything works fine. However, any non-zero
value would cause the calculation to lead to the wrong memory location.

I was able to get things working by making the following change:

    //bits = pf->bits + (pf->offset? pf->offset[ch]: (pf->height * ch));
    bits = pf->bits + (pf->offset? ((unsigned short *)pf->offset)[ch] :
(pf->height * ch));

Now, I believe that the root of all problems was that the 'offset' variable
was declared as an 'unsigned long *', while 'bits' was an 'unsigned short
*'. Actually, 'bits' is declared as  a pointer to the MWIMAGEBITS type,
which in turn is defined as an unsigned short. I'm not sure it is a good
thing to have these two types mixed when doing pointer arithmetic.

My question is: does this qualify as a bug? Shouldn't it be fixed in
upcoming versions? I'm just getting started with microwindows, so I don't
feel confident enough for making changes in its core code, but this really
seems the right thing to do.

I would appreciate any comments on this matter. Cheers!!

Ricardo Jasinski.



 

: I realized that the C language files created with the convfnt32.exe tool
: generated a font structure like:

The convfnt32.exe is a very old tool.  Much better to try to
convert source based .bdf files which could then
capture all information, including the proportional
width information.  Convfnt32.exe works by reading
bits from the screen, not exactly a great way.  Of course,
it could be enhanced by getting font information internally
and adding the width array from that information.
I'll write up the missing member data as a bug.

:
: My first attempt was replacing 'winArial8x22_offset' with the value 0. It
: indeed solved the problem, but it only worked for monospaced (fixed width)
: fonts.

Of course: the missing data points to the width table, which isn't
present.


:
: Single-stepping through the code, I located the source of the problem
(line
: 171 in genfont.c):
:
: /* get font bitmap depending on fixed pitch or not*/
: bits = pf->bits + (pf->offset? pf->offset[ch]: (pf->height * ch));
:
: So, when offset is 0, the actual offset value is not uset to calculate the
: bitmap address in memory, and everything works fine. However, any non-zero
: value would cause the calculation to lead to the wrong memory location.

Yes, and if you're lucky, no core dump.


:
: I was able to get things working by making the following change:
:
:     //bits = pf->bits + (pf->offset? pf->offset[ch]: (pf->height * ch));
:     bits = pf->bits + (pf->offset? ((unsigned short *)pf->offset)[ch] :
: (pf->height * ch));
:
: Now, I believe that the root of all problems was that the 'offset'
variable
: was declared as an 'unsigned long *', while 'bits' was an 'unsigned short
: *'. Actually, 'bits' is declared as  a pointer to the MWIMAGEBITS type,
: which in turn is defined as an unsigned short. I'm not sure it is a good
: thing to have these two types mixed when doing pointer arithmetic.

No, this has nothing to do with pointer arithmetic, the
bits = pf->bits + ... means that we want to increment/index
the pf->bits pointer MWIMAGEBITS amount over.  In other
words, this is used to find exactly the start of an MWIMAGEBITS
which happens to be the start of a proportional character.  We
can't index it using a straight multiply because there isn't a
standard character width.

The pf->offset is declared as an 'unsigned long *' because that
allows font bitmap data longer than  64k.  This was earlier declared
to be 'unsigned short *' but was buggy for large fonts.



:
: My question is: does this qualify as a bug? Shouldn't it be fixed in
: upcoming versions?

Nope, see above.


I'm just getting started with microwindows, so I don't
: feel confident enough for making changes in its core code, but this really
: seems the right thing to do.

No problem, but you're just lucky that the program started working.
The font still displays incorrectly, doesn't it?

Regards,

Greg

Hi Greg,

thanks a lot for your response! Please see the replies to your comments
below.

> ... Convfnt32.exe works by reading bits from the 
> screen, not exactly a great way.  Of course, it could be enhanced by
getting 
> font information internally and adding the width array from that
information.
> I'll write up the missing member data as a bug.
>
> : My first attempt was replacing 'winArial8x22_offset' with the value 0.
It
> : indeed solved the problem, but it only worked for monospaced (fixed
width)
> : fonts.
>
> Of course: the missing data points to the width table, which isn't
present.

Actually, if you are talking about the static unsigned char 
winFreeSansSerif11x13_width[] = { ... }; table, it is generated perfectly
fine, 
for all fonts I have tried. Also, all width calculations are carried out 
correctly.

> : I was able to get things working by making the following change:
> :
> :     //bits = pf->bits + (pf->offset? pf->offset[ch]: (pf->height * ch));
> :     bits = pf->bits + (pf->offset? ((unsigned short *)pf->offset)[ch] :
> : (pf->height * ch));
> :
> : Now, I believe that the root of all problems was that the 'offset'
> variable
> : was declared as an 'unsigned long *', while 'bits' was an 'unsigned
short
> : *'. Actually, 'bits' is declared as  a pointer to the MWIMAGEBITS type,
> : which in turn is defined as an unsigned short. I'm not sure it is a good
> : thing to have these two types mixed when doing pointer arithmetic.
> 
> No, this has nothing to do with pointer arithmetic, the bits = pf->bits +
... 
> means that we want to increment/index the pf->bits pointer MWIMAGEBITS
amount 
> over.  In other words, this is used to find exactly the start of an
MWIMAGEBITS 
> which happens to be the start of a proportional character.  We can't index
it 
> using a straight multiply because there isn't a standard character width.
> 
> The pf->offset is declared as an 'unsigned long *' because that allows
font 
> bitmap data longer than  64k.  This was earlier declared to be 'unsigned
short 
> *' but was buggy for large fonts.

I'll illustrate the situation with actual values from a debug session.

So, these are the types we're dealing with:

  typedef unsigned short	MWIMAGEBITS;	/* bitmap image unit size*/
  typedef struct {
    MWIMAGEBITS *	bits;			/* 16-bit right-padded
bitmap data*/
    unsigned long *offset;		/* offsets into bitmap data*/
    unsigned char *	width;	/* character widths or 0 if fixed*/
    ...
  } MWCFONT, *PMWCFONT;

And these are the code lines that perform the offset calculation. 'bits' is 
the original calculation as in genfont.c, and 'fixedBits' is my modified
version 
of the calculation:

  bits = pf->bits + (pf->offset? pf->offset[ch]: (pf->height * ch));
  fixedBits = pf->bits + (pf->offset? ((unsigned short *)pf->offset)[ch] :
(pf->height * ch));

Right before these code lines, I added the following test code:

  unsigned long * p_longIndexed_char0_offset = &(pf->offset[0]);
  unsigned long * p_longIndexed_char1_offset = &(pf->offset[1]);
  unsigned long * p_longIndexed_char2_offset = &(pf->offset[2]);

  unsigned long * p_shortIndexed_char0_offset = &((unsigned short
*)pf->offset)[0];
  unsigned long * p_shortIndexed_char1_offset = &((unsigned short
*)pf->offset)[1];
  unsigned long * p_shortIndexed_char2_offset = &((unsigned short
*)pf->offset)[2];
    
  unsigned long longIndexed_char0_offset = pf->offset[0];
  unsigned long longIndexed_char1_offset = pf->offset[1];
  unsigned long longIndexed_char2_offset = pf->offset[2];

  unsigned long shortIndexed_char0_offset = ((unsigned short
*)pf->offset)[0];
  unsigned long shortIndexed_char1_offset = ((unsigned short
*)pf->offset)[1];
  unsigned long shortIndexed_char2_offset = ((unsigned short
*)pf->offset)[2];

And the results were:

  pf = 0x0099e980
    size = 224
      bits = 0x0099d020
        +-> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
            00 00 00 00 00 00 00 00 00 00 00 00 00 40 00 40
            00 40 00 40 00 40 00 40 00 40 00 00 00 00 00 40
            00 00 00 00 00 00 00 48 00 48 00 48 00 00 00 00 ...		  
      offset = 0x0099e6e0
        +-> 00 00 0d 00 1a 00 27 00 34 00 41 00 4e 00 5b 00 ...
      width = 0x0099e8a0
        +-> 03 03 05 07 06 08 06 02 03 03 04 06 03 03 03 05 ...
  p_longIndexed_char0_offset = 0x0099e6e0
  p_longIndexed_char1_offset = 0x0099e6e4
  p_longIndexed_char2_offset = 0x0099e6e8
  p_shortIndexed_char0_offset = 0x0099e6e0
  p_shortIndexed_char1_offset = 0x0099e6e2
  p_shortIndexed_char2_offset = 0x0099e6e4
  longIndexed_char0_offset = 	0xd0000 = 	851968
  longIndexed_char1_offset = 	0x27001a = 	2555930
  longIndexed_char2_offset = 	0x410034 = 	4259892
  shortIndexed_char0_offset = 0x0 = 	0
  shortIndexed_char1_offset = 0xd = 	13
  shortIndexed_char2_offset = 0x1a =	26

As can be seen, when pf->offset is treated as an unsigned long, it is
indexed at 4 bytes 
increments, placing the pointer at the wrong location. Also, the value
retrieved is 4 bytes
long, which results in an abnormally huge offset value.

Casting pf->offset to an unsigned short *, the pointer is positioned
corrrectly at 2 bytes
increments, and retrieved offset values are 16-bit, giving back the correct
values from the
font offsets table {0, 13, 26, etc.}.

As a test, I have recompiled the original microwindows code, simply changing
pf->offset type
from 'unsigned long *' to 'MWIMAGEBITS *'. Apparently, everything ran
smoothly. 

> The font still displays incorrectly, doesn't it?

No, it shows up perfectly for all fonts I've tried so far.

Final comments: perhaps this is a consequence of the change you have
mentioned in the 
previous message, since pf->offset used to be declared as 'unsigned short
*'.

If you have new thoughts on this subject I'll be glad to hear them, and also
to perform
any necessary tests.

Thanks for your time and attention,

Ricardo P. Jasinski.

: : As can be seen, when pf->offset is treated as an unsigned long, it is
: indexed at 4 bytes
: increments, placing the pointer at the wrong location. Also, the value
: retrieved is 4 bytes
: long, which results in an abnormally huge offset value.
:
: Casting pf->offset to an unsigned short *, the pointer is positioned
: corrrectly at 2 bytes
: increments, and retrieved offset values are 16-bit, giving back the
correct
: values from the
: font offsets table {0, 13, 26, etc.}.
:
: As a test, I have recompiled the original microwindows code, simply
changing
: pf->offset type
: from 'unsigned long *' to 'MWIMAGEBITS *'. Apparently, everything ran
: smoothly.

It's been so long since I wrote the windows font conversion
utility I forgot how it worked.  I think the bug is that
the font conversion utility spits out an incorrectly
declared offset array: its using unsigned shorts, and should
be declared unsigned long, since Microwindows was changed
to allow larger font offsets after the font conversion utility
was written.

Try changing the declaration of the offset table in the .c file
produced, that'll fix it, right?

Regards,

Greg