1575293 - Update lz4 to 1.9.2

Status: RESOLVED FIXED

(Core :: MFBT, task)

Description

[Ryan VanderMeulen [:RyanVM]]

Given the note below, we should probably take a closer look at this update and see if we should consider backporting or not (we updated to v1.9.1 in 68).

This is primarily a bugfix release, driven by the bugs found and fixed since LZ4 recent
integration into Google's oss-fuzz, provided by @cmeister2 . The new capability was
put to good use by @terrelln, which dramatically expanded the number of scenarios
covered by the profile-guided fuzzer. These scenarios were already covered by
unguided fuzzers, but a few bugs require a large combinations of factors that unguided
fuzzer are unable to produce in a reasonable timeframe.

Due to these fixes, an upgrade of LZ4 to its latest version is recommended.

Changes in v1.9.2:

• fix : out-of-bound read in exceptional circumstances when using decompress_partial(), by @terrelln
• fix : rare data corruption bug with LZ4_compress_destSize(), by @terrelln
• fix : data corruption bug when Streaming with an Attached Dict in HC Mode, by @felixhandte
• perf: enable LZ4_FAST_DEC_LOOP on aarch64/GCC by default, by @prekageo
• perf: improved lz4frame streaming API speed, by @dreambottle
• perf: speed up lz4hc on slow patterns when using external dictionary, by @terrelln
• api: better in-place decompression and compression support

Comment 1

[Ryan VanderMeulen [:RyanVM]]

Attachment: Bug 1575293 - Update lz4 to version 1.9.2. r=froydnj,jfkthame

Comment 2

[Ryan VanderMeulen [:RyanVM]]

Green on Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fe52f04075d6fd457a2846b7a10d4db86f80311b

Comment 3

[Nathan Froyd [:froydnj]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #0)

Changes in v1.9.2:

• fix : out-of-bound read in exceptional circumstances when using decompress_partial(), by @terrelln
• fix : rare data corruption bug with LZ4_compress_destSize(), by @terrelln
• fix : data corruption bug when Streaming with an Attached Dict in HC Mode, by @felixhandte

I think the first two would be worth considering uplifts for, depending on how late in the release cycle we are. The last doesn't apply to our uses of lz4, AIUI, because we don't provide dictionaries.

It's a shame we can't benefit from the aarch64 improvements, because they are literally gcc-only, due to perf concerns with clang. :(

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d738b8cf1664
Update lz4 to version 1.9.2. r=froydnj

Comment 5

[Cristina Coroiu [:ccoroiu]]

Backed out changeset d738b8cf1664 (bug 1575293) for reftest failures at text-decoration/skip-ink-multiline-position.html

Backout: https://hg.mozilla.org/integration/autoland/rev/ef5531817da87e7a812eb334508dff9de429c633

Failure push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=d738b8cf1664ba8ef563e9abdb62874381eff99b

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=262690695&repo=autoland&lineNumber=10471

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Updating lz4 apparently made a skip-ink reftest start perma-passing. Any thoughts, Dan?

Comment 7

[Daniel Holbert [:dholbert]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #6)

Updating lz4 apparently made a skip-ink reftest start perma-passing. Any thoughts, Dan?

Per IRC: "yay", I guess!

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cfb420e8ae50
Update lz4 to version 1.9.2. r=froydnj

Comment 9

[Jonathan Kew [:jfkthame]]

This has broken the rendering of Graphite fonts that use lz4 to internally compress the shaping tables.

E.g. compare the rendering of https://jfkthame.github.io/test/awami.html with builds before and after this patch landed.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Backed out for the regression in comment 9.

https://hg.mozilla.org/integration/autoland/rev/bde65e4083091e78c494a6b5f1a611da1d3c7b0d

Comment 11

[Jonathan Kew [:jfkthame]]

So the failure isn't in graphite itself, which uses lz4 directly, and still works if the font is locally installed; it's in OTS, which uses lz4 through the mfbt wrapper in Compression.h when checking internally-compressed graphite 'Glat' and 'Silf' tables. The decompression step there is failing, causing OTS to discard all the graphite tables. Looking deeper...

Comment 12

[Jonathan Kew [:jfkthame]]

OK, I think I know what may be going on here. When the 'Glat' or 'Silf' tables are compressed, the format does not provide an explicit length of the compressed data; it is assumed to be "all the remaining data in the OpenType table" after the compressed-table header. From https://github.com/silnrsi/graphite/blob/master/doc/GTF.txt:

|===============================================================================
|Type     |Name            |Description                          |Version notes

|FIXED    |version         |Uncompressed Table version number    |
|ULONG:5  |scheme          |Compression scheme must not be 0     |5.0 – added
|ULONG:27 |full_size       |Size of uncompressed table           |5.0 – added
|BYTE[]   |compressed_data |Compression scheme data              |5.0 – added
|===============================================================================

In OTS, we decompress this using mozilla::Compression::LZ4::decompressPartial, which is explicitly documented in the Compression.h header as ignoring unconsumed input: https://searchfox.org/mozilla-central/rev/597a69c70a5cce6f42f159eb54ad1ef6745f5432/mfbt/Compression.h#103-106. So given that we know the expected output size, it shouldn't matter if the table in the font includes padding bytes at the end, beyond the actual compressed data.

However, the underlying lz4 code now wants to check that the input was exactly consumed, at https://github.com/lz4/lz4/blob/28964f4bea7fa95dc76154c5190d77fc18b2f8c8/lib/lz4.c#L1922. That check is failing for the Awami 'Glat' table because it appears to have a couple of bytes of padding at the end of the compressed data. Previously, decompressPartial didn't care about this; now, it rejects it.

Comment 13

[Jonathan Kew [:jfkthame]]

Pretty convinced by now that this is a bug in LZ4_decompress_safe_partial. I'm going to try and put together a reduced testcase to demonstrate the problem, and then file an issue upstream.

Comment 14

[Jonathan Kew [:jfkthame]]

Filed https://github.com/lz4/lz4/issues/783 for the upstream bug that is causing this breakage.

Comment 15

[Jonathan Kew [:jfkthame]]

(In reply to Nathan Froyd [:froydnj] from comment #3)

(In reply to Ryan VanderMeulen [:RyanVM] from comment #0)

Changes in v1.9.2:

• fix : out-of-bound read in exceptional circumstances when using decompress_partial(), by @terrelln
• fix : rare data corruption bug with LZ4_compress_destSize(), by @terrelln
• fix : data corruption bug when Streaming with an Attached Dict in HC Mode, by @felixhandte

I think the first two would be worth considering uplifts for, depending on how late in the release cycle we are.

FWIW, although I don't fully understand all the code involved, I suspect the first of the above fixes is responsible for the regression that's breaking OTS support for Graphite fonts.

Comment 16

[BugBot [:suhaib / :marco/ :calixte]]

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:RyanVM, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

This can't land until comment 14 is resolved upstream.

Comment 18

[Marc]

This bug is now fixed from comment #14.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

I've rebased the patch onto m-c tip and cherry-picked the upstream fix which landed for comment 14. The testcase from comment 9 now renders correctly and our reftests appear to pass on all platforms. I think this is ready for a rubberstamp and re-land unless there's more testing you wanted to do first, Jonathan?
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f8860ce34adec7545fef3994705e4c56897b8145

Comment 20

[Jonathan Kew [:jfkthame]]

Yes, with the fix from upstream this should be fine.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/45648501b873
Update lz4 to version 1.9.2. r=froydnj,jfkthame

Comment 22

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/45648501b873