AVRO-4048: [java] Handle InputStream.skip() robustly in binary decoders - #3967
Open
iemejia wants to merge 1 commit into
Open
AVRO-4048: [java] Handle InputStream.skip() robustly in binary decoders#3967iemejia wants to merge 1 commit into
iemejia wants to merge 1 commit into
Conversation
The binary decoders mishandled InputStream.skip() returning 0. Per its contract, skip() may return 0 without being at end of stream, and negative returns are not specified. The previous code treated two consecutive 0 returns (and any negative return) as EOF, which could raise a spurious EOFException on streams that legitimately return 0 from skip(). It also carried a dead "negative return" branch. Additionally, BinaryDecoder.InputStreamByteSource.trySkipBytes requested the original full length on every iteration (in.skip(length)) instead of the remaining amount (in.skip(leftToSkip)). On a partial skip this re-requested too much and could advance the stream past the intended position (over-skip / stream corruption). Replace the fragile heuristics with a single-byte read() probe: when skip() returns a non-positive value, read one byte to distinguish a genuine EOF (read() == -1) from a transient inability to skip, then continue. Fix trySkipBytes to skip only the remaining count. The same read()-probe logic is applied to DirectBinaryDecoder.doSkipBytes, which previously treated skip() == 0 as immediate EOF. Adds TestBinaryDecoderSkip covering: no spurious EOF on a zero-skip stream (buffered and direct), genuine EOF still detected when skipping past the end, and no over-skip on partial-skip streams.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
What changes were proposed in this pull request?
The binary decoders mishandled
InputStream.skip(). Per its contract,skip()may return0without being at end of stream, and negative returns are not specified. This PR fixes three related problems:Spurious EOF (the reported issue).
BinaryDecoder.InputStreamByteSource.skipSourceBytes/trySkipBytestreated two consecutive0returns as EOF (EOFException), so a stream that legitimately returns0fromskip()could fail spuriously. It also carried a dead "negative return" branch (AVRO-4048).Over-skip / stream corruption.
trySkipBytesrequested the original full length on every iteration (in.skip(length)) instead of the remaining amount (in.skip(leftToSkip)). On a partial skip this re-requests too much and can advance the underlying stream past the intended position, returning a skipped-count larger than requested.DirectBinaryDecoder.doSkipBytestreatedskip() <= 0as immediate EOF, sharing the same spurious-EOF problem.How was this patch fixed?
Replace the fragile "two zeros = EOF" / negative-branch heuristics with a single-byte
read()probe: whenskip()returns a non-positive value, read one byte to tell a genuine EOF (read() == -1) apart from a transient inability to skip, then continue. This removes the dead negative branch, eliminates the spurious EOF, and cannot infinite-loop.trySkipBytesnow skips only the remaining count (leftToSkip). The same probe is applied toDirectBinaryDecoder.doSkipBytes.How was this patch tested?
New
TestBinaryDecoderSkip:bufferedSkipFixedWithZeroSkipStream/directSkipFixedWithZeroSkipStream— a stream whoseskip()always returns0no longer causes a spuriousEOFException; the decoder is positioned exactly after the skipped bytes.bufferedSkipFixedPastEndThrows/directSkipFixedPastEndThrows— skipping past the real end still raisesEOFException.inputStreamSkipDoesNotOverSkip— a partial-skip stream is skipped by exactly the requested count (no over-skip).Verified the tests fail against the current code (over-skip returns 6 instead of 5; the zero-skip cases throw
EOFException) and pass with the fix. No regressions acrossTestBinaryDecoder(68),TestValidatingIO(972),TestBlockingIO(376), andTestDataFile, under the default, custom-coders, and without-fast-reader surefire profiles.