[Webkit-unassigned] [Bug 24682] New: SVG glyph rotation for vertical text is incomplete, and possibly crashy

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=24682

           Summary: SVG glyph rotation for vertical text is incomplete, and
                    possibly crashy
           Product: WebKit
           Version: 528+ (Nightly build)
          Platform: Macintosh
        OS/Version: Mac OS X 10.5
            Status: NEW
          Severity: Normal
          Priority: P2
         Component: SVG
        AssignedTo: webkit-unassigned at lists.webkit.org
        ReportedBy: eric at webkit.org
                CC: ap at webkit.org, zimmermann at kde.org


SVG glyph rotation for vertical text is incomplete, and possibly crashy

I started down this by reading some static analysis reports about WebCore which
showed that this function:

unsigned int findCharUnicodeRange(UChar32 ch)
{
    if (ch >= 0xFFFF)
        return 0;

    unsigned int range;

    //search the first table
    range = gUnicodeSubrangeTable[0][ch >> 12];

    if (range < cRangeTableBase)
        // we try to get a specific range 
        return range;

    // otherwise, we have one more table to look at
    range = gUnicodeSubrangeTable[range - cRangeTableBase][(ch & 0x0f00) >> 8];
    if (range < cRangeTableBase)
        return range;
    if (range < cRangeTertiaryTable)
        return gUnicodeSubrangeTable[range - cRangeTableBase][(ch & 0x00f0) >>
4];

    // Yet another table to look at : U+0700 - U+16FF : 128 code point blocks
    return gUnicodeTertiaryRangeTable[(ch - 0x0700) >> 7];
}


could be made to crash, if it's possible for "range" to ever be 144 (Aka
cRangeTableBase + 16).  144 is < cRangeTertiaryTable (145) so return
gUnicodeSubrangeTable[range - cRangeTableBase][(ch & 0x00f0) >> 4]; will then
crash.

Fortunately cRangeTableBase + 16 is not currently found in
gUnicodeSubrangeTable so we'll never hit that path.

This got me wondering what this code was for?  Turns out it's only ever called
by:

static inline float glyphOrientationToAngle(const SVGRenderStyle* svgStyle,
bool isVerticalText, const UChar& character)
{
    switch (isVerticalText ? svgStyle->glyphOrientationVertical() :
svgStyle->glyphOrientationHorizontal()) {
    case GO_AUTO:
    {
        // Spec: Fullwidth ideographic and fullwidth Latin text will be set
with a glyph-orientation of 0-degrees.
        //       Text which is not fullwidth will be set with a
glyph-orientation of 90-degrees.
        unsigned int unicodeRange = findCharUnicodeRange(character);
        if (unicodeRange == cRangeSetLatin || unicodeRange == cRangeArabic)
            return 90.0f;

        return 0.0f;
    }

Which, looking at the spec is probably wrong.  We're only ever rotating latin
and arabic text when drawn vertically.  Probably basically all scripts but for
a few eastasian scripts want text to rotate 90 degrees when written vertically
in the "Default" orientation.

Talking about this with Alexey, we decided that the following code would
probably work for ICU platforms:

bool glyphShouldBeRotatedWhenDrawnVertically(UChar ch)
{
  return u_getIntPropertyValue(ch, UCHAR_EAST_ASIAN_WIDTH) != U_EA_FULLWIDTH
}
A simpler version of the current code could be used for non-icu platforms w/o
causing any regressions.

For now I'm leaving this.  But it would be "easy" to introduce a crasher to
this code w/o trying to.  So we should get rid of this complicated code and
replace it with something more correct and easier to read. :)


-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.