1193050 - Update OTS library to upstream tip [was: Uninitialized access of |prev_record| members in |ots_name_parse|]

Status: RESOLVED FIXED

(Core :: Graphics: Text, defect)

Description

[Eric Rahm [:erahm]]

+++ This bug was initially created as a clone of Bug #1057488 +++

Coverity indicates that members of |prev_record| can be accessed uninitialized [1] in its comparison operator [2]. 

It appears that if any of the continue statements in the switch are hit during the first iteration(s) of the loop this will happen.

[1] https://dxr.mozilla.org/mozilla-central/rev/d6ea652c579992daa9041cc9718bb7c6abefbc91/gfx/ots/src/name.cc#158
[2] https://dxr.mozilla.org/mozilla-central/rev/d6ea652c579992daa9041cc9718bb7c6abefbc91/gfx/ots/src/name.h#35

Comment 1

[Khaled Hosny]

I don’t see how this would happen since the if statement first checks for (i > 0), and after the first iteration prev_record is guaranteed to be initialised because of the assignment in [1].

[1] https://dxr.mozilla.org/mozilla-central/rev/d6ea652c579992daa9041cc9718bb7c6abefbc91/gfx/ots/src/name.cc#164

Comment 2

[Khaled Hosny]

OK, I see it now.

Comment 3

[Khaled Hosny]

I think checking for (name->names.size() > 0) instead of (i > 0) should do the trick.

Comment 4

[Khaled Hosny]

Should be fixed with https://github.com/khaledhosny/ots/commit/9da865c8bb1cf59279dc4134377ce774460cc2b8.

Comment 5

[Nicolas Silva [:nical]]

Frédéric, I see in the mercurial log that you took care of syncing up our ots code with upstream a few times in the past, so assigning to you (assuming that's what's left to do to close this bug).

Comment 6

[Frédéric Wang (:fredw)]

Attachment: Update OTS to 9da865c8bb1cf59279dc4134377ce774460cc2b8

Sorry, I missed the bugmails for this bug. Here is an untested patch to update to the revision mentioned by Khaled.

Comment 7

[Frédéric Wang (:fredw)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=38f32cd30c31

Comment 8

[Frédéric Wang (:fredw)]

(In reply to Frédéric Wang (:fredw) from comment #7)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=38f32cd30c31

It seems that the OTS upgrade is causing many test failures...

Comment 9

[Khaled Hosny]

It looks like passing special tables through is not working here, but it works for test utils.

Comment 10

[Khaled Hosny]

Replacing TRUETYPE_TAG with OTS_TAG in https://dxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxUserFontSet.cpp#184 might or might not help.

Comment 11

[Frédéric Wang (:fredw)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=40f145eadf1c

Comment 12

[Frédéric Wang (:fredw)]

(In reply to Khaled Hosny from comment #10)
> Replacing TRUETYPE_TAG with OTS_TAG in
> https://dxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxUserFontSet.
> cpp#184 might or might not help.

This does not seem to help.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=40f145eadf1c

Comment 13

[Khaled Hosny]

OK, I tried it locally and it seems that the tags are passed to GetTableAction() in the wrong endianness. Removing htonl() call in https://dxr.mozilla.org/mozilla-central/source/gfx/ots/src/ots.cc#417 fixes this, but I’m not sure how this works on big endian machines. This is a regression from https://github.com/khaledhosny/ots/commit/e62f5490f56faae2b6b8b213e82f0e2c123587ab which replaced ReadTag() with ReadU32() which calls ntohl().

I have to admit this all endianness stuff just confuses me, so if someone can asses this and tell me if I should just drop htonl() from GetTableAction() or recover ReadTag().

Comment 14

[Frédéric Wang (:fredw)]

(In reply to Khaled Hosny from comment #13)
> OK, I tried it locally and it seems that the tags are passed to
> GetTableAction() in the wrong endianness. Removing htonl() call in
> https://dxr.mozilla.org/mozilla-central/source/gfx/ots/src/ots.cc#417 fixes
> this, but I’m not sure how this works on big endian machines. This is a
> regression from
> https://github.com/khaledhosny/ots/commit/
> e62f5490f56faae2b6b8b213e82f0e2c123587ab which replaced ReadTag() with
> ReadU32() which calls ntohl().
> 
> I have to admit this all endianness stuff just confuses me, so if someone
> can asses this and tell me if I should just drop htonl() from
> GetTableAction() or recover ReadTag().

I'm not sure what is meant by "Make tags big-endian everywhere" in this commit. Here is how I understand the situation:

1) The old uint32_t Tag() just did memory copying of the 4 chars, so the byte order is independent of endianness.
2) The new OTS_TAG macro in OTS as well as the TRUETYPE_TAG macro in Gecko use the same integer operations to output an unsigned 32 integer and so the byte order is dependent of endianness

The Gecko code assumes that the byte order of tags passed to GetTableAction depends on endianness:
https://dxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxUserFontSet.cpp#184

The byte order of the old OTS tags were independent of endianness and passing htonl(tag) to GetTableAction makes it dependent of endianness as expected by Gecko (I guess this is a confusing use of htonl, probably ntohl should have been used instead).

The byte order of the new OTS tags are dependent of endianness and are constructed the same as Gecko tags, so we can just pass them without modification to GetTableAction.

So I'd recommend to just drop htonl() from GetTableAction(). That way the same tags are used in Gecko and OTS.

Comment 15

[Frédéric Wang (:fredw)]

Attachment: Do not change byte order of OTS tags passed to GetTableAction

Comment 16

[Frédéric Wang (:fredw)]

The reftests seem to pass with this second patch:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a8be0e2714bd

@Jonathan: What do you think? Should Khaled just apply attachment 8668393 [details] [diff] [review] before we update to the latest git version?

Comment 17

[Jonathan Kew [:jfkthame]]

OK, let's see if I'm understanding correctly what's going on here.

OTS used to read tags from big-endian data formats (like sfnt and WOFF) by memcpy'ing the bytes directly into a uint32_t variable. So the order of the bytes in the "tag" variable within OTS was the same as the order of the bytes in the sfnt data -- which means that the value of the "tag" variable when interpreted as an integer would depend on the system's endianness.

That can work OK, as long as it's done consistently, though it did mean that it was necessary to byte-swap when sorting tags, for example.

Now, OTS is changing so that it reads tag fields via ReadU32, which interprets the 4-byte field as a bigendian integer, and therefore byte-swaps when running on little-endian. So the integer value of a tag (within OTS) is constant across systems, and SortByTag can be simplified as in the latest OTS source.

The remaining question is how the 'tag' parameter to GetTableAction should be passed. The comments in opentype-sanitizer.h currently say:

    //   tag: table tag as an integer in big-endian byte order, independent of
    //   platform endianness

but was this true? I don't think so.... AFAICS, the use of htonl() in the line

  action = header->context->GetTableAction(htonl(tag));

was actually *wrong*, because within OTS the tag values were already being passed around in their big-endian order thanks to how ReadTag() was defined.

Now, with the change to the internal representation of tags in OTS, this comment will become "correct" in that it'll match the behavior of the code -- but it is a breaking change from the previous behavior, as evidenced by the tests that start failing.

IMO, it's probably better to stay with the old behavior, and just change the comment here to say that the table tag is passed as a platform-native uint32 so that it reflects reality. And so that means yes, the htonl() call here should be removed.

Khaled, is that OK with you? In short: we think you should apply the patch from attachment 8668393 [details] [diff] [review] upstream in OTS, and change the doc for GetTableAction accordingly.

Comment 18

[Khaled Hosny]

(In reply to Jonathan Kew (:jfkthame) from comment #17)
> Khaled, is that OK with you? In short: we think you should apply the patch
> from attachment 8668393 [details] [diff] [review] upstream in OTS, and
> change the doc for GetTableAction accordingly.

Thanks Jonathan, I think I got confused because I believed the GetTableAction() comment (which is not surprising since I wrote it :). Pushed as https://github.com/khaledhosny/ots/commit/097291983d12fb662e9aed677b556fd539aff436.

Comment 19

[Frédéric Wang (:fredw)]

Attachment: Update OTS to 097291983d12fb662e9aed677b556fd539aff436

I changed the sed command because "-i" seems to behave differently on different UNIX platforms...

https://developer.apple.com/library/mac/documentation/Darwin/Reference/ManPages/man1/sed.1.html
http://linux.about.com/od/commands/l/blcmdl1_sed.htm

Comment 20

[Frédéric Wang (:fredw)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5503933d81e9

Comment 21

[Jonathan Kew [:jfkthame]]

Comment on attachment 8668635 [details] [diff] [review]
Update OTS to 097291983d12fb662e9aed677b556fd539aff436

Review of attachment 8668635 [details] [diff] [review]:
-----------------------------------------------------------------

OK, let's take the update; rs=me.

Please update the commit message here to something like "update OTS to latest upstream version", as that's what we're really doing here; we're not just patching the original issue in ots_name_parse.

Comment 22

[Frédéric Wang (:fredw)]

(In reply to Frédéric Wang 
(:fredw) from comment #20)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=5503933d81e9

The try server with the latest version is still running, but there seem to be more build errors:
https://treeherder.mozilla.org/logviewer.html#?job_id=12157889&repo=try

Comment 23

[Khaled Hosny]

(In reply to Frédéric Wang (:fredw) from comment #22)
> (In reply to Frédéric Wang 
> (:fredw) from comment #20)
> > https://treeherder.mozilla.org/#/jobs?repo=try&revision=5503933d81e9
> 
> The try server with the latest version is still running, but there seem to
> be more build errors:
> https://treeherder.mozilla.org/logviewer.html#?job_id=12157889&repo=try

I pushed few commits that should address this and other static analysis issues.

Comment 24

[Frédéric Wang (:fredw)]

(In reply to Khaled Hosny from comment #23)
> I pushed few commits that should address this and other static analysis
> issues.

OK, I stopped the current try build and restarted again with the latest OTS changes:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0e550367609a

Comment 25

[Jonathan Kew [:jfkthame]]

Drat...looks like it's rejecting the GentiumPlus-R.woff file that we have in a couple of testcases. It might be useful to see what errors get logged to the web console when it tries to load that font.

Comment 26

[Khaled Hosny]

(In reply to Jonathan Kew (:jfkthame) from comment #25)
> Drat...looks like it's rejecting the GentiumPlus-R.woff file that we have in
> a couple of testcases. It might be useful to see what errors get logged to
> the web console when it tries to load that font.

Running ot-sanitize on it, I get:
ERROR: Layout: bad start coverage index.
ERROR: GPOS: Failed to parse coverage table
ERROR: Layout: Failed to parse lookup subtable 1
ERROR: Layout: Failed to parse lookup from extension lookup
ERROR: Layout: Failed to parse lookup subtable 8
ERROR: Layout: Failed to parse subtable 0
ERROR: Layout: Failed to parse lookup 5
ERROR: GPOS: Failed to parse lookup list table
Failed to sanitise file!

The error are probably old, but previously we would just drop the GPOS table but now we reject the whole font (per https://github.com/khaledhosny/ots/issues/74).

Comment 27

[Jonathan Kew [:jfkthame]]

Ah, OK - that makes sense. We should just fix the font we're using, then.

Comment 28

[Jonathan Kew [:jfkthame]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a69b4030368b

Comment 29

[Frédéric Wang (:fredw)]

It seems that the test failures no longer show up with this modification of GentiumPlus...

Comment 30

[Jonathan Kew [:jfkthame]]

Attachment: Update the copy of GentiumPlus used in font-inspector test

This is simply the latest release of GentiumPlus (direct from SIL's site); our current copy has errors in the OpenType tables, and the OTS update here rejects it, leading to test failures.

Comment 31

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a9a45d9ed1ba5f9923ecfa94ae53c2e8ee003b04
Bug 1193050 - Update the copy of GentiumPlus used in font-inspector test. r=jdaggett

https://hg.mozilla.org/integration/mozilla-inbound/rev/a94b86bdf54bf53fdfbed5063651e6cbf2f0255b
Bug 1193050 - Update OTS to latest upstream version. r=jfkthame

Comment 32

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7c8c39371dbd
Backed out for build bustage: https://treeherder.mozilla.org/logviewer.html#?job_id=15631186&repo=mozilla-inbound
https://hg.mozilla.org/integration/mozilla-inbound/rev/41166ce51f02

Comment 33

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c341ed5e63409a03a901daf9d5252ee0e57b9bdc
Bug 1193050 - Update the copy of GentiumPlus used in font-inspector test. r=jdaggett

https://hg.mozilla.org/integration/mozilla-inbound/rev/67084d8fe8860db544eb9435f0600f88039c00fd
Bug 1193050 - Update OTS to latest upstream version. r=jfkthame

Comment 34

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c341ed5e6340
https://hg.mozilla.org/mozilla-central/rev/67084d8fe886

Comment 35

[Jonathan Kew [:jfkthame]]

Release Note Request (optional, but appreciated)
[Why is this notable]:
The OTS update here makes validation stricter, which means that some currently-working webfonts may stop working; users will therefore see a fallback font instead.

[Suggested wording]:
Stricter validation of webfonts may block some fonts that previously worked.

[Links (documentation, blog post, etc)]:
https://github.com/khaledhosny/ots/issues/74 -- the issue for the change in how OpenType layout errors are handled by OTS, now rejecting the entire font.

https://lists.w3.org/Archives/Public/public-webfonts-wg/2016Jan/0000.html -- post to the WebFonts Working Group list mentioning this issue.

Comment 36

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Added this to Fx44 release notes.