Closed
Bug 1193050
Opened 10 years ago
Closed 10 years ago
Update OTS library to upstream tip [was: Uninitialized access of |prev_record| members in |ots_name_parse|]
Categories
(Core :: Graphics: Text, defect)
Core
Graphics: Text
Tracking
()
RESOLVED
FIXED
mozilla44
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)
316.83 KB,
patch
|
jfkthame
:
review+
|
Details | Diff | Splinter Review |
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
Comment 1•10 years ago
|
||
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•10 years ago
|
||
OK, I see it now.
Comment 3•10 years ago
|
||
I think checking for (name->names.size() > 0) instead of (i > 0) should do the trick.
Comment 4•10 years ago
|
||
Should be fixed with https://github.com/khaledhosny/ots/commit/9da865c8bb1cf59279dc4134377ce774460cc2b8.
Comment 5•10 years ago
|
||
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]
Assignee | ||
Comment 6•10 years ago
|
||
Sorry, I missed the bugmails for this bug. Here is an untested patch to update to the revision mentioned by Khaled.
Assignee | ||
Comment 7•10 years ago
|
||
Assignee | ||
Comment 8•10 years ago
|
||
(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•10 years ago
|
||
It looks like passing special tables through is not working here, but it works for test utils.
Comment 10•10 years ago
|
||
Replacing TRUETYPE_TAG with OTS_TAG in https://dxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxUserFontSet.cpp#184 might or might not help.
Assignee | ||
Comment 11•10 years ago
|
||
Assignee | ||
Comment 12•10 years ago
|
||
(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•10 years ago
|
||
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().
Assignee | ||
Comment 14•10 years ago
|
||
(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.
Assignee | ||
Updated•10 years ago
|
Attachment #8666826 -
Attachment description: Patch → Update OTS to 9da865c8bb1cf59279dc4134377ce774460cc2b8
Assignee | ||
Comment 15•10 years ago
|
||
Assignee | ||
Comment 16•10 years ago
|
||
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)
Comment 17•10 years ago
|
||
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)
Comment 18•10 years ago
|
||
(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)
Assignee | ||
Comment 19•10 years ago
|
||
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
Attachment #8668635 -
Flags: review?(jfkthame)
Assignee | ||
Comment 20•10 years ago
|
||
Comment 21•10 years ago
|
||
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+
Assignee | ||
Updated•10 years ago
|
Attachment #8666826 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8668393 -
Attachment is obsolete: true
Assignee | ||
Comment 22•10 years ago
|
||
(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•10 years ago
|
||
(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.
Assignee | ||
Comment 24•10 years ago
|
||
(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•10 years ago
|
||
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•10 years ago
|
||
(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•10 years ago
|
||
Ah, OK - that makes sense. We should just fix the font we're using, then.
Comment 28•10 years ago
|
||
Assignee | ||
Comment 29•10 years ago
|
||
It seems that the test failures no longer show up with this modification of GentiumPlus...
Comment 30•10 years ago
|
||
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)
Updated•10 years ago
|
Attachment #8669015 -
Flags: review?(jdaggett) → review+
Comment 31•10 years ago
|
||
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 33•10 years ago
|
||
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•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c341ed5e6340
https://hg.mozilla.org/mozilla-central/rev/67084d8fe886
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox44:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Comment 35•9 years ago
|
||
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|]
Added this to Fx44 release notes.
Updated•7 years ago
|
Blocks: coverity-analysis
You need to log in
before you can comment on or make changes to this bug.
Description
•