[HarfBuzz] Use of uninitialised in Harfbuzz

[Simon Hausmann]

Hi Adam,

On Friday 07 August 2009 ext Adam Langley, wrote:
> Some recent changes to the WebKit layout tests[1] started triggering
> this for us:
>
> ==8875== Conditional jump or move depends on uninitialised value(s)
> ==8875==    at 0x8EE7406: HB_HeuristicPosition
> chromium/third_party/harfbuzz/src/harfbuzz-shaper.cpp:417
> ==8875==    by 0x8EECCA2: HB_HebrewShape
> chromium/third_party/harfbuzz/src/harfbuzz-hebrew.c:183
> ==8875==    by 0x8EE6D66: HB_ShapeItem
> chromium/third_party/harfbuzz/src/harfbuzz-shaper.cpp:1308
> ==8875==    by 0x88CBE11: WebCore::TextRunWalker::shapeGlyphs()
> chromium/third_party/WebKit/WebCore/platform/graphics/chromium/FontLinux.cp
>p:352 ==8875==    by 0x88CBF27: WebCore::TextRunWalker::nextScriptRun()
> chromium/third_party/WebKit/WebCore/platform/graphics/chromium/FontLinux.cp
>p:218 ==8875==    by 0x88CBFCC: WebCore::TextRunWalker::widthOfFullRun()
> chromium/third_party/WebKit/WebCore/platform/graphics/chromium/FontLinux.cp
>p:279
>
> Below is the patch that I wrote that appears to fix the problem. I've
> landed it for Chromium already because I wanted to fix the
> intermittent test failures, but I really have no idea when I'm doing
> here!

I think your change there is technically correct, as it will ensure that the 
glyph attributes are initialized even if the script features needed for Hebrew 
are not supported by the font you're using.

I think we don't run into this case when using with Qt because we will try to 
avoid selecting that font due to the missing features.

I'm not quite sure about the assertion changes though:

> I believe the changes to the assertions are correct. I believe that
> item->num_glyphs at this point contains the number of output elements
> in the various arrays and therefore should be >= length, but again,
> not at all sure.

num_glyphs contains the number of elements for which there is space in the 
output array before calling StringToGlyphIndices. Afterwards it contains the 
number of actual glyphs.


> diff --git a/third_party/harfbuzz/src/harfbuzz-shaper.cpp
> b/third_party/harfbuzz/src/harfbuzz-shaper.cpp
> index 36b9282..3628c88 100644
> --- a/third_party/harfbuzz/src/harfbuzz-shaper.cpp
> +++ b/third_party/harfbuzz/src/harfbuzz-shaper.cpp
> @@ -433,7 +433,7 @@ void HB_HeuristicSetGlyphAttributes(HB_ShaperItem
> *item)
>
>      // ### zeroWidth and justification are missing here!!!!!
>
> -    assert(item->num_glyphs <= length);
> +    assert(length <= item->num_glyphs);

So here the number of glyphs cannot exceed the number of characters, as we 
haven't performed any shaping yet. Instead the number of glyphs can be less 
than the number of characters due to surrogate pairs.

>  //     qDebug("QScriptEngine::heuristicSetGlyphAttributes,
> num_glyphs=%d", item->num_glyphs);
>      HB_GlyphAttributes *attributes = item->attributes;
> @@ -451,7 +451,6 @@ void HB_HeuristicSetGlyphAttributes(HB_ShaperItem
> *item) }
>          ++glyph_pos;
>      }
> -    assert(glyph_pos == item->num_glyphs);

This assertion I believe is correct, too, as it verifies that 
StringToGlyphIndices produced the correct number of glyphs for surrogate pairs 
in the input string.

Could the reason for you the failing assertions for you that Skia's 
textToGlyphs produces two glyphs for a surrogate pair (real glyph, followed by 
dummy zero glyph)?

Simon
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 197 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/harfbuzz/attachments/20090813/98e943ba/attachment.pgp>