Repository navigation
GH-54025: [C++][Engine] Validate Substrait emit output_mapping indices - #53521
Nayana-Naik73 wants to merge 2 commits into
Conversation
|
@Nayana-Naik73 I don't think this is an MINOR PR, could you perhaps open an issue for this. |
dmitry-chirkov-dremio
left a comment
There was a problem hiding this comment.
The bounds checks look correct and consistently cover both emit paths. However, given that this fixes an untrusted-input memory-safety issue, the ASan reproducer is not a substitute for regression coverage.
Let's add tests verifying that negative and out-of-range output_mapping indices return Status::Invalid through both the generic and ProjectRel paths.
|
@Reranko05 fair enough, I've opened #54025 for it and retitled the PR to match. @dmitry-chirkov-dremio added ReadRelWithOutOfBoundsEmit and ProjectRelWithOutOfBoundsEmit to serde_test.cc. Each one feeds a negative and an out-of-range outputMapping index through DeserializePlans and expects Status::Invalid, so both the generic GetEmitInfo path and ProcessEmitProject are covered. I checked them against the unfixed code too, where both trip the Schema::field DCHECK in a debug build. |
|
|
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?
Yes. Added ReadRelWithOutOfBoundsEmit and ProjectRelWithOutOfBoundsEmit to serde_test.cc, which deserialise plans with negative and out-of-range outputMapping indices through the generic emit path (ReadRel) and the ProjectRel path and expect Status::Invalid. Without the fix both tests hit the Schema::field DCHECK in a debug build. The existing emit round-trip tests still cover the valid-index path.
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: