MINOR: [c] reject out-of-range union discriminant in resolver.c - #3904
Open
arib06 wants to merge 1 commit into
Open
MINOR: [c] reject out-of-range union discriminant in resolver.c#3904arib06 wants to merge 1 commit into
arib06 wants to merge 1 commit into
Conversation
avro_resolver_union_branch indexed child_resolvers with the union branch number taken directly from the datum, so a crafted discriminant read past the array and could return a wild consumer that avro_consume_binary then dispatched through. Reject discriminants that are not below num_children, matching the check the value decoder already performs.
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
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 is the purpose of the change
avro_resolver_union_branch(theavro_consume_binary/avro_resolver_newconsumer path) read the writer union branch number from the datum and used it directly to indexchild_resolvers, with no bounds check.read_unioninconsume-binary.cdecodes the discriminant as anint64_tand passes it to a callback typedunsigned int, so a negative or large value becomes a huge index;avro_resolver_union_branchthen readchild_resolvers[discriminant]past the array and, when that slot was non-NULL, handed the garbage pointer back as anavro_consumer_t *thatavro_consume_binarydispatched through. On a two-branch union fed discriminant 100 (bytesC8 01), ASAN reports a heap-buffer-overflow READ of size 8 atresolver.c:1119.The value decoder already rejects out-of-range discriminants; this older consumer path was missed. The check belongs here because this is the single place that indexes the resolver array with the wire value (the
avro_schema_union_branchchokepoint used elsewhere already returns NULL on a miss).Verifying this change
This change added tests and can be verified as follows:
test_avro_resolver_union_bounds, which builds a resolver for a["null","string"]union and feedsavro_consume_binarya datum whose discriminant (100) exceeds the branch count, asserting the read fails instead of crashing. Without the fix the test triggers a heap out-of-bounds read (confirmed under AddressSanitizer); with it the call returnsEILSEQ. The fullctestsuite passes.Documentation