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

Thu Apr 19 17:14:57 PDT 2012

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

--- Comment #14 from Koji Ishii <kojiishi at gmail.com>  2012-04-19 17:14:57 PST ---
(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.

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