Skip to content

Commit

Permalink
Fix more serde flatten bugs in the fuzzer (#523)
Browse files Browse the repository at this point in the history 
* Work on more fuzzer bugs with serde

* Avoid quadratic backtracking for unterminated strings in struct type check

* Account for variant tag and content in length minimisation

* Improve invalid raw ron detection in fuzz

* Add fix for untagged flattened None

* Fix another weird fuzz case

* Fix rustfmt and remove unfixed tests

* Rebase with explicit_struct_names extension

* Remove debug print
  • Loading branch information
@juntyr
juntyr authored Jan 15, 2024
1 parent d509208 commit d3e9ebe
Show file tree
Hide file tree
Showing 4 changed files with 262 additions and 18 deletions.
2 changes: 1 addition & 1 deletion README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,7 @@ While data structures with any of these attributes should generally roundtrip th

- ron only supports string keys inside maps flattened into structs
- internally (or adjacently) tagged or untagged enum variants or `#[serde(flatten)]`ed fields must not contain:
- struct names, e.g. by enabling the `PrettyConfig::struct_names` setting
- struct names, e.g. by enabling the `#[enable(explicit_struct_names)]` extension or the `PrettyConfig::struct_names` setting
- newtypes
- zero-length arrays / tuples / tuple structs / structs / tuple variants / struct variants
- `Option`s with `#[enable(implicit_some)]` must not contain any of these or a unit, unit struct, or an untagged unit variant
Expand Down
119 changes: 107 additions & 12 deletions fuzz/fuzz_targets/bench/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5063,21 +5063,34 @@ impl<'a> SerdeDataType<'a> {
SerdeDataValue::UnitStruct
}
SerdeDataType::Newtype { name, inner } => {
let inner = inner.arbitrary_value(u, pretty)?;
let inner_value = inner.arbitrary_value(u, pretty)?;

// ron::value::RawValue cannot safely be constructed from syntactically invalid ron
if *name == RAW_VALUE_TOKEN {
if let SerdeDataValue::String(ron) = &inner {
if ron::value::RawValue::from_ron(ron).is_err() {
return Err(arbitrary::Error::IncorrectFormat);
}
// Hacky way to find out if a string is serialised:
// 1. serialise into RON
// 2. try to deserialise into a String
let Ok(inner_ron) = ron::to_string(&BorrowedTypedSerdeData {
ty: inner,
value: &inner_value,
}) else {
return Err(arbitrary::Error::IncorrectFormat);
};

let Ok(ron) = ron::from_str::<String>(&inner_ron) else {
return Err(arbitrary::Error::IncorrectFormat);
};

// Check that the raw value content is valid
if ron::value::RawValue::from_ron(&ron).is_err() {
return Err(arbitrary::Error::IncorrectFormat);
}
}

name_length += name.len();

SerdeDataValue::Newtype {
inner: Box::new(inner),
inner: Box::new(inner_value),
}
}
SerdeDataType::TupleStruct { name, fields } => {
Expand Down Expand Up @@ -5127,13 +5140,19 @@ impl<'a> SerdeDataType<'a> {
if !ty.supported_flattened_map_inside_flatten_field(
pretty,
*flatten,
false,
&mut has_flatten_map,
&mut has_unknown_key_inside_flatten,
) {
// Flattened fields with maps must fulfil certain criteria
return Err(arbitrary::Error::IncorrectFormat);
}
if *flatten && pretty.struct_names {
if *flatten
&& (pretty.struct_names
|| pretty
.extensions
.contains(Extensions::EXPLICIT_STRUCT_NAMES))
{
// BUG: struct names inside flattend structs do not roundtrip
return Err(arbitrary::Error::IncorrectFormat);
}
Expand All @@ -5152,7 +5171,7 @@ impl<'a> SerdeDataType<'a> {
representation,
SerdeEnumRepresentation::Untagged |
SerdeEnumRepresentation::InternallyTagged { tag: _ }
if pretty.struct_names
if pretty.struct_names || pretty.extensions.contains(Extensions::EXPLICIT_STRUCT_NAMES)
) {
return Err(arbitrary::Error::IncorrectFormat);
}
Expand All @@ -5178,6 +5197,15 @@ impl<'a> SerdeDataType<'a> {

name_length += variant.len();

match representation {
SerdeEnumRepresentation::ExternallyTagged
| SerdeEnumRepresentation::Untagged => (),
SerdeEnumRepresentation::InternallyTagged { tag } => name_length += tag.len(),
SerdeEnumRepresentation::AdjacentlyTagged { tag, content } => {
name_length += tag.len() + content.len();
}
};

let value = match ty {
SerdeDataVariantType::Unit => SerdeDataVariantValue::Unit,
SerdeDataVariantType::TaggedOther => {
Expand Down Expand Up @@ -5276,13 +5304,19 @@ impl<'a> SerdeDataType<'a> {
if !ty.supported_flattened_map_inside_flatten_field(
pretty,
*flatten,
false,
&mut has_flatten_map,
&mut has_unknown_key_inside_flatten,
) {
// Flattened fields with maps must fulfil certain criteria
return Err(arbitrary::Error::IncorrectFormat);
}
if *flatten && pretty.struct_names {
if *flatten
&& (pretty.struct_names
|| pretty
.extensions
.contains(Extensions::EXPLICIT_STRUCT_NAMES))
{
// BUG: struct names inside flattend structs do not roundtrip
return Err(arbitrary::Error::IncorrectFormat);
}
Expand Down Expand Up @@ -5666,6 +5700,7 @@ impl<'a> SerdeDataType<'a> {
&self,
pretty: &PrettyConfig,
is_flattened: bool,
is_untagged: bool,
has_flattened_map: &mut bool,
has_unknown_key: &mut bool,
) -> bool {
Expand All @@ -5690,10 +5725,17 @@ impl<'a> SerdeDataType<'a> {
SerdeDataType::String => true,
SerdeDataType::ByteBuf => true,
SerdeDataType::Option { inner } => {
if is_flattened || pretty.extensions.contains(Extensions::IMPLICIT_SOME) {
if is_flattened && is_untagged {
// BUG: (serde)
// - serialising a flattened None only produces an empty struct
// - deserialising content from an empty flatten struct produces an empty map
// - deserialising an option from a content empty map produces some
false
} else if is_flattened || pretty.extensions.contains(Extensions::IMPLICIT_SOME) {
inner.supported_flattened_map_inside_flatten_field(
pretty,
is_flattened,
is_untagged,
has_flattened_map,
has_unknown_key,
)
Expand Down Expand Up @@ -5724,6 +5766,7 @@ impl<'a> SerdeDataType<'a> {
inner.supported_flattened_map_inside_flatten_field(
pretty,
is_flattened,
is_untagged,
has_flattened_map,
has_unknown_key,
)
Expand All @@ -5734,10 +5777,25 @@ impl<'a> SerdeDataType<'a> {
SerdeDataType::TupleStruct { name: _, fields: _ } => true,
SerdeDataType::Struct {
name: _,
tag: _,
tag,
fields,
} => {
if is_flattened {
// TODO: is this really the correct requirement?
// case clusterfuzz-testcase-minimized-arbitrary-6364230921879552
if tag.is_some() {
if *has_flattened_map {
// BUG: a flattened map will also see the unknown key (serde)
return false;
}
if *has_unknown_key && is_untagged {
// BUG: an untagged struct will use a map intermediary and see
// the unknown key (serde)
return false;
}
*has_unknown_key = true;
}

fields
.1
.iter()
Expand All @@ -5746,6 +5804,7 @@ impl<'a> SerdeDataType<'a> {
field.supported_flattened_map_inside_flatten_field(
pretty,
*is_flattened,
is_untagged,
has_flattened_map,
has_unknown_key,
)
Expand Down Expand Up @@ -5787,16 +5846,33 @@ impl<'a> SerdeDataType<'a> {
inner.supported_flattened_map_inside_flatten_field(
pretty,
is_flattened,
true,
has_flattened_map,
has_unknown_key,
)
} else if is_flattened {
if matches!(representation, SerdeEnumRepresentation::ExternallyTagged) && *has_unknown_key {
// BUG: flattened enums are deserialised using the content deserialiser,
// which expects to see a map with just one field (serde)
return false;
}

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) {
return false;
}

if !inner.supported_flattened_map_inside_flatten_field(
pretty,
is_flattened,
false,
has_flattened_map,
has_unknown_key,
) {
return false;
}
}

if *has_flattened_map {
Expand All @@ -5820,14 +5896,33 @@ impl<'a> SerdeDataType<'a> {
true
}
SerdeDataVariantType::Struct { fields } => {
if (is_flattened && !matches!(representation, SerdeEnumRepresentation::Untagged)) || matches!(representation, SerdeEnumRepresentation::Untagged if fields.2.iter().any(|x| *x))
if is_flattened {
if *has_flattened_map {
// BUG: a flattened map will also see the unknown key (serde)
return false;
}
*has_unknown_key = true;
}

if matches!(representation, SerdeEnumRepresentation::Untagged if is_flattened || fields.2.iter().any(|x| *x))
{
if *has_flattened_map {
// BUG: a flattened map will also see the unknown key (serde)
return false;
}
if *has_unknown_key {
// BUG: a flattened untagged enum struct will also see the unknown key (serde)
return false;
}
*has_unknown_key = true;
}

if is_flattened && matches!(representation, SerdeEnumRepresentation::ExternallyTagged) && *has_unknown_key {
// BUG: flattened enums are deserialised using the content deserialiser,
// which expects to see a map with just one field (serde)
return false;
}

true
}
}),
Expand Down
14 changes: 10 additions & 4 deletions src/parse.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -612,12 +612,18 @@ impl<'a> Parser<'a> {
parser.set_cursor(cursor_backup);
}
let cursor_backup = parser.cursor;
if parser.string().is_err() {
parser.set_cursor(cursor_backup);
match parser.string() {
Ok(_) => (),
// prevent quadratic complexity backtracking for unterminated string
Err(err @ (Error::ExpectedStringEnd | Error::Eof)) => return Err(err),
Err(_) => parser.set_cursor(cursor_backup),
}
let cursor_backup = parser.cursor;
if parser.byte_string().is_err() {
parser.set_cursor(cursor_backup);
match parser.byte_string() {
Ok(_) => (),
// prevent quadratic complexity backtracking for unterminated byte string
Err(err @ (Error::ExpectedStringEnd | Error::Eof)) => return Err(err),
Err(_) => parser.set_cursor(cursor_backup),
}

let c = parser.next_char()?;
Expand Down
Loading

0 comments on commit d3e9ebe

Please sign in to comment.