From 171752d9eea6f2ff83ae45c462e7f5090f48c549 Mon Sep 17 00:00:00 2001
From: Alexandre Courbot <acourbot@chromium.org>
Date: Thu, 24 Nov 2022 13:50:15 +0900
Subject: [PATCH] serde_keyvalue: fix parsing of quoted strings in a sequence

Parsing a quoted string as the last element of a sequence would fail
since ']' was not recognized as a separator. Fix this and factorize the
code recognizing separators into a single function to prevent this kind
of issue from happening again in the future.

BUG=None
TEST=cargo test -p serde_keyvalue

Change-Id: I1367da843787c40fb6c15a615d7cd036bddd1381
Reviewed-on: https://chromium-review.googlesource.com/c/crosvm/crosvm/+/4054812
Auto-Submit: Alexandre Courbot <acourbot@chromium.org>
Reviewed-by: Daniel Verkamp <dverkamp@chromium.org>
Commit-Queue: Alexandre Courbot <acourbot@chromium.org>
---
 serde_keyvalue/src/key_values.rs | 71 ++++++++++++++++++++++++++++----
 1 file changed, 63 insertions(+), 8 deletions(-)

diff --git a/serde_keyvalue/src/key_values.rs b/serde_keyvalue/src/key_values.rs
index 68683b03d1..b0b8ceaf79 100644
--- a/serde_keyvalue/src/key_values.rs
+++ b/serde_keyvalue/src/key_values.rs
@@ -215,6 +215,10 @@ fn any_identifier(s: &str) -> IResult<&str, &str> {
     ident(s)
 }
 
+fn is_separator(c: Option<char>) -> bool {
+    matches!(c, Some(',') | Some(']') | None)
+}
+
 /// Serde deserializer for key-values strings.
 pub struct KeyValueDeserializer<'de> {
     /// Full input originally received for parsing.
@@ -316,11 +320,12 @@ impl<'de> KeyValueDeserializer<'de> {
 
         self.input = remainder;
 
-        // The character following a string will be either a comma or EOS. If we have something
-        // else, this means an unquoted string should probably have been quoted.
-        match self.peek_char() {
-            Some(',') | None => Ok(res),
-            Some(_) => Err(self.error_here(ErrorKind::InvalidCharInString)),
+        // The character following a string will be either a comma, a closing bracket, or EOS. If
+        // we have something else, this means an unquoted string should probably have been quoted.
+        if is_separator(self.peek_char()) {
+            Ok(res)
+        } else {
+            Err(self.error_here(ErrorKind::InvalidCharInString))
         }
     }
 
@@ -396,8 +401,8 @@ impl<'de> de::MapAccess<'de> for KeyValueDeserializer<'de> {
                 Ok(val)
             }
             // Ok if we are parsing a boolean where an empty value means true.
-            Some(',') | Some(']') | None => Ok(val),
-            Some(_) => Err(self.error_here(ErrorKind::ExpectedEqual)),
+            c if is_separator(c) => Ok(val),
+            _ => Err(self.error_here(ErrorKind::ExpectedEqual)),
         }
     }
 
@@ -553,7 +558,7 @@ impl<'de, 'a> de::Deserializer<'de> for &'a mut KeyValueDeserializer<'de> {
     {
         match self.peek_char() {
             // If we have no value following, then we are dealing with a boolean flag.
-            Some(',') | None => return self.deserialize_bool(visitor),
+            c if is_separator(c) => return self.deserialize_bool(visitor),
             // Opening bracket means we have a sequence.
             Some('[') => return self.deserialize_seq(visitor),
             _ => (),
@@ -1408,6 +1413,56 @@ mod tests {
         );
     }
 
+    #[test]
+    fn deserialize_vector_of_strings() {
+        #[derive(Deserialize, PartialEq, Debug)]
+        struct TestStruct {
+            strs: Vec<String>,
+        }
+
+        // Unquoted strings
+        let res: TestStruct =
+            from_key_values(r#"strs=[singleword,camel_cased,kebab-cased]"#).unwrap();
+        assert_eq!(
+            res,
+            TestStruct {
+                strs: vec![
+                    "singleword".into(),
+                    "camel_cased".into(),
+                    "kebab-cased".into()
+                ],
+            }
+        );
+
+        // All quoted strings
+        let res: TestStruct =
+            from_key_values(r#"strs=["first string","second string","third string"]"#).unwrap();
+        assert_eq!(
+            res,
+            TestStruct {
+                strs: vec![
+                    "first string".into(),
+                    "second string".into(),
+                    "third string".into()
+                ],
+            }
+        );
+
+        // Mix
+        let res: TestStruct =
+            from_key_values(r#"strs=[unquoted,"quoted string",'quoted with escape "']"#).unwrap();
+        assert_eq!(
+            res,
+            TestStruct {
+                strs: vec![
+                    "unquoted".into(),
+                    "quoted string".into(),
+                    "quoted with escape \"".into()
+                ],
+            }
+        );
+    }
+
     #[test]
     fn deserialize_vector_of_structs() {
         #[derive(Deserialize, PartialEq, Debug)]