Skip to content

Commit

Permalink
seq: Move has_value_field into MapAccess, because it is not used …
Browse files Browse the repository at this point in the history
…outside it

This fixes a data race with nested maps in sequence fields.
Also, top-level sequences always have `has_value_field == false`,
so no need to check it
  • Loading branch information
Mingun committed May 21, 2022
1 parent 1100895 commit 0266831
Show file tree
Hide file tree
Showing 4 changed files with 19 additions and 21 deletions.
3 changes: 3 additions & 0 deletions Changelog.md
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,9 @@
- [#9]: Deserialization erroneously was successful in some cases where error is expected.
This broke deserialization of untagged enums which rely on error if variant cannot be parsed
- [#12]: Allow to have an ordinary elements together with a `$value` field
- [#12]: Internal deserializer state can be broken when deserialize a map with
a sequence field (such as `Vec<T>`), where elements of this sequence contains
another sequence. This error affects only users with a `serialize` feature enabled

### Misc Changes

Expand Down
14 changes: 12 additions & 2 deletions src/de/map.rs
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,14 @@ where
source: ValueSource,
/// List of field names of the struct. It is empty for maps
fields: &'static [&'static str],
/// If `true`, then deserialized struct have a field with a special name
/// [`INNER_VALUE`]. That field should be deserialized from the text content
/// of an XML node:
///
/// ```xml
/// <tag>value for INNER_VALUE field<tag>
/// ```
has_value_field: bool,
/// list of fields yet to unflatten (defined as starting with $unflatten=)
unflatten_fields: Vec<&'static [u8]>,
}
Expand All @@ -193,6 +201,7 @@ where
iter: IterState::new(0, false),
source: ValueSource::Unknown,
fields,
has_value_field: fields.contains(&INNER_VALUE),
unflatten_fields: fields
.iter()
.filter(|f| f.starts_with(UNFLATTEN_PREFIX))
Expand All @@ -217,7 +226,6 @@ where
// FIXME: There error positions counted from end of tag name - need global position
let slice = self.start.attributes_raw();
let decoder = self.de.reader.decoder();
let has_value_field = self.de.has_value_field;

if let Some(a) = self.iter.next(slice).transpose()? {
// try getting map from attributes (key= "value")
Expand Down Expand Up @@ -255,7 +263,9 @@ where
// }
// TODO: This should be handled by #[serde(flatten)]
// See https://github.com/serde-rs/serde/issues/1905
DeEvent::Start(e) if has_value_field && is_unknown(self.fields, e, decoder)? => {
DeEvent::Start(e)
if self.has_value_field && is_unknown(self.fields, e, decoder)? =>
{
self.source = ValueSource::Content;
seed.deserialize(INNER_VALUE.into_deserializer()).map(Some)
}
Expand Down
11 changes: 0 additions & 11 deletions src/de/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -255,14 +255,6 @@ where
{
reader: R,
peek: Option<DeEvent<'de>>,
/// Special sing that deserialized struct have a field with the special
/// name (see constant `INNER_VALUE`). That field should be deserialized
/// from the text content of the XML node:
///
/// ```xml
/// <tag>value for INNER_VALUE field<tag>
/// ```
has_value_field: bool,
}

/// Deserialize an instance of type `T` from a string of XML text.
Expand Down Expand Up @@ -354,7 +346,6 @@ where
Deserializer {
reader,
peek: None,
has_value_field: false,
}
}

Expand Down Expand Up @@ -544,10 +535,8 @@ where
// Try to go to the next `<tag ...>...</tag>` or `<tag .../>`
if let Some(e) = self.next_start()? {
let name = e.name().to_vec();
self.has_value_field = fields.contains(&INNER_VALUE);
let map = map::MapAccess::new(self, e, fields)?;
let value = visitor.visit_map(map)?;
self.has_value_field = false;
self.read_to_end(&name)?;
Ok(value)
} else {
Expand Down
12 changes: 4 additions & 8 deletions src/de/seq.rs
Original file line number Diff line number Diff line change
Expand Up @@ -90,15 +90,11 @@ where
{
/// Creates a new accessor to a top-level sequence of XML elements.
pub fn new(de: &'a mut Deserializer<'de, R>) -> Result<Self, DeError> {
let filter = if de.has_value_field {
TagFilter::Exclude(&[])
let filter = if let DeEvent::Start(e) = de.peek()? {
// Clone is cheap if event borrows from the input
TagFilter::Include(e.clone())
} else {
if let DeEvent::Start(e) = de.peek()? {
// Clone is cheap if event borrows from the input
TagFilter::Include(e.clone())
} else {
TagFilter::Exclude(&[])
}
TagFilter::Exclude(&[])
};
Ok(Self { de, filter })
}
Expand Down

0 comments on commit 0266831

Please sign in to comment.