[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


Kenichi Ishibashi <bashi at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |bashi at chromium.org,
                   |                            |hyatt at apple.com,
                   |                            |mitz at webkit.org




--- Comment #11 from Kenichi Ishibashi <bashi at chromium.org>  2012-04-17 15:36:46 PST ---
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.

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