From d781d09d99428b1626d76171868fe16c2cf8ede2 Mon Sep 17 00:00:00 2001
From: Jan Schmidt <jan@centricular.com>
Date: Thu, 9 Mar 2017 12:09:57 +1100
Subject: [PATCH] gstvalue: Do more checks when guessing at flagset strings

If guessing that a string matches a flagset, be more thorough
at checking that the string following a string of hex:hex:
actually looks like a flag set string. Add some unit tests
to catch more cases.

https://bugzilla.gnome.org/show_bug.cgi?id=779755
---
 gst/gstvalue.c                 | 46 +++++++++++++++++++++++++++++++++++-------
 tests/check/gst/gstcaps.c      |  3 ++-
 tests/check/gst/gststructure.c | 26 ++++++++++++++++++++++++
 3 files changed, 67 insertions(+), 8 deletions(-)

diff --git a/gst/gstvalue.c b/gst/gstvalue.c
index 7d6ef29..f0dbae2 100644
--- a/gst/gstvalue.c
+++ b/gst/gstvalue.c
@@ -7055,6 +7055,14 @@ gst_value_serialize_flagset (const GValue * value)
 }
 
 static gboolean
+is_valid_flags_string (const gchar * s)
+{
+  /* We're looking to match +this/that+other-thing/not-this-thing type strings */
+  return g_regex_match_simple ("^([\\+\\/][\\w\\d-]+)+$", s, G_REGEX_CASELESS,
+      0);
+}
+
+static gboolean
 gst_value_deserialize_flagset (GValue * dest, const gchar * s)
 {
   gboolean res = FALSE;
@@ -7088,21 +7096,45 @@ gst_value_deserialize_flagset (GValue * dest, const gchar * s)
   if (G_UNLIKELY ((mask == 0 && errno == EINVAL) || cur == next))
     goto try_as_flags_string;
 
-  /* Next char should be NULL terminator, or a ':' */
-  if (G_UNLIKELY (next[0] != 0 && next[0] != ':'))
-    goto try_as_flags_string;
+  /* Next char should be NULL terminator, or a ':'. If ':', we need the flag string after */
+  if (G_UNLIKELY (next[0] == 0)) {
+    res = TRUE;
+    goto done;
+  }
+
+  if (next[0] != ':')
+    return FALSE;
+
+  s = next + 1;
 
+  if (g_str_equal (g_type_name (G_VALUE_TYPE (dest)), "GstFlagSet")) {
+    /* If we're parsing a generic flag set, that can mean we're guessing
+     * at the type in deserialising a GstStructure so at least check that
+     * we have a valid-looking string, so we don't cause deserialisation of
+     * other types of strings like 00:01:00:00 - https://bugzilla.gnome.org/show_bug.cgi?id=779755 */
+    if (is_valid_flags_string (s)) {
+      res = TRUE;
+      goto done;
+    }
+    return FALSE;
+  }
+
+  /* Otherwise, we already got a hex string for a valid non-generic flagset type */
   res = TRUE;
+  goto done;
 
 try_as_flags_string:
 
-  if (!res) {
+  {
     const gchar *set_class = g_type_name (G_VALUE_TYPE (dest));
     GFlagsClass *flags_klass = NULL;
     const gchar *end;
 
-    if (g_str_equal (set_class, "GstFlagSet"))
-      goto done;                /* There's no hope to parse a generic flag set */
+    if (g_str_equal (set_class, "GstFlagSet")) {
+      /* There's no hope to parse the fields of generic flag set if we didn't already
+       * catch a hex-string above */
+      return FALSE;
+    }
 
     /* Flags class is the FlagSet class with 'Set' removed from the end */
     end = g_strrstr (set_class, "Set");
@@ -7128,9 +7160,9 @@ try_as_flags_string:
     }
   }
 
+done:
   if (res)
     gst_value_set_flagset (dest, flags, mask);
-done:
   return res;
 
 }
diff --git a/tests/check/gst/gstcaps.c b/tests/check/gst/gstcaps.c
index 80aa055..0809d7a 100644
--- a/tests/check/gst/gstcaps.c
+++ b/tests/check/gst/gstcaps.c
@@ -984,7 +984,8 @@ GST_START_TEST (test_intersect_flagset)
   c2 = gst_caps_from_string (test_string);
   g_free (test_string);
 
-  fail_unless (gst_caps_is_equal (c1, c2));
+  fail_unless (gst_caps_is_equal (c1, c2), "Caps %s != %s",
+      gst_caps_to_string (c1), gst_caps_to_string (c2));
 
   gst_caps_unref (c1);
   gst_caps_unref (c2);
diff --git a/tests/check/gst/gststructure.c b/tests/check/gst/gststructure.c
index aac00d9..a531717 100644
--- a/tests/check/gst/gststructure.c
+++ b/tests/check/gst/gststructure.c
@@ -159,6 +159,32 @@ GST_START_TEST (test_from_string)
   fail_unless_equals_int (g_value_get_boolean (val), TRUE);
   gst_structure_free (structure);
 
+  /* Tests for flagset deserialisation */
+  s = "foobar,value=0010:ffff";
+  structure = gst_structure_from_string (s, NULL);
+  fail_if (structure == NULL, "Could not get structure from string %s", s);
+  fail_unless ((val = gst_structure_get_value (structure, "value")) != NULL);
+  fail_unless (GST_VALUE_HOLDS_FLAG_SET (val));
+  gst_structure_free (structure);
+
+  /* In the presence of the hex values, the strings don't matter as long as they
+   * have the right form */
+  s = "foobar,value=0010:ffff:+random+other/not-the-other";
+  structure = gst_structure_from_string (s, NULL);
+  fail_if (structure == NULL, "Could not get structure from string %s", s);
+  fail_unless ((val = gst_structure_get_value (structure, "value")) != NULL);
+  fail_unless (GST_VALUE_HOLDS_FLAG_SET (val));
+  gst_structure_free (structure);
+
+  /* Test that a timecode string is deserialised as a string, not a flagset:
+   * https://bugzilla.gnome.org/show_bug.cgi?id=779755 */
+  s = "foobar,timecode=00:01:00:00";
+  structure = gst_structure_from_string (s, NULL);
+  fail_if (structure == NULL, "Could not get structure from string %s", s);
+  fail_unless ((val = gst_structure_get_value (structure, "timecode")) != NULL);
+  fail_unless (G_VALUE_HOLDS_STRING (val));
+  gst_structure_free (structure);
+
   s = "0.10:decoder-video/mpeg, abc=(boolean)false";
   ASSERT_CRITICAL (structure = gst_structure_from_string (s, NULL));
   fail_unless (structure == NULL, "Could not get structure from string %s", s);
-- 
2.7.4