geoHeil opened a new issue, #27004:
URL: https://github.com/apache/datafusion/issues/27004
### Describe the bug
`datafusion-proto-common`'s `from_proto` conversions panic at four sites
when a peer sends input that a well-formed encoder never produces. Nearby code
returns an error in the same situations.
1. **Decimal scalar value of the wrong length.** `Decimal32Value`,
`Decimal64Value`, `Decimal128Value` and `Decimal256Value` pass their `value`
bytes to `vec_to_array`, which panics when the length is not the width:
https://github.com/apache/datafusion/blob/636590bf4f4b4f792d3f2fdacedc5fee0ba54950/datafusion/proto-common/src/from_proto/mod.rs#L552-L568
https://github.com/apache/datafusion/blob/636590bf4f4b4f792d3f2fdacedc5fee0ba54950/datafusion/proto-common/src/from_proto/mod.rs#L1421-L1425
2. **Nested scalar whose batch disagrees with its kind.** The nested
`ScalarValue` arm decodes an IPC batch, then calls `record_batch.column(0)`,
which panics on a zero-field schema. It then calls `as_list` /
`as_fixed_size_list` / `as_list_view` / `as_struct` / `as_map`, which panic
when the column is not the type the value kind names:
https://github.com/apache/datafusion/blob/636590bf4f4b4f792d3f2fdacedc5fee0ba54950/datafusion/proto-common/src/from_proto/mod.rs#L523-L544
3. **Union with more than 128 fields and no `type_ids`.**
`UnionFields::from_fields` panics with `"UnionFields cannot contain more than
128 fields"`. The `else` branch just below calls `UnionFields::try_new` and
maps its error. `UnionFields::try_from_fields` is the non-panicking equivalent:
https://github.com/apache/datafusion/blob/636590bf4f4b4f792d3f2fdacedc5fee0ba54950/datafusion/proto-common/src/from_proto/mod.rs#L319-L325
4. **CSV options with an empty `delimiter` or `quote`.** These are indexed
with `[0]`:
https://github.com/apache/datafusion/blob/636590bf4f4b4f792d3f2fdacedc5fee0ba54950/datafusion/proto-common/src/from_proto/mod.rs#L1129-L1130
The same code is in 55.1.0, at `src/from_proto/mod.rs` lines 549-565 and
1256, 521-541, 323, and 1009-1010.
### To Reproduce
Decode a physical plan with
`datafusion_proto::bytes::physical_plan_from_bytes` (or a `ScalarValue` /
`Schema` with `TryFrom`), where the input contains one of:
1. a `Decimal32Value` literal whose `value` is 3 bytes long;
2. a `ListValue` literal whose IPC batch holds a single `Int32` column;
3. a schema with a `Union` field of 129 children and an empty `type_ids`;
4. `CsvOptions` with an empty `delimiter`.
**What we observed, and what we did not.** We decode plan stages from peers,
so we keep a patched copy of 55.1.0 in which each site returns an error. In our
CI, inputs 1-3 reach their sites in that copy and get the error placed there. A
test pins each error message, and a mutation of that message makes the test
fail. We have not run the unpatched crate on these inputs. The panics above are
the documented behaviour of the callees (`vec_to_array`'s own `panic!`,
`as_list`'s "panicking if not possible", `from_fields`'s `expect`). Input 4 has
no test on our side.
### Expected behavior
Each site returns `Err` naming the bad field: `as_*_opt` accessors for site
2, `UnionFields::try_from_fields` for site 3, and `.first().ok_or_else(..)` for
site 4. The caller can then refuse the plan and keep running.
### Additional context
This matters wherever a process decodes plans it did not encode itself, such
as a distributed worker that receives plan stages from peers. Under `panic =
"abort"`, one malformed message stops the whole process. #24726 fixed the same
class of problem for `Constraint` and `TableParquetOptions`. #26998 and #26999
are related cases in `datafusion-proto` and `datafusion-physical-plan`.
The patch is small and each site has a decode test. I can open a PR if that
is welcome.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
