Repository navigation
MINOR: [C++][Engine] Validate Substrait emit output_mapping indices - #53521
Open
Nayana-Naik73 wants to merge 1 commit into
Open
Nayana-Naik73 wants to merge 1 commit into
Nayana-Naik73 wants to merge 1 commit into
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
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.
Rationale for this change
While reading the Substrait relation conversion I noticed that
GetEmitInfoandProcessEmitProjecttake theemit.output_mappingvalues straight from the plan and use them to indexinput_schema->field(map_id)and the project expression vector, with no range check.Schema::fieldonly has debug assertions, so a deserialised plan carrying an out-of-bounds or negative mapping index reads past the end of those vectors (and in the project case moves an out-of-boundscompute::Expression). This is reachable from the publicDeserializePlanentry point, which parses untrusted plan bytes, and it applies to essentially every relation kind that goes through the emit path. The neighbouringProcessExtensionEmitalready range-checks the same index, so the two emit helpers were simply missing the equivalent guard.What changes are included in this PR?
A range check on
map_idin bothGetEmitInfoandProcessEmitProjectthat returnsStatus::Invalidfor negative or out-of-bounds indices, matching the existing check inProcessExtensionEmit. For a ProjectRel the emit input schema and the project expression vector are the same length by construction (input columns are prefixed as passthrough refs), so the singlenum_fields()bound covers both accesses.Are these changes tested?
No new test is included. I confirmed the out-of-bounds access with an AddressSanitizer reproducer of the
std::vector::operator[]sink reached fromSchema::field, and the existing emit round-trip tests continue to cover the valid-index path. Happy to add aserde_testcase that feeds an out-of-rangeoutputMappingand expectsStatus::Invalidif you would prefer one committed.Are there any user-facing changes?
Deserialising a Substrait plan whose emit
output_mappingcontains a negative or out-of-range index now returnsStatus::Invalidrather than reading out of bounds. Valid plans are unaffected.This PR contains a "Critical Fix". It prevents an out-of-bounds read (and an out-of-bounds object move) when converting an untrusted Substrait plan with a malformed emit
output_mapping.Was AI used for this PR?
In accordance to the AI generation guidelines, please disclose below whether and how AI was used in this PR.
PR code and description written by:
Reviewed before submission by: