Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

More fuzzer flatten fixes #537

Merged
merged 3 commits into from
Apr 11, 2024
+104 −22
Merged

More fuzzer flatten fixes #537

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bf670eb
Start fixing more flatten-related fuzzer bugs
juntyr Apr 10, 2024
5a8ab5d
More bug fixes
juntyr Apr 10, 2024
e063a25
More fixes for flattened enums
juntyr Apr 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
94 changes: 72 additions & 22 deletions fuzz/fuzz_targets/bench/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1145,18 +1145,28 @@ impl<'a, 'de> DeserializeSeed<'de> for BorrowedTypedSerdeData<'a> {
) -> Result<Self::Value, ()> {
match self.value {
None => Ok(()),
Some(expected) => BorrowedTypedSerdeData {
Some(expected) => match (BorrowedTypedSerdeData {
ty: self.ty,
value: expected,
}
})
.deserialize(deserializer)
.map_err(|err| {
panic!(
{
Ok(()) => Ok(()),
// Duplicate struct fields only cause issues inside internally (or adjacently)
// tagged or untagged enums (or in flattened fields where we detect them
// before they cause issues), so we allow them in arbitrary otherwise
Err(err)
if format!("{err}")
.starts_with("Unexpected duplicate field named") =>
{
Ok(())
}
Err(err) => panic!(
"expected untagged {:?} but failed with {}",
Some(expected),
err
)
}),
),
},
}
}
}
Expand Down Expand Up @@ -5131,7 +5141,7 @@ impl<'a> SerdeDataType<'a> {
}
let value = SerdeDataValue::Struct { fields: r#struct };
let mut has_flatten_map = false;
let mut has_unknown_key_inside_flatten = false;
let mut has_unknown_key_inside_flatten = tag.is_some();
for (ty, flatten) in fields.1.iter().zip(fields.2.iter()) {
if *flatten && !ty.supported_inside_untagged(pretty, false, false) {
// Flattened fields are deserialised through serde's content type
Expand Down Expand Up @@ -5232,7 +5242,7 @@ impl<'a> SerdeDataType<'a> {
{
return Err(arbitrary::Error::IncorrectFormat);
}
if matches!(representation, SerdeEnumRepresentation::InternallyTagged { tag: _ } if !inner.supported_inside_internally_tagged_newtype(false))
if matches!(representation, SerdeEnumRepresentation::InternallyTagged { tag: _ } if !inner.supported_inside_internally_tagged_newtype(false, false))
{
return Err(arbitrary::Error::IncorrectFormat);
}
Expand Down Expand Up @@ -5530,6 +5540,7 @@ impl<'a> SerdeDataType<'a> {
fn supported_inside_internally_tagged_newtype(
&self,
inside_untagged_newtype_variant: bool,
has_unknown_key: bool,
) -> bool {
// See https://github.com/serde-rs/serde/blob/ddc1ee564b33aa584e5a66817aafb27c3265b212/serde/src/private/ser.rs#L94-L336
match self {
Expand Down Expand Up @@ -5568,9 +5579,11 @@ impl<'a> SerdeDataType<'a> {
// which is only serialised with the tag
!inside_untagged_newtype_variant
}
SerdeDataType::Newtype { name: _, inner: ty } => {
ty.supported_inside_internally_tagged_newtype(inside_untagged_newtype_variant)
}
SerdeDataType::Newtype { name: _, inner: ty } => ty
.supported_inside_internally_tagged_newtype(
inside_untagged_newtype_variant,
has_unknown_key,
),
SerdeDataType::TupleStruct { name: _, fields: _ } => false,
SerdeDataType::Struct {
name: _,
Expand All @@ -5582,6 +5595,15 @@ impl<'a> SerdeDataType<'a> {
variants,
representation,
} => {
if matches!(representation, SerdeEnumRepresentation::ExternallyTagged)
&& has_unknown_key
{
// BUG: an externally tagged enum inside an internally tagged
// enum expects to find a map with a single key, but an
// unknown key will cause a failure
return false;
}

variants.1.iter().all(|ty| match ty {
SerdeDataVariantType::Unit | SerdeDataVariantType::TaggedOther => {
// BUG: an untagged unit variant requires a unit,
Expand All @@ -5591,7 +5613,7 @@ impl<'a> SerdeDataType<'a> {
}
SerdeDataVariantType::Newtype { inner: ty } => {
if matches!(representation, SerdeEnumRepresentation::Untagged) {
ty.supported_inside_internally_tagged_newtype(true)
ty.supported_inside_internally_tagged_newtype(true, has_unknown_key)
} else {
true
}
Expand All @@ -5607,12 +5629,17 @@ impl<'a> SerdeDataType<'a> {
}
}

fn supported_inside_flatten(&self, inside_untagged_newtype_variant: bool) -> bool {
fn supported_inside_flatten(
&self,
inside_untagged_newtype_variant: bool,
inside_flattened_option: bool,
) -> bool {
match self {
SerdeDataType::Unit => {
// BUG: a unit inside an untagged newtype variant expects a unit
// but only the tag is there
!inside_untagged_newtype_variant
// BUG: a unit inside a flattened Some is interpreted as None
!inside_untagged_newtype_variant && !inside_flattened_option
}
SerdeDataType::Bool => false,
SerdeDataType::I8 => false,
Expand All @@ -5633,19 +5660,25 @@ impl<'a> SerdeDataType<'a> {
SerdeDataType::String => false,
SerdeDataType::ByteBuf => false,
SerdeDataType::Option { inner } => {
inner.supported_inside_flatten(inside_untagged_newtype_variant)
inner.supported_inside_flatten(inside_untagged_newtype_variant, true)
}
SerdeDataType::Array { kind: _, len: _ } => false,
SerdeDataType::Tuple { elems: _ } => false,
SerdeDataType::Vec { item: _ } => false,
SerdeDataType::Map { key, value: _ } => key.supported_inside_flatten_key(),
SerdeDataType::Map { key, value } => {
key.supported_inside_flatten_key()
&& value.supported_inside_flatten(inside_untagged_newtype_variant, false)
}
SerdeDataType::UnitStruct { name: _ } => false,
SerdeDataType::Newtype { name, inner } => {
if *name == RAW_VALUE_TOKEN {
return false;
}

inner.supported_inside_flatten(inside_untagged_newtype_variant)
inner.supported_inside_flatten(
inside_untagged_newtype_variant,
inside_flattened_option,
)
}
SerdeDataType::TupleStruct { name: _, fields: _ } => false,
SerdeDataType::Struct {
Expand Down Expand Up @@ -5674,9 +5707,12 @@ impl<'a> SerdeDataType<'a> {
}
SerdeDataVariantType::Newtype { inner } => {
if matches!(representation, SerdeEnumRepresentation::Untagged) {
inner.supported_inside_flatten(true)
inner.supported_inside_flatten(true, inside_flattened_option)
} else {
inner.supported_inside_flatten(inside_untagged_newtype_variant)
inner.supported_inside_flatten(
inside_untagged_newtype_variant,
inside_flattened_option,
)
}
}
SerdeDataVariantType::Tuple { fields: _ } => {
Expand Down Expand Up @@ -5796,6 +5832,13 @@ impl<'a> SerdeDataType<'a> {
*has_unknown_key = true;
}

// once there are some flattened fields, the unflattened
// ones will be unknown keys for later maps
// e.g. clusterfuzz-testcase-minimized-arbitrary-6266237281697792
if fields.2.iter().any(|x| *x) && !fields.2.iter().all(|x| *x) {
*has_unknown_key = true;
}

fields
.1
.iter()
Expand Down Expand Up @@ -5860,14 +5903,16 @@ impl<'a> SerdeDataType<'a> {
if matches!(representation, SerdeEnumRepresentation::InternallyTagged { tag: _ }) {
// BUG: an flattened internally tagged newtype alongside other flattened data
// must not contain a unit, unit struct, or untagged unit variant
if !inner.supported_inside_internally_tagged_newtype(true) {
if !inner.supported_inside_internally_tagged_newtype(true, *has_unknown_key) {
return false;
}

if !inner.supported_flattened_map_inside_flatten_field(
pretty,
is_flattened,
false,
// TODO: find a good explanation
// e.g. clusterfuzz-testcase-minimized-arbitrary-6729835718180864
matches!(representation, SerdeEnumRepresentation::InternallyTagged { tag: _ }),
has_flattened_map,
has_unknown_key,
) {
Expand Down Expand Up @@ -5923,6 +5968,11 @@ impl<'a> SerdeDataType<'a> {
return false;
}

if is_flattened && matches!(representation, SerdeEnumRepresentation::InternallyTagged { .. }) && *has_unknown_key {
// TODO: find a good explanation and example
return false;
}

true
}
}),
Expand Down Expand Up @@ -5998,7 +6048,7 @@ fn arbitrary_struct_fields_recursion_guard<'a>(
while u.arbitrary()? {
fields.push(<&str>::arbitrary(u)?);
let ty = SerdeDataType::arbitrary(u)?;
flattened.push(u.arbitrary()? && ty.supported_inside_flatten(false));
flattened.push(u.arbitrary()? && ty.supported_inside_flatten(false, false));
types.push(ty);
}

Expand Down
32 changes: 32 additions & 0 deletions tests/502_known_bugs.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3028,6 +3028,38 @@ fn flattened_untagged_struct_beside_flattened_untagged_struct() {
);
}

#[test]
fn flattened_field_inside_flattened_struct_alongside_map() {
#[derive(PartialEq, Debug, Serialize, Deserialize)]
struct Units {
a: i32,
#[serde(flatten)]
b: (),
}

#[derive(PartialEq, Debug, Serialize, Deserialize)]
struct Flattened {
#[serde(flatten)]
a: Units,
#[serde(flatten)]
b: BTreeMap<String, i32>,
}

assert_eq!(
check_roundtrip(
&Flattened {
a: Units {
a: 42,
b: (),
},
b: [(String::from("c"), 24)].into_iter().collect(),
},
PrettyConfig::default()
),
Err(Ok(Error::Message(String::from("ROUNDTRIP error: Flattened { a: Units { a: 42, b: () }, b: {\"c\": 24} } != Flattened { a: Units { a: 42, b: () }, b: {\"a\": 42, \"c\": 24} }"))))
);
}

fn check_roundtrip<T: PartialEq + std::fmt::Debug + Serialize + serde::de::DeserializeOwned>(
val: &T,
config: PrettyConfig,
Expand Down
Loading