detect flatten #454
detect flatten #454
Conversation
| @@ -666,14 +679,28 @@ impl<'de, 'a> de::Deserializer<'de> for &'a mut Deserializer<'de> { | |||
| } | |||
| } | |||
|
|
|||
| #[derive(Clone, Copy, PartialEq, Eq)] | |||
| enum Terminator { | |||
| Map, MapAsStruct, Tuple, Struct, Seq | |||
| } | ||
| } | ||
|
|
||
| let terminator = if VisitorExpecting(&visitor).to_string().starts_with("struct ") { |
There was a problem hiding this comment.
Is there a way we could avoid the allocation by just reading into an array that's just big enough to check if the string starts with "struct "?
Also, fantastic job for finding the side channel we can use to detect flatten!
There was a problem hiding this comment.
maybe try this?
let mut cursor = std::io::Cursor::new([0u8; 7]);
write!(cursor, "{}", VisitorExpecting(&visitor)).ok(); // this is expected to failed
let terminator = if &cursor.into_inner() == b"struct " {
Terminator::MapAsStruct
} else {
Terminator::Map
};| @@ -19,7 +20,14 @@ impl<'a, 'b: 'a, 'c> de::Deserializer<'b> for &'c mut IdDeserializer<'a, 'b> { | |||
| where | |||
| V: Visitor<'b>, | |||
| { | |||
| self.d.deserialize_identifier(visitor) | |||
| if self.map_as_struct { | |||
There was a problem hiding this comment.
I don't like that the quotation marks are both consumed without checks.
(1) Should we allow flattened identifiers without quotations marks? This would not match serialisation and is not valid RON either, so I would say no.
(2) Hence, could we require that all identifiers are now in strings or does this break any tests?
(3) In any case, if there is a leading quotation mark, we should expect a closing one too and error if it doesn't exist. Similarly, if there is no opening one, then we also should not consume a closing one.
There was a problem hiding this comment.
Hence, could we require that all identifiers are now in strings or does this break any tests?
I thing in map_as_struct context, we should require identifier to be a string.
So no, it won't break any tests
|
Thank you @clouds56 for coming up with this idea! With some small changes (as indicated above), I'd like to accept your PR :) |
Codecov ReportPatch coverage:
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more Additional details and impacted files@@ Coverage Diff @@
## master #454 +/- ##
==========================================
- Coverage 86.65% 86.06% -0.59%
==========================================
Files 60 60
Lines 7364 7434 +70
==========================================
+ Hits 6381 6398 +17
- Misses 983 1036 +53
... and 2 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
|
Superseded by #455 |
No description provided.