Closed Bug 1193050 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: Graphics: Text, defect)

Product:
Core
Component:
Graphics: Text
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla44
Tracking Flags:
Tracking Status
firefox43 --- affected
firefox44 --- fixed
relnote-firefox --- 44+

People

(Reporter: erahm, Assigned: fredw)

References

(Depends on 2 open bugs, Blocks 1 open bug)

Details

(Keywords: coverity, Whiteboard: [CID 1286017][gfx-noted])

Attachments

(2 files, 2 obsolete files)

Update OTS to 097291983d12fb662e9aed677b556fd539aff436
316.83 KB, patch
jfkthame
: review+
Details | Diff | Splinter Review
Update the copy of GentiumPlus used in font-inspector test
814.07 KB, patch
jtd
: review+
Details | Diff | Splinter Review 
+++ 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
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
OK, I see it now.
I think checking for (name->names.size() > 0) instead of (i > 0) should do the trick.
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).
Assignee: nobody → fred.wang
Whiteboard: [CID 1286017] → [CID 1286017][gfx-noted]
Sorry, I missed the bugmails for this bug. Here is an untested patch to update to the revision mentioned by Khaled.
(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...
It looks like passing special tables through is not working here, but it works for test utils.
Replacing TRUETYPE_TAG with OTS_TAG in https://dxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxUserFontSet.cpp#184 might or might not help.
(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
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().
(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.
Attachment #8666826 - Attachment description: Patch → Update OTS to 9da865c8bb1cf59279dc4134377ce774460cc2b8
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?
Flags: needinfo?(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.
Flags: needinfo?(jfkthame) → needinfo?(khaledhosny)
(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.
Flags: needinfo?(khaledhosny)
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.
Attachment #8668635 - Flags: review?(jfkthame) → review+
Attachment #8666826 - Attachment is obsolete: true
Attachment #8668393 - Attachment is obsolete: true
(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
(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.
(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
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.
(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).
Ah, OK - that makes sense. We should just fix the font we're using, then.
It seems that the test failures no longer show up with this modification of GentiumPlus...
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.
Attachment #8669015 - Flags: review?(jdaggett)
Attachment #8669015 - Flags: review?(jdaggett) → review+
https://hg.mozilla.org/mozilla-central/rev/c341ed5e6340
https://hg.mozilla.org/mozilla-central/rev/67084d8fe886
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
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.
relnote-firefox: --- → ?
Summary: Uninitialized access of |prev_record| members in |ots_name_parse| → Update OTS library to upstream tip [was: Uninitialized access of |prev_record| members in |ots_name_parse|]
Depends on: 1239176
Depends on: 1265753
Depends on: 1294448
Depends on: 1340298
No longer depends on: 1340298
Depends on: 1389957
Depends on: 1405964
Depends on: 1499248
You need to log in before you can comment on or make changes to this bug.