Dictionaries don't need to be > 4 bytes, they need to be >= 4 bytes. This test
was overly conservative.
Also removes the test in `LZ4_attach_dictionary()`.
Fixes a mismatch in behavior between loading into the context (via
`LZ4_loadDict()`) a very small (<= 4 bytes) non-contiguous dictionary, versus
attaching it with `LZ4_attach_dictionary()`.
Before this patch, this divergence could be reproduced by running
```
make -C tests fuzzer MOREFLAGS="-m32"
tests/fuzzer -v -s1239 -t3146
```
Making sure these two paths behave exactly identically is an easy way to test
the correctness of the attach path, so it's desirable that this remain an
unpolluted, high signal test.
so "funny" thing with cppcheck
is that no 2 versions give the same list of warnings.
On Mac, I'm using v1.81, which had all warnings fixed.
On Travis CI, it's v1.61, and it complains about a dozen more/different things.
On Linux, it's v1.72, and it finds a completely different list of a half dozen warnings.
Some of these seems to be bugs/limitations in cppcheck itself.
The TravisCI version v1.61 seems unable to understand %zu correctly, and seems to assume it means %u.
The error can be reproduced using following command :
./frametest -v -i100000000 -s1659 -t31096808
It's actually a bug in the stream LZ4 API,
when starting a new stream
and providing a first chunk to complete with size < MINMATCH.
In which case, the chunk becomes a dictionary.
No hash was generated and stored,
but the chunk is accessible as default position 0 points to dictStart,
and position 0 is still within MAX_DISTANCE.
Then, next attempt to read 32-bits from position 0 fails.
The issue would have been mitigated by starting from index 64 KB,
effectively eliminating position 0 as too far away.
The proper fix is to eliminate such "dictionary" as too small.
Which is what this patch does.
The change is very similar to that of the LZ4_decompress_safe_continue
case. The only reason a make this a separate change is to ensure that
the fuzzer, after it's been enhanced, can detect the flaw in
LZ4_decompress_fast_continue, and that the change indeed fixes the flaw.
The previous change broke decoding with a ring buffer. That's because
I didn't realize that the "double dictionary mode" was possible, i.e.
that the decoding routine can look both at the first part of the
dictionary passed as prefix and the second part passed via dictStart+dictSize.
So this change introduces the LZ4_decompress_safe_doubleDict helper,
which handles this "double dictionary" situation. (This is a bit of
a misnomer, there is only one dictionary, but I can't think of a better
name, and perhaps the designation is not all too bad.) The helper is
used only once, in LZ4_decompress_safe_continue, it should be inlined
with LZ4_FORCE_O2_GCC_PPC64LE attached to LZ4_decompress_safe_continue.
(Also, in the helper functions, I change the dictStart parameter type
to "const void*", to avoid a cast when calling helpers. In the helpers,
the upcast to "BYTE*" is still required, for compatibility with C++.)
So this fixes the case of LZ4_decompress_safe_continue, and I'm
surprised by the fact that the fuzzer is now happy and does not detect
a similar problem with LZ4_decompress_fast_continue. So before fixing
LZ4_decompress_fast_continue, the next logical step is to enhance
the fuzzer.
I noticed that LZ4_decompress_generic is sometimes instantiated with
identical set of parameters, or (what's worse) with a subtly different
sets of parameters. For example, LZ4_decompress_fast_withPrefix64k is
instantiated as follows:
return LZ4_decompress_generic(source, dest, 0, originalSize, endOnOutputSize,
full, 0, withPrefix64k, (BYTE*)dest - 64 KB, NULL, 64 KB);
while the equivalent withPrefix64k call in LZ4_decompress_usingDict_generic
passes 0 for the last argument instead of 64 KB. It turns out that there
is no difference in this case: if you change 64 KB to 0 KB in
LZ4_decompress_fast_withPrefix64k, you get the same binary code.
Moreover, because it's been clarified that LZ4_decompress_fast doesn't
check match offsets, it is now obvious that both of these fast/withPrefix64k
instantiations are simply redundant. Exactly because LZ4_decompress_fast
doesn't check offsets, it serves well with any prefixed dictionary.
There's a difference, though, with LZ4_decompress_safe_withPrefix64k.
It also passes 64 KB as the last argument, and if you change that to 0,
as in LZ4_decompress_usingDict_generic, you get a completely different
binary code. It seems that passing 0 enables offset checking:
const int checkOffset = ((safeDecode) && (dictSize < (int)(64 KB)));
However, the resulting code seems to run a bit faster. How come
enabling extra checks can make the code run faster? Curiouser and
curiouser! This needs extra study. Currently I take the view that
the dictSize should be set to non-zero when nothing else will do,
i.e. when passing the external dictionary via dictStart. Otherwise,
lowPrefix betrays just enough information about the dictionary.
* * *
Anyway, with this change, I instantiate all the necessary cases as
functions with distinctive names, which also take fewer arguments and
are therefore less error-prone. I also make the functions non-inline.
(The compiler won't inline the functions because they are used more than
once. Hence I attach LZ4_FORCE_O2_GCC_PPC64LE to the instances while
removing from the callers.) The number of instances is now is reduced
from 18 (safe+fast+partial+4*continue+4*prefix+4*dict+2*prefix64+forceExtDict)
down to 7 (safe+fast+partial+2*prefix+2*dict). The size of the code is
not the only issue here. Separate helper function are much more
amenable to profile-guided optimization: it is enough to profile only
a few basic functions, while the other less-often used functions, such
as LZ4_decompress_*_continue, will benefit automatically.
This is the list of LZ4_decompress* functions in liblz4.so, sorted by size.
Exported functions are marked with a capital T.
$ nm -S lib/liblz4.so |grep -wi T |grep LZ4_decompress |sort -k2
0000000000016260 0000000000000005 T LZ4_decompress_fast_withPrefix64k
0000000000016dc0 0000000000000025 T LZ4_decompress_fast_usingDict
0000000000016d80 0000000000000040 T LZ4_decompress_safe_usingDict
0000000000016d10 000000000000006b T LZ4_decompress_fast_continue
0000000000016c70 000000000000009f T LZ4_decompress_safe_continue
00000000000156c0 000000000000059c T LZ4_decompress_fast
0000000000014a90 00000000000005fa T LZ4_decompress_safe
0000000000015c60 00000000000005fa T LZ4_decompress_safe_withPrefix64k
0000000000002280 00000000000005fa t LZ4_decompress_safe_withSmallPrefix
0000000000015090 000000000000062f T LZ4_decompress_safe_partial
0000000000002880 00000000000008ea t LZ4_decompress_fast_extDict
0000000000016270 0000000000000993 t LZ4_decompress_safe_forceExtDict
The bug is a read up to 2 bytes past the end of the buffer.
There are three cases for this bug, one for each test case added.
* An empty input causes `token = *ip++` to read one byte too far.
* A one byte input with `(token >> ML_BITS) == RUN_MASK` causes
one extra byte to be read without validation. This could be
combined with the first bug to cause 2 extra bytes to be read.
* The case pointed out in issue #508, where `ip == iend` at the
beginning of the loop after taking the shortcut.
Benchmarks show no regressions on clang or gcc-7 on both my mac
and devserver.
Fixes#508.
The simple change from
`matchIndex+MAX_DISTANCE < current`
towards
`current - matchIndex > MAX_DISTANCE`
is enough to generate a 10% performance drop under clang.
Quite massive.
(I missed as my eyes were concentrated on gcc performance at that time).
The second version is more robust, because it also survives a situation where
`matchIndex > current`
due to overflows.
The first version requires matchIndex to not overflow.
Hence were added `assert()` conditions.
The only case where this can happen is with dictCtx compression,
in the case where the dictionary context is not initialized before loading the dictionary.
So it's enough to always initialize the context while loading the dictionary.
Someone found it would be a great idea to define there a global variable under the very generic name "index".
Cause problem with shadow warnings, so no variable can be named "index" now ...
Also : automatically update API manual
The LZ4 block format specification
states that the last match must start
at a minimum distance of 12 bytes from the end of the block.
However, out of an abundance of caution,
the reference implementation would actually stop searching matches
at 13 bytes from the end of the block.
This patch fixes this small detail.
The new version is now able to properly compress a limit case
such as `aaaaaaaabaaa\n`
as reported by Gao Xiang (@hsiangkao).
Obviously, it doesn't change a lot of things.
This is just one additional match candidate per block, with a maximum match length of 7 (since last 5 bytes must remain literals).
With default policy, blocks are 4 MB long, so it doesn't happen too often
Compressing silesia.tar at default level 1 saves 5 bytes (100930101 -> 100930096).
At max level 12, it saves a grand 16 bytes (77389871 -> 77389855).
The impact is a bit more visible when blocks are smaller, hence more numerous.
For example, compressing silesia with blocks of 64 KB (using -12 -B4D) saves 543 bytes (77304583 -> 77304040).
So the smaller the packet size, the more visible the impact.
And it happens we have a ton of scenarios with little blocks using LZ4 compression ...
And a useless "hooray" sidenote :
the patch improves the LZ4 compression record of silesia (using -12 -B7D --no-frame-crc) by 16 bytes (77270672 -> 77270656)
and the record on enwik9 by 44 bytes (371680396 -> 371680352) (previously claimed by [smallz4](http://create.stephan-brumme.com/smallz4/) ).
For example, with clang:
lz4.c:XXX:36: error: 'register' storage class specifier is deprecated and incompatible with C++17 [-Werror,-Wdeprecated-register]
static unsigned LZ4_NbCommonBytes (register reg_t val)
^~~~~~~~~
static analyzer `cppcheck` complains about a shift-by-32 on a 32 bits value,
which is an undefined behavior.
However, the flagged code path is never triggered in 32-bits mode,
(actually, it's not even generated if DCE kicks in),
the shift-by-32 is necessarily performed on a 64-bits value.
While it doesn't change anything regarding lz4 code generation, for both 32 and 64 bits mode,
(can be checked by md5sum on the generated binary),
the shift has been rewritten in a way which should please this static analyzer,
since it now pretends to shift by 16 on 32-bits cpu (note : it doesn't matter since the code will not even be generated in this case).
Note : this is a blind fix, the new code has not been tested with cppcheck, because cppcheck only works on Windows.
Other static analyzer, such as scan-build, do not trigger this false positive.
FORCE_INLINE macro is defined based on the compiler used. When using
gcc, it will include "__attribute__((always_inline))" forcing gcc to
always inline all the functions marked as FORCE_INLINE. However, this
can cause a performance degradation of about 15%.
This patch allows to set the FORCE_INLINE macro from the compiler
command line to either "static" or "static inline" giving allowing it
to inline functions as needed when performing optimizations.
The bug would make the bt search read one byte in an invalid memory region,
and make a branch decision based on its value.
Impact was small (missed compression opportunity).
It only happens in -BD mode, with extDict-prefix overlapping matches.
The bt match search is supposed to work also in extDict mode.
In which case, the match ptr can point into Dict.
When the match was overlapping Dict<->Prefix,
match[matchLength] would end up outside of Dict, in an invalid memory area.
The correction ensures that in such a case,
match[matchLength] ends up at intended location, inside prefix.
This macro is susceptible to be triggered from user side
typically through compiler flag (-DLZ4_HEAPMODE=1).
In which case, it makes sense to prefix the macro
since we want to reduce potential side-effect on namespace.
