GH-2402: RDF Patch Binary handles malformed inputs better #2408
Conversation
This file contains 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
rvesse
force-pushed
the
rdf-patch-binary-invalid-inputs
branch
from
3c9d53e
to
b028b3e
Compare
rvesse
changed the title
Previously the RDFPatchReaderBinary simply exited its read loop when it detected an EOF error as Thrift doesn't have a clean way of apriori detecting whether we've reached the EOF. The downside of this approach was that it meant the reader would silently ignore some genuinely malformed inputs if they happened to be the right bytes for Thrift to think it had encountered a field it knew about. To address this a new method is introduced that inspects the StackTraceElement's associated with the Thrift EOF exception to detect a couple of cases where the input is clearly malformed and throw an appropriate error. Test cases around malformed patch inputs are also expanded to further test this capability.
rvesse
force-pushed
the
rdf-patch-binary-invalid-inputs
branch
from
b028b3e
to
ef4dc79
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Discovered in $dayjob work that
RDFPatchReaderBinarywould silently accept malformed inputs in some cases. Started by adding several test cases, some of which failed, to demonstrate that the RDF Patch Binary reader can silently accept invalid streams. Then debugged from there are introduced additional logic which makes a best effort attempt to distinguish between genuine EOF and EOF due to malformed input that should produce an error.Unfortunately the Thrift API doesn't have a clean way to detect that you've genuinely reached the EOF so we just have to attempt to read the next row from the patch, and detect the obvious cases of malformed input:
TProtocolUtil.skip()getting called, this indicates that Thrift recognised a Field ID but that the Type ID was not the expected type for that field. So if we hit a EOF while skipping over such a field we're definitely reading malformed input.TUnion.read()getting called multiple times. This means that we're partway through reading an otherwise valid data structure at the point where we encounter the EOF so again is a clear indicator of malformed input.I appreciate that this solution is somewhat hacky, if anyone has a more robust solution please feel free to suggest it
GitHub issue resolved #2402
By submitting this pull request, I acknowledge that I am making a contribution to the Apache Software Foundation under the terms and conditions of the Contributor's Agreement.
See the Apache Jena "Contributing" guide.