Since bug 81326 is still too big, I'm splitting code to handle vertical alternate glyph (GSUB) into this bug.
Created attachment 132354 [details] Vertical alternate glyph (GSUB) support for OpenTypeVerticalData A patch split from bug 81326
Created attachment 132453 [details] Vertical alternate glyph (GSUB) support for OpenTypeVerticalData Rebased new patch at 81326, the same name cleanup as 81326, and added ChangeLog
Created attachment 132502 [details] Vertical alternate glyph (GSUB) support for OpenTypeVerticalData Rebased to the new patch in bug 81326, and also updated to reflect review comments in the bug.
Created attachment 134404 [details] Vertical alternate glyph (GSUB) support for OpenTypeVerticalData Changes include: * Rebased to the new patch for bug 81326. * Removed OT prefixes to align with changes in bug 81326. * Fixed one potential out-of-bounds reads. * A little refactored to make the patch shorter.
Created attachment 134718 [details] Vertical alternate glyph (GSUB) support for OpenTypeVerticalData Added ChangeLog. Also updated code a bit to remove begin/end pattern to array index to match better to coding style guidelines.
Created attachment 134722 [details] Vertical alternate glyph (GSUB) support for OpenTypeVerticalData Updated ChangeLog and a couple of oob reads fixes.
Created attachment 134970 [details] Vertical alternate glyph (GSUB) support for OpenTypeVerticalData Since no one has looked into this yet, I'm moving one line to avoid conflict with other patches working in parallel. No real code changes. I'll r? again after EWS is done.
Comment on attachment 134970 [details] Vertical alternate glyph (GSUB) support for OpenTypeVerticalData View in context: https://bugs.webkit.org/attachment.cgi?id=134970&action=review Drive-by comments. (I'm not a reviewer) > Source/WebCore/ChangeLog:11 > + Reviewed by NOBODY (OOPS!). nit: We usually put "Reviewed by" line before the description. > 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). > 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). > Source/WebCore/platform/graphics/opentype/OpenTypeVerticalData.cpp:181 > + uint16_t countSubTable = subTableCount; Why do we need this substitution? > Source/WebCore/platform/graphics/opentype/OpenTypeVerticalData.cpp:186 > + for (int i = 0; i < countSubTable; ++i) { int -> uint16_t? > Source/WebCore/platform/graphics/opentype/OpenTypeVerticalData.cpp:187 > + const SubstitutionSubTable* sub = validatedPtr<SubstitutionSubTable>(subTableOffsets[i], buffer); Please don't use abbreviation variable names (The same applies hereafter). > 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? > 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? > Source/WebCore/platform/graphics/opentype/OpenTypeVerticalData.cpp:240 > + LOG_ERROR("LookupType %d not supported", lookupType); Is it unlikely that "lookupType != 1"?
Comment on attachment 134970 [details] Vertical alternate glyph (GSUB) support for OpenTypeVerticalData 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.
Created attachment 137513 [details] Reflected changes from Kenichi's review
Thank you for revising the patch. There are still a lot of abbreviated variable names. Please not use such name. We dislike variable names such as "s1" or "c1". (In reply to comment #9) > > 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? That sounds all platform could have similar validation code. If we validate here, we can avoid such duplication. However, I'm ok not validating here if you think checking OOB read is enough. > >> 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. You should add an comment then. > >> 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. > Then, don't we need functions for followings? validatedPtr<FeatureTable>(features[index].featureOffset, buffer); validatedPtr<LangSysTable>(langSysRecords[0].langSysOffset, buffer); validatedPtr<ScriptTable>(scripts[i].scriptOffset, buffer); > >> 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. I see. Thank you for detailed explanation. LOG_ERROR() looks too strong to me, though.
Comment on attachment 134970 [details] Vertical alternate glyph (GSUB) support for OpenTypeVerticalData Cleared review? from obsolete attachment 134970 [details] so that this bug does not appear in http://webkit.org/pending-review. If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).
Comment on attachment 137513 [details] Reflected changes from Kenichi's review cancelled r? to work on Kenichi's comments
(In reply to comment #11) > Thank you for revising the patch. There are still a lot of abbreviated variable names. Please not use such name. We dislike variable names such as "s1" or "c1". Thank you, I'm working on. > (In reply to comment #9) > > > 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? > > That sounds all platform could have similar validation code. If we validate here, we can avoid such duplication. However, I'm ok not validating here if you think checking OOB read is enough. Yeah, I prefer doing it in a separate layer because some platforms (specifically Win32) provides table-level API for OpenType, so analyzing and checking directory structure is redundant. If there were more than one platform that doesn't support table-level API, it makes sense to have a common class that reads a raw OpenType file, check directory structure and check-sum, and extract tables as needed, but platforms should be able to choose whether to use the common class or not. Just like Mac doesn't need this class because it has even higher level APIs. > > >> 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. > > You should add an comment then. I started doing this, but then figured out that the comment appear all over the place because all fields in all OpenType tables are in big-endian and therefore conversion is implied. I guess we don't want the same comment appearing 20-30 times in a file, do we? Should I put one comment before "TableBase" struct? > > >> 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. > > > > Then, don't we need functions for followings? > > validatedPtr<FeatureTable>(features[index].featureOffset, buffer); > validatedPtr<LangSysTable>(langSysRecords[0].langSysOffset, buffer); > validatedPtr<ScriptTable>(scripts[i].scriptOffset, buffer); They're nested structs, so they don't ihnerit from TableBase where validateOffset is implemented, and I think they're local enough not to worry about. Does this sound reasonable? > > >> 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. > > I see. Thank you for detailed explanation. LOG_ERROR() looks too strong to me, though. Okay, I'll remove them.
Created attachment 138058 [details] Reflected changes from Kenichi's review Changes from the last patch: * Replaced short variable names to long ones * Removed LOG_ERROR for fonts we do not support
Comment on attachment 138058 [details] Reflected changes from Kenichi's review View in context: https://bugs.webkit.org/attachment.cgi?id=138058&action=review Is it possible to write a GTest style unittest for this code to verify that different malformed inputs are rejected? For example, for a chromium build, you could add a test to Source/WebKit/chromium/tests/. > Source/WebCore/ChangeLog:9 > + This patch adds support for reading 'GSUB' OpenType table to get > + vertical alternate glyphs. Can you include a link to the OpenType spec about this table? > Source/WebCore/platform/graphics/opentype/OpenTypeVerticalData.cpp:51 > +enum { > + DefaultScriptTag = OT_MAKE_TAG('D', 'F', 'L', 'T'), > +}; This doesn't need to be an enum does it? It could just be a uint32_t. I'm not sure why the tags above are in an enum either. > Source/WebCore/platform/graphics/opentype/OpenTypeVerticalData.cpp:55 > +enum { > + VertFeatureTag = OT_MAKE_TAG('v', 'e', 'r', 't'), > +}; Also doesn't need to be an enum. > Source/WebCore/platform/graphics/opentype/OpenTypeVerticalData.cpp:59 > + static bool isValidEnd(const SharedBuffer& buffer, const void* p) Please use a longer variable name than |p|. position would be ok. > Source/WebCore/platform/graphics/opentype/OpenTypeVerticalData.cpp:61 > + size_t offset = reinterpret_cast<const char*>(p) - buffer.data(); Just to be safe should we verify that p >= buffer.data()? > Source/WebCore/platform/graphics/opentype/OpenTypeVerticalData.cpp:65 > + template <typename T> static const T* validatePtr(const SharedBuffer& buffer, const void* p) Please use a longer variable name than |p|. > Source/WebCore/platform/graphics/opentype/OpenTypeVerticalData.cpp:75 > + return validatePtr<T>(buffer, reinterpret_cast<const int8_t*>(this) + offset); Is it possible for this + offset to overflow? I guess if we do the extra check mentioned above for line 61 it would be OK. > Source/WebCore/platform/graphics/opentype/OpenTypeVerticalData.cpp:185 > + switch (lookupType) { > + case 1: // Single Substitution Subtable Do you need a default: for this switch statement? I think some compilers will complain if it's missing. If we only care about case 1, maybe we should early return if lookupType != 1? > Source/WebCore/platform/graphics/opentype/OpenTypeVerticalData.cpp:194 > + switch (substitution->substFormat) { > + case 2: { // Single Substitution Format 2 Maybe use an if statement here too? > Source/WebCore/platform/graphics/opentype/OpenTypeVerticalData.cpp:211 > + break; } Nit: Closing } goes on a new line and lines up with the left side of 'case'. > Source/WebCore/platform/graphics/opentype/OpenTypeVerticalData.cpp:219 > + for (uint16_t i = 0, indexTo = 0; i < countRange; i++) { Nit: ++i > Source/WebCore/platform/graphics/opentype/OpenTypeVerticalData.cpp:222 > + if (indexTo + (fromEnd - from) > countTo) Nit: accidental double space? > Source/WebCore/platform/graphics/opentype/OpenTypeVerticalData.cpp:261 > + for (uint16_t i = 0; i < count; i++) { Nit: ++i > Source/WebCore/platform/graphics/opentype/OpenTypeVerticalData.cpp:342 > + for (uint16_t i = 0; i < count; i++) { Nit: ++i > Source/WebCore/platform/graphics/opentype/OpenTypeVerticalData.cpp:363 > + const ScriptTable* s = defaultScript(buffer); > + if (!s) Nit: Rename s to scriptTable > Source/WebCore/platform/graphics/opentype/OpenTypeVerticalData.cpp:400 > + const FeatureTable* vert = feature(OpenType::VertFeatureTag, buffer); > + const LookupList* lookups = lookupList(buffer); > + return vert && lookups && vert->getGlyphSubstitutions(lookups, map, buffer); Nit: If vert is null, I would early return before calling lookupList. Also, vert should be verticalFeatureTable or something rather than an abbreviation.
agl may be able to provide an informal review as well.
Created attachment 152462 [details] Reflected items from Tony's review All items in the review are fixed. Overflow is unlikely because the offset is uint16_t, but theoretically possible and fixed as advised. Added gtest style unittest at WebKit/chromium/tests/OpenTypeVerticalDataTest.cpp for testing pointer validations.
Attachment 152462 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebKit/chromium/tests/OpenTypeVerticalDataTest.cpp:32: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 152463 [details] Reflected items from Tony's review
Comment on attachment 152463 [details] Reflected items from Tony's review * All items in the Tony's review are fixed. * Overflow in TableBase::validateOffset is unlikely because the offset is uint16_t, but theoretically possible and fixed as advised. * Added gtest style unittest at WebKit/chromium/tests/OpenTypeVerticalDataTest.cpp for testing pointer validations.
Comment on attachment 152463 [details] Reflected items from Tony's review Clearing flags on attachment: 152463 Committed r122788: <http://trac.webkit.org/changeset/122788>
All reviewed patches have been landed. Closing bug.