Fix null checks for engine unwrap buffer lists. #1403
Draft
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fixes google#1372 I verified on OpenJDK 8, 11 and 21 that this is the RI expected behaviour, i.e. a null entry in a ByteBuffer array that is outside the selected range should *not* cause wrap or unwrap to fail. Also made the unwrap check methods for srcs and dests more aligned with each other, although ultimately this should probably be more like the wrap approach where we simply take a copy of the selected ByteBuffer array range. Now we have better tests we can do that in a future CL. This is a slight behaviour change on all platforms (including Mainline) but as there were no tests for it, it won't break any CI, and as it is making the checks slightly more lenient it seems very unlikely to break any application code. No impact on SSLSocket implementation as that only ever uses a single buffer. I added a preconditions check for unwrap along the lines of the one for wrap (and converted to assertThrows). Also because my spidey sense was tingling and I wasn't sure that this miscount could ever cause unwrap to produce more data than it was meant to I added an array offsets test along the lines of the one for wrap, only a bit more involved as it first checks that the exact amount of data flows as well as returning BUFFER_UNDERFLOW if you try to unwrap more data than is supposed to fit. Didn't find any bugs but it's still a good regression test.
Collaborator
|
Can we split this? I think it would be better to first just add the tests, and probably also check in a test that exposes the fautly behavior. And then do a minimal PR that only fixes the bug. |
Contributor
Author
|
Very good point, thanks, I'll rework this. On re-reading it also seems to me that (1) some of the new tests are not very readable (sorry!) and (2) I'm not sure we're being consistent with the use of longs vs ints when calculating capacities. Gonna leave this PR open as a reference point and submit new PRs for the component parts. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #1372
I verified on OpenJDK 8, 11 and 21 that this is the RI expected behaviour, i.e. a null entry in a ByteBuffer array that is outside the selected range should not cause wrap or unwrap to fail.
Also made the unwrap check methods for srcs and dests more aligned with each other, although ultimately this should probably be more like the wrap approach where we simply take a copy of the selected ByteBuffer array range. Now we have better tests we can do that in a future CL.
This is a slight behaviour change on all platforms (including Mainline) but as there were no tests for it, it won't break any CI, and as it is making the checks slightly more lenient it seems very unlikely to break any application code.
No impact on SSLSocket implementation as that only ever uses a single buffer.
I added a preconditions check for unwrap along the lines of the one for wrap (and converted to assertThrows).
Also because my spidey sense was tingling and I wasn't sure that this miscount could ever cause unwrap to produce more data than it was meant to I added an array offsets test along the lines of the one for wrap, only a bit more involved as it first checks that the exact amount of data flows as well as returning BUFFER_UNDERFLOW if you try to unwrap more data than is supposed to fit. Didn't find any bugs but it's still a good regression test.