[Webkit-unassigned] [Bug 81389] Vertical alternate glyph (GSUB) support for OpenTypeVerticalData

[bugzilla-daemon at webkit.org]

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


Koji Ishii <kojiishi at gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #134970|review?                     |
               Flag|                            |




--- Comment #9 from Koji Ishii <kojiishi at gmail.com>  2012-04-16 22:11:50 PST ---
(From update of attachment 134970)
View in context: https://bugs.webkit.org/attachment.cgi?id=134970&action=review

I'll be uploading the revised patch once it passed the builds and tests. Thank you for the review and for your time!

>> Source/WebCore/ChangeLog:11
>> +        Reviewed by NOBODY (OOPS!).
> 
> nit: We usually put "Reviewed by" line before the description.

Ah, I missed that. Thank you, I'll fix it.

>> Source/WebCore/platform/graphics/opentype/OpenTypeVerticalData.cpp:65
>> +    template <typename T> static const T* validatedPtr(const void* p, const SharedBuffer& buffer)
> 
> A template function named validatedPtr is already defined in the same file. Using the same name might confuse (The order of the arguments are also inconsistent between them).

Okay, I'll change them to: validateSize, validateOffset, and validatePtr, and will make the order consistent.

>> Source/WebCore/platform/graphics/opentype/OpenTypeVerticalData.cpp:68
>> +        if (!isValidEnd(&casted[1], buffer))
> 
> Just checking whether the |p| is in the range of |buffer.data()| to |buffer.data() + buffer.size()| looks optimistic to me. How do you guarantee the casted pointer is valid? Since an attacker can send malformed font as webfont, we should be careful here. (Chromium uses font sanitizing library for webfonts, but not all port use such library).

I'm trying to avoid out-of-bound reads here, so you're right that the data in the buffer is still broken, but at least we can avoid out-of-bound reads. Checking value validity is responsible for callers. Or are you saying that this check isn't enough to prevent out-of-bound reads?
What OpenTypeSanitizer does is, as far as I understand, to check directory structure, byte align, checksum etc. OTS needs to extract tables from WOFF stream, so that makes sense, but this library requests tables to platform, so analyzing and validating table directory structure is platform's responsibility. Does this sound reasonable?

>> Source/WebCore/platform/graphics/opentype/OpenTypeVerticalData.cpp:181
>> +        uint16_t countSubTable = subTableCount;
> 
> Why do we need this substitution?

Because subTableCount is used multiple times, and although it looks a normal variable, it's actually a struct and converts big-Endian as we read the value. Storing to uint16_t saves converting Endians multiple times.

>> Source/WebCore/platform/graphics/opentype/OpenTypeVerticalData.cpp:186
>> +            for (int i = 0; i < countSubTable; ++i) {
> 
> int -> uint16_t?

Right, thank you for catching this.

>> Source/WebCore/platform/graphics/opentype/OpenTypeVerticalData.cpp:190
>> +                const CoverageTable* coverage = sub->coverage(buffer);
> 
> This line looks essentially the same semantics of other cast such as L187 and L195. Why only this cast is defined as a function?

I'm trying to avoid reading offset-type values from other than this since offsets are the most dangerous value. L187/195 are reading fields of this while this line reads a field of sub. For that purpose, validating functions above are protected, not public, so I need a function to read offsets of non-this tables.

>> Source/WebCore/platform/graphics/opentype/OpenTypeVerticalData.cpp:234
>> +                    LOG_ERROR("SubstFormat %d not supported", sub->substFormat);
> 
> Is LOG_ERROR() appropriate here? Is it unlikely that most fonts have other formats(e.g. Single Substitution Format 1 and Multiple Substitution Format) here?

For the later question, as far as I know, yes. GSUB is general-purpose glyph substition talbe, and for purposes of 'vert', lookupType=1 and Single Substitution format is used. There's a chance where font designer chose Single Substitution Format 1, but I don't of know of any real examples.
For the former question, I'm not sure. I'd like to see WebKit logs something when it encountered such fonts without running debugger. It's not an error of font file but rather means "we don't support this font." If LOG_ERROR isn't supposed to use for such purpose, I'm ok to remove this line.

>> Source/WebCore/platform/graphics/opentype/OpenTypeVerticalData.cpp:240
>> +            LOG_ERROR("LookupType %d not supported", lookupType);
> 
> Is it unlikely that "lookupType != 1"?

See above. Other lookupTypes are used for ligatures or for shapings. Theoretically font designers can use other lookup types, the OT spec does not prohibit it, but I don't see them in the real world.

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