[webkit-reviews] review canceled: [Bug 81389] Vertical alternate glyph (GSUB) support for OpenTypeVerticalData : [Attachment 134970] Vertical alternate glyph (GSUB) support for OpenTypeVerticalData

[bugzilla-daemon at webkit.org]

Koji Ishii <kojiishi at gmail.com> has canceled Koji Ishii <kojiishi at gmail.com>'s
request for review:
Bug 81389: Vertical alternate glyph (GSUB) support for OpenTypeVerticalData
https://bugs.webkit.org/show_bug.cgi?id=81389

Attachment 134970: Vertical alternate glyph (GSUB) support for
OpenTypeVerticalData
https://bugs.webkit.org/attachment.cgi?id=134970&action=review

------- Additional Comments from Koji Ishii <kojiishi at gmail.com>
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.