Daniel Bratell Reporter
	Description • 24 years ago

There is uninitialized memory read in GetGlyphOutlineW called from 
nsGlyphAgent::GetGlyphMetrics(HDC aDC, PRUnichar aChar, PRUint16 
aGlyphIndex, GLYPHMETRICS* aGlyphMetrics). 

It happens for both calls in the function. 

It could be a bug inside GetGlyphOutlineW, but it could also be some error in 
the parameters passed to it which is why I'm filing this bug. 

This was seen in a current debug build.

Assigning to rbs@maths.uq.edu.au who write the code.

	rbs Assignee
	Comment 1 • 24 years ago

I have been wondering what was going on with this function. See the discussion 
to that effect in n.p.m.mathml - "Problems getting MathML to display properly
with M16", started 10/Jul/2000.

I still don't see where the problem is (as the reported pointed out, it might
well be a bug in GetGlyphOutlineW itself) - the code that is using it is in 
gfx/src/windows/nsFontMetricsWin.cpp.

More info:

Since GetGlyphOutlineW is only implemented on WinNT/2000, the code path under
consideration is only reached on these platforms.

The calling syntax of GetGlyphOutline[A/W] is described here:
http://msdn.microsoft.com/library/psdk/gdi/fontext_52at.htm

	Daniel Bratell Reporter
	Comment 2 • 24 years ago

I tried to look closer at this and this is my analyze (I hope it's not a Windows 
bug. :-) ):
The first time it happens is when doing 
    gGlyphAgent.GetGlyphMetrics(dc, PRUnichar('x'), 0, &gm); 
in nsFontMetrics::RealizeFont(). gm is a newly allocated structure (not filled 
with any data, could test to null it and see if that changes anything).
dc is a device context. No (visible) windows are opened yet except for the 
splash screen. Don't know if that makes any difference.

Since this is the first time GetGlyphMetrics is called mState is 
eGlyphAgent_UNKNOWN and the first "test"-call is made to GetGlyphOutlineW. The 
parameters are:
aDC           (the same dc as before)
aChar         (PRUnichar with the value 120 ('x') )
GGO_METRICS   (constant saying that we want to get the metrics in the structure)
aGlyphMetrics (pointer to the same GLYPHMETRICS structure as before)
0             (zero: the length of the data buffer. According to the
               documentation: 
                   [in] Specifies the size of the buffer where the function 
                   is to copy information about the outline character. If this
                   value is zero, the function returns the required size of 
                   the buffer.
               so this function returns the size of a required buffer.)
nsnull        (The buffer to put information in (what information?). If null
               it returns the size required (the same as before)).
&mMat         (The address to the member identity matrix. Fully initialized).

So what could be wrong? The DC could be wrong somehow, but it's difficult to 
analyze it. At least for me who is a total stranger to these things. It might be 
necessary to null out the aGlyphMetrics buffer before using it. The rest looks 
fine. The function call returns 24 by the way and the structure is filled with 7 
and 0. (gmBlackBoxX, gmBlackBoxY, gmptGlyphOrigin.y and gmCellIncX are 7 and 
gmptGlyphOrigin.x and gmCellIncY are 0).

	Daniel Bratell Reporter
	Comment 3 • 24 years ago

I just realized that Purify tells you the whole story. 

    Reading 20 bytes from 0x0013e320 (20 bytes at 0x0013e320 uninitialized)
    Address 0x0013e320 is the local variable 'gm' in 
nsFontMetricsWin::RealizeFont(void).

In other words, the structure sent in should be zeroed (or something) because 
for some reason it's read inside the call. I don't think this really is a bug 
in our code but if we do it we silence Purify at least. And you could see if it 
affect those having problems with this function.

Proposed patch:

Index: windows/nsFontMetricsWin.cpp
===================================================================
RCS file: /cvsroot/mozilla/gfx/src/windows/nsFontMetricsWin.cpp,v
retrieving revision 3.87
diff -u -r3.87 nsFontMetricsWin.cpp
--- windows/nsFontMetricsWin.cpp        2000/07/25 08:32:39     3.87
+++ windows/nsFontMetricsWin.cpp        2000/07/26 15:10:58
@@ -2795,6 +2795,7 @@

     // Begin -- section of code to get the real x-height with GetGlyphOutline()

     GLYPHMETRICS gm;
+    memset(&gm, 0, sizeof(gm));
     DWORD len = gGlyphAgent.GetGlyphMetrics(dc, PRUnichar('x'), 0, &gm);
     if (GDI_ERROR != len && gm.gmptGlyphOrigin.y > 0)
     {
@@ -3076,7 +3077,8 @@

     // measure the string
     nscoord descent;
     GLYPHMETRICS gm;
+    memset(&gm, 0, sizeof(gm));
     DWORD len = gGlyphAgent.GetGlyphMetrics(aDC, aString[0], pstr[0], &gm);
     if (GDI_ERROR == len) {
       return NS_ERROR_UNEXPECTED;
@@ -3227,6 +3229,7 @@
     // measure the string
     nscoord descent;
     GLYPHMETRICS gm;
+    memset(&gm, 0, sizeof(gm));
     DWORD len = gGlyphAgent.GetGlyphMetrics(aDC, PRUint8(pstr[0]), &gm);
     if (GDI_ERROR == len) {
       return NS_ERROR_UNEXPECTED;
@@ -3415,6 +3418,7 @@
     // measure the string
     nscoord descent;
     GLYPHMETRICS gm;
+    memset(&gm, 0, sizeof(gm));
     DWORD len = gGlyphAgent.GetGlyphMetrics(aDC, pstr[0], ps[0], &gm);
     if (GDI_ERROR == len) {
       return NS_ERROR_UNEXPECTED;
@@ -3693,6 +3697,7 @@
     // measure the string
     nscoord descent;
     GLYPHMETRICS gm;
+    memset(&gm, 0, sizeof(gm));
     DWORD len = gGlyphAgent.GetGlyphMetrics(aDC, PRUint8(pstr[0]), &gm);
     if (GDI_ERROR == len) {
       return NS_ERROR_UNEXPECTED;

	rbs Assignee
	Comment 4 • 24 years ago

Great analysis. This is really helpful. Sounds like there is a problem inside
GetGlyphOutlineW. aGlyphMetrics parameter is documented as an OUT parameter, and
members in that struct are clearly waiting to be filled (not read :-)

I am curious to know if the same UMR also occurs with GetGlyphOutlineA. What do
you see when you set mState = eGlyphAgent_ANSI (instead of eGlyphAgent_UNKNOWN)
in the constructor? This setting will force the Win9x code-path to be executed.

I will later just add memset(aGlyphMetrics, 0, sizeof(GLYPHMETRICS)) in
nsGlyphAgent::GetGlyphMetrics() to silence Purify.

	rbs Assignee
	Comment 5 • 24 years ago

I tried memset-ting to zero and launching that binary on a Win 2000 system.
It didn't solve the failures in the function.

I tried setting mState = eGlyphAgent_ANSI in the ctor to always force
the non-Unicode path, but it didn't solve the failures either. 
Hence, the failures are happening with GetGlyphOutlineA().
What is peculiar about these failures is that they don't happen for certain
font-sizes, i.e., with some maneuvring in View -> Enlarge/Reduce Text Size.

	rbs Assignee
	Comment 6 • 24 years ago

I noted that alecf has checked something for this:
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&su
bdir=mozilla/gfx/src/windows&command=DIFF_FRAMESET&file=nsFontMetricsWin.cpp&rev
1=3.88&rev2=3.89&root=/cvsroot

Unfortunately, it isn't at the right spot. My preferred spot would be in the
method itself.

	Alec Flett
	Comment 7 • 24 years ago

that's the right spot, I swear, right after the gm declaration.

the fact that gm is passed to another function before the UMR occurs is not a 
reason to put the memset elsewhere.

reassigning to me to mark fixed

	Daniel Bratell Reporter
	Comment 8 • 24 years ago

But the bug is in a Windows function. I would like to see the patch (hack) to 
get rid of the message as near as that function call as possible.

	Alec Flett
	Comment 9 • 24 years ago

Well, in any case, I don't see anything wrong with the memset where it is, and 
from my perspective it makes more sense right after the declaration - we're 
creating gm on the stack, now we want to set it to 0. this is a common pattern 
in any C/C++ programming...  besides, it might just be that the documentation is 
bad and that it's an inout parameter taking some default value or something.

since nobody else stepped up to fix it, this is where it ended up. I'm not 
seeing UMRs in GetGlyphOutlineA so the failures you're seeing may be unrelated.

	rbs Assignee
	Comment 10 • 24 years ago

Re-opening. There other callers (in #ifdef). That's why I am favoring
a memset next to GetGlyphOutlineW itself.

	rbs Assignee
	Comment 11 • 24 years ago

Taking the bug due to re-open.

	Alec Flett
	Comment 12 • 24 years ago

but they are not triggering the UMR in purify..why do the extra memset?

	rbs Assignee
	Comment 13 • 24 years ago

The other callers are in #ifdef (i.e., not in the default build). That's why
they are not visible. The other UMRs are surfacing when the #ifdef are activated 
in MathML-enabled builds. (BTW, making a bit of advocacy, what are you waiting
to try out MathML for a change :-)

	rbs Assignee
	Comment 14 • 24 years ago

The error value when GetGlyphOutlineA() fails is:

ERROR_CAN_NOT_COMPLETE (#1003): Cannot complete this function. 
                                                                               

	rbs Assignee
	Comment 16 • 24 years ago

Erik, could you please review this patch for me. It contains three elements:

1) Fix for UMR by zeroing the param that is passed to GetGlyphOutline().

2) Remove unnecessary code from FindSubstituteFont() : since this function
is called last, all the fonts (including the global ones) have been tried out.
Hence the fonts in the global list are also in the mLoadedFonts list scanned
earlier. The removed code was dead code.

3) The other element in the patch is an #ifdef to avoid the Windows GDI to
become confused for a reason that I don't understand. (There are more details on 
news://news.mozilla.org/39E15C74.6E7C849%40maths.uq.edu.au
and the associated thread "Failures of GetGlyphOutline()")

	Erik van der Poel
	Comment 17 • 24 years ago

Hi Roger, I would prefer to see only UMR-related code in the patch for this bug
report. The other parts of the patch don't seem related to this bug report. Am I
missing something?

	rbs Assignee
	Comment 18 • 24 years ago

Index: nsFontMetricsWin.cpp
===================================================================
RCS file: /cvsroot/mozilla/gfx/src/windows/nsFontMetricsWin.cpp,v
retrieving revision 3.91
diff -u -r3.91 nsFontMetricsWin.cpp
--- nsFontMetricsWin.cpp        2000/09/07 03:20:05     3.91
+++ nsFontMetricsWin.cpp        2000/10/09 07:44:53
@@ -1536,6 +1536,7 @@
                               PRUint8       aChar,
                               GLYPHMETRICS* aGlyphMetrics)
 {
+  memset(aGlyphMetrics, 0, sizeof(GLYPHMETRICS)); // UMR: bug 46438
   return GetGlyphOutlineA(aDC, aChar, GGO_METRICS, aGlyphMetrics, 0, nsnull,
&mMat);
 }
 
@@ -1548,6 +1549,7 @@
                               PRUint16      aGlyphIndex,
                               GLYPHMETRICS* aGlyphMetrics)
 {
+  memset(aGlyphMetrics, 0, sizeof(GLYPHMETRICS)); // UMR: bug 46438
   if (eGlyphAgent_UNKNOWN == mState) { // first time we have been in this
function
     // see if this platform implements GetGlyphOutlineW()

     DWORD len = GetGlyphOutlineW(aDC, aChar, GGO_METRICS, aGlyphMetrics, 0,
nsnull, &mMat);

(Although not strictly related to this bug, the other parts are useful too.
It may take all sort of detours to go in the tree, but I will be putting
them on an imminent CD that Dawn is preparing. If you anticipate any
problems with them, do well to tell, thanks.)

	Daniel (Leaf) Nunes
	Comment 19 • 24 years ago

patch 16497 builds fine on the MOZILLA_0_6_BRANCH; verifying that it *runs* as well.

	rbs Assignee
	Comment 20 • 24 years ago

Any reservations if I now follow on and check patch 16497 to the main tree
before it rots even further? The patch solves a problem that is dear to MathML
when getting the exact glyph metrics on WinNT/2K.

	Brendan Eich [:brendan]
	Comment 21 • 24 years ago

I'm not the best reviewer for this change, and defer to erik, but one nit:

-    if (res == eGetName_OK) {
+    if (res != eGetName_OK) {
+    	continue;
+    }
+    else {

else after continue (and after break or return) is a non-sequitur.  The else
should be removed, and the else block should be unindented.

/be

	rbs Assignee
	Comment 23 • 24 years ago

Erik, long time no see... Does that mean you are okay if I checkin the patch?

	Erik van der Poel
	Comment 24 • 24 years ago

sr=erik

	rbs Assignee
	Comment 25 • 24 years ago

Thanks, checked-in. Marking as FIXED.

	Chris Petersen
	Comment 26 • 24 years ago

Marking verified in the March 5th builds.