From 40a1f0d7ac2e7446b81002283f397e4c0d61ffdf Mon Sep 17 00:00:00 2001 From: Juniper Tyree <50025784+juntyr@users.noreply.github.com> Date: Tue, 28 Nov 2023 17:17:14 +0000 Subject: [PATCH 01/10] Work on more fuzzer bugs with serde --- fuzz/fuzz_targets/bench/lib.rs | 21 +++++- tests/502_known_bugs.rs | 114 ++++++++++++++++++++++++++++++++- 2 files changed, 133 insertions(+), 2 deletions(-) diff --git a/fuzz/fuzz_targets/bench/lib.rs b/fuzz/fuzz_targets/bench/lib.rs index 7e1f89c18..9fb596fdd 100644 --- a/fuzz/fuzz_targets/bench/lib.rs +++ b/fuzz/fuzz_targets/bench/lib.rs @@ -56,6 +56,8 @@ pub fn roundtrip_arbitrary_typed_ron_or_panic(data: &[u8]) -> Option panic!("{:#?} -! {:#?}", typed_value, err), }; + println!("{:?} -> {:?} -> {}", typed_value.ty, typed_value.value, ron); + if let Err(err) = options.from_str::(&ron) { match err.code { // Erroring on deep recursion is better than crashing on a stack overflow @@ -5791,6 +5793,14 @@ impl<'a> SerdeDataType<'a> { has_unknown_key, ) } else if is_flattened { + if matches!(representation, SerdeEnumRepresentation::ExternallyTagged) { + if *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 @@ -5820,7 +5830,7 @@ 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 && !matches!(representation, SerdeEnumRepresentation::Untagged)) || 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) @@ -5828,6 +5838,15 @@ impl<'a> SerdeDataType<'a> { } *has_unknown_key = true; } + + if is_flattened && matches!(representation, SerdeEnumRepresentation::ExternallyTagged) { + if *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 } }), diff --git a/tests/502_known_bugs.rs b/tests/502_known_bugs.rs index 6c300824e..f9d70345e 100644 --- a/tests/502_known_bugs.rs +++ b/tests/502_known_bugs.rs @@ -1,4 +1,4 @@ -use std::collections::HashMap; +use std::collections::{HashMap, BTreeMap}; use ron::{error::Position, error::SpannedError, extensions::Extensions, ser::PrettyConfig, Error}; use serde::{Deserialize, Serialize}; @@ -2885,6 +2885,118 @@ fn untagged_unit_variant_inside_internally_tagged_newtype_variant_inside_multi_f ); } +#[test] +fn flattened_externally_tagged_newtype_variant_beside_flattened_intenally_tagged_enum() { + #[derive(PartialEq, Debug, Serialize, Deserialize)] + enum ExternallyTagged { + Newtype(()), + Other(()), + } + + #[derive(PartialEq, Debug, Serialize, Deserialize)] + #[serde(tag = "tag")] + enum InternallyTagged { + Newtype(ExternallyTagged), + } + + #[derive(PartialEq, Debug, Serialize, Deserialize)] + struct Flattened { + #[serde(flatten)] + a: InternallyTagged, + #[serde(flatten)] + b: ExternallyTagged, + } + + assert_eq!( + check_roundtrip( + &Flattened { + a: InternallyTagged::Newtype(ExternallyTagged::Other(())), + b: ExternallyTagged::Newtype(()), + }, + PrettyConfig::default() + ), + Err(Err(SpannedError { + code: Error::InvalidValueForType { + expected: String::from("map with a single key"), + found: String::from("a map"), + }, + position: Position { line: 5, col: 1 } + })) + ); +} + +#[test] +fn flattened_externally_tagged_struct_variant_beside_flattened_intenally_tagged_enum() { + #[derive(PartialEq, Debug, Serialize, Deserialize)] + enum ExternallyTagged { + Struct { a: i32 }, + Other(()), + } + + #[derive(PartialEq, Debug, Serialize, Deserialize)] + #[serde(tag = "tag")] + enum InternallyTagged { + Newtype(ExternallyTagged), + } + + #[derive(PartialEq, Debug, Serialize, Deserialize)] + struct Flattened { + #[serde(flatten)] + a: InternallyTagged, + #[serde(flatten)] + b: ExternallyTagged, + } + + assert_eq!( + check_roundtrip( + &Flattened { + a: InternallyTagged::Newtype(ExternallyTagged::Other(())), + b: ExternallyTagged::Struct { a: 42 }, + }, + PrettyConfig::default() + ), + Err(Err(SpannedError { + code: Error::InvalidValueForType { + expected: String::from("map with a single key"), + found: String::from("a map"), + }, + position: Position { line: 7, col: 1 } + })) + ); +} + +#[test] +fn flattened_map_inside_option_beside_flattened_struct() { + #[derive(PartialEq, Debug, Serialize, Deserialize)] + #[serde(untagged)] + enum Untagged { + Struct { + a: i32, + }, + } + + #[derive(PartialEq, Debug, Serialize, Deserialize)] + struct Flattened { + #[serde(flatten)] + a: Untagged, + #[serde(flatten)] + b: Option>, + } + + assert_eq!( + check_roundtrip( + &Flattened { + a: Untagged::Struct { + a: 42, + }, + b: Some([(String::from("b"), 24)].into_iter().collect()), + }, + PrettyConfig::default() + ), + Err(Ok(Error::Message(String::from("ROUNDTRIP error: Flattened { a: Struct { a: 42 }, b: Some({\"b\": 24}) } != Flattened { a: Struct { a: 42 }, b: Some({\"a\": 42, \"b\": 24}) }")))) + ); +} + fn check_roundtrip( val: &T, config: PrettyConfig, From 098d38d855cd399aecbe20dcb83be3a7e7c9feed Mon Sep 17 00:00:00 2001 From: Juniper Tyree <50025784+juntyr@users.noreply.github.com> Date: Tue, 28 Nov 2023 19:21:58 +0000 Subject: [PATCH 02/10] More slow progress --- fuzz/fuzz_targets/bench/lib.rs | 14 ++++- tests/502_known_bugs.rs | 99 +++++++++++++++++++++++++++++++++- 2 files changed, 111 insertions(+), 2 deletions(-) diff --git a/fuzz/fuzz_targets/bench/lib.rs b/fuzz/fuzz_targets/bench/lib.rs index 9fb596fdd..567f9ea89 100644 --- a/fuzz/fuzz_targets/bench/lib.rs +++ b/fuzz/fuzz_targets/bench/lib.rs @@ -5830,12 +5830,24 @@ impl<'a> SerdeDataType<'a> { true } SerdeDataVariantType::Struct { fields } => { - if (is_flattened && !matches!(representation, SerdeEnumRepresentation::Untagged)) || matches!(representation, SerdeEnumRepresentation::Untagged if is_flattened || 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; } diff --git a/tests/502_known_bugs.rs b/tests/502_known_bugs.rs index f9d70345e..4238dbef8 100644 --- a/tests/502_known_bugs.rs +++ b/tests/502_known_bugs.rs @@ -2966,7 +2966,7 @@ fn flattened_externally_tagged_struct_variant_beside_flattened_intenally_tagged_ } #[test] -fn flattened_map_inside_option_beside_flattened_struct() { +fn flattened_map_inside_option_beside_flattened_struct_variant() { #[derive(PartialEq, Debug, Serialize, Deserialize)] #[serde(untagged)] enum Untagged { @@ -2997,6 +2997,103 @@ fn flattened_map_inside_option_beside_flattened_struct() { ); } +#[test] +fn flattened_untagged_struct_beside_flattened_untagged_struct() { + #[derive(PartialEq, Debug, Serialize, Deserialize)] + #[serde(untagged)] + enum Untagged { + Struct { + a: i32, + }, + Other { + b: i32, + }, + } + + #[derive(PartialEq, Debug, Serialize, Deserialize)] + struct Flattened { + #[serde(flatten)] + a: Untagged, + #[serde(flatten)] + b: Untagged, + } + + assert_eq!( + check_roundtrip( + &Flattened { + a: Untagged::Struct { + a: 42, + }, + b: Untagged::Other { + b: 24, + }, + }, + PrettyConfig::default() + ), + Err(Ok(Error::Message(String::from("ROUNDTRIP error: Flattened { a: Struct { a: 42 }, b: Other { b: 24 } } != Flattened { a: Struct { a: 42 }, b: Struct { a: 42 } }")))) + ); +} + +// TODO: still not fixed +#[test] +fn flattened_map_inside_option_beside_struct_with_flattened_field() { + #[derive(PartialEq, Debug, Serialize, Deserialize)] + #[serde(tag = "tag")] + struct Struct { + a: i32, + #[serde(flatten)] + u: (), + } + + #[derive(PartialEq, Debug, Serialize, Deserialize)] + #[serde(tag = "tag")] + struct Flattened { + a: Struct, + #[serde(flatten)] + b: Option>>, + } + + assert_eq!( + check_roundtrip( + &Flattened { + a: Struct { + a: 42, + u: (), + }, + b: Some([(String::from("b"), Some(24))].into_iter().collect()), + }, + PrettyConfig::default() + ), + Err(Ok(Error::Message(String::from("ROUNDTRIP error: Flattened { a: Struct { a: 42, u: () }, b: Some({\"b\": Some(24)}) } != Flattened { a: Struct { a: 42, u: () }, b: None }")))) + ); +} + +// TODO: still not fixed +#[test] +fn none_inside_flattened_untagged_newtype_variant() { + #[derive(PartialEq, Debug, Serialize, Deserialize)] + #[serde(untagged)] + enum Untagged { + Newtype(Option<()>), + } + + #[derive(PartialEq, Debug, Serialize, Deserialize)] + struct Flattened { + #[serde(flatten)] + a: Untagged, + } + + assert_eq!( + check_roundtrip( + &Flattened { + a: Untagged::Newtype(None), + }, + PrettyConfig::default() + ), + Err(Err(SpannedError { code: Error::Message(String::from("data did not match any variant of untagged enum Untagged")), position: Position { line: 2, col: 1 } })) + ); +} + fn check_roundtrip( val: &T, config: PrettyConfig, From 2ee17f7e5daea82d94a9dccec60b9a87bcd8ab07 Mon Sep 17 00:00:00 2001 From: Juniper Tyree <50025784+juntyr@users.noreply.github.com> Date: Sun, 14 Jan 2024 06:47:23 +0000 Subject: [PATCH 03/10] Avoid quadratic backtracking for unterminated strings in struct type check --- src/parse.rs | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/src/parse.rs b/src/parse.rs index 0144d0ade..08fd7fe8c 100644 --- a/src/parse.rs +++ b/src/parse.rs @@ -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()?; From 5632ce2363db32072e655682e050816e36469cf9 Mon Sep 17 00:00:00 2001 From: Juniper Tyree <50025784+juntyr@users.noreply.github.com> Date: Sun, 14 Jan 2024 07:05:42 +0000 Subject: [PATCH 04/10] Account for variant tag and content in length minimisation --- fuzz/fuzz_targets/bench/lib.rs | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/fuzz/fuzz_targets/bench/lib.rs b/fuzz/fuzz_targets/bench/lib.rs index 567f9ea89..d19f3f851 100644 --- a/fuzz/fuzz_targets/bench/lib.rs +++ b/fuzz/fuzz_targets/bench/lib.rs @@ -56,8 +56,6 @@ pub fn roundtrip_arbitrary_typed_ron_or_panic(data: &[u8]) -> Option panic!("{:#?} -! {:#?}", typed_value, err), }; - println!("{:?} -> {:?} -> {}", typed_value.ty, typed_value.value, ron); - if let Err(err) = options.from_str::(&ron) { match err.code { // Erroring on deep recursion is better than crashing on a stack overflow @@ -5180,6 +5178,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 => { From 2c6e8eb60365a6ff184167b9c3732dc293408c44 Mon Sep 17 00:00:00 2001 From: Juniper Tyree <50025784+juntyr@users.noreply.github.com> Date: Sun, 14 Jan 2024 08:12:24 +0000 Subject: [PATCH 05/10] Improve invalid raw ron detection in fuzz --- fuzz/fuzz_targets/bench/lib.rs | 64 +++++++++++++++++++++++----------- 1 file changed, 44 insertions(+), 20 deletions(-) diff --git a/fuzz/fuzz_targets/bench/lib.rs b/fuzz/fuzz_targets/bench/lib.rs index d19f3f851..767b928e7 100644 --- a/fuzz/fuzz_targets/bench/lib.rs +++ b/fuzz/fuzz_targets/bench/lib.rs @@ -56,6 +56,11 @@ pub fn roundtrip_arbitrary_typed_ron_or_panic(data: &[u8]) -> Option panic!("{:#?} -! {:#?}", typed_value, err), }; + println!( + "{:#?} -> {:#?} -> {}", + typed_value.ty, typed_value.value, ron + ); + if let Err(err) = options.from_str::(&ron) { match err.code { // Erroring on deep recursion is better than crashing on a stack overflow @@ -5063,21 +5068,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::(&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 } => { @@ -5183,7 +5201,7 @@ impl<'a> SerdeDataType<'a> { | SerdeEnumRepresentation::Untagged => (), SerdeEnumRepresentation::InternallyTagged { tag } => name_length += tag.len(), SerdeEnumRepresentation::AdjacentlyTagged { tag, content } => { - name_length += tag.len() + content.len() + name_length += tag.len() + content.len(); } }; @@ -5743,10 +5761,20 @@ 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; + } + *has_unknown_key = true; + } + fields .1 .iter() @@ -5800,12 +5828,10 @@ impl<'a> SerdeDataType<'a> { has_unknown_key, ) } else if is_flattened { - if matches!(representation, SerdeEnumRepresentation::ExternallyTagged) { - if *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::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: _ }) { @@ -5858,12 +5884,10 @@ impl<'a> SerdeDataType<'a> { *has_unknown_key = true; } - if is_flattened && matches!(representation, SerdeEnumRepresentation::ExternallyTagged) { - if *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 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 From 1ecb2f65c6f015f4ed06c140b0fe7fe9b526721e Mon Sep 17 00:00:00 2001 From: Juniper Tyree <50025784+juntyr@users.noreply.github.com> Date: Mon, 15 Jan 2024 02:42:57 +0000 Subject: [PATCH 06/10] Add fix for untagged flattened None --- fuzz/fuzz_targets/bench/lib.rs | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/fuzz/fuzz_targets/bench/lib.rs b/fuzz/fuzz_targets/bench/lib.rs index 767b928e7..0031e26d5 100644 --- a/fuzz/fuzz_targets/bench/lib.rs +++ b/fuzz/fuzz_targets/bench/lib.rs @@ -5145,6 +5145,7 @@ 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, ) { @@ -5303,6 +5304,7 @@ 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, ) { @@ -5693,6 +5695,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 { @@ -5717,10 +5720,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, ) @@ -5751,6 +5761,7 @@ impl<'a> SerdeDataType<'a> { inner.supported_flattened_map_inside_flatten_field( pretty, is_flattened, + is_untagged, has_flattened_map, has_unknown_key, ) @@ -5783,6 +5794,7 @@ impl<'a> SerdeDataType<'a> { field.supported_flattened_map_inside_flatten_field( pretty, *is_flattened, + is_untagged, has_flattened_map, has_unknown_key, ) @@ -5824,6 +5836,7 @@ impl<'a> SerdeDataType<'a> { inner.supported_flattened_map_inside_flatten_field( pretty, is_flattened, + true, has_flattened_map, has_unknown_key, ) From ec977f90627a0b12dc32027e3bb512aa0d67e566 Mon Sep 17 00:00:00 2001 From: Juniper Tyree <50025784+juntyr@users.noreply.github.com> Date: Mon, 15 Jan 2024 03:17:28 +0000 Subject: [PATCH 07/10] Fix another weird fuzz case --- fuzz/fuzz_targets/bench/lib.rs | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/fuzz/fuzz_targets/bench/lib.rs b/fuzz/fuzz_targets/bench/lib.rs index 0031e26d5..b752a86d1 100644 --- a/fuzz/fuzz_targets/bench/lib.rs +++ b/fuzz/fuzz_targets/bench/lib.rs @@ -5783,6 +5783,11 @@ impl<'a> SerdeDataType<'a> { // 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; } @@ -5853,6 +5858,16 @@ impl<'a> SerdeDataType<'a> { 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 { From b81e058b54207cc3c6146f464cb96dbb60523e3a Mon Sep 17 00:00:00 2001 From: Juniper Tyree <50025784+juntyr@users.noreply.github.com> Date: Mon, 15 Jan 2024 03:23:39 +0000 Subject: [PATCH 08/10] Fix rustfmt and remove unfixed tests --- tests/502_known_bugs.rs | 74 +++-------------------------------------- 1 file changed, 4 insertions(+), 70 deletions(-) diff --git a/tests/502_known_bugs.rs b/tests/502_known_bugs.rs index 4238dbef8..11f8abf59 100644 --- a/tests/502_known_bugs.rs +++ b/tests/502_known_bugs.rs @@ -1,4 +1,4 @@ -use std::collections::{HashMap, BTreeMap}; +use std::collections::{BTreeMap, HashMap}; use ron::{error::Position, error::SpannedError, extensions::Extensions, ser::PrettyConfig, Error}; use serde::{Deserialize, Serialize}; @@ -2970,9 +2970,7 @@ fn flattened_map_inside_option_beside_flattened_struct_variant() { #[derive(PartialEq, Debug, Serialize, Deserialize)] #[serde(untagged)] enum Untagged { - Struct { - a: i32, - }, + Struct { a: i32 }, } #[derive(PartialEq, Debug, Serialize, Deserialize)] @@ -3002,12 +3000,8 @@ fn flattened_untagged_struct_beside_flattened_untagged_struct() { #[derive(PartialEq, Debug, Serialize, Deserialize)] #[serde(untagged)] enum Untagged { - Struct { - a: i32, - }, - Other { - b: i32, - }, + Struct { a: i32 }, + Other { b: i32 }, } #[derive(PartialEq, Debug, Serialize, Deserialize)] @@ -3034,66 +3028,6 @@ fn flattened_untagged_struct_beside_flattened_untagged_struct() { ); } -// TODO: still not fixed -#[test] -fn flattened_map_inside_option_beside_struct_with_flattened_field() { - #[derive(PartialEq, Debug, Serialize, Deserialize)] - #[serde(tag = "tag")] - struct Struct { - a: i32, - #[serde(flatten)] - u: (), - } - - #[derive(PartialEq, Debug, Serialize, Deserialize)] - #[serde(tag = "tag")] - struct Flattened { - a: Struct, - #[serde(flatten)] - b: Option>>, - } - - assert_eq!( - check_roundtrip( - &Flattened { - a: Struct { - a: 42, - u: (), - }, - b: Some([(String::from("b"), Some(24))].into_iter().collect()), - }, - PrettyConfig::default() - ), - Err(Ok(Error::Message(String::from("ROUNDTRIP error: Flattened { a: Struct { a: 42, u: () }, b: Some({\"b\": Some(24)}) } != Flattened { a: Struct { a: 42, u: () }, b: None }")))) - ); -} - -// TODO: still not fixed -#[test] -fn none_inside_flattened_untagged_newtype_variant() { - #[derive(PartialEq, Debug, Serialize, Deserialize)] - #[serde(untagged)] - enum Untagged { - Newtype(Option<()>), - } - - #[derive(PartialEq, Debug, Serialize, Deserialize)] - struct Flattened { - #[serde(flatten)] - a: Untagged, - } - - assert_eq!( - check_roundtrip( - &Flattened { - a: Untagged::Newtype(None), - }, - PrettyConfig::default() - ), - Err(Err(SpannedError { code: Error::Message(String::from("data did not match any variant of untagged enum Untagged")), position: Position { line: 2, col: 1 } })) - ); -} - fn check_roundtrip( val: &T, config: PrettyConfig, From d5060c4634b57f7a46434d433b63bb66b623faa4 Mon Sep 17 00:00:00 2001 From: Juniper Tyree <50025784+juntyr@users.noreply.github.com> Date: Mon, 15 Jan 2024 03:31:33 +0000 Subject: [PATCH 09/10] Rebase with explicit_struct_names extension --- README.md | 2 +- fuzz/fuzz_targets/bench/lib.rs | 16 +++++++++++++--- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index ec1ed283c..f365495df 100644 --- a/README.md +++ b/README.md @@ -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 diff --git a/fuzz/fuzz_targets/bench/lib.rs b/fuzz/fuzz_targets/bench/lib.rs index b752a86d1..d7edf9893 100644 --- a/fuzz/fuzz_targets/bench/lib.rs +++ b/fuzz/fuzz_targets/bench/lib.rs @@ -5152,7 +5152,12 @@ impl<'a> SerdeDataType<'a> { // 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); } @@ -5171,7 +5176,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); } @@ -5311,7 +5316,12 @@ impl<'a> SerdeDataType<'a> { // 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); } From 8fe81888b3259c33fbfe890962488df658a8f6ce Mon Sep 17 00:00:00 2001 From: Juniper Tyree <50025784+juntyr@users.noreply.github.com> Date: Mon, 15 Jan 2024 03:34:27 +0000 Subject: [PATCH 10/10] Remove debug print --- fuzz/fuzz_targets/bench/lib.rs | 5 ----- 1 file changed, 5 deletions(-) diff --git a/fuzz/fuzz_targets/bench/lib.rs b/fuzz/fuzz_targets/bench/lib.rs index d7edf9893..b61b592e0 100644 --- a/fuzz/fuzz_targets/bench/lib.rs +++ b/fuzz/fuzz_targets/bench/lib.rs @@ -56,11 +56,6 @@ pub fn roundtrip_arbitrary_typed_ron_or_panic(data: &[u8]) -> Option panic!("{:#?} -! {:#?}", typed_value, err), }; - println!( - "{:#?} -> {:#?} -> {}", - typed_value.ty, typed_value.value, ron - ); - if let Err(err) = options.from_str::(&ron) { match err.code { // Erroring on deep recursion is better than crashing on a stack overflow