1544980 - Update lz4 to 1.9.1

Status: RESOLVED FIXED

(Core :: MFBT, task)

Description

[Ryan VanderMeulen [:RyanVM]]

Changes in v1.9.0:
perf: large decompression speed improvement on x86/x64 (up to +20%) by @djwatson
api : changed : _destSize() compression variants are promoted to stable API
api : new : LZ4_initStream(HC), replacing LZ4_resetStream(HC)
api : changed : LZ4_resetStream(HC) as recommended reset function, for better performance on small data

Comment 1

[Ryan VanderMeulen [:RyanVM]]

Ruh roh, ASAN says it has a heap-buffer-overflow issue :(
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=240847972&repo=try&lineNumber=2463

Comment 2

[Mike Hommey [:glandium]]

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

Ruh roh, ASAN says it has a heap-buffer-overflow issue :(
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=240847972&repo=try&lineNumber=2463

I wouldn't be surprised if that's why it's faster.

Comment 3

[Ryan VanderMeulen [:RyanVM]]

Kris, do you have any idea why our Addon Manager startup code might be causing LZ4 to heap-buffer-overflow in ASAN builds? Non-ASAN builds all appears to be OK AFAICT.
https://hg.mozilla.org/try/rev/5273a90f351b7c5fe46473467d127dd1a6b098a3

Comment 4

[Kris Maglione [:kmag]]

Hm. No obvious reason that I can see or think of. This is a buffer read overflow, and unfortunately the LZ4 API doesn't let us pass an input buffer size, only an output buffer size, so it's up to the LZ4 library to figure out how much to read.

Comment 5

[Kris Maglione [:kmag]]

Actually, I guess there is a version that lets us specify the input size. I guess we should switch to that. But it would still be nice to know why the other version is broken.

Comment 6

[Kris Maglione [:kmag]]

(In reply to Kris Maglione [:kmag] from comment #5)

Actually, I guess there is a version that lets us specify the input size. I guess we should switch to that. But it would still be nice to know why the other version is broken.

Although it looks like that version is meant to be slower...

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Someone else found the same issue and reported it (publicly) upstream.
https://github.com/lz4/lz4/issues/676

Upstream maintainer told me via email that a point release will be coming soon to fix it.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Updating the summary to reflect that we won't be updating until 1.9.1 comes out.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

v1.9.1 is out. Changes in the release:
fix : decompression functions were reading a few bytes beyond input size (introduced in v1.9.0, reported by @ppodolsky and @danlark1)
api : fix : lz4frame initializers compatibility with c++, reported by @degski

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Attachment: Bug 1544980 - Update lz4 to version 1.9.1. r=froydnj

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Looking better on Try than v1.9.0.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=14119eb3741ae47d470554efd1125ddb0a049b18

Comment 12

[Pulsebot]

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

Comment 13

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/8d09ff545a5b

Comment 14

[Sylvestre Ledru [:Sylvestre]]

FYI, Coverity found 3 new issues in this lib. Two Explicit null dereferenced (medium severity) and one Large stack use (low severity)

Comment 15

[Ryan VanderMeulen [:RyanVM]]

I passed that info along to the upstream developer and they replied:

I've been looking at the static analyzer warnings,
and as suspected, they are false positives.

One common situation is that some variables have relations, while the static analyzer consider them freely independent.
For example, if dict == usingExtDict, then necessarily dictEnd != NULL,
but that's something that the static analyzer doesn't understand, so it analyzes an impossible combination.

In the past, I've been trying to add assert() to help the analysis, but the static analyzer would ignore them.
Not sure if there is a reasonable solution, I don't want to contort the source code just to please the analyzer,
but anything that helps readability in the other hand is welcomed.