From c82771e751770bb7986f719851ee8171a6dbabf1 Mon Sep 17 00:00:00 2001 From: jackylee Date: Thu, 3 Sep 2026 13:08:25 +0800 Subject: [PATCH 1/2] fix: don't panic on malformed struct field dtypes Opening a crafted file whose struct has a field with an undecodable dtype panicked instead of erroring. Field dtypes are decoded lazily, so the top-level dtype segment parsed cleanly and the panic fired later, in StructFields::fields() while reading the file-statistics footer. The accessors that decode a field dtype return a plain DType, and the PartialEq and Hash impls on FieldDType are trait methods, so no signature there can carry a failure. The check therefore belongs where the fields are built from a flatbuffer: try_from_fields decodes each field dtype, and deserialization goes through it. UnionVariants had the identical shape and gets the same check. This costs one decode per field at open time, which is the price of the accessors staying infallible. Signed-off-by: jackylee --- vortex-array/src/dtype/serde/flatbuffers.rs | 119 ++++++++++++++++++-- vortex-array/src/dtype/struct_.rs | 40 +++++++ vortex-array/src/dtype/union.rs | 10 ++ 3 files changed, 160 insertions(+), 9 deletions(-) diff --git a/vortex-array/src/dtype/serde/flatbuffers.rs b/vortex-array/src/dtype/serde/flatbuffers.rs index 8a1e7e6b611..d63bba5f505 100644 --- a/vortex-array/src/dtype/serde/flatbuffers.rs +++ b/vortex-array/src/dtype/serde/flatbuffers.rs @@ -89,15 +89,7 @@ impl StructFields { }) .collect::>(); - if names.len() != dtypes.len() { - vortex_bail!( - "length mismatch between struct names ({}) and dtypes ({})", - names.len(), - dtypes.len() - ); - } - - Ok(StructFields::from_fields(names, dtypes)) + StructFields::try_from_fields(names, dtypes) } } @@ -754,6 +746,115 @@ mod test { assert_eq!(viewed, eager); } + /// The reported crash (#8848): a struct field whose own dtype cannot be decoded. + /// Field dtypes used to be decoded lazily, so this parsed cleanly here and + /// panicked later, in `StructFields::fields()` while reading the file-statistics + /// footer. It must be an error at parse time instead. + #[test] + fn test_struct_field_with_undecodable_dtype_errors() { + let mut fbb = FlatBufferBuilder::new(); + let name = fbb.create_string("bad"); + let names = fbb.create_vector(&[name]); + // A struct dtype with `names` omitted: the flatbuffer verifier accepts it + // (`names` is optional in the schema) but decoding it fails. This is the shape + // the fuzzer produced — structurally valid, semantically undecodable. + let inner_struct = fb::Struct_::create( + &mut fbb, + &fb::Struct_Args { + names: None, + dtypes: None, + nullable: false, + }, + ); + let bad_field = fb::DType::create( + &mut fbb, + &fb::DTypeArgs { + type_type: fb::Type::Struct_, + type_: Some(inner_struct.as_union_value()), + }, + ); + let dtypes = fbb.create_vector(&[bad_field]); + let struct_table = fb::Struct_::create( + &mut fbb, + &fb::Struct_Args { + names: Some(names), + dtypes: Some(dtypes), + nullable: false, + }, + ); + let dtype = fb::DType::create( + &mut fbb, + &fb::DTypeArgs { + type_type: fb::Type::Struct_, + type_: Some(struct_table.as_union_value()), + }, + ); + fbb.finish_minimal(dtype); + let (vec, start) = fbb.collapse(); + let end = vec.len(); + let buffer = FlatBuffer::align_from(ByteBuffer::from(vec).slice(start..end)); + + let root_fb = root::(&buffer).unwrap(); + let view = ViewedDType::from_fb_loc(root_fb._tab.loc(), buffer, SESSION.clone()); + + let err = DType::try_from(view).expect_err("undecodable field dtype must not parse"); + assert!( + err.to_string().contains("bad"), + "error should name the field: {err}" + ); + } + + /// The same defect on the union side: `UnionVariants::variants()` also decodes lazily + /// and also cannot report a failure. + #[test] + fn test_union_variant_with_undecodable_dtype_errors() { + let mut fbb = FlatBufferBuilder::new(); + let name = fbb.create_string("bad"); + let names = fbb.create_vector(&[name]); + let inner_struct = fb::Struct_::create( + &mut fbb, + &fb::Struct_Args { + names: None, + dtypes: None, + nullable: false, + }, + ); + let bad_variant = fb::DType::create( + &mut fbb, + &fb::DTypeArgs { + type_type: fb::Type::Struct_, + type_: Some(inner_struct.as_union_value()), + }, + ); + let dtypes = fbb.create_vector(&[bad_variant]); + let type_ids = fbb.create_vector(&[0i8]); + let union_table = fb::Union::create( + &mut fbb, + &fb::UnionArgs { + names: Some(names), + dtypes: Some(dtypes), + type_ids: Some(type_ids), + nullable: false, + }, + ); + let dtype = fb::DType::create( + &mut fbb, + &fb::DTypeArgs { + type_type: fb::Type::Union, + type_: Some(union_table.as_union_value()), + }, + ); + fbb.finish_minimal(dtype); + let (vec, start) = fbb.collapse(); + let end = vec.len(); + let buffer = FlatBuffer::align_from(ByteBuffer::from(vec).slice(start..end)); + + let root_fb = root::(&buffer).unwrap(); + let view = ViewedDType::from_fb_loc(root_fb._tab.loc(), buffer, SESSION.clone()); + + DType::try_from(view).expect_err("undecodable variant dtype must not parse"); + } + #[test] fn test_struct_malformed_flatbuffer() { let mut fbb = FlatBufferBuilder::new(); diff --git a/vortex-array/src/dtype/struct_.rs b/vortex-array/src/dtype/struct_.rs index 95f73295d52..438a797fc63 100644 --- a/vortex-array/src/dtype/struct_.rs +++ b/vortex-array/src/dtype/struct_.rs @@ -319,6 +319,46 @@ impl StructFields { Self::from_fields(names, dtypes) } + /// Create a new [`StructFields`] from names and [`FieldDType`]s that may be backed by a + /// flatbuffer, decoding every field dtype so that later reads cannot fail. + /// + /// Deserialization must go through this rather than [`Self::from_fields`]. A + /// [`FieldDType`] holding a view decodes lazily, and the accessors that decode it + /// ([`Self::field`], [`Self::field_by_index`], [`Self::fields`]) return a plain + /// [`DType`], as do the [`PartialEq`] and [`Hash`] impls on the field dtype — those + /// are trait methods, so their signatures cannot carry a failure. A field dtype whose + /// union discriminant is unknown therefore has nowhere to surface except a panic, and + /// because the check was lazy the top-level dtype segment parsed cleanly and the panic + /// only fired later, while reading the file-statistics footer. + /// + /// Validating here costs one decode per field at open time, which is the price of the + /// accessors staying infallible. + /// + /// # Errors + /// + /// Returns an error if `names` and `dtypes` differ in length, or if any field dtype + /// cannot be decoded. + pub fn try_from_fields(names: FieldNames, dtypes: Vec) -> VortexResult { + if names.len() != dtypes.len() { + vortex_bail!( + "length mismatch between names ({}) and dtypes ({})", + names.len(), + dtypes.len() + ); + } + + for (name, dtype) in names.iter().zip_eq(dtypes.iter()) { + dtype + .value() + .map_err(|e| e.with_context(format!("invalid dtype for struct field {name}")))?; + } + + Ok(Self(Arc::new(StructFieldsInner::from_fields( + names, + dtypes.into(), + )))) + } + /// Create a new [`StructFields`] from a list of names and [`FieldDType`] which can be either lazily or eagerly serialized. pub fn from_fields(names: FieldNames, dtypes: Vec) -> Self { if names.len() != dtypes.len() { diff --git a/vortex-array/src/dtype/union.rs b/vortex-array/src/dtype/union.rs index 30df46e2d80..0bcbe78600d 100644 --- a/vortex-array/src/dtype/union.rs +++ b/vortex-array/src/dtype/union.rs @@ -266,6 +266,16 @@ impl UnionVariants { ) -> VortexResult { Self::validate_shape(&names, dtypes.len(), &type_ids)?; + // Same reasoning as `StructFields::try_from_fields`: `variants`, `variant` and + // `variant_by_index` return a plain `DType`, and the `PartialEq`/`Hash` impls on + // the variant dtype are trait methods, so a lazily decoded variant dtype has + // nowhere to report a bad union discriminant except a panic. + for (name, dtype) in names.iter().zip_eq(dtypes.iter()) { + dtype + .value() + .map_err(|e| e.with_context(format!("invalid dtype for union variant {name}")))?; + } + Ok(Self(Arc::new(UnionVariantsInner::from_fields( names, dtypes.into(), From db8df700f9fdd1e3b123371b9c7cdf82190e35c8 Mon Sep 17 00:00:00 2001 From: jackylee Date: Thu, 3 Sep 2026 19:48:55 +0800 Subject: [PATCH 2/2] docs: trim the comments added by this PR They restated the PR description, and one sentence blamed an unknown union discriminant: the flatbuffer verifier rejects that before decoding, so it never reaches the lazy decode this change fixes. Signed-off-by: jackylee --- vortex-array/src/dtype/serde/flatbuffers.rs | 14 +++++--------- vortex-array/src/dtype/struct_.rs | 18 +++++------------- vortex-array/src/dtype/union.rs | 5 +---- 3 files changed, 11 insertions(+), 26 deletions(-) diff --git a/vortex-array/src/dtype/serde/flatbuffers.rs b/vortex-array/src/dtype/serde/flatbuffers.rs index d63bba5f505..efda432d7ab 100644 --- a/vortex-array/src/dtype/serde/flatbuffers.rs +++ b/vortex-array/src/dtype/serde/flatbuffers.rs @@ -746,18 +746,15 @@ mod test { assert_eq!(viewed, eager); } - /// The reported crash (#8848): a struct field whose own dtype cannot be decoded. - /// Field dtypes used to be decoded lazily, so this parsed cleanly here and - /// panicked later, in `StructFields::fields()` while reading the file-statistics - /// footer. It must be an error at parse time instead. + /// A struct field whose own dtype cannot be decoded must fail here, not later in + /// `StructFields::fields()` (#8848). #[test] fn test_struct_field_with_undecodable_dtype_errors() { let mut fbb = FlatBufferBuilder::new(); let name = fbb.create_string("bad"); let names = fbb.create_vector(&[name]); - // A struct dtype with `names` omitted: the flatbuffer verifier accepts it - // (`names` is optional in the schema) but decoding it fails. This is the shape - // the fuzzer produced — structurally valid, semantically undecodable. + // `names` is optional in the schema, so the verifier accepts this struct but + // decoding it fails. let inner_struct = fb::Struct_::create( &mut fbb, &fb::Struct_Args { @@ -804,8 +801,7 @@ mod test { ); } - /// The same defect on the union side: `UnionVariants::variants()` also decodes lazily - /// and also cannot report a failure. + /// The same on the union side. #[test] fn test_union_variant_with_undecodable_dtype_errors() { let mut fbb = FlatBufferBuilder::new(); diff --git a/vortex-array/src/dtype/struct_.rs b/vortex-array/src/dtype/struct_.rs index 438a797fc63..5cb5b0271ca 100644 --- a/vortex-array/src/dtype/struct_.rs +++ b/vortex-array/src/dtype/struct_.rs @@ -319,20 +319,12 @@ impl StructFields { Self::from_fields(names, dtypes) } - /// Create a new [`StructFields`] from names and [`FieldDType`]s that may be backed by a - /// flatbuffer, decoding every field dtype so that later reads cannot fail. + /// Create a new [`StructFields`] from names and [`FieldDType`]s, decoding every field + /// dtype up front. /// - /// Deserialization must go through this rather than [`Self::from_fields`]. A - /// [`FieldDType`] holding a view decodes lazily, and the accessors that decode it - /// ([`Self::field`], [`Self::field_by_index`], [`Self::fields`]) return a plain - /// [`DType`], as do the [`PartialEq`] and [`Hash`] impls on the field dtype — those - /// are trait methods, so their signatures cannot carry a failure. A field dtype whose - /// union discriminant is unknown therefore has nowhere to surface except a panic, and - /// because the check was lazy the top-level dtype segment parsed cleanly and the panic - /// only fired later, while reading the file-statistics footer. - /// - /// Validating here costs one decode per field at open time, which is the price of the - /// accessors staying infallible. + /// Deserialization must use this rather than [`Self::from_fields`]: a [`FieldDType`] + /// backed by a view decodes lazily, and the accessors that decode it return a plain + /// [`DType`], so a field dtype that fails to decode has nowhere to surface but a panic. /// /// # Errors /// diff --git a/vortex-array/src/dtype/union.rs b/vortex-array/src/dtype/union.rs index 0bcbe78600d..1b94e4905e7 100644 --- a/vortex-array/src/dtype/union.rs +++ b/vortex-array/src/dtype/union.rs @@ -266,10 +266,7 @@ impl UnionVariants { ) -> VortexResult { Self::validate_shape(&names, dtypes.len(), &type_ids)?; - // Same reasoning as `StructFields::try_from_fields`: `variants`, `variant` and - // `variant_by_index` return a plain `DType`, and the `PartialEq`/`Hash` impls on - // the variant dtype are trait methods, so a lazily decoded variant dtype has - // nowhere to report a bad union discriminant except a panic. + // Decode up front for the same reason as `StructFields::try_from_fields`. for (name, dtype) in names.iter().zip_eq(dtypes.iter()) { dtype .value()