605616 - Uninitialised value use in UniscribeItem::SaveGlyphs

Status: NEW

(Core :: Layout: Text and Fonts, defect)

Description

[Julian Seward [:jseward]]

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

at http://www.bbc.co.uk/news

Conditional jump or move depends on uninitialised value(s)
   at 0x102A21DD: UniscribeItem::SaveGlyphs (gfxuniscribeshaper.cpp:297)
   by 0x102A198F: gfxUniscribeShaper::InitTextRun (gfxuniscribeshaper.cpp:643)
   by 0x1029D886: gfxGDIFont::InitTextRun (gfxgdifont.cpp:181)
   by 0x10278057: gfxFontGroup::InitTextRun (gfxfont.cpp:2276)
   by 0x10277F44: gfxFontGroup::InitTextRun (gfxfont.cpp:2244)
   by 0x10277E9C: gfxFontGroup::MakeTextRun (gfxfont.cpp:2212)
   by 0x10288C60: TextRunWordCache::MakeTextRun (gfxtextrunwordcache.cpp:693)
   by 0x10289BFE: gfxTextRunWordCache::MakeTextRun
                  (gfxtextrunwordcache.cpp:1002)
   by 0x1047AAB3: MakeTextRun (nstextframethebes.cpp:504)
   by 0x1047A6F4: BuildTextRunsScanner::BuildTextRunForFrames
                  (nstextframethebes.cpp:1864)
   by 0x10479291: BuildTextRunsScanner::FlushFrames (nstextframethebes.cpp:1296)
   by 0x1047BBDA: BuildTextRuns (nstextframethebes.cpp:1230)

 Uninitialised value was created by a stack allocation
   at 0x102A1733: gfxUniscribeShaper::InitTextRun (gfxuniscribeshaper.cpp:558)

Now it might be the case that this is triggered by Wine's somewhat
krunky font machinery causing Fx to take some unusual path through its
windows font code.  Immediately before this, Wine produces a bunch of
these

 fixme:uniscribe:GSUB_apply_lookup We do not handle SubType 6

That said, it ought to be impossible to get Fx to use uninitialised
anything on any input, so I think it's still a legit bug.

Comment 1

[Julian Seward [:jseward]]

I chased this around at some length today, but I can't make much sense
of it.

From adding printfs in UniscribeItem::IsGlyphMissing, it's apparent
that appears one of the values in the UniscribeItem::mGlyphs array is
uninitialised.  Figuring out why, though, defeats me so far.

One thing I did establish is that the allegedly uninitialised data is
created by the following constructor call in
gfxUniscribeShaper::InitTextRun:

        UniscribeItem item(aContext, aDC, this,
                           aString + aRunStart + iCharPos,
                           iCharPosNext - iCharPos,
                           us.ScriptItem(i), ivs);
 
UniscribeItem::UniscribeItem (there's only one constructor) looks ok.
It appears to give an initial value to all its fields, except these 5:

    nsAutoTArray<WORD, PRUint32(ESTIMATE_MAX_GLYPHS(AVERAGE_ITEM_LENGTH))> mGlyphs;
    nsAutoTArray<WORD, AVERAGE_ITEM_LENGTH + 1> mClusters;
    nsAutoTArray<SCRIPT_VISATTR, PRUint32(ESTIMATE_MAX_GLYPHS(AVERAGE_ITEM_LENGTH))> mAttr;

    nsAutoTArray<GOFFSET, 2 * AVERAGE_ITEM_LENGTH> mOffsets;
    nsAutoTArray<int, 2 * AVERAGE_ITEM_LENGTH> mAdvances;

Perhaps nsAutoTArray is self-initialising, by some C++ magic.  I seem
to remember nsTArray behaving like that.

I remember from bug 582668 comment 3, that nsTArray::SetLength
auto-fills new bits of array with a default value only when the value
type is non-POD.  So I'm wondering if something is resizing the
mGlyphs array larger, and assuming the new values are auto-zeroed,
whereas they aren't.

Comment 2

[Julian Seward [:jseward]]

(In reply to comment #1)
> From adding printfs in UniscribeItem::IsGlyphMissing [...]

Here's what I got from that -- various calls to
gfxUniscribeShaper::InitTextRun.  Most are harmless.  The offending
one produces this, when running on V:

InitTextRun-BEGIN
  AllocateBuffers: mMaxGlyphs = 23
  call .SaveGlyphs
  IsGlyphMissing: aGlyphIndex 0,  mGlyphs[aGlyphIndex] 299,  aSFP->wgDefault 31
  IsGlyphMissing: aGlyphIndex 1,  mGlyphs[aGlyphIndex] 230,  aSFP->wgDefault 31
  IsGlyphMissing: aGlyphIndex 2,  mGlyphs[aGlyphIndex] 261,  aSFP->wgDefault 31
  IsGlyphMissing: aGlyphIndex 3,  mGlyphs[aGlyphIndex] 183,  aSFP->wgDefault 31
  IsGlyphMissing: aGlyphIndex 4,  mGlyphs[aGlyphIndex] 32545,  aSFP->wgDefault 31
InitTextRun-END

See that last mGlyphs[] value, 32545?  That's what V is complaining
about.  JohnD, is this a plausible value?

Comment 3

[Julian Seward [:jseward]]

(In reply to comment #1)
> type is non-POD.  So I'm wondering if something is resizing the
> mGlyphs array larger, and assuming the new values are auto-zeroed,

Changing UniscribeItem::AllocateBuffers to zero out mGlyphs

    PRBool AllocateBuffers() {
        PRBool ok = mGlyphs.SetLength(mMaxGlyphs) &&
                    mClusters.SetLength(mItemLength + 1) &&
                    mAttr.SetLength(mMaxGlyphs);
        if (ok && mMaxGlyphs > 0)
            memset(mGlyphs.Elements(), 0, mMaxGlyphs * sizeof(PRUint32));
        return ok;
    }

stops Valgrind complaining.  This does suggest that we're reading
slots of mGlyphs that never got written to, but why I don't know.

Giving up on this for the time being.  If I can get guidance from
folks familiar with it, I'll happily pick it up again.

Comment 4

[Julian Seward [:jseward]]

while I remember: gfxUniscribeShaper::InitTextRun has this

    AutoSelectFont fs(aDC, font->GetHFONT());

It doesn't seem to be used though.  Maybe removable?

Comment 5

[Jonathan Kew [:jfkthame]]

(In reply to comment #4)
> while I remember: gfxUniscribeShaper::InitTextRun has this
> 
>     AutoSelectFont fs(aDC, font->GetHFONT());
> 
> It doesn't seem to be used though.  Maybe removable?

I believe that's responsible for selecting the appropriate font into the device context; various Uniscribe functions will depend on this being set properly.