Improve bounds checks for LZ4 #112
Conversation
Marcono1234
force-pushed
the
lz4-bounds-checks
branch
from
12d7927 to
bfa555d
Compare
| if (bound < 0) { | ||
| throw new IllegalArgumentException("Invalid size"); | ||
| } | ||
| return bound; |
There was a problem hiding this comment.
Checks for overflow. An alternative could be to change the return type to long, but I am not sure if the callers can handle that better, and it would be an incompatible change to this public API.
src/main/java/one/nio/lz4/LZ4.java
Outdated
|
|
||
| public static int compress(byte[] src, int srcOffset, byte[] dst, int dstOffset, int length) { | ||
| if (srcOffset < 0 || srcOffset + length > src.length || dstOffset < 0 || length < 0) { | ||
| if (srcOffset < 0 || srcOffset > src.length - length || dstOffset < 0 || length < 0) { |
There was a problem hiding this comment.
Should be identical (performs a - length on both sides of the comparison), except that it is overflow-safe for large length values.
| int s; | ||
| do { | ||
| int s = 255; | ||
| while (ip < srcEnd - RUN_MASK && s == 255) { | ||
| s = unsafe.getByte(src, ip++) & 0xff; | ||
| length += s; | ||
| } while (ip < srcEnd - RUN_MASK && s == 255); | ||
| } |
There was a problem hiding this comment.
Previously performed an out-of-bounds read by one byte if the input buffer did not have enough data.
Not sure if that is only an issue for the first iteration of the enclosing for loop. In that case maybe the do-while loop could be kept and instead an additional bounds check could be added outside the for loop. Though I am not very familiar with the LZ4 format, and whether such a change would work.
src/main/java/one/nio/lz4/LZ4.java
Outdated
| return size + size / 255 + 16; | ||
| int bound = size + size / 255 + 16; | ||
| if (bound < 0) { | ||
| throw new IllegalArgumentException("Invalid size"); |
There was a problem hiding this comment.
Please add detail to message, on which input it fails
There was a problem hiding this comment.
Do you mean something like "Invalid size: " + size? Besides the size parameter this method does not have any additional context it could add.
And because this method is public, it might not necessarily be a "too large" size, but could also be a negative size provided by the user (though in that case this validation does not catch all negative values).
The main point here is to protect against overflow, because otherwise at the call sites the bounds checks which rely on the result of compressBound would be ineffective in case of overflow.
|
Have added tests and also added a missing check for |
| } | ||
|
|
||
| @Test | ||
| public void compress_BoundsChecks() { |
There was a problem hiding this comment.
Please use standart java code style with camelCase, please avoid _ usage here and below?
As possible variant: testCompressBoundsChecks
There was a problem hiding this comment.
Ok I can omit the _ if you prefer. That makes it a bit more difficult to tell apart tested method (e.g. compress) from the scenario (e.g. BoundsChecks).
There was a problem hiding this comment.
Interesting point, but it differs from used test naming approach in project
| int s; | ||
| do { | ||
| int s = 255; | ||
| while (ip < srcEnd - RUN_MASK && s == 255) { |
There was a problem hiding this comment.
is decompress_Incomplete cover this change?
There was a problem hiding this comment.
If yes, please be aware about this check, most likely first branch in native code is executed:
if (NativeLibrary.IS_SUPPORTED) {
result = decompress0(src, srcOffset, dst, dstOffset, length, dst.length - dstOffset);
} else {
result = decompress(src, byteArrayOffset + srcOffset, dst, byteArrayOffset + dstOffset, length, dst.length - dstOffset);
}
There was a problem hiding this comment.
Yes the test decompress_Incomplete is supposed to cover this, but good point that this is not reached if the native library is used.
But there is nothing I can do about it, can I? Or do you have any suggestion?
(locally I was testing with a modified version where always the non-native implementation was used)
There was a problem hiding this comment.
@Marcono1234 I think we can add environment flag to choose testing branch or simply make
private static int decompress
package local and directly call it in test
2 participants
Tries to improve the bounds checks for
LZ4.A few general notes:
LZ4.java; not sure if other classes or the native code might have similar issuesSee the review comments below for additional notes.