Closed Bug 1575293 Opened 1 year ago Closed 3 months ago

Update lz4 to 1.9.2

Categories

(Core :: MFBT, task)

task
Not set
normal

Tracking

()

RESOLVED FIXED
84 Branch
Tracking Status
firefox84 --- fixed

People

(Reporter: RyanVM, Assigned: RyanVM)

References

()

Details

(Whiteboard: [third-party-lib-audit])

Attachments

(1 file)

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

(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. :(

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

Flags: needinfo?(ryanvm) → needinfo?(dholbert)
Blocks: 1572302

(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!

Flags: needinfo?(dholbert)

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.

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...

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.

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.

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

(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.

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.

Flags: needinfo?(ryanvm)

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

Flags: needinfo?(ryanvm)

This bug is now fixed from comment #14.

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

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

Flags: needinfo?(jfkthame)

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

Flags: needinfo?(jfkthame)
Pushed by rvandermeulen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/45648501b873
Update lz4 to version 1.9.2. r=froydnj,jfkthame
Status: NEW → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → 84 Branch
No longer blocks: 1572302
Blocks: 1682604
You need to log in before you can comment on or make changes to this bug.