Nayana-Naik73 opened a new pull request, #53521:
URL: https://github.com/apache/arrow/pull/53521

   ### 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](https://arrow.apache.org/docs/dev/developers/overview.html#ai-generated-code),
 please disclose below whether and how AI was used in this PR.
   
   **PR code and description written by:**
   
   - [ ] Human
   - [x] AI
   
   **Reviewed before submission by:**
   
   - [x] Human
   - [ ] AI
   - [ ] Not reviewed
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to