Google has published their OpenType sanitizer library: http://code.google.com/p/ots/ This library essentially validates OpenType fonts on a table-by-table basis, reconstructing each on the fly. Chromium tracking bug: http://code.google.com/p/chromium/issues/detail?id=17818 Integration into WebKit: https://bugs.webkit.org/show_bug.cgi?id=31106 The question here is whether this revalidation is worthwhile or whether it's just enforcing spec-correctness that doesn't actually guarantee safety.

I'm not sure if this is the right way to go. You could have a perfectly valid font that triggers bugs in the underlying shaper or rasterizer, so you haven't really reduced the attack surface. Assuming you're doing shaping and rasterization in a sandboxed content process, I don't really see that this buys you much.

I guess I changed my mind. I propose we sanitize all downloaded fonts before we pass them to any platform font code. We could use the sanitizer at http://code.google.com/p/ots/ as a starting point. Sanitization would (among other things) strip all tables that are not known to be needed for glyph measurement and rasterization. We would continue to provide full tables to Harfbuzz.

One thing to note is that OTS won't work "as is" with Gecko because it uses STL containers, so the code will require a certain amount of reworking to use Gecko containers instead.

We've started using some STL, bug 556699 tracks ensuring that various headers are safe for our use.

This could help work around a lot of platform-level font issues.

This is the OTS source from http://code.google.com/p/ots (svn r.35). bsmedberg, could you advise in particular regarding the use of STL here?

Minor fixes needed to resolve compilation failures when this is built as part of the mozilla build. (I'll file an issue in the Chromium tracker about these.)

This applies the OTS sanitizer to all downloaded fonts. It causes several reftest failures at present - this is expected because this patch causes several of our current test fonts to be rejected (it is stricter than our existing validation on some issues).

(In reply to comment #8) > Created attachment 470213 [details] [diff] [review] > part 1a - fixes to OTS source > > Minor fixes needed to resolve compilation failures when this is built as part > of the mozilla build. (I'll file an issue in the Chromium tracker about these.) FTR - filed http://code.google.com/p/chromium/issues/detail?id=53821.

Comment on attachment 470212 [details] [diff] [review] part 1 - OTS source STL stuff is now fully in cjones court, not mine. Whew.

We have a number of tests of "slightly broken" fonts which will begin failing when OTS lands. It also affects font-features tests on Linux because it strips the GSUB table, etc.; we preserve these tables for the new harfbuzz backend to use, but they don't get passed to the existing Pango font code.

Comment on attachment 471830 [details] [diff] [review] part 3 - integrate the OTS sanitizer into gfxUserFonts (refreshed) +// Find the GDEF, GSUB, GPOS tables in aFontData (if present) +// and cache copies in the given font entry. +// The sfnt table directory has already been sanity-checked by +// gfxFontUtils::ValidateSFNTHeaders before this is called, +// so we can assume entries are valid. +static void +CacheLayoutTables(const PRUint8* aFontData, PRUint32 aLength, + gfxFontEntry* aFontEntry) +{ + const SFNTHeader *sfntHeader = reinterpret_cast<const SFNTHeader*>(aFontData); + PRUint16 numTables = sfntHeader->numTables; + + // table directory entries begin immediately following SFNT header + const TableDirEntry *dirEntry = + reinterpret_cast<const TableDirEntry*>(aFontData + sizeof(SFNTHeader)); + + while (numTables-- > 0) { + switch (dirEntry->tag) { + case TRUETYPE_TAG('G','D','E','F'): + case TRUETYPE_TAG('G','P','O','S'): + case TRUETYPE_TAG('G','S','U','B'): { + nsTArray<PRUint8> buffer; + if (!buffer.AppendElements(aFontData + dirEntry->offset, + dirEntry->length)) { + NS_WARNING("failed to cache font table - out of memory?"); + break; + } + aFontEntry->PreloadFontTable(dirEntry->tag, buffer); + } + break; + + default: + if (dirEntry->tag > TRUETYPE_TAG('G','S','U','B')) { + // directory entries are required to be sorted, + // so we can terminate as soon as we find a tag > 'GSUB' + numTables = 0; + } + break; + } + ++dirEntry; + } +} + if (mFontEntry->IsUserFont() && !mFontEntry->IsLocalUserFont()) { + // for downloaded fonts, there may be layout tables cached in the entry + // even though they're absent from the sanitized platform font + return mFontEntry->GetFontTable(aTag); + } This seems really peculiar, we sanitize the font but not some tables so for those we pull them out of a pre-populated cache? I realize that this is because harfbuzz is doing its own sanitizing of GDEF/GSUB/GPOS but I think we should just add stub routines to the sanitizer that copy these tables since harfbuzz is going handle the sanitizing. Eventually the sanitizer will support sanitizing these tables also (separate bug? Chromium bug?). + if (mFontEntry->IsUserFont() && !mFontEntry->IsLocalUserFont()) { + // for downloaded fonts, there may be layout tables cached in the entry + // even though they're absent from the sanitized platform font + return mFontEntry->GetFontTable(aTag); + } More of the same peculiarity, plus this logic seems to only be in the DirectWrite and MacFont versions but not the GDI/Linux versions. +#define GFX_DOWNLOADABLE_FONTS_SANITIZE "gfx.downloadable_fonts.sanitize" I actually don't see a need for this, I think we should go without this. But I do think we should have a control that governs use of platform shapers for downloadable fonts - gfx.downloadable_fonts.allow_platform_shaping perhaps? That way we could control this per-platform (e.g. off on OSX, on under Windows?). If we're explicitly going to disable downloadable Indic fonts (no platform shaper use for downloadable fonts and harfbuzz doesn't do Indic yet) then we need to be more proactive about warning folks in India about this. > // Unwrap/decompress or otherwise munge the downloaded data > // to make a usable sfnt structure. > // This may cause aFontData to point to a new buffer, or be NULL. > aFontData = PrepareOpenTypeData(aFontData, &aLength); > - if (aFontData && > - gfxFontUtils::ValidateSFNTHeaders(aFontData, aLength)) { > - // Here ownership of aFontData is passed to the platform, > - // which will delete it when no longer required > - fe = gfxPlatform::GetPlatform()->MakePlatformFont(pe, > - aFontData, > - aLength); > - if (fe) { > - if (pe->mFeatureSettings) { > - fe->mFeatureSettings = new nsTArray<gfxFontFeature>; > - fe->mFeatureSettings->AppendElements(*pe->mFeatureSettings); > + > + if (aFontData) { > + PRUint32 saneLen; > + const PRUint8 *saneData; > + > + if (gfxPlatform::GetPlatform()->SanitizeDownloadedFonts()) { > + saneData = SanitizeOpenTypeData(aFontData, aLength, saneLen); The OTS code already does WOFF handling as part of the unpacking process, that seems far more efficient than first unbundling the data and then sanitizing it again since some tables will be dropped and making an extra copy of the font data doesn't help performance. I think we should just use the OTS WOFF handling code and obsolete the existing WOFF loading code. (Note: this only works if sanitizer is essentially "always on"). + gfxFontUtils::ValidateSFNTHeaders(saneData, + saneLen)) { For fonts that have been run through the sanitizer, what's the point of calling this? If there's something being checked here we should probably add it to ots and trim out this code.

(In reply to comment #15) > This seems really peculiar, we sanitize the font but not some tables > so for those we pull them out of a pre-populated cache? I realize > that this is because harfbuzz is doing its own sanitizing of > GDEF/GSUB/GPOS but I think we should just add stub routines to the > sanitizer that copy these tables since harfbuzz is going handle the > sanitizing. No, that will fail because the presence of bad OpenType layout tables in the font we pass to Windows or OS X can lead to crashes in the platform font code. Unless OTS is enhanced to fully sanitize these, we must NOT pass them through unchecked to the platform. > Eventually the sanitizer will support sanitizing these > tables also (separate bug? Chromium bug?). Maybe so, but until then, we need to manage them separately so that they're available to harfbuzz but are not part of the sfnt that we hand off to the platform rasterizer. > + if (mFontEntry->IsUserFont() && !mFontEntry->IsLocalUserFont()) { > + // for downloaded fonts, there may be layout tables cached in the > entry > + // even though they're absent from the sanitized platform font > + return mFontEntry->GetFontTable(aTag); > + } > > More of the same peculiarity, plus this logic seems to only be in the > DirectWrite and MacFont versions but not the GDI/Linux versions. The GDI version is always reading from the gfxFontEntry cache, as there's no GetFontTable override in the gfxGDIFont class, AFAIR. And on Linux, we don't have harfbuzz support yet, so the question doesn't arise! > > +#define GFX_DOWNLOADABLE_FONTS_SANITIZE "gfx.downloadable_fonts.sanitize" > > I actually don't see a need for this, I think we should go without > this. Possibly, but I figured people might want the option to use fonts that are blocked by OTS for some reason (it's not necessarily infallible!). This would also let people opt to retain the OpenType layout tables in fonts for Uniscribe or Pango, for example, at the cost of not having sanitization, if they want the extra functionality and are willing to take the risk involved.

If the pref is useful for testing, it might be worth having, but otherwise it's probably not worth it. Hardly anyone is going to use it.

(In reply to comment #14) > We have a number of tests of "slightly broken" fonts which will begin failing > when OTS lands. If we're deliberately testing broken fonts, shouldn't we just make these tests != tests, or remove them, since we no longer expect them to pass?

Comment on attachment 470212 [details] [diff] [review] part 1 - OTS source I'm rubberstamping this.

(In reply to comment #17) > If the pref is useful for testing, it might be worth having, but otherwise it's > probably not worth it. Hardly anyone is going to use it. I agree that hardly anyone is going to use it - but I'd guess that's true for MANY of our prefs! Personally, I'd favor initially landing OTS with a pref that can be used to turn it off, so that it's easy to test and compare behavior (and performance) with and without the sanitization step, and so that we can easily bypass OTS in case we find it introduces new, unexpected problems. Once it's been in place (and enabled by default, of course) for a while, we might remove the pref and make sanitization "compulsory".

OK, I buy that. What about comment #18?

Yes, in principle that makes sense. It's worth noting that some of these cases relate to "slightly broken" fonts where we had a bug about a certain font NOT working (because our validation rejected it), and we decided the brokenness was safe/harmless and modified our code to accept the font. But OTS is rejecting the font, so that means we'll "regress" those bugs when we introduce the sanitizer. (See bugs 483459 and 534352, for example.) So does that mean we'll have to reopen those bugs and then resolve them as WONTFIX or INVALID, or what?

(In reply to comment #15) > The OTS code already does WOFF handling as part of the unpacking > process, that seems far more efficient than first unbundling the data > and then sanitizing it again since some tables will be dropped and > making an extra copy of the font data doesn't help performance. I > think we should just use the OTS WOFF handling code and obsolete the > existing WOFF loading code. (Note: this only works if sanitizer is > essentially "always on"). As indicated above, I'd prefer to initially land OTS with a pref that allows it to be turned off, and this seems like the simplest way to hook in the sanitizer in that case. If/when we decide we're happy to remove that pref, we can also drop the separate WOFF decoder and let OTS handle everything. > + gfxFontUtils::ValidateSFNTHeaders(saneData, > + saneLen)) { > > For fonts that have been run through the sanitizer, what's the point > of calling this? If there's something being checked here we should > probably add it to ots and trim out this code. Assuming we land OTS with a pref that makes it possible to bypass the sanitizer, I'd like to leave our existing checks untouched for the time being. Yes, that means there's some redundancy in the normal case, but I don't think it's significant in comparison to the overall process of downloading and activating the font. We can streamline this later at the point where we make OTS a non-optional part of the downloadable font management process.

(In reply to comment #22) > It's worth noting that some of these cases relate to "slightly broken" fonts > where we had a bug about a certain font NOT working (because our validation > rejected it), and we decided the brokenness was safe/harmless and modified our > code to accept the font. But OTS is rejecting the font, so that means we'll > "regress" those bugs when we introduce the sanitizer. (See bugs 483459 and > 534352, for example.) So does that mean we'll have to reopen those bugs and > then resolve them as WONTFIX or INVALID, or what? We're going to have to do that for some subset of fonts, yes. I think we should try to avoid making OTS more lenient unilaterally; staying compatible with Chrome is highly desirable. I suggest that where we think OTS should be more lenient, we file bugs upstream against it.

> > This seems really peculiar, we sanitize the font but not some > > tables so for those we pull them out of a pre-populated cache? I > > realize that this is because harfbuzz is doing its own sanitizing > > of GDEF/GSUB/GPOS but I think we should just add stub routines to > > the sanitizer that copy these tables since harfbuzz is going > > handle the sanitizing. > > No, that will fail because the presence of bad OpenType layout > tables in the font we pass to Windows or OS X can lead to crashes in > the platform font code. Unless OTS is enhanced to fully sanitize > these, we must NOT pass them through unchecked to the platform. With harfbuzz enabled the only time we use platform shapers is for complex scripts that almost always require these tables. So we should just disable the platform shapers rather than passing through denuded fonts that won't render correctly for the scripts for which they are intended. Note that disabling platform shapers would effectively break downloadable fonts in India. Speaking with Chris Blizzard, I don't think we can ship that way. In the Windows case, I think we can live with Uniscribe use of "unsanitized" GSUB/GPOS/GDEF tables but not on OSX unless Apple API's suddenly get a lot of 10.6.x update love. That's why I think there should be a pref for controlling platform shaper use that has different defaults for different platforms. > > + gfxFontUtils::ValidateSFNTHeaders(saneData, > > + saneLen)) { > > > > For fonts that have been run through the sanitizer, what's the point > > of calling this? If there's something being checked here we should > > probably add it to ots and trim out this code. > > Assuming we land OTS with a pref that makes it possible to bypass the > sanitizer, I'd like to leave our existing checks untouched for the time being. > Yes, that means there's some redundancy in the normal case, but I don't think > it's significant in comparison to the overall process of downloading and > activating the font. We can streamline this later at the point where we make > OTS a non-optional part of the downloadable font management process. This is a simple code-structure issue, this check is not needed if OTS is being used. Just move it to the else-clause of the if (sanitize) block. > As indicated above, I'd prefer to initially land OTS with a pref > that allows it to be turned off, and this seems like the simplest > way to hook in the sanitizer in that case. If/when we decide we're > happy to remove that pref, we can also drop the separate WOFF > decoder and let OTS handle everything. Hmmm, okay. But I think we should have two clearly separate paths, one that uses OTS and the OTS WOFF loading and another that uses our existing code. Additionally, I think we should change the interface to ots::Process to take in a name parameter that would be used for the family/fullname/psname in the name table. Then we can eliminate the use of the additional rebuild of the name table that occurs on Windows when OTS is used.

(In reply to comment #15) > The OTS code already does WOFF handling as part of the unpacking > process, that seems far more efficient than first unbundling the data > and then sanitizing it again since some tables will be dropped and > making an extra copy of the font data doesn't help performance. I > think we should just use the OTS WOFF handling code and obsolete the > existing WOFF loading code. (Note: this only works if sanitizer is > essentially "always on"). This isn't as simple as it sounds, because the OTS process will discard the OpenType layout tables that we need for harfbuzz. So we'd need to keep extra code to retrieve these from the WOFF file and decompress them separately.

Revised this so that it relies on OTS for WOFF decoding (when sanitizer is enabled). The non-sanitization code path is now well separated and will be easy to pull out once we're comfortable forcing the use of OTS for all downloaded fonts. For initial landing and testing purposes, I'd like us to retain the option to disable the sanitizer.

This implements an extension to the OTS library to provide an option to preserve (unchecked) the OpenType Layout tables GDEF, GPOS, GSUB. This will allow us to continue supporting downloadable Indic fonts (for example) on Windows and Linux using platform shaping APIs. The expectation is that this is a temporary solution, only needed until either (a) OTS gains real support for the OpenType Layout tables, or (b) we get full non-Latin shaping supported via the harfbuzz shaper, which handles the layout tables separately from the main sanitized sfnt. Therefore, there is no intention to push this upstream to OTS, it's purely for internal use in Gecko as an interim solution.

This implements a preference setting to control whether the OpenType Layout tables are preserved in the sfnt during sanitization. The default setting is to drop the OTL tables on OS X (where we have seen a number of OS font bugs due to fuzzed OpenType tables, and they are not useful for Indic shaping), and preserve them on Windows and Linux (so that downloadable Indic fonts will continue to work with Uniscribe and Pango). This preference will eventually become obsolete, once we have full support for all scripts in Harfbuzz and/or real support for OTL sanitization in the OTS library.

Did you really mean to obsolete the reftests?

(In reply to comment #30) > Did you really mean to obsolete the reftests? There'll be an update coming - several of them are affected as a result of preserving the OpenType tables on Windows & Linux, so the patch is obsolete, yes.

I think this is now up-to-date with the effects of landing the full set of sanitizer patches. Note that there are several @font-face based tests that begin failing because the sanitizer rejects some of our test fonts. We need to investigate the cause of each of those failures, and fix the fonts so as to restore the functionality of the tests. I have filed bugs 600821, 601099 and 601110 for these.

If we fix bug 600821, bug 601099, bug 601110 and bug 601487 at the same time as this, we won't need "part 6" as those patches resolve all the reftest issues that arise from landing OTS.

Sorry for the delay in reviewing these, lots of other distractions. I'll write up more specific comments on the patch tomorrow. I'm concerned that we're doing a lot of copying of font data during the load process with ots enabled, I think several of these result from the structure of the code and can be trimmed out. Right now we have worst-case six copies of the font data: 1. Original font data from the stream 2. Parsed into individual structs (ots parse) 3. Copied into otsstream (ots serialize) 4. [woff] Copied when buffer expanded during serialization (ots serialize) 5. Result copied into a new buffer (SanitizeOpenTypeData) 6. [gdi-cff, dwrite] Copied on rename The last three can be trimmed out I think. Copy (4) can be eliminated by using an initial size of (2 * length) when the data is WOFF data. To eliminate copy (5) we should allow a way of detaching the memory from the OTSStream object rather than copying it. Copy (6) can be eliminated by adding another input parameter to ots::Process to pass in the font name, which the code in name.cc would use when serializing the new name table. The current ots code uses the same names for all fonts which seems a bit dicey to me.

It's true that we end up copying the data (or parts of it) several times; OTOH, I doubt this is very significant in comparison to the download (or read-from-cache) time. It's written this way because I was reluctant to modify the ots APIs unless we can be reasonably confident the changes will be welcomed upstream as well; we don't want to be maintaining a fork of the ots codebase. So I've tried to work with the code as-is, as far as possible. We could eliminate copy (5) if we implement our own subclass of ots::OTSStream, similar to ots::ExpandingMemoryStream but using NS_* allocation instead, so that ownership of the buffer can be passed over to the existing gfx code. And yes, using a larger initial allocation could eliminate (4) in most cases. I'd prefer not to modify the ots::Process() API unilaterally, but we could file an issue on the Google code site and see if they are willing to consider this as an enhancement.

I've adjusted this to reduce copying of font data, by providing our own version of OTSStream with a detachable buffer, and initializing the buffer to 2*length for WOFF fonts.

Comment on attachment 479733 [details] [diff] [review] part 4 - patch OTS to allow OpenType Layout tables to be preserved > +// This is used to tell the GDEF/GPOS/GSUB parsers whether to preserve the > +// OpenType Layout tables (**without** any checking). > +// Using a global for this is a total hack, not intended as a permanent > +// solution, but it minimizes impact on the rest of the OTS code. > +extern bool g_preserve_otl; > + > -bool Process(OTSStream *output, const uint8_t *data, size_t length) { > +bool Process(OTSStream *output, const uint8_t *data, size_t length, bool preserveOTL) { > OpenTypeFile header; > if (length < 4) { > return OTS_FAILURE(); > } > > + g_preserve_otl = preserveOTL; > + Make this part of the OpenTypeFile struct, that gets passed to the code that needs to reference this.

Comment on attachment 479735 [details] [diff] [review] part 5 - implement preference to control the preservation of OTL tables What are the examples of Apple bugs related to GDEF/GSUB/GPOS handling? Looking over the long list of Apple font bugs, I don't see any that are specifically related to OTL tables but maybe I missed one.

Comment on attachment 480763 [details] [diff] [review] part 3, v3 - integrate the OTS sanitizer into gfxUserFonts (revised) +// Find the GDEF, GSUB, GPOS tables in aFontData (if present) +// and cache copies in the given font entry. +// The sfnt table directory has already been accepted by the OTS +// sanitizer before this is called, so we can assume entries are valid. +static void +CacheLayoutTablesFromSFNT(const PRUint8* aFontData, PRUint32 aLength, + gfxFontEntry* aFontEntry) *sigh* The only reason these are needed are to support the harfbuzz / ots strips otl tables case and I think they should be marked as such. This code is really ugly and it's not really "caching" these tables, it's tucking them off to the side. Once the sanitizer has GDEF/GSUB/GPOS support these methods should die an ignominious death. > I'd prefer not to modify the ots::Process() API unilaterally, but we > could file an issue on the Google code site and see if they are > willing to consider this as an enhancement. If the changes are small I don't see a huge problem in managing the diff's. We're still doing extra unnecessary work in the Windows case; after sanitization the Windows MakePlatformFont still swizzles the table data around, this should really be eliminated. If you'd prefer to land this patch ASAP, that's fine, make a follow-on bug and assign it to me.

(In reply to comment #37) > > +extern bool g_preserve_otl; > Make this part of the OpenTypeFile struct, that gets passed to the > code that needs to reference this. Ok - I considered that, but was a bit reluctant to touch their struct. I'm fine with doing it that way if you prefer, though. It is slightly less ugly, I agree. :)

(In reply to comment #39) > Once the sanitizer has > GDEF/GSUB/GPOS support these methods should die an ignominious death. Agreed! This is a workaround for the current incompleteness. (There's an open Chromium bug about adding G*** table support already, so it's a known issue.)

Comment on attachment 480090 [details] [diff] [review] part 6 - update reftest manifests due to OTS sanitization of test fonts Clearing out the review since I've gone through the dependent bugs.

Comment on attachment 470212 [details] [diff] [review] part 1 - OTS source They're using a subset of the STL data structures we use; OK on that. For completeness's sake, you need add these headers cassert climits cstdarg cstdio cstdlib cstring limits utility to the file http://mxr.mozilla.org/mozilla-central/source/config/stl-headers. (If that's not set up properly, the code should fail to compile, but no need to stress-test that system ;) ).

(In reply to comment #45) > For completeness's sake, you need add these headers > > cassert > climits > cstdarg > cstdio > cstdlib > cstring > limits > utility > > to the file http://mxr.mozilla.org/mozilla-central/source/config/stl-headers. > (If that's not set up properly, the code should fail to compile, but no need to > stress-test that system ;) ). Ok, I'll include that in the patch for part 2, the build system integration, although it's been compiling quite happily (both locally and on tryserver) without it. :)

http://hg.mozilla.org/mozilla-central/rev/6d49509b28a4 (part 1) http://hg.mozilla.org/mozilla-central/rev/88ff5f0a9681 (part 1a) http://hg.mozilla.org/mozilla-central/rev/ff826d1ba112 (part 2) http://hg.mozilla.org/mozilla-central/rev/9120bc25cfb7 (part 3) http://hg.mozilla.org/mozilla-central/rev/4756e676e0fb (part 4) http://hg.mozilla.org/mozilla-central/rev/2fa4872fffeb (part 5)

Aside from an MSVC 2010 build complication (see bug 602558), this seems to have landed without problems on trunk. Backporting to 1.9.2 should resolve many font-fuzzbugs that have been reported there, especially on OS X, but requires some reworking as the font code has changed significantly. I'll work on a patch for the 1.9.2 branch and post here when ready.

> I'll work on a patch for the 1.9.2 branch and post here when ready. Please do! Exciting to see this hit the tree. I assume this is not worth back-porting to the 1.9.1 branch? It's almost EOL plus no WOFF support (though it did have @font-face).

Comment on attachment 480763 [details] [diff] [review] part 3, v3 - integrate the OTS sanitizer into gfxUserFonts (revised) What destroys the hb_blob_t created in PreloadFontTable?

(In reply to comment #50) > Comment on attachment 480763 [details] [diff] [review] > part 3, v3 - integrate the OTS sanitizer into gfxUserFonts (revised) > > What destroys the hb_blob_t created in PreloadFontTable? Errr. Good question. As far as I can see offhand: nothing. So we leak. I suppose we could make the gfxFontEntry destructor take care of this for now.

(In reply to comment #51) > (In reply to comment #50) > > What destroys the hb_blob_t created in PreloadFontTable? > > Errr. Good question. As far as I can see offhand: nothing. So we leak. Filed bug 605872 on this issue.

This is a slightly later rev of the OTS code than was initially landed on trunk, and includes the fixes that were landed as "part 1a" there.

Marking for re-review as it's somewhat different from the trunk patch.

Basically the same as the trunk patch, but simplified because (a) we don't have to worry about saving aside font tables for harfbuzz to use (and hence the issue noted in comment 50 doesn't arise here), and (b) the downloadable-fonts prefs are not live-apply on 1.9.2.

This combines parts 4 and 5 from the trunk landing - allow us to preserve OTL tables in the sanitized font, for use by uniscribe and pango. The preference means we (or users) can easily turn this off if a significant problem is found.

Comment on attachment 484727 [details] [diff] [review] part 1 (for 1.9.2) - OTS source code from google (svn r.38) Does it make sense to approve this part when the other four haven't had reviews yet? Can this land as a separate piece?

(In reply to comment #57) > Comment on attachment 484727 [details] [diff] [review] > part 1 (for 1.9.2) - OTS source code from google (svn r.38) > > Does it make sense to approve this part when the other four haven't had reviews > yet? Can this land as a separate piece? No rush until all 4 parts are ready to go (though pre-landing the OTS source would be harmless - it won't even get compiled until part 2 goes in, and won't get linked to Gecko until part 3). In practice, though, I wasn't intending to push anything until all the parts are (re-)reviewed and approved.

3.5 didn't support WOFF, but still supported downloadable OpenType fonts right? We probably do want to consider whether to take this on 3.5 if it's not EOL when we land this on 3.6.

(In reply to comment #59) > 3.5 didn't support WOFF, but still supported downloadable OpenType fonts right? > We probably do want to consider whether to take this on 3.5 if it's not EOL > when we land this on 3.6. Some reworking of the "part 3" patch would be needed; the font-download code changed enough in 3.6 (with the addition of WOFF support) that merging won't be entirely trivial. But I'll work on a patch for 1.9.1 if we decide it's wanted.

Comment on attachment 484727 [details] [diff] [review] part 1 (for 1.9.2) - OTS source code from google (svn r.38) Approved for 1.9.2.13, a=dveditz for release-drivers

Comment on attachment 484728 [details] [diff] [review] part 2 (for 1.9.2) - add OTS lib to the build process Approved for 1.9.2.13, a=dveditz for release-drivers

Comment on attachment 484731 [details] [diff] [review] part 3 (for 1.9.2) - apply OTS sanitizer to downloaded fonts Approved for 1.9.2.13, a=dveditz for release-drivers

Comment on attachment 484732 [details] [diff] [review] part 4 (for 1.9.2) - extend OTS to preserve OTL tables and implement pref to control this Approved for 1.9.2.13, a=dveditz for release-drivers

Pushed to 1.9.2 (parts 1-4): http://hg.mozilla.org/releases/mozilla-1.9.2/rev/1b222e9d12ea http://hg.mozilla.org/releases/mozilla-1.9.2/rev/9addaf2c223d http://hg.mozilla.org/releases/mozilla-1.9.2/rev/986ec93224f0 http://hg.mozilla.org/releases/mozilla-1.9.2/rev/4e5375c001fa

standard8 landed a bustage fix for shared builds (1.9.1 will want that, too): http://hg.mozilla.org/releases/mozilla-1.9.2/rev/c99f0ba9074b It would have been nice to have had a heads-up on this landing so that our builds didn't break in the middle of the night; I could have built with this patch beforehand and had a patch for Camino ready to go (and probably have caught the shared bustage standard8 fixed, too) ;)

We're still having discussions about if we want to risk taking this back on 1.9.1...

(In reply to comment #67) > We're still having discussions about if we want to risk taking this back on > 1.9.1... Do we know an EOL date for 1.9.1? Backporting OTS would involve also backporting the changes to management of the downloaded data (from bug 507970), even though we wouldn't be adding the actual WOFF-decoding part. This is because 1.9.1 currently doesn't manage the font data separately from the streamloader that initially downloaded it, but inserting OTS into the process will require us to allocate and manage a new, separate buffer, similarly to what we did in 1.9.2 to insert WOFF decoding. We could certainly do this, just noting that it's a bit more invasive than might initially be apparent. Also note that backporting OTS would probably have the side-effect of "magically" supporting WOFF fonts on 1.9.1 unless we take specific steps to disable this, because the sanitizer includes built-in WOFF decoding. (I don't see any particular harm in that, but it might seem a bit odd for a security update to add such support.)

(In reply to comment #68) > Also note that backporting OTS would probably have the side-effect of > "magically" supporting WOFF fonts on 1.9.1 unless we take specific steps to > disable this, because the sanitizer includes built-in WOFF decoding. (I don't > see any particular harm in that, but it might seem a bit odd for a security > update to add such support.) I don't think this would be hard to change, instead of testing for the 'wOFF' tag and branching to either ProcessWOFF or ProcessTTF, the code could simply always call ProcessTTF, which would bail quickly on WOFF fonts because of the lack of a header tag.

After talking this bug over with a bunch of folks we would like to take ots back on 1.9.1. Do you think this is doable by code freeze this Thursday (2010-11-18)?

(In reply to comment #70) > After talking this bug over with a bunch of folks we would like to take ots > back on 1.9.1. > > Do you think this is doable by code freeze this Thursday (2010-11-18)? I have a WIP patch for 1.9.1 that I started previously; I'll try to finish it up today and put up for review ASAP. (Wishing we had 1.9.1 tryserver.....)

To support OTS on gecko-1.9.1, we need to revise the management of downloaded font data, similarly to what was done on trunk and 1.9.2 for WOFF support. This patch adds a new behavior to the streamloader to allow the data to be extracted by gfx. (patch has r=bzbarsky in bug 507970)

Same code as we've landed on 1.9.2, just repeating in the 1.9.1 series to keep things simple.

Simple backport of the 1.9.2 patch.

This is a backport of the 1.9.2 integration; there are additional changes because of the need to rework memory management for the downloaded font data. This also disables WOFF support in the OTS code so that we won't be adding that feature to 1.9.1 as a side-effect of sanitization.

Same as part 4 on 1.9.2, just merged to the 1.9.1 tree. I've tested this patch series locally on OS X, Windows and Linux and it seems to work as expected; I don't know how to get a useful set of unit tests for 1.9.1 from tryserver, unfortunately. We will also need to take bug 601099 and bug 601110 (fixes for reftest fonts) on the branch in order to avoid reftest failures due to the sanitizer rejecting slightly-broken fonts used in our tests.

Comment on attachment 490963 [details] [diff] [review] part 3 (for 1.9.1) - apply OTS sanitizer to downloaded fonts Looks good. One small nit, gfx code at least usually uses #if defined(xxx) or #ifdef xxx rather than #if xxx For consistency I think it would be better to use existing #ifdef style.

Pushed parts 0-4 to 1.9.1: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/3441f3d03fb8 http://hg.mozilla.org/releases/mozilla-1.9.1/rev/184db395cb7d http://hg.mozilla.org/releases/mozilla-1.9.1/rev/e2c5567c982f http://hg.mozilla.org/releases/mozilla-1.9.1/rev/9813d7496377 http://hg.mozilla.org/releases/mozilla-1.9.1/rev/f7f82f777137

branch 1.9.1: on AIX 4.3.3 I got following compilation error: gmake[5]: Entering directory `/home/ulink/src/comm-1.9.1/mozilla/obj-fx35-aix43/gfx/ots/src' cff.cc Building deps for /home/ulink/src/comm-1.9.1/mozilla/gfx/ots/src/cff.cc xlC_r -o cff.o -c -DMOZILLA_INTERNAL_API -D_IMPL_NS_COM -DEXPORT_XPT_API -DEXPORT_XPTC_API -D_IMPL_NS_COM_OBSOLETE -D_IMPL_NS_GFX -D_IMPL_NS_WIDGET -DIMPL_XREAPI -DIMPL_NS_NET -DIMPL_THEBES -DOSTYPE=\"AIX4.3\" -DOSARCH=AIX -DPACKAGE_VERSION="\"moz\"" -DPACKAGE_BUGREPORT="\"http://bugzilla.mozilla.org/\"" -I/home/ulink/src/comm-1.9.1/mozilla/gfx/ots/src -I/home/ulink/src/comm-1.9.1/mozilla/gfx/ots/src -I. -I../../../dist/include -I../../../dist/include/ots -I/home/ulink/src/comm-1.9.1/mozilla/obj-fx35-aix43/dist/include/nspr -I/home/ulink/src/comm-1.9.1/mozilla/obj-fx35-aix43/dist/sdk/include -qflag=w:w -DNDEBUG -DTRIMMED -O2 -qmaxmem=-1 -DMOZILLA_VERSION=\"1.9.1.16pre\" -DMOZILLA_VERSION_U=1.9.1.16pre -DAIX=1 -DHAVE_SYS_INTTYPES_H=1 -DNSCAP_DISABLE_DEBUG_PTR_TYPES=1 -DD_INO=d_ino -DSTDC_HEADERS=1 -DHAVE_ST_BLKSIZE=1 -DHAVE_SIGINFO_T=1 -DHAVE_INT16_T=1 -DHAVE_INT32_T=1 -DHAVE_INT64_T=1 -DHAVE_INT64=1 -DHAVE_UINT=1 -DHAVE_UINT_T=1 -DHAVE_UINT16_T=1 -DHAVE_DIRENT_H=1 -DHAVE_MEMORY_H=1 -DHAVE_UNISTD_H=1 -DHAVE_NL_TYPES_H=1 -DHAVE_MALLOC_H=1 -DHAVE_X11_XKBLIB_H=1 -DHAVE_SYS_STATVFS_H=1 -DHAVE_SYS_STATFS_H=1 -DHAVE_LIBC_R=1 -DHAVE_LIBM=1 -DHAVE_LIBDL=1 -DHAVE_LIBC_R=1 -DFUNCPROTO=15 -DHAVE_XSHM=1 -DHAVE_FT_BITMAP_SIZE_Y_PPEM=1 -DHAVE_FT_GLYPHSLOT_EMBOLDEN=1 -DHAVE_FT_LOAD_SFNT_TABLE=1 -DHAVE_FT_SELECT_SIZE=1 -DHAVE_ARM_SIMD=1 -D_REENTRANT=1 -DHAVE_RANDOM=1 -DHAVE_STRERROR=1 -DHAVE_LCHOWN=1 -DHAVE_FCHMOD=1 -DHAVE_SNPRINTF=1 -DHAVE_MEMMOVE=1 -DHAVE_RINT=1 -DHAVE_STAT64=1 -DHAVE_LSTAT64=1 -DHAVE_TRUNCATE64=1 -DHAVE_FLOCKFILE=1 -DHAVE_LOCALTIME_R=1 -DHAVE_STRTOK_R=1 -DHAVE_RES_NINIT=1 -DHAVE_LANGINFO_CODESET=1 -DHAVE_I18N_LC_MESSAGES=1 -DMOZ_EMBEDDING_LEVEL_DEFAULT=1 -DMOZ_EMBEDDING_LEVEL_BASIC=1 -DMOZ_EMBEDDING_LEVEL_MINIMAL=1 -DMOZ_PHOENIX=1 -DMOZ_BUILD_APP=browser -DMOZ_XUL_APP=1 -DMOZ_DEFAULT_TOOLKIT=\"cairo-gtk2\" -DMOZ_X11=1 -DMOZ_WIDGET_GTK2=1 -DMOZ_ENABLE_XREMOTE=1 -DMOZ_DISTRIBUTION_ID=\"org.mozilla\" -DMOZ_PANGO=1 -DOJI=1 -DIBMBIDI=1 -DMOZ_VIEW_SOURCE=1 -DMOZ_XPINSTALL=1 -DMOZ_JSLOADER=1 -DNS_PRINTING=1 -DNS_PRINT_PREVIEW=1 -DMOZ_NO_XPCOM_OBSOLETE=1 -DMOZ_OGG=1 -DMOZ_WAVE=1 -DMOZ_SYDNEYAUDIO=1 -DMOZ_MEDIA=1 -DMOZ_XTF=1 -DMOZ_CRASHREPORTER_ENABLE_PERCENT=100 -DMOZ_MATHML=1 -DMOZ_ENABLE_CANVAS=1 -DMOZ_SVG=1 -DMOZ_UPDATE_CHANNEL=default -DMOZ_PLACES=1 -DMOZ_FEEDS=1 -DMOZ_STORAGE=1 -DMOZ_SAFE_BROWSING=1 -DMOZ_URL_CLASSIFIER=1 -DMOZ_LOGGING=1 -DMOZ_USER_DIR=\".mozilla\" -DMOZ_ENABLE_LIBXUL=1 -DHAVE_INTTYPES_H=1 -DMOZ_XUL=1 -DMOZ_PROFILELOCKING=1 -DMOZ_RDF=1 -DMOZ_MORKREADER=1 -DMOZ_DLL_SUFFIX=\".so\" -DXP_UNIX=1 -DUNIX_ASYNC_DNS=1 -D_MOZILLA_CONFIG_H_ -DMOZILLA_CLIENT /home/ulink/src/comm-1.9.1/mozilla/gfx/ots/src/cff.cc "../../../dist/include/ots/opentype-sanitiser.h", line 22.10: 1540-0836 (S) The #include file <stdint.h> is not found. "/home/ulink/src/comm-1.9.1/mozilla/gfx/ots/src/ots.h", line 35.33: 1540-0831 (S) The parameter list for the macro "OTS_WARNING" contains a syntax error at "...". gmake[5]: *** [cff.o] Error 1 gmake[5]: Leaving directory `/home/ulink/src/comm-1.9.1/mozilla/obj-fx35-aix43/gfx/ots/src' gmake[4]: *** [libs] Error 2 AIX 5.1 with a newer Compiler (IBM XLC/C++ 7.0.0.10) built branch 1.9.1 fine.

After including <sys/types.h> instead of <stdint.h> and replacing in ots.h: #define OTS_WARNING(format, args...) with #define OTS_WARNING(format,...) following error: Building deps for /home/ulink/src/comm-1.9.1/mozilla/gfx/ots/src/cmap.cc xlC_r -o cmap.o -c -DMOZILLA_INTERNAL_API -D_IMPL_NS_COM -DEXPORT_XPT_API -DEXPORT_XPTC_API -D_IMPL_NS_COM_OBSOLETE -D_IMPL_NS_GFX -D_IMPL_NS_WIDGET -DIMPL_XREAPI -DIMPL_NS_NET -DIMPL_THEBES -DOSTYPE=\"AIX4.3\" -DOSARCH=AIX -DPACKAGE_VERSION="\"moz\"" -DPACKAGE_BUGREPORT="\"http://bugzilla.mozilla.org/\"" -I/home/ulink/src/comm-1.9.1/mozilla/gfx/ots/src -I/home/ulink/src/comm-1.9.1/mozilla/gfx/ots/src -I. -I../../../dist/include -I../../../dist/include/ots -I/home/ulink/src/comm-1.9.1/mozilla/obj-fx35-aix43/dist/include/nspr -I/home/ulink/src/comm-1.9.1/mozilla/obj-fx35-aix43/dist/sdk/include -qflag=w:w -DNDEBUG -DTRIMMED -O2 -qmaxmem=-1 -DMOZILLA_VERSION=\"1.9.1.16pre\" -DMOZILLA_VERSION_U=1.9.1.16pre -DAIX=1 -DHAVE_SYS_INTTYPES_H=1 -DNSCAP_DISABLE_DEBUG_PTR_TYPES=1 -DD_INO=d_ino -DSTDC_HEADERS=1 -DHAVE_ST_BLKSIZE=1 -DHAVE_SIGINFO_T=1 -DHAVE_INT16_T=1 -DHAVE_INT32_T=1 -DHAVE_INT64_T=1 -DHAVE_INT64=1 -DHAVE_UINT=1 -DHAVE_UINT_T=1 -DHAVE_UINT16_T=1 -DHAVE_DIRENT_H=1 -DHAVE_MEMORY_H=1 -DHAVE_UNISTD_H=1 -DHAVE_NL_TYPES_H=1 -DHAVE_MALLOC_H=1 -DHAVE_X11_XKBLIB_H=1 -DHAVE_SYS_STATVFS_H=1 -DHAVE_SYS_STATFS_H=1 -DHAVE_LIBC_R=1 -DHAVE_LIBM=1 -DHAVE_LIBDL=1 -DHAVE_LIBC_R=1 -DFUNCPROTO=15 -DHAVE_XSHM=1 -DHAVE_FT_BITMAP_SIZE_Y_PPEM=1 -DHAVE_FT_GLYPHSLOT_EMBOLDEN=1 -DHAVE_FT_LOAD_SFNT_TABLE=1 -DHAVE_FT_SELECT_SIZE=1 -DHAVE_ARM_SIMD=1 -D_REENTRANT=1 -DHAVE_RANDOM=1 -DHAVE_STRERROR=1 -DHAVE_LCHOWN=1 -DHAVE_FCHMOD=1 -DHAVE_SNPRINTF=1 -DHAVE_MEMMOVE=1 -DHAVE_RINT=1 -DHAVE_STAT64=1 -DHAVE_LSTAT64=1 -DHAVE_TRUNCATE64=1 -DHAVE_FLOCKFILE=1 -DHAVE_LOCALTIME_R=1 -DHAVE_STRTOK_R=1 -DHAVE_RES_NINIT=1 -DHAVE_LANGINFO_CODESET=1 -DHAVE_I18N_LC_MESSAGES=1 -DMOZ_EMBEDDING_LEVEL_DEFAULT=1 -DMOZ_EMBEDDING_LEVEL_BASIC=1 -DMOZ_EMBEDDING_LEVEL_MINIMAL=1 -DMOZ_PHOENIX=1 -DMOZ_BUILD_APP=browser -DMOZ_XUL_APP=1 -DMOZ_DEFAULT_TOOLKIT=\"cairo-gtk2\" -DMOZ_X11=1 -DMOZ_WIDGET_GTK2=1 -DMOZ_ENABLE_XREMOTE=1 -DMOZ_DISTRIBUTION_ID=\"org.mozilla\" -DMOZ_PANGO=1 -DOJI=1 -DIBMBIDI=1 -DMOZ_VIEW_SOURCE=1 -DMOZ_XPINSTALL=1 -DMOZ_JSLOADER=1 -DNS_PRINTING=1 -DNS_PRINT_PREVIEW=1 -DMOZ_NO_XPCOM_OBSOLETE=1 -DMOZ_OGG=1 -DMOZ_WAVE=1 -DMOZ_SYDNEYAUDIO=1 -DMOZ_MEDIA=1 -DMOZ_XTF=1 -DMOZ_CRASHREPORTER_ENABLE_PERCENT=100 -DMOZ_MATHML=1 -DMOZ_ENABLE_CANVAS=1 -DMOZ_SVG=1 -DMOZ_UPDATE_CHANNEL=default -DMOZ_PLACES=1 -DMOZ_FEEDS=1 -DMOZ_STORAGE=1 -DMOZ_SAFE_BROWSING=1 -DMOZ_URL_CLASSIFIER=1 -DMOZ_LOGGING=1 -DMOZ_USER_DIR=\".mozilla\" -DMOZ_ENABLE_LIBXUL=1 -DHAVE_INTTYPES_H=1 -DMOZ_XUL=1 -DMOZ_PROFILELOCKING=1 -DMOZ_RDF=1 -DMOZ_MORKREADER=1 -DMOZ_DLL_SUFFIX=\".so\" -DXP_UNIX=1 -DUNIX_ASYNC_DNS=1 -D_MOZILLA_CONFIG_H_ -DMOZILLA_CLIENT /home/ulink/src/comm-1.9.1/mozilla/gfx/ots/src/cmap.cc "/home/ulink/src/comm-1.9.1/mozilla/gfx/ots/src/cmap.cc", line 145.42: 1540-0861 (S) Too few arguments are specified for macro "OTS_WARNING". Empty arguments are used. "/home/ulink/src/comm-1.9.1/mozilla/gfx/ots/src/ots.h", line 36.9: 1540-0425 (I) "OTS_WARNING" is defined on line 36 of "/home/ulink/src/comm-1.9.1/mozilla/gfx/ots/src/cmap.cc". "/home/ulink/src/comm-1.9.1/mozilla/gfx/ots/src/cmap.cc", line 165.54: 1540-0861 (S) Too few arguments are specified for macro "OTS_WARNING". Empty arguments are used. "/home/ulink/src/comm-1.9.1/mozilla/gfx/ots/src/ots.h", line 36.9: 1540-0425 (I) "OTS_WARNING" is defined on line 36 of "/home/ulink/src/comm-1.9.1/mozilla/gfx/ots/src/cmap.cc". gmake[2]: *** [cmap.o] Error 1 gmake[2]: Leaving directory `/home/ulink/src/comm-1.9.1/mozilla/obj-fx35-aix43/gfx/ots/src' Seems the old VisualAge 6.0 compiler does not accept an empty argument for a variadic argument macro... There is no newer compiler for this platform.

appending an additional empty macro argument makes old VisualAge Compiler happy. variadic argument macros are NOT part of any ANSI C++ standard, though every recent compiler will support this C99 standard feature with GCC syntax.

(In reply to comment #65) > http://hg.mozilla.org/releases/mozilla-1.9.2/rev/9addaf2c223d Thunderbird 3.1: http://hg.mozilla.org/releases/comm-1.9.2/rev/aba0b380582d Include the ots libs in what is linked to, to fix bustage from bug 527276

Unlike GCC, VC only warns about too many actual parameters for a macro. Since we don't need them, we can just ignore them and disable the warning.

Comment on attachment 493106 [details] [diff] [review] branch 1.9.1 hack for making variadic macro OTS_WARNING compile on VC7.1 This really should have gone into a new "can't build with VC7.1 after bug 527191" bug, we now have two confusing choices: 1) leave this marked 1.9.1.16-fixed and not be sure this landed 2) update to 1.9.1.17-fixed which loses the fact that the main underlying bug -- an important security fix -- was fixed in an earlier release. Approved for 1.9.1.17, a=dveditz for release-drivers

Pushed changeset e393647190a2 to releases/mozilla-1.9.1 (In reply to comment #84) > This really should have gone into a new "can't build with VC7.1 after bug > 527191" bug Sorry, I saw attachment 491604 [details] [diff] [review] here and just piled on ;-)

(In reply to comment #85) > (In reply to comment #84) > > This really should have gone into a new "can't build with VC7.1 after bug > > 527191" bug > Sorry, I saw attachment 491604 [details] [diff] [review] here and just piled on ;-) That was me. I intentionally didn't asked for review because AIX 4.3.3 is obsolete, but the compiler is very,very strict ANSI C++98 and has shown quite a few little bugs in a last few years. On AIX 5.1 and later the next release of the compiler had no problem with this non-standard GNUism. This patch was posted as a hint for any weird UNIX porting effort. Not worth filing a new bug as resolved wontfix, but if I should file a new follow-up bug instead just tell me the preferred way.