Last Comment Bug 527276 - (CVE-2010-3768) [@font-face] investigate support for OpenType sanitizer library
(CVE-2010-3768)
: [@font-face] investigate support for OpenType sanitizer library
Status: RESOLVED FIXED
[sg:want P1][attachment 493106 landed...
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: ---
Assigned To: Jonathan Kew (:jfkthame)
:
Mentors:
http://code.google.com/p/ots/
: 578482 (view as bug list)
Depends on: 600821 601099 601110 601487 605872 613374 615196 619180 637481
Blocks: 574368 580212 580730 581029 581359 582151 583715 586847 586895 587742 588233 594456 594618 594627 594628 594638 594651 594926 594966 595026 595689 595703 595960 595997 596110 596112 596227 597942 598190 599061 599068 602558
  Show dependency treegraph
 
Reported: 2009-11-07 18:19 PST by John Daggett (:jtd)
Modified: 2011-03-01 06:59 PST (History)
23 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
final+
.13+
.13-fixed
.16+
.16-fixed


Attachments
part 1 - OTS source (202.88 KB, patch)
2010-08-28 13:09 PDT, Jonathan Kew (:jfkthame)
cjones.bugs: review+
roc: review+
Details | Diff | Review
part 1a - fixes to OTS source (1.95 KB, patch)
2010-08-28 13:10 PDT, Jonathan Kew (:jfkthame)
roc: review+
Details | Diff | Review
part 2 - compile OTS library as part of the mozilla build (5.51 KB, patch)
2010-08-28 13:11 PDT, Jonathan Kew (:jfkthame)
ted: review+
Details | Diff | Review
part 3 - integrate the OTS sanitizer into gfxUserFonts (23.12 KB, patch)
2010-08-28 13:14 PDT, Jonathan Kew (:jfkthame)
no flags Details | Diff | Review
part 3 - integrate the OTS sanitizer into gfxUserFonts (refreshed) (23.10 KB, patch)
2010-09-03 06:57 PDT, Jonathan Kew (:jfkthame)
jd.bugzilla: review-
Details | Diff | Review
part 4 - update reftest manifest because of OTS sanitizing test fonts (8.74 KB, patch)
2010-09-03 07:10 PDT, Jonathan Kew (:jfkthame)
no flags Details | Diff | Review
part 3, v2 - integrate the OTS sanitizer into gfxUserFonts (revised) (33.16 KB, patch)
2010-09-24 05:37 PDT, Jonathan Kew (:jfkthame)
no flags Details | Diff | Review
part 4 - patch OTS to allow OpenType Layout tables to be preserved (10.25 KB, patch)
2010-09-30 02:53 PDT, Jonathan Kew (:jfkthame)
jd.bugzilla: review+
Details | Diff | Review
part 5 - implement preference to control the preservation of OTL tables (6.26 KB, patch)
2010-09-30 02:57 PDT, Jonathan Kew (:jfkthame)
jd.bugzilla: review+
Details | Diff | Review
part 6 - update reftest manifests due to OTS sanitization of test fonts (5.33 KB, patch)
2010-10-01 06:15 PDT, Jonathan Kew (:jfkthame)
no flags Details | Diff | Review
part 3, v3 - integrate the OTS sanitizer into gfxUserFonts (revised) (34.99 KB, patch)
2010-10-04 15:30 PDT, Jonathan Kew (:jfkthame)
jd.bugzilla: review+
Details | Diff | Review
part 1 (for 1.9.2) - OTS source code from google (svn r.38) (207.72 KB, patch)
2010-10-20 10:19 PDT, Jonathan Kew (:jfkthame)
dveditz: approval1.9.2.13+
Details | Diff | Review
part 2 (for 1.9.2) - add OTS lib to the build process (5.67 KB, patch)
2010-10-20 10:22 PDT, Jonathan Kew (:jfkthame)
ted: review+
dveditz: approval1.9.2.13+
Details | Diff | Review
part 3 (for 1.9.2) - apply OTS sanitizer to downloaded fonts (20.05 KB, patch)
2010-10-20 10:24 PDT, Jonathan Kew (:jfkthame)
jd.bugzilla: review+
dveditz: approval1.9.2.13+
Details | Diff | Review
part 4 (for 1.9.2) - extend OTS to preserve OTL tables and implement pref to control this (15.53 KB, patch)
2010-10-20 10:27 PDT, Jonathan Kew (:jfkthame)
jd.bugzilla: review+
dveditz: approval1.9.2.13+
Details | Diff | Review
part 0 (for 1.9.1) - allow adoption of data from nsIStreamLoader (patch copied from bug 507970) (7.32 KB, patch)
2010-11-16 12:49 PST, Jonathan Kew (:jfkthame)
christian: approval1.9.1.16+
Details | Diff | Review
part 1 (for 1.9.1) - OTS source code from google (svn r.38) (207.72 KB, patch)
2010-11-16 12:51 PST, Jonathan Kew (:jfkthame)
christian: approval1.9.1.16+
Details | Diff | Review
part 2 (for 1.9.1) - add OTS lib to the build process (8.36 KB, patch)
2010-11-16 12:54 PST, Jonathan Kew (:jfkthame)
ted: review+
christian: approval1.9.1.16+
Details | Diff | Review
part 3 (for 1.9.1) - apply OTS sanitizer to downloaded fonts (40.49 KB, patch)
2010-11-16 12:56 PST, Jonathan Kew (:jfkthame)
jd.bugzilla: review+
christian: approval1.9.1.16+
Details | Diff | Review
part 4 (for 1.9.1) - extend OTS to preserve OTL tables and implement pref to control this (15.81 KB, patch)
2010-11-16 13:00 PST, Jonathan Kew (:jfkthame)
christian: approval1.9.1.16+
Details | Diff | Review
branch 1.9.1 hack for making variadic macro OTS_WARNING compile on AIX 4.3.3 (18.16 KB, patch)
2010-11-18 11:40 PST, Uli Link (:ul-mcamafia)
no flags Details | Diff | Review
branch 1.9.1 hack for making variadic macro OTS_WARNING compile on VC7.1 (423 bytes, patch)
2010-11-24 13:29 PST, neil@parkwaycc.co.uk
jd.bugzilla: review+
dveditz: approval1.9.1.17+
Details | Diff | Review

Description John Daggett (:jtd) 2009-11-07 18:19:40 PST
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.
Comment 1 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2009-11-08 13:12:50 PST
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.
Comment 2 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-07-13 17:29:17 PDT
*** Bug 578482 has been marked as a duplicate of this bug. ***
Comment 3 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-07-13 17:29:53 PDT
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.
Comment 4 John Daggett (:jtd) 2010-07-13 18:04:50 PDT
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.
Comment 5 Ted Mielczarek [:ted.mielczarek] 2010-07-13 18:29:29 PDT
We've started using some STL, bug 556699 tracks ensuring that various headers are safe for our use.
Comment 6 Joe Drew (not getting mail) 2010-08-25 18:34:55 PDT
This could help work around a lot of platform-level font issues.
Comment 7 Jonathan Kew (:jfkthame) 2010-08-28 13:09:15 PDT
Created attachment 470212 [details] [diff] [review]
part 1 - OTS source

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?
Comment 8 Jonathan Kew (:jfkthame) 2010-08-28 13:10:53 PDT
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.)
Comment 9 Jonathan Kew (:jfkthame) 2010-08-28 13:11:40 PDT
Created attachment 470214 [details] [diff] [review]
part 2 - compile OTS library as part of the mozilla build
Comment 10 Jonathan Kew (:jfkthame) 2010-08-28 13:14:57 PDT
Created attachment 470215 [details] [diff] [review]
part 3 - integrate the OTS sanitizer into gfxUserFonts

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).
Comment 11 Jonathan Kew (:jfkthame) 2010-08-30 04:00:57 PDT
(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 12 Benjamin Smedberg [:bsmedberg] 2010-08-30 06:05:12 PDT
Comment on attachment 470212 [details] [diff] [review]
part 1 - OTS source

STL stuff is now fully in cjones court, not mine. Whew.
Comment 13 Jonathan Kew (:jfkthame) 2010-09-03 06:57:41 PDT
Created attachment 471830 [details] [diff] [review]
part 3 - integrate the OTS sanitizer into gfxUserFonts (refreshed)
Comment 14 Jonathan Kew (:jfkthame) 2010-09-03 07:10:59 PDT
Created attachment 471831 [details] [diff] [review]
part 4 - update reftest manifest because of OTS sanitizing test fonts

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 15 John Daggett (:jtd) 2010-09-07 23:22:18 PDT
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.
Comment 16 Jonathan Kew (:jfkthame) 2010-09-08 09:12:33 PDT
(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.
Comment 17 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-09-08 21:33:41 PDT
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.
Comment 18 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-09-08 22:47:03 PDT
(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 19 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-09-09 04:56:16 PDT
Comment on attachment 470212 [details] [diff] [review]
part 1 - OTS source

I'm rubberstamping this.
Comment 20 Jonathan Kew (:jfkthame) 2010-09-13 11:10:03 PDT
(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".
Comment 21 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-09-13 11:16:10 PDT
OK, I buy that.

What about comment #18?
Comment 22 Jonathan Kew (:jfkthame) 2010-09-13 12:48:59 PDT
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?
Comment 23 Jonathan Kew (:jfkthame) 2010-09-13 12:55:07 PDT
(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.
Comment 24 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-09-13 13:21:36 PDT
(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.
Comment 25 John Daggett (:jtd) 2010-09-13 17:49:04 PDT
> > 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.
Comment 26 Jonathan Kew (:jfkthame) 2010-09-23 02:48:28 PDT
(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.
Comment 27 Jonathan Kew (:jfkthame) 2010-09-24 05:37:33 PDT
Created attachment 478241 [details] [diff] [review]
part 3, v2 - integrate the OTS sanitizer into gfxUserFonts (revised)

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.
Comment 28 Jonathan Kew (:jfkthame) 2010-09-30 02:53:16 PDT
Created attachment 479733 [details] [diff] [review]
part 4 - patch OTS to allow OpenType Layout tables to be preserved

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.
Comment 29 Jonathan Kew (:jfkthame) 2010-09-30 02:57:42 PDT
Created attachment 479735 [details] [diff] [review]
part 5 - implement preference to control the preservation of OTL tables

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.
Comment 30 John Daggett (:jtd) 2010-09-30 19:00:16 PDT
Did you really mean to obsolete the reftests?
Comment 31 Jonathan Kew (:jfkthame) 2010-09-30 23:29:46 PDT
(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.
Comment 32 Jonathan Kew (:jfkthame) 2010-10-01 06:15:43 PDT
Created attachment 480090 [details] [diff] [review]
part 6 - update reftest manifests due to OTS sanitization of test fonts

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.
Comment 33 Jonathan Kew (:jfkthame) 2010-10-03 13:27:43 PDT
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.
Comment 34 John Daggett (:jtd) 2010-10-04 06:28:08 PDT
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.
Comment 35 Jonathan Kew (:jfkthame) 2010-10-04 06:52:51 PDT
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.
Comment 36 Jonathan Kew (:jfkthame) 2010-10-04 15:30:42 PDT
Created attachment 480763 [details] [diff] [review]
part 3, v3 - integrate the OTS sanitizer into gfxUserFonts (revised)

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 37 John Daggett (:jtd) 2010-10-04 23:14:32 PDT
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 38 John Daggett (:jtd) 2010-10-04 23:17:08 PDT
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 39 John Daggett (:jtd) 2010-10-04 23:31:38 PDT
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.
Comment 40 Jonathan Kew (:jfkthame) 2010-10-05 00:01:41 PDT
(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. :)
Comment 41 Jonathan Kew (:jfkthame) 2010-10-05 00:05:14 PDT
(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 43 John Daggett (:jtd) 2010-10-05 00:17:14 PDT
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 45 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2010-10-05 13:56:12 PDT
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 ;) ).
Comment 46 Jonathan Kew (:jfkthame) 2010-10-06 14:50:23 PDT
(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. :)
Comment 48 Jonathan Kew (:jfkthame) 2010-10-12 14:12:02 PDT
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.
Comment 49 Daniel Veditz [:dveditz] 2010-10-13 11:06:24 PDT
> 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 50 Karl Tomlinson (ni?:karlt) 2010-10-17 16:45:44 PDT
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?
Comment 51 Jonathan Kew (:jfkthame) 2010-10-20 06:59:28 PDT
(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.
Comment 52 Jonathan Kew (:jfkthame) 2010-10-20 10:08:45 PDT
(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.
Comment 53 Jonathan Kew (:jfkthame) 2010-10-20 10:19:18 PDT
Created attachment 484727 [details] [diff] [review]
part 1 (for 1.9.2) - OTS source code from google (svn r.38)

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.
Comment 54 Jonathan Kew (:jfkthame) 2010-10-20 10:22:10 PDT
Created attachment 484728 [details] [diff] [review]
part 2 (for 1.9.2) - add OTS lib to the build process

Marking for re-review as it's somewhat different from the trunk patch.
Comment 55 Jonathan Kew (:jfkthame) 2010-10-20 10:24:52 PDT
Created attachment 484731 [details] [diff] [review]
part 3 (for 1.9.2) - apply OTS sanitizer to downloaded fonts

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.
Comment 56 Jonathan Kew (:jfkthame) 2010-10-20 10:27:06 PDT
Created attachment 484732 [details] [diff] [review]
part 4 (for 1.9.2) - extend OTS to preserve OTL tables and implement pref to control this

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 57 Daniel Veditz [:dveditz] 2010-10-20 10:40:15 PDT
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?
Comment 58 Jonathan Kew (:jfkthame) 2010-10-20 10:56:50 PDT
(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.
Comment 59 Daniel Veditz [:dveditz] 2010-10-29 10:42:00 PDT
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.
Comment 60 Jonathan Kew (:jfkthame) 2010-10-29 15:34:46 PDT
(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 61 Daniel Veditz [:dveditz] 2010-11-01 10:39:37 PDT
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 62 Daniel Veditz [:dveditz] 2010-11-01 10:40:00 PDT
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 63 Daniel Veditz [:dveditz] 2010-11-01 10:40:32 PDT
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 64 Daniel Veditz [:dveditz] 2010-11-01 10:41:30 PDT
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
Comment 66 Smokey Ardisson (offline for a while; not following bugs - do not email) 2010-11-04 13:43:37 PDT
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) ;)
Comment 67 christian 2010-11-05 10:38:32 PDT
We're still having discussions about if we want to risk taking this back on 1.9.1...
Comment 68 Jonathan Kew (:jfkthame) 2010-11-07 08:42:50 PST
(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.)
Comment 69 John Daggett (:jtd) 2010-11-09 22:06:42 PST
(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.
Comment 70 christian 2010-11-15 15:37:15 PST
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)?
Comment 71 Jonathan Kew (:jfkthame) 2010-11-15 23:50:42 PST
(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.....)
Comment 72 Jonathan Kew (:jfkthame) 2010-11-16 12:49:31 PST
Created attachment 490959 [details] [diff] [review]
part 0 (for 1.9.1) - allow adoption of data from nsIStreamLoader (patch copied from bug 507970)

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)
Comment 73 Jonathan Kew (:jfkthame) 2010-11-16 12:51:53 PST
Created attachment 490960 [details] [diff] [review]
part 1 (for 1.9.1) - OTS source code from google (svn r.38)

Same code as we've landed on 1.9.2, just repeating in the 1.9.1 series to keep things simple.
Comment 74 Jonathan Kew (:jfkthame) 2010-11-16 12:54:17 PST
Created attachment 490962 [details] [diff] [review]
part 2 (for 1.9.1) - add OTS lib to the build process

Simple backport of the 1.9.2 patch.
Comment 75 Jonathan Kew (:jfkthame) 2010-11-16 12:56:56 PST
Created attachment 490963 [details] [diff] [review]
part 3 (for 1.9.1) - apply OTS sanitizer to downloaded fonts

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.
Comment 76 Jonathan Kew (:jfkthame) 2010-11-16 13:00:48 PST
Created attachment 490965 [details] [diff] [review]
part 4 (for 1.9.1) - extend OTS to preserve OTL tables and implement pref to control this

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 77 John Daggett (:jtd) 2010-11-17 09:43:50 PST
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.
Comment 79 Uli Link (:ul-mcamafia) 2010-11-18 09:53:25 PST
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.
Comment 80 Uli Link (:ul-mcamafia) 2010-11-18 10:37:59 PST
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.
Comment 81 Uli Link (:ul-mcamafia) 2010-11-18 11:40:05 PST
Created attachment 491604 [details] [diff] [review]
branch 1.9.1 hack for making variadic macro OTS_WARNING compile on AIX 4.3.3

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.
Comment 82 Serge Gautherie (:sgautherie) 2010-11-22 20:36:44 PST
(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
Comment 83 neil@parkwaycc.co.uk 2010-11-24 13:29:56 PST
Created attachment 493106 [details] [diff] [review]
branch 1.9.1 hack for making variadic macro OTS_WARNING compile on VC7.1

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 84 Daniel Veditz [:dveditz] 2011-01-07 10:34:41 PST
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
Comment 85 neil@parkwaycc.co.uk 2011-01-07 16:06:37 PST
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 ;-)
Comment 86 Uli Link (:ul-mcamafia) 2011-01-08 02:54:36 PST
(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.

Note You need to log in before you can comment on or make changes to this bug.