gst/gststructure.c: Don't crash in _from_string() if the structure name is not valid...

Original commit message from CVS:
* gst/gststructure.c: (gst_structure_validate_name),
(gst_structure_new_valist), (gst_structure_parse_value),
(gst_structure_from_string):
Don't crash in _from_string() if the structure name is not valid
(fixes #501560).  Allow structure names to start with a number
again (this apparently broke the ubuntu codec installer).
* tests/check/gst/gststructure.c: (GST_START_TEST), (GST_START_TEST),
(GST_START_TEST):
Add unit test for the crash; update unit tests for new behaviour.

diff --git a/ChangeLog b/ChangeLog
index 9885e0eb62d0a307861404a69cbc11fa531ad661..d487edeeb3523e55d2a8b9eb6b24082f250488ef 100644 (file)
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,16 @@
+2007-12-08  Tim-Philipp Müller  <tim at centricular dot net>
+
+       * gst/gststructure.c: (gst_structure_validate_name),
+         (gst_structure_new_valist), (gst_structure_parse_value),
+         (gst_structure_from_string):
+         Don't crash in _from_string() if the structure name is not valid
+         (fixes #501560).  Allow structure names to start with a number
+         again (this apparently broke the ubuntu codec installer).
+
+       * tests/check/gst/gststructure.c: (GST_START_TEST), (GST_START_TEST),
+         (GST_START_TEST):
+         Add unit test for the crash; update unit tests for new behaviour.
+
 2007-12-03  Wim Taymans  <wim.taymans@gmail.com>
 
        * gst/gstutils.c:

diff --git a/gst/gststructure.c b/gst/gststructure.c
index e996ca4f5644af5d1735460a54c4fdae31300116..537bb4ac5ce9075fa6ec239c0d2279a0712dc8f9 100644 (file)
--- a/gst/gststructure.c
+++ b/gst/gststructure.c
@@ -145,7 +145,8 @@ gst_structure_validate_name (const gchar * name)
 
   g_return_val_if_fail (name != NULL, FALSE);
 
-  if (!g_ascii_isalpha (*name)) {
+  /* FIXME 0.11: use g_ascii_isalpha() */
+  if (!g_ascii_isalnum (*name)) {
     GST_WARNING ("Invalid character '%c' at offset 0 in structure name: %s",
         *name, name);
     return FALSE;
@@ -236,7 +237,9 @@ gst_structure_new_valist (const gchar * name,
   g_return_val_if_fail (name != NULL, NULL);
 
   structure = gst_structure_empty_new (name);
-  gst_structure_set_valist (structure, firstfield, varargs);
+
+  if (structure)
+    gst_structure_set_valist (structure, firstfield, varargs);
 
   return structure;
 }
@@ -1883,7 +1886,7 @@ gst_structure_parse_value (gchar * str,
  * where parsing ended will be returned.
  *
  * Returns: a new #GstStructure or NULL when the string could not
- * be parsed. Free after usage.
+ * be parsed. Free with gst_structure_free() after use.
  */
 GstStructure *
 gst_structure_from_string (const gchar * string, gchar ** end)
@@ -1901,7 +1904,7 @@ gst_structure_from_string (const gchar * string, gchar ** end)
   copy = g_strdup (string);
   r = copy;
 
-  /* skipe spaces */
+  /* skip spaces (FIXME: _isspace treats tabs and newlines as space!) */
   while (*r && (g_ascii_isspace (*r) || (r[0] == '\\'
               && g_ascii_isspace (r[1]))))
     r++;
@@ -1917,6 +1920,9 @@ gst_structure_from_string (const gchar * string, gchar ** end)
   structure = gst_structure_empty_new (name);
   *w = save;
 
+  if (structure == NULL)
+    goto error;
+
   do {
     while (*r && (g_ascii_isspace (*r) || (r[0] == '\\'
                 && g_ascii_isspace (r[1]))))

diff --git a/tests/check/gst/gststructure.c b/tests/check/gst/gststructure.c
index b0e423921929c63266dbe128f5d856943b5af7f6..6e032d2350acd10a9e9fd56a71a004a372563723 100644 (file)
--- a/tests/check/gst/gststructure.c
+++ b/tests/check/gst/gststructure.c
@@ -115,6 +115,17 @@ GST_START_TEST (test_from_string)
   fail_unless (G_VALUE_HOLDS_BOOLEAN (val));
   fail_unless_equals_int (g_value_get_boolean (val), TRUE);
   gst_structure_free (structure);
+
+  /* This should still work for now (FIXME: 0.11) */
+  s = "0.10:decoder-video/mpeg, abc=(boolean)false";
+  structure = gst_structure_from_string (s, NULL);
+  fail_if (structure == NULL, "Could not get structure from string %s", s);
+  gst_structure_free (structure);
+
+  /* make sure we bail out correctly in case of an error or if parsing fails */
+  s = "***foo***, abc=(boolean)false";
+  structure = gst_structure_from_string (s, NULL);
+  fail_unless (structure == NULL);
 }
 
 GST_END_TEST;
@@ -131,14 +142,18 @@ GST_START_TEST (test_to_string)
 #if 0
   ASSERT_CRITICAL (st1 = gst_structure_new ("Foo with whitespace", NULL));
   fail_unless (st1 == NULL);
+  ASSERT_CRITICAL (st1 = gst_structure_new ("1st", NULL));
+  fail_unless (st1 == NULL);
 #else
   st1 = gst_structure_new ("Foo with whitespace is still allowed", NULL);
   fail_unless (st1 != NULL);
   gst_structure_free (st1);
-#endif
 
-  ASSERT_CRITICAL (st1 = gst_structure_new ("1st", NULL));
-  fail_unless (st1 == NULL);
+  /* structure names starting with a number are also still allowed */
+  st1 = gst_structure_new ("1st", NULL);
+  fail_unless (st1 != NULL);
+  gst_structure_free (st1);
+#endif
 }
 
 GST_END_TEST;
@@ -239,6 +254,14 @@ GST_START_TEST (test_structure_new)
   s = gst_structure_new ("name", "key", GST_TYPE_G_ERROR, e, NULL);
   g_error_free (e);
   gst_structure_free (s);
+
+  /* This should still work for now (FIXME 0.11) */
+  gst_structure_free (gst_structure_new ("0.10:decoder-video/mpeg", NULL));
+
+  /* make sure we bail out correctly in case of an error or if parsing fails */
+  ASSERT_CRITICAL (s = gst_structure_new ("^joo\nba\ndoo^",
+          "abc", G_TYPE_BOOLEAN, FALSE, NULL));
+  fail_unless (s == NULL);
 }
 
 GST_END_TEST;