Validate arguments for Unsafe coders
#54
Conversation
Marcono1234
force-pushed
the
unsafe-arg-validation
branch
from
907efb3
to
9cd0e20
Compare
| @@ -7,7 +7,7 @@ | |||
| * Implementation to use on Little Endian architectures. | |||
| */ | |||
| @SuppressWarnings("restriction") | |||
| public class UnsafeChunkEncoderLE | |||
| public final class UnsafeChunkEncoderLE | |||
There was a problem hiding this comment.
For consistency with UnsafeChunkEncoderBE, which was already final.
| @@ -83,7 +88,7 @@ protected int tryCompress(byte[] in, int inPos, int inEnd, byte[] out, int outPo | |||
| ++inPos; | |||
| } | |||
| // try offlining the tail | |||
| return _handleTail(in, inPos, inEnd+4, out, outPos, literals); | |||
| return _handleTail(in, inPos, inEnd+TAIL_LENGTH, out, outPos, literals); | |||
There was a problem hiding this comment.
I hope this is correct; to me it looked like this undoes the -= TAIL_LENGTH at the beginning of the method:
| // Sanity checks; otherwise if any of the arguments are invalid `Unsafe` might corrupt memory | ||
| _checkArrayIndices(in, inPos, inEnd); | ||
| _checkArrayIndices(out, outPos, out.length); | ||
| // TODO: Validate that `out.length - outPos` is large enough? |
There was a problem hiding this comment.
I think this has to be validated because it does not look like the code below performs any bounds checks, but I am not sure what the correct minimum size here is. When I used LZFChunk.MAX_CHUNK_LEN1, a few unit tests were failing, which were using smaller output buffers.
Or alternatively it would be necessary to add bounds checks below to make sure Unsafe does not write outside out.
Footnotes
-
Maybe the minimum size would have to be even larger, including
LZFChunk.MAX_HEADER_LEN? ↩
|
I think these make sense, but I would suggest splitting this PR into multiple smaller PRs. One thing to note: I don't seem to have commit access to this repo any more, so I can not merge PRs. |
|
Thanks for the feedback! That is quite unfortunate that you lost access to this repository. Would you like to continue maintaining this library? In that case, do you think it would be possible for you to obtain maintainer and publishing rights again (or maybe even transfer the repository to your GitHub account)? I am not sure if or how many alternatives to this library exists, and there seem to be some large projects currently using this library, see https://mvnrepository.com/artifact/com.ning/compress-lzf/usages (the latest version of some of these might not have a dependency on compress-lzf anymore though). I have also written a mail to your |
|
@Marcono1234 I can file an issue and see if any of maintainers is around. Yes, I could continue maintaining this package, although its usage is definitely declining -- note that MvnRepository has no easy way to find out if the latest version(s) of some package still depends on package, just that some version does. |
Adds validation for the encoder and decoder using
Unsafeto protect against accidental memory corruption. This is mainly to protect against implementation bugs, and against users who might by accident provide invalid arguments.I am not very familiar with this code, so any feedback and corrections are welcome!
I have added some review comments below to explain the rationale for the changes.
Assuming users are not using any of these low level methods or classes directly, this probably has no security impact.