fix(arrow-ipc): return an error instead of panicking on malformed IPC schema - #10647
fix(arrow-ipc): return an error instead of panicking on malformed IPC schema#10647bit2swaz wants to merge 3 commits into
Conversation
25b6faa to
09664fd
Compare
|
also, while migrating the readers i hit one related panic i left out of scope here: it predates this PR and sits on a different path from the @alamb pinging you because i think you're the best person for this: do you think i should fold the fix into this PR, or file a separate issue? |
| /// Deserialize an ipc [`crate::Schema`] from flat buffers to an arrow [Schema]. | ||
| /// | ||
| /// This panics on malformed input; prefer the fallible [`try_fb_to_schema`]. | ||
| /// kept for backwards compatibility only. |
There was a problem hiding this comment.
we can deprecate this function
09664fd to
335d596
Compare
|
@Jefffrey took your other two notes, pushed
the do you want me to deprecate |
Which issue does this PR close?
Rationale for this change
arrow_ipc::convert::fb_to_schemais infallible and panics on schema messages that the flatbuffer verifier itself accepts. the first thing it does isfb.fields().unwrap(), andfieldsis optional in the flatbuffer schema, so a message with no fields aborts the process.get_data_typehas the same problem deeper down: about 20 morepanic!/unimplemented!and 30unwrap()reachable from decoder input (unknown float precision,Type NONE, out of range enum tags, and so on).every reader path funnels through
fb_to_schema, so there was no way to read Arrow IPC without exposing the process to an abort on untrusted input. it also reaches parquet (an untrusted.parquetwhose footer carries theARROW:schemakey) and arrow-flight.StreamReader::try_newreturnsResult, so malformed input should be anErr, not a process abort.What changes are included in this PR?
get_data_type(alreadypub(crate)) now returnsResult<DataType, ArrowError>; every input reachableunwrap/panic!/unimplemented!is now a returnedParseError.pub fn try_fb_to_schemaand a private fallible field conversion. the readers in arrow-ipc, the parquetARROW:schemapath, and arrow-flight are switched onto it.fb_to_schemaand the publicFrom<crate::Field>impl keep their signatures for compatibility and delegate to the fallible path.Are these changes tested?
yes. a schema message with no fields, one with an unknown float precision, and one with
Type NONEnow returnErrinstead of aborting, asserted by new tests inarrow-ipc. the flatbuffer verifier accepts all three, so they exercise the exact gap. existing round trip tests still pass.Are there any user-facing changes?
no breaking changes.
try_fb_to_schemais additive, andfb_to_schemastill exists with the same signature.