1581145 - update sfntly to master

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Nathan Froyd [:froydnj]]

When trying to address some C++17 issues on our aarch64-linux builds, I found that sfntly failed to compile with a more recent libstdc++ due to static_assert violations in libstdc++ code. Investigating a little more, this particular problem had been fixed upstream, along with several other fuzzing-related (ubsan and asan) bugs found by Chromium.

https://bugs.chromium.org/p/chromium/issues/detail?id=699510
https://bugs.chromium.org/p/chromium/issues/detail?id=724890
https://bugs.chromium.org/p/chromium/issues/detail?id=811938

etc.

Rather than cherry-picking fixes, I'd like to update the entire tree in one fell swoop. The sfntly update fixes my build issues:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bbb8b0f231c852ccf9b0f4a130968d188ac6f890&selectedJob=266544306

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: Bug 1581145 - update sfntly to master; r=jfkthame

This update brings in several bugfixes and compatibility with newer
libstdc++ versions.

Comment 2

[Nathan Froyd [:froydnj]]

Comment on attachment 9092689 [details]
Bug 1581145 - update sfntly to master; r=jfkthame

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Motivated people can go find the original ASan/UbSan fuzzing testcases, and might be able to construct something out of those.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which older supported branches are affected by this flaw?: Everything
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: They should be straightforward; if the patch doesn't apply as-is, we can just cp in new files from sfntly master.
• How likely is this patch to cause regressions; how much testing does it need?: I would rate regression potential as low, given the stability of this library and the testing upstream has gone through, but I'm not even a novice about the area the patch touches. Jonathan would be a better judge of that than I would.

Comment 3

[Nathan Froyd [:froydnj]]

ni? to Jonathan for comment 2.

Comment 4

[Jonathan Kew [:jfkthame]]

In general, I would agree that regression potential is low; this is mostly minor updates to pretty stable code.

However, having said that, I just noticed what I think may be a bug in the new upstream code; I've queried it at https://github.com/googlefonts/sfntly/commit/e24c73130c663c9f329e78f5ca3fd5bd83b02622#r35100304. Might want to see what response we get to that before moving forward here.

Comment 5

[Jonathan Kew [:jfkthame]]

https://github.com/googlefonts/sfntly has just been updated with a fix for the issue I noticed in comment 4, so you may want to pull that into our copy as well.

Comment 6

[Nathan Froyd [:froydnj]]

(In reply to Jonathan Kew (:jfkthame) from comment #5)

https://github.com/googlefonts/sfntly has just been updated with a fix for the issue I noticed in comment 4, so you may want to pull that into our copy as well.

Thank you, updated!

Comment 7

[Al Billings [:abillings - ex-MoCo]]

Calling this sec-high just to be safe and giving this sec-approval for mozilla-central.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2400f298812b85710e974b93c2ab4bcb726198be

Please nominate this for Beta and ESR68 approval when you get a chance. It grafts cleanly as-landed.

Comment 9

[Nathan Froyd [:froydnj]]

Comment on attachment 9092689 [details]
Bug 1581145 - update sfntly to master; r=jfkthame

Beta/Release Uplift Approval Request

• User impact if declined: Vulnerable to crashes, security holes, etc.
• Is this code covered by automated tests?: Unknown
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Minor updates to stable code, cf comment 4.
• String changes made/needed: None

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration:
• User impact if declined: Vulnerable to crashes, security holes, etc.
• Fix Landed on Version: 71
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Minor updates to stable code, cf comment 4.
• String or UUID changes made by this patch: None

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9092689 [details]
Bug 1581145 - update sfntly to master; r=jfkthame

Fixes some bugs in a third-party library. Approved for 70.0b9 and 68.2esr.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/2a42a19c5dea

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr68/rev/a2a07716001a