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

Jump to bottom

Further flatten fuzzer fixes #523

Merged
merged 10 commits into from
Jan 15, 2024
Merged

Further flatten fuzzer fixes #523

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
40a1f0d
Work on more fuzzer bugs with serde
juntyr Nov 28, 2023
098d38d
More slow progress
juntyr Nov 28, 2023
2ee17f7
Avoid quadratic backtracking for unterminated strings in struct type …
juntyr Jan 14, 2024
5632ce2
Account for variant tag and content in length minimisation
juntyr Jan 14, 2024
2c6e8eb
Improve invalid raw ron detection in fuzz
juntyr Jan 14, 2024
1ecb2f6
Add fix for untagged flattened None
juntyr Jan 15, 2024
ec977f9
Fix another weird fuzz case
juntyr Jan 15, 2024
b81e058
Fix rustfmt and remove unfixed tests
juntyr Jan 15, 2024
d5060c4
Rebase with explicit_struct_names extension
juntyr Jan 15, 2024
8fe8188
Remove debug print
juntyr Jan 15, 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
2 changes: 1 addition & 1 deletion README.md
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Loading