Skip to content

Commit

Permalink
Further investigation of cursed flattened maps
Browse files Browse the repository at this point in the history
  • Loading branch information
@juntyr
juntyr committed Oct 4, 2023
1 parent 9737eba commit 6b67757
Show file tree
Hide file tree
Showing 2 changed files with 204 additions and 15 deletions.
18 changes: 15 additions & 3 deletions README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -185,22 +185,34 @@ RON is not designed to be a fully self-describing format (unlike JSON) and is th

While data structures with any of these attributes should generally roundtrip through RON, some restrictions apply [^serde-restrictions] and their textual representation may not always match your expectation:

- flattened structs are only serialised as maps and deserialised from maps
- 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:
- `i128` or `u128` values
- struct names, e.g. by enabling 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
- externally tagged tuple variants with just one field (that are not newtype variants)
- tuples or arrays with just one element are not supported inside newtype variants with `#[enable(unwrap_variant_newtypes)]`
- a `ron::value::RawValue`
- internally tagged newtype variants must not contain:
- a unit or a unit struct inside an untagged newtype variant
- an untagged unit variant
- untagged tuple / struct variants with no fields are not supported
- untagged tuple variants with just one field (that are not newtype variants) are not supported when the `#![enable(unwrap_variant_newtypes)]` extension is enabled
- flattened structs with conflicting keys (e.g. an earlier inner-struct key matches a later outer-struct key or two flattened maps in the same struct share a key) are not supported by serde

Furthermore, serde imposes the following restrictions for data to roundtrip:

- structs or struct variants that contain a `#[serde(flatten)]`ed field:
- are only serialised as maps and deserialised from maps
- must not contain duplicate fields / keys, e.g. where an inner-struct field matches an outer-struct or inner-struct field
- may only contain one (within the super-struct of all flattened structs) `#[serde(flatten)]`ed map field, which collects all unknown fields
- if they contain a `#[serde(flatten)]`ed map, they must not contain:
- a struct that is not flattened itself but contains some flattened fields
- a flattened newtype, tuple, or struct variant
- an internally tagged struct variant
- an untagged struct variant that contains some flattened fields
- internally (or adjacently) tagged or untagged enum variants or `#[serde(flatten)]`ed fields must not contain:
- `i128` or `u128` values

Please file a [new issue](https://github.com/ron-rs/ron/issues/new) if you come across a use case which is not listed among the above restrictions but still breaks.

Expand Down
201 changes: 189 additions & 12 deletions fuzz/fuzz_targets/bench/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -87,8 +87,9 @@ pub fn roundtrip_arbitrary_typed_ron_or_panic(data: &[u8]) -> Option<TypedSerdeD
match err.code {
// Erroring on deep recursion is better than crashing on a stack overflow
ron::error::Error::ExceededRecursionLimit => return None,
// Duplicate struct fields only cause issues inside internally tagged
// or untagged enums, so we allow them otherwise
// 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
ron::error::Error::DuplicateStructField { .. } => return None,
// Everything else is actually a bug we want to find
_ => panic!("{:#?} -> {} -! {:#?} @ {:#?}", typed_value, ron, err, path),
Expand Down Expand Up @@ -5098,11 +5099,22 @@ impl<'a> SerdeDataType<'a> {
r#struct.push(ty.arbitrary_value(u, pretty)?);
}
let value = SerdeDataValue::Struct { fields: r#struct };
let mut has_flatten_map = false;
let mut has_unknown_key_inside_flatten = false;
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
return Err(arbitrary::Error::IncorrectFormat);
}
if !ty.supported_flattened_map_inside_flatten_field(
pretty,
*flatten,
&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 {
// BUG: struct names inside flattend structs do not roundtrip
return Err(arbitrary::Error::IncorrectFormat);
Expand Down Expand Up @@ -5236,11 +5248,22 @@ impl<'a> SerdeDataType<'a> {
{
return Err(arbitrary::Error::IncorrectFormat);
}
let mut has_flatten_map = false;
let mut has_unknown_key_inside_flatten = false;
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
return Err(arbitrary::Error::IncorrectFormat);
}
if !ty.supported_flattened_map_inside_flatten_field(
pretty,
*flatten,
&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 {
// BUG: struct names inside flattend structs do not roundtrip
return Err(arbitrary::Error::IncorrectFormat);
Expand Down Expand Up @@ -5387,11 +5410,7 @@ impl<'a> SerdeDataType<'a> {
fields
.1
.iter()
.zip(fields.2.iter())
.all(|(field, flatten)| {
field.supported_inside_untagged(pretty, false, false)
&& (!*flatten || field.supported_inside_flatten(false))
})
.all(|field| field.supported_inside_untagged(pretty, false, false))
}
SerdeDataType::Enum {
name: _,
Expand Down Expand Up @@ -5440,11 +5459,7 @@ impl<'a> SerdeDataType<'a> {
fields
.1
.iter()
.zip(fields.2.iter())
.all(|(field, flatten)| {
field.supported_inside_untagged(pretty, false, false)
&& (!*flatten || field.supported_inside_flatten(false))
})
.all(|field| field.supported_inside_untagged(pretty, false, false))
}
}),
}
Expand Down Expand Up @@ -5652,6 +5667,168 @@ impl<'a> SerdeDataType<'a> {
// Only allowing string keys (for now) is both sensible and easier
matches!(self, SerdeDataType::String)
}

fn supported_flattened_map_inside_flatten_field(
&self,
pretty: &PrettyConfig,
is_flattened: bool,
has_flattened_map: &mut bool,
has_unknown_key: &mut bool,
) -> bool {
match self {
SerdeDataType::Unit => true,
SerdeDataType::Bool => true,
SerdeDataType::I8 => true,
SerdeDataType::I16 => true,
SerdeDataType::I32 => true,
SerdeDataType::I64 => true,
SerdeDataType::I128 => true,
SerdeDataType::ISize => true,
SerdeDataType::U8 => true,
SerdeDataType::U16 => true,
SerdeDataType::U32 => true,
SerdeDataType::U64 => true,
SerdeDataType::U128 => true,
SerdeDataType::USize => true,
SerdeDataType::F32 => true,
SerdeDataType::F64 => true,
SerdeDataType::Char => true,
SerdeDataType::String => true,
SerdeDataType::ByteBuf => true,
SerdeDataType::Option { inner } => {
if is_flattened || pretty.extensions.contains(Extensions::IMPLICIT_SOME) {
inner.supported_flattened_map_inside_flatten_field(
pretty,
is_flattened,
has_flattened_map,
has_unknown_key,
)
} else {
true
}
}
SerdeDataType::Array { kind: _, len: _ } => true,
SerdeDataType::Tuple { elems: _ } => true,
SerdeDataType::Vec { item: _ } => true,
SerdeDataType::Map { key: _, value: _ } => {
if is_flattened {
if *has_unknown_key {
// BUG: a flattened map will also see the unknown key (serde)
return false;
}
if *has_flattened_map {
// BUG: at most one flattened map is supported (serde)
return false;
}
*has_flattened_map = true;
}
true
}
SerdeDataType::UnitStruct { name: _ } => true,
SerdeDataType::Newtype { name: _, inner } => {
if is_flattened || pretty.extensions.contains(Extensions::UNWRAP_NEWTYPES) {
inner.supported_flattened_map_inside_flatten_field(
pretty,
is_flattened,
has_flattened_map,
has_unknown_key,
)
} else {
true
}
}
SerdeDataType::TupleStruct { name: _, fields: _ } => true,
SerdeDataType::Struct {
name: _,
tag: _,
fields,
} => {
if is_flattened {
fields
.1
.iter()
.zip(fields.2.iter())
.all(|(field, is_flattened)| {
field.supported_flattened_map_inside_flatten_field(
pretty,
*is_flattened,
has_flattened_map,
has_unknown_key,
)
})
} else if fields.2.iter().any(|x| *x) {
if *has_flattened_map {
// BUG: a flattened map will also see the unknown key (serde)
return false;
}
*has_unknown_key = true;
true
} else {
true
}
}
SerdeDataType::Enum {
name: _,
variants,
representation,
} => variants.1.iter().all(|variant| match variant {
SerdeDataVariantType::Unit => true,
SerdeDataVariantType::TaggedOther => true,
SerdeDataVariantType::Newtype { inner } => {
if is_flattened {
if *has_flattened_map {
// BUG: a flattened map will also see the unknown key (serde)
return false;
}
*has_unknown_key = true;
true
} else if matches!(representation, SerdeEnumRepresentation::Untagged) {
inner.supported_flattened_map_inside_flatten_field(
pretty,
is_flattened,
has_flattened_map,
has_unknown_key,
)
} else {
true
}
}
SerdeDataVariantType::Tuple { fields: _ } => {
if is_flattened {
if *has_flattened_map {
// BUG: a flattened map will also see the unknown key (serde)
return false;
}
*has_unknown_key = true;
}
true
}
SerdeDataVariantType::Struct { fields } => {
if is_flattened
|| matches!(
representation,
SerdeEnumRepresentation::InternallyTagged { tag: _ }
)
{
if *has_flattened_map {
// BUG: a flattened map will also see the unknown key (serde)
return false;
}
*has_unknown_key = true;
} else if matches!(representation, SerdeEnumRepresentation::Untagged)
&& fields.2.iter().any(|x| *x)
{
if *has_flattened_map {
// BUG: a flattened map will also see the unknown key (serde)
return false;
}
*has_unknown_key = true;
}
true
}
}),
}
}
}

#[derive(Debug, Default, PartialEq, Arbitrary, Serialize)]
Expand Down

0 comments on commit 6b67757

Please sign in to comment.