On Sun, Feb 18, 2018 at 3:33 AM, Alan Bateman <[email protected]> wrote: > Thanks for bringing this one up again
Thanks for taking the time to review. > I see Sherman is looking at implementation so I'll stick with the javadoc > for now. At some point it will need a security review to ensure that there > no possibility of tricking the implementation to access memory outside of > the input/output. That is, we have to assume the user of > setInput(ByteBuffer) is evil and will change the position/limit during > deflate operations. I see the patch computes clamps the remainder before > accessing memory, it will just need a closer look to make sure there are no > issues. The patch will also need adjustments to make it consistent with the > existing code but that can come later. I did write the code with this in mind: that the address should always be within the bounds of the buffer (be it heap or direct) at the time of the call, and that the offset into the buffer should not be beyond its end (or beginning). So the effect could be a thrown exception but escaping the buffer _should_ be impossible (by intent anyway; more eyes are always better for noticing mistakes of course). > On the APIs then the inflate and deflate methods look okay, I'll less sure > that setDcitionary(ByteBuffer) is really needed. Are you adding for > setDcitionary(ByteBuffer) for consistency? Yes; it was easy enough to add it so I did. > The javadoc doesn't currently specify how it works with the buffer, e.g. > inflate(ByteBuffer) doesn't specify that adds it bytes to the buffer > starting at its position, it doesn't say if the position is adjusted. The > javadoc will also need to set expectations on behavior when > DataFormatException is thrown, is the position guaranteed to be unchanged or > is it unspecified? I intended for it to work similarly to the old code. But I'll go through the zlib docs and just make sure all the assumptions are still correct, and the next version will have more concise docs in this regard and also with regards to the disposition of the buffer in each case. -- - DML
