605605 - Uninitialised value use in nsMathMLChar::StretchInternal

Status: RESOLVED FIXED

(Core :: MathML, defect)

Description

[Julian Seward [:jseward]]

Whilst running crashtests on WinXP build of M-C of yesterday, on Wine:

layout/generic/crashtests/472587-1.xhtml

Conditional jump or move depends on uninitialised value(s)
   at 0x1093EF91: nsMathMLChar::StretchInternal (nsmathmlchar.cpp:1748)
   by 0x1093F83E: nsMathMLChar::GetMaxWidth (nsmathmlchar.cpp:1835)
   by 0x1092B757: nsMathMLmencloseFrame::PlaceInternal
                  (nsmathmlmencloseframe.cpp:530)
   by 0x1092AE1A: nsMathMLmencloseFrame::MeasureForWidth
                  (nsmathmlmencloseframe.cpp:338)
   by 0x10928E77: nsMathMLContainerFrame::GetIntrinsicWidth
                  (nsmathmlcontainerframe.cpp:1080)
   by 0x10928DA0: nsMathMLContainerFrame::GetMinWidth
                  (nsmathmlcontainerframe.cpp:1039)
   by 0x1043AF6D: nsLayoutUtils::IntrinsicForContainer (nslayoututils.cpp:2012)
   by 0x1048E7AF: nsFrame::AddInlineMinWidth (nsframe.cpp:3146)
   by 0x104A3993: nsBlockFrame::GetMinWidth (nsblockframe.cpp:743)
   by 0x104D17E4: nsColumnSetFrame::GetMinWidth (nscolumnsetframe.cpp:461)
   by 0x1043AF6D: nsLayoutUtils::IntrinsicForContainer (nslayoututils.cpp:2012)
   by 0x104A38B6: nsBlockFrame::GetMinWidth (nsblockframe.cpp:724)

 Uninitialised value was created by a heap allocation
   at 0x72184B2: RtlAllocateHeap (heap.c:205)
   by 0x78583A57: ???
   by 0x7F67107C: ??? (in /mnt/win32/MC-19-10-2010/ff-exp/dist/bin/mozalloc.dll)
   by 0x10CC85FC: NS_Alloc_P (nsmemoryimpl.cpp:210)
   by 0x10257A38: nsTArray_base::EnsureCapacity (nstarray.cpp:76)
   by 0x1092C5CF: nsTArray<nsMathMLChar>::AppendElements (nstarray.h:678)
   by 0x1092C550: nsTArray<nsMathMLChar>::AppendElement (nstarray.h:693)
   by 0x1092A130: nsMathMLmencloseFrame::AllocateMathMLChar
                  (nsmathmlmencloseframe.cpp:101)
   by 0x1092E9F5: nsMathMLmsqrtFrame::Init (nsmathmlmsqrtframe.cpp:81)
   by 0x1041DA51: nsCSSFrameConstructor::InitAndRestoreFrame
                  (nscssframeconstructor.cpp:4547)
   by 0x1041C286: nsCSSFrameConstructor::ConstructFrameFromItemInternal
                  (nscssframeconstructor.cpp:3760)
   by 0x1041FDB4: nsCSSFrameConstructor::ConstructFramesFromItem
                  (nscssframeconstructor.cpp:5475)

Comment 1

[Karl Tomlinson (:karlt)]

Fred, the stretch process avoids setting mGlyph when doing NS_STRETCH_MAXWIDTH.
I can't remember whether that is necessary or not (I think it was to avoid undoing a previous stretch if merely measuring widths),
but do we need some special max-width handling for the
"if (mGlyph.font == -1 && largeop)" block in StretchInternal?

Comment 2

[Frédéric Wang (:fredw)]

(In reply to comment #1)
> Fred, the stretch process avoids setting mGlyph when doing NS_STRETCH_MAXWIDTH.
> I can't remember whether that is necessary or not (I think it was to avoid
> undoing a previous stretch if merely measuring widths),
> but do we need some special max-width handling for the
> "if (mGlyph.font == -1 && largeop)" block in StretchInternal?

I think this block is still necessary when doing NS_STRETCH_MAXWIDTH, because it updates the width. This "mGlyph.font == -1" (and the one a bit above, protected by !maxWidth) basically means that no glyph has been found in our StretchInternal function. Maybe we can replace this by a variable glyphFound in StretchInternal that we update after the each call to font.EnumerateFamilies (maybe we need to send this variable to the StretchEnumContext routine). Then, I guess setting mGlyph.font = 1 will no longer be necessary in the Stretch function.

Comment 3

[Karl Tomlinson (:karlt)]

(In reply to comment #2)
> Maybe we can replace this by a variable glyphFound in
> StretchInternal that we update after the each call to font.EnumerateFamilies
> (maybe we need to send this variable to the StretchEnumContext routine).

That might be the best option.

I thought about recording the original size and replacing "if (mGlyph.font == -1 && largeop)" with a check on whether the size had changed, but it looks like that check might be complicated or not quite right with maxWidth.

Comment 4

[Frédéric Wang (:fredw)]

If we want to prevent undoing a previous stretch with maxWidth, then mUnscaledAscent, mDrawNormal, mScaleX, mScaleY should not be modified. This is not currently the case for mScaleX and mScaleY.

It is not really possible from the StretchEnumContext to know where the computed max width comes from (it is the max of all the metrics of "variants" and "parts"). So maybe a simpler way to solve this bug is to always execute the block when maxWidth+largeop i.e. replacing the condition by

if ((maxWidth || mGlyph.font == -1) && largeop) {

Comment 5

[Karl Tomlinson (:karlt)]

(In reply to comment #4)
> If we want to prevent undoing a previous stretch with maxWidth, then
> mUnscaledAscent, mDrawNormal, mScaleX, mScaleY should not be modified. This is
> not currently the case for mScaleX and mScaleY.

They should probably not be modified.

> It is not really possible from the StretchEnumContext to know where the
> computed max width comes from (it is the max of all the metrics of "variants"
> and "parts"). So maybe a simpler way to solve this bug is to always execute the
> block when maxWidth+largeop i.e. replacing the condition by
> 
> if ((maxWidth || mGlyph.font == -1) && largeop) {

For the largeoponly case, it should be possible to get the exact width, and it would be preferable to have the exact width where possible.

I expect your earlier suggestion, with tracking whether a variant was found in the StretchEnumContext, would also give a better width for non-largeoponly largeops.

Comment 6

[Frédéric Wang (:fredw)]

Attachment: Patch V1

Comment 7

[Karl Tomlinson (:karlt)]

Comment on attachment 488692 [details] [diff] [review]
Patch V1

>         if (maxWidth) {
>+          mGlyphFound = PR_TRUE;
>+

>         else {
>           mBoundingMetrics = bm;
>           haveBetter = PR_TRUE;
>           bestSize = charSize;
>           mChar->mGlyphTable = aGlyphTable;
>           mChar->mGlyph = ch;
>+          mGlyphFound = PR_TRUE;
>           mChar->mFamily = font.name;

Set mGlyphFound in only one place before the conditional blocks.

>   if (maxWidth)
>     return PR_FALSE; // Continue to check other sizes
> 
>   // reset
>   mChar->mGlyph = kNullGlyph; // this will tell paint to build by parts
>+  mGlyphFound = PR_TRUE;

For consistency of the maxWidth/normal-stretch cases, mGlyphFound should be set
before the "if (maxWidth)" early return, right?  (Because at that point we know
that the parts would have been used if it were a stretch.)

>-  // This will be updated if a better match than the base character is found
>-  mGlyph.font = -1;
>-

It looks like we need to set mDrawNormal, perhaps here, for the case when
StretchInternal returns early because the normal character is now appropriate
(but was not appropriate in a previous stretch).

>-  PRInt32 glue, bottom;
>+  PRInt32 glue = 0, bottom = 0;

>-  PRInt32 glue, right;
>+  PRInt32 glue = 0, right = 0;

I'd prefer not to initialize these to incorrect values just for the sake of
silencing a compiler that does understand the code, because it comes at the
cost of preventing valgrind from detecting if they don't get set
appropriately.

Comment 8

[Karl Tomlinson (:karlt)]

i mean "a compiler that does *not* understand the code".

Comment 9

[Frédéric Wang (:fredw)]

> 
> >   if (maxWidth)
> >     return PR_FALSE; // Continue to check other sizes
> > 
> >   // reset
> >   mChar->mGlyph = kNullGlyph; // this will tell paint to build by parts
> >+  mGlyphFound = PR_TRUE;
> 
> For consistency of the maxWidth/normal-stretch cases, mGlyphFound should be set
> before the "if (maxWidth)" early return, right?  (Because at that point we know
> that the parts would have been used if it were a stretch.)
> 

OK, that's not what was doing the old code (mGlyph.font is no longer -1 only after we set mGlyph = kNullGlyph) but maybe is was a mistake. However I'm not sure it changes anything to also move mChar->mGlyph = kNullGlyph before the "if (maxWidth)", now we rely on mGlyphFound?

Comment 10

[Frédéric Wang (:fredw)]

> OK, that's not what was doing the old code (mGlyph.font is no longer -1 only
> after we set mGlyph = kNullGlyph) but maybe is was a mistake.

Anyway, It was not the case either here:

>         if (maxWidth) {
>+          mGlyphFound = PR_TRUE;
>+

>         else {
>           mBoundingMetrics = bm;
>           haveBetter = PR_TRUE;
>           bestSize = charSize;
>           mChar->mGlyphTable = aGlyphTable;
>           mChar->mGlyph = ch;
>+          mGlyphFound = PR_TRUE;
>           mChar->mFamily = font.name;

Comment 11

[Karl Tomlinson (:karlt)]

(In reply to comment #9)
> OK, that's not what was doing the old code (mGlyph.font is no longer -1 only
> after we set mGlyph = kNullGlyph) but maybe is was a mistake.

Yes, being as consistent as possible between GetMaxWidth/Stretch is the goal.

> However I'm not
> sure it changes anything to also move mChar->mGlyph = kNullGlyph before the "if
> (maxWidth)", now we rely on mGlyphFound?

I'm not sure I completely understand your point here, so let me know if I'm not addressing your question.
I think mChar->mGlyph = kNullGlyph should stay where it is (because we don't want to mess up a previous stretch when we do a measure);
only the mGlyphFound = PR_TRUE should move.

Comment 12

[Frédéric Wang (:fredw)]

> I'm not sure I completely understand your point here, so let me know if I'm not
> addressing your question.
> I think mChar->mGlyph = kNullGlyph should stay where it is (because we don't
> want to mess up a previous stretch when we do a measure);
> only the mGlyphFound = PR_TRUE should move.

Yes, I meant that it is not necessary to move mChar->mGlyph = kNullGlyph and as you say it will even be incorrect. So I agree that only mGlyphFound = PR_TRUE should move.

Comment 13

[Frédéric Wang (:fredw)]

Attachment: Patch V2

Comment 14

[Karl Tomlinson (:karlt)]

Comment on attachment 489600 [details] [diff] [review]
Patch V2

This looks very similar to the first patch.
Wrong patch or forgot to qref?

Comment 15

[Frédéric Wang (:fredw)]

Attachment: Patch V2

oops, sorry...

Comment 16

[Karl Tomlinson (:karlt)]

Comment on attachment 489757 [details] [diff] [review]
Patch V2

No problem.  Looks good, thanks.

Comment 17

[Karl Tomlinson (:karlt)]

I assume layout/generic/crashtests/472587-1.xhtml doesn't show any visual differences due to the uninitialized variable, but I wonder whether some similar set-up perhaps with tables might do.

Frédéric, would you mind having a look to see if a reftest (perhaps a dynamic test similar to 472587-1.xhtml) could be written to test this, please?
Some effort should be made to write a test before requesting approval2.0, but, if you don't come up with anything, we'll just have to rely on valgrind.

Comment 18

[Julian Seward [:jseward]]

(In reply to comment #15)
> Created attachment 489757 [details] [diff] [review]
> Patch V2

I verified that this stops Valgrind complaining.

Comment 19

[Frédéric Wang (:fredw)]

(In reply to comment #17)
> I assume layout/generic/crashtests/472587-1.xhtml doesn't show any visual
> differences due to the uninitialized variable, but I wonder whether some
> similar set-up perhaps with tables might do.
> 
> Frédéric, would you mind having a look to see if a reftest (perhaps a dynamic
> test similar to 472587-1.xhtml) could be written to test this, please?
> Some effort should be made to write a test before requesting approval2.0, but,
> if you don't come up with anything, we'll just have to rely on valgrind.

Sorry for the delay, I've just seen your comment today (for some reason I did not receive bugspam for this). Unfortunately, I don't think I'll have time to contribute until next January so I fear it is too late for Gecko 2.0...

Comment 20

[Frédéric Wang (:fredw)]

I'm no longer able to get Valgrind's warning from the crashtest...

Comment 21

[Frédéric Wang (:fredw)]

> Frédéric, would you mind having a look to see if a reftest (perhaps a dynamic
> test similar to 472587-1.xhtml) could be written to test this, please?
> Some effort should be made to write a test before requesting approval2.0, but,
> if you don't come up with anything, we'll just have to rely on valgrind.

Should we add this bug to post2.0 now?

Comment 22

[Karl Tomlinson (:karlt)]

I'm wondering whether this triggered a reftest failure in scale-stretchy-3.xhtml
(Might have been bug 534970 or bug 21479, though.)

MacOS:
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1300956320.1300957795.18944.gz#err0

Fedora:
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1300955131.1300956686.13938.gz#err0

NT:
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1300958433.1300961828.3448.gz#err0

Comment 23

[Karl Tomlinson (:karlt)]

No, wasn't this patch.

Comment 24

[Frédéric Wang (:fredw)]

I think it's been a while since scale-stretchy-3 is failing (can't remember when it happened). The purpose of the test is to check that the width of the arrow does not exceed the one of the rectangle. However, the failure is because the blue arrow and red mspace are not positioned at the same height. This can be fixed by adding some depth to the first yellow mspace, so that the blue arrow will be slightly lower.

Comment 25

[Karl Tomlinson (:karlt)]

Traced the reftest failure to bug 534970 comment 62.

Comment 26

[Karl Tomlinson (:karlt)]

http://hg.mozilla.org/mozilla-central/rev/014fb62e0905