apache / apache/arrow

[C++][Substrait] RelCommon.emit indices are not bounds-checked before they index the schema

Open
#51,243 1 comment 0 reactions 0 assignees View on GitHub
Dominant language
C++
Stars
17.1k
Forks
4.3k
Avg merge
3d 13h
Merged PRs (30d)
88

Description

`GetEmitInfo` passes every value in `RelCommon.emit.output_mapping` to both `FieldRef(map_id)` and the unchecked `input_schema->field(map_id)`. `ProcessEmitProject` does the same on the project path, where the value also indexes `proj_options.expressions`. With this implementation, a positive out-of-range index still reads past the end of a `FieldVector` and the process dies. For `-1`, Acero later rejects `FieldRef(-1)` with `ArrowInvalid`, but the unchecked schema lookup happens first. `ProcessExtensionEmit`, in the same file, already returns `Status::Invalid("Out of bounds emit index ", emit_idx)`.

```
read, emit [1, 0] [DataType(double), DataType(int64)]
read, emit [5] exit 139
read, emit [-1] ArrowInvalid: No match for FieldRef.FieldPath(-1)
project, emit [5] exit 139
aggregate, emit [1, 0] exit 139
```

The last variant is why this is more than a check on malformed input: that plan is valid Substrait. Arrow's vendored proto has no `expression_references`, so Arrow drops the grouping keys and derives an empty aggregate schema. The plan's valid `[1, 0]` mapping is then out of range for that empty schema. That grouping-key loss is #50634. A bounds check would turn the process crash into an error that reports the rejected `[1, 0]` mapping; fixing #50634 is still required for the plan to run.

Rechecked with PyArrow `26.0.0.dev244+g79e074ace`, built from `main` at `79e074ace3a7c4ca26f211dfd84a1984ad53c362`, on macOS arm64. The two positive out-of-range cases and the aggregate case still exit 139; the negative case now returns `ArrowInvalid`. The original PyArrow 25.0.1 reproduction crashed for the negative case as well on macOS arm64 and Linux x86-64. The source is `cpp/src/arrow/engine/substrait/relation_internal.cc`.

Reproducer — one file, pyarrow only

```python
"""RelCommon.emit.output_mapping indices are used to index the input schema unchecked.

The control succeeds and the negative case is caught as `ArrowInvalid`. Run each variant in a process of its own because the other three terminate:
for v in 0 1 2 3 4; do python3 emit_repro.py $v || echo " exit $?"; done
"""
import json, sys
import pyarrow as pa
import pyarrow.substrait as ps
from pyarrow._substrait import _parse_json_plan

SCHEMA = pa.schema([pa.field("c0", pa.int64(), nullable=False),
pa.field("c1", pa.float64(), nullable=False)])

def provider(names, schema=None):
return pa.table({"c0": [1], "c1": [2.5]}, schema=schema or SCHEMA)

def ref(i):
return {"selection": {"directReference": {"structField": {"field": i} if i else {}},
"rootReference": {}}}

def emit(m):
return {"common": {"emit": {"outputMapping": m}}}

def read(m=None):
r = {"baseSchema": {"names": ["c0", "c1"], "struct": {
"types": [{"i64": {"nullability": "NULLABILITY_REQUIRED"}},
{"fp64": {"nullability": "NULLABILITY_REQUIRED"}}],
"nullability": "NULLABILITY_REQUIRED"}},
"namedTable": {"names": ["t"]}}
return {"read": dict(r, **(emit(m) if m else {}))}

def run(rel, names):
plan = {"version": {"minorNumber": 102, "producer": "repro"},
"relations": [{"root": {"input": rel, "names": names}}]}
return ps.run_query(_parse_json_plan(json.dumps(plan).encode()), table_provider=provider)

# A plan that is valid Substrait: the grouping keys are where current Substrait puts them.
# Arrow's vendored proto has no expression_references, so it drops them (#50634) and the
# aggregate's output schema has no fields at all - which puts a correct mapping out of range.
AGG = {"aggregate": dict({"input": read(),
"groupings": [{"expressionReferences": [0, 1]}],
"groupingExpressions": [ref(0), ref(1)]}, **emit([1, 0]))}

VARIANTS = [
("read, emit [1, 0]", lambda: run(read([1, 0]), ["c1", "c0"])),
("read, emit [5]", lambda: run(read([5]), ["x"])),
("read, emit [-1]", lambda: run(read([-1]), ["x"])),
("project, emit [5]", lambda: run({"project": dict(
{"input": read(), "expressions": [ref(0)]},
**emit([5]))}, ["x"])),
("aggregate, emit [1, 0]", lambda: run(AGG, ["c1", "c0"])),
]

label, case = VARIANTS[int(sys.argv[1])]
print("%-22s" % label, end=" ", flush=True)
try:
print(case().read_all().schema.types)
except Exception as e:
print(type(e).__name__ + ":", str(e).replace("\n", " ")[:70])
```

Contributor guide

Open the contributing guide

Research direction

Start with cpp/src/arrow/engine/substrait/relation_internal.cc, reading GetEmitInfo, ProcessEmitProject, and ProcessExtensionEmit to compare their emit-index handling. Run the supplied emit_repro.py variants to reproduce the crashes and invalid-index behavior. Done means out-of-range mappings return an error instead of indexing past the schema, while valid mappings continue to work.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp, python
Domain
data-engineering
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
72/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.