Skip to content

Commit

Permalink
More fuzzer flatten fixes (#537)
Browse files Browse the repository at this point in the history 
* More fixes for flatten-related fuzzer bugs

* More bug fixes

* More fixes for flattened enums
  • Loading branch information
@juntyr
juntyr committed Apr 11, 2024
1 parent 08c691d commit 8d0261a
Show file tree
Hide file tree
Showing 2 changed files with 104 additions and 22 deletions.
94 changes: 72 additions & 22 deletions fuzz/fuzz_targets/bench/lib.rs
View file
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
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

0 comments on commit 8d0261a

Please sign in to comment.