taglists: warn if someone tries to add empty or NULL string tags to a taglist

Also warn if an element or application tries to add a field with an
empty string to a structure (NULL strings are still needed and
allowed though) and do all those checks in the right function.

Fixes #559643.

diff --git a/gst/gststructure.c b/gst/gststructure.c
index 3ac5555..af1243a 100644 (file)
--- a/gst/gststructure.c
+++ b/gst/gststructure.c
@@ -59,6 +59,7 @@
 #include <string.h>
 
 #include "gst_private.h"
+#include "gstquark.h"
 #include <gst/gst.h>
 #include <gobject/gvaluecollector.h>
 
@@ -77,6 +78,9 @@ struct _GstStructureField
     (!(structure)->parent_refcount || \
      g_atomic_int_get ((structure)->parent_refcount) == 1)
 
+#define IS_TAGLIST(structure) \
+    (structure->name == GST_QUARK (TAGLIST))
+
 static void gst_structure_set_field (GstStructure * structure,
     GstStructureField * field);
 static GstStructureField *gst_structure_get_field (const GstStructure *
@@ -432,17 +436,6 @@ gst_structure_id_set_value (GstStructure * structure,
   g_return_if_fail (G_IS_VALUE (value));
   g_return_if_fail (IS_MUTABLE (structure));
 
-  if (G_VALUE_HOLDS_STRING (value)) {
-    const gchar *s;
-
-    s = g_value_get_string (value);
-    if (G_UNLIKELY (s != NULL && !g_utf8_validate (s, -1, NULL))) {
-      g_warning ("Trying to set string field '%s' on structure, but string is "
-          "not valid UTF-8. Please file a bug.", g_quark_to_string (field));
-      return;
-    }
-  }
-
   gsfield.name = field;
   gst_value_init_and_copy (&gsfield.value, value);
 
@@ -653,6 +646,31 @@ gst_structure_set_field (GstStructure * structure, GstStructureField * field)
   GstStructureField *f;
   guint i;
 
+  if (G_UNLIKELY (G_VALUE_HOLDS_STRING (&field->value))) {
+    const gchar *s;
+
+    s = g_value_get_string (&field->value);
+    /* only check for NULL strings in taglists, as they are allowed in message
+     * structs, e.g. error message debug strings */
+    if (G_UNLIKELY (s == NULL && IS_TAGLIST (structure))) {
+      g_warning ("Trying to set NULL string on field '%s' on taglist. "
+          "Please file a bug.", g_quark_to_string (field->name));
+      return;
+    } else if (G_UNLIKELY (s != NULL && *s == '\0')) {
+      /* empty strings never make sense */
+      g_warning ("Trying to set empty string on %s field '%s'. Please file a "
+          "bug.", IS_TAGLIST (structure) ? "taglist" : "structure",
+          g_quark_to_string (field->name));
+      return;
+    } else if (G_UNLIKELY (s != NULL && !g_utf8_validate (s, -1, NULL))) {
+      g_warning ("Trying to set string on %s field '%s', but string is not "
+          "valid UTF-8. Please file a bug.",
+          IS_TAGLIST (structure) ? "taglist" : "structure",
+          g_quark_to_string (field->name));
+      return;
+    }
+  }
+
   for (i = 0; i < structure->fields->len; i++) {
     f = GST_STRUCTURE_FIELD (structure, i);
 

diff --git a/tests/check/gst/gststructure.c b/tests/check/gst/gststructure.c
index 3c06e46..e2cf577 100644 (file)
--- a/tests/check/gst/gststructure.c
+++ b/tests/check/gst/gststructure.c
@@ -385,6 +385,18 @@ GST_START_TEST (test_structure_nested)
 
 GST_END_TEST;
 
+GST_START_TEST (test_empty_string_fields)
+{
+  GstStructure *s;
+
+  s = gst_structure_empty_new ("con/struct");
+  ASSERT_WARNING (gst_structure_set (s, "layout", G_TYPE_STRING, "", NULL));
+  gst_structure_set (s, "debug-string", G_TYPE_STRING, NULL, NULL);
+  gst_structure_free (s);
+}
+
+GST_END_TEST;
+
 static Suite *
 gst_structure_suite (void)
 {
@@ -401,6 +413,7 @@ gst_structure_suite (void)
   tcase_add_test (tc_chain, test_fixate);
   tcase_add_test (tc_chain, test_fixate_frac_list);
   tcase_add_test (tc_chain, test_structure_nested);
+  tcase_add_test (tc_chain, test_empty_string_fields);
   return s;
 }
 

diff --git a/tests/check/gst/gsttag.c b/tests/check/gst/gsttag.c
index 4adc59a..898db31 100644 (file)
--- a/tests/check/gst/gsttag.c
+++ b/tests/check/gst/gsttag.c
@@ -383,6 +383,21 @@ GST_START_TEST (test_buffer_tags)
 
 GST_END_TEST;
 
+GST_START_TEST (test_empty_tags)
+{
+  GstTagList *tags;
+
+  tags = gst_tag_list_new ();
+  ASSERT_WARNING (gst_tag_list_add (tags, GST_TAG_MERGE_APPEND,
+          GST_TAG_ARTIST, NULL, NULL));
+  ASSERT_WARNING (gst_tag_list_add (tags, GST_TAG_MERGE_APPEND,
+          GST_TAG_ARTIST, "", NULL));
+  gst_tag_list_add (tags, GST_TAG_MERGE_APPEND, GST_TAG_ARTIST, "xyz", NULL);
+  gst_tag_list_free (tags);
+}
+
+GST_END_TEST;
+
 static Suite *
 gst_tag_suite (void)
 {
@@ -397,6 +412,7 @@ gst_tag_suite (void)
   tcase_add_test (tc_chain, test_type);
   tcase_add_test (tc_chain, test_set_non_utf8_string);
   tcase_add_test (tc_chain, test_buffer_tags);
+  tcase_add_test (tc_chain, test_empty_tags);
 
   return s;
 }