Skip to content

MINOR: [C++][Engine] Validate Substrait emit output_mapping indices - #53521

Open
Nayana-Naik73 wants to merge 1 commit into
apache:mainfrom
Nayana-Naik73:substrait-emit-index-bounds
Open

Nayana-Naik73 wants to merge 1 commit into
apache:mainfrom
Nayana-Naik73:substrait-emit-index-bounds

Conversation

@Nayana-Naik73

Copy link
Copy Markdown

Rationale for this change

While reading the Substrait relation conversion I noticed that GetEmitInfo and ProcessEmitProject take the emit.output_mapping values straight from the plan and use them to index input_schema->field(map_id) and the project expression vector, with no range check. Schema::field only 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-bounds compute::Expression). This is reachable from the public DeserializePlan entry point, which parses untrusted plan bytes, and it applies to essentially every relation kind that goes through the emit path. The neighbouring ProcessExtensionEmit already 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_id in both GetEmitInfo and ProcessEmitProject that returns Status::Invalid for negative or out-of-bounds indices, matching the existing check in ProcessExtensionEmit. 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 single num_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 from Schema::field, and the existing emit round-trip tests continue to cover the valid-index path. Happy to add a serde_test case that feeds an out-of-range outputMapping and expects Status::Invalid if you would prefer one committed.

Are there any user-facing changes?

Deserialising a Substrait plan whose emit output_mapping contains a negative or out-of-range index now returns Status::Invalid rather 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:

  • Human
  • AI

Reviewed before submission by:

  • Human
  • AI
  • Not reviewed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant