Merge branch 'th/gdataset-fix-zero-key' into 'main'

[th/gdataset-fix-zero-key] fix and cleanup related to using a zero GQuark for keys in GData

See merge request GNOME/glib!4628

glib/gdataset.c

@@ -301,6 +301,10 @@ datalist_append (GData **data, GQuark key_id, gpointer new_data, GDestroyNotify
   gboolean reallocated;
   GData *d;
 
+#ifdef G_ENABLE_DEBUG
+  g_assert (key_id != 0);
+#endif
+
   d = *data;
   if (!d)
     {
@@ -613,6 +617,10 @@ g_data_set_internal (GData	  **datalist,
   GDataElt old, *data;
   guint32 idx;
 
+#ifdef G_ENABLE_DEBUG
+  g_assert (key_id != 0);
+#endif
+
   d = g_datalist_lock_and_get (datalist);
 
   data = datalist_find (d, key_id, &idx);
@@ -1009,8 +1017,11 @@ g_dataset_id_remove_no_notify (gconstpointer  dataset_location,
 
   g_return_val_if_fail (dataset_location != NULL, NULL);
   
+  if (key_id == 0)
+    return NULL;
+
   G_LOCK (g_dataset_global);
-  if (key_id && g_dataset_location_ht)
+  if (g_dataset_location_ht)
     {
       GDataset *dataset;
   
@@ -1082,8 +1093,6 @@ g_datalist_id_remove_no_notify (GData	**datalist,
  * @user_data.
  *
  * Returns: the value returned by @callback.
- *
- * Since: 2.80
  */
 gpointer
 g_datalist_id_update_atomic (GData **datalist,
@@ -1098,6 +1107,9 @@ g_datalist_id_update_atomic (GData **datalist,
   GDestroyNotify new_destroy;
   guint32 idx;
 
+  g_return_val_if_fail (datalist, NULL);
+  g_return_val_if_fail (key_id != 0, NULL);
+
   d = g_datalist_lock_and_get (datalist);
 
   data = datalist_find (d, key_id, &idx);
@@ -1198,9 +1210,12 @@ g_dataset_id_get_data (gconstpointer  dataset_location,
   gpointer retval = NULL;
 
   g_return_val_if_fail (dataset_location != NULL, NULL);
-  
+
+  if (key_id == 0)
+    return NULL;
+
   G_LOCK (g_dataset_global);
-  if (key_id && g_dataset_location_ht)
+  if (g_dataset_location_ht)
     {
       GDataset *dataset;
       
@@ -1414,6 +1429,9 @@ g_datalist_get_data (GData **datalist,
 
   g_return_val_if_fail (datalist != NULL, NULL);
 
+  if (G_UNLIKELY (!key))
+    return NULL;
+
   d = g_datalist_lock_and_get (datalist);
 
   if (!d)
@@ -1427,6 +1445,8 @@ g_datalist_get_data (GData **datalist,
 
       for (i = 0; i < d->len; i++)
         {
+          const char *qstr;
+
           data_elt = &d->data[i];
           /* Here we intentionally compare by strings, instead of calling
            * g_quark_try_string() first.
@@ -1434,7 +1454,8 @@ g_datalist_get_data (GData **datalist,
            * See commit 1cceda49b60b ('Make g_datalist_get_data not look up the
            * quark').
            */
-          if (g_strcmp0 (g_quark_to_string (data_elt->key), key) == 0)
+          qstr = g_quark_to_string (data_elt->key);
+          if (qstr && strcmp (qstr, key) == 0)
             {
               res = data_elt->data;
               goto out;
@@ -1444,7 +1465,7 @@ g_datalist_get_data (GData **datalist,
     }
 
   key_id = g_quark_try_string (key);
-  if (key_id == 0 && key)
+  if (key_id == 0)
     goto out;
 
   data_elt = g_hash_table_lookup (index, &key_id);

glib/tests/dataset.c

@@ -224,14 +224,29 @@ test_datalist_clear (void)
 }
 
 static void
-test_datalist_basic (void)
+test_datalist_basic (gconstpointer test_data)
 {
+  const gboolean HAS_MANY = GPOINTER_TO_INT (test_data);
+  const GQuark BOGUS_QUARK = 1000000000u;
   GData *list = NULL;
   gpointer data;
   gpointer ret;
+  guint i;
 
   g_datalist_init (&list);
   data = "one";
+
+  if (HAS_MANY)
+    {
+      /* Add many entries. This ensures we cover the code path where GData uses
+       * a lookup hash table. */
+      for (i = 0; i < 200; i++)
+        {
+          /* pick a bogus GQuark. They work fine here. */
+          g_datalist_id_set_data (&list, BOGUS_QUARK + 1u + i, data);
+        }
+    }
+
   g_datalist_set_data (&list, "one", data);
   ret = g_datalist_get_data (&list, "one");
   g_assert_true (ret == data);
@@ -242,6 +257,16 @@ test_datalist_basic (void)
   ret = g_datalist_get_data (&list, NULL);
   g_assert_null (ret);
 
+  /* It is not enforced that GQuark are valid. They are just numbers and work
+   * too. */
+  g_datalist_id_set_data (&list, BOGUS_QUARK, data);
+  ret = g_datalist_id_get_data (&list, BOGUS_QUARK);
+  g_assert_true (ret == data);
+
+  /* Ensure that we don't find the BOGUS_QUARK when looking up by NULL. */
+  ret = g_datalist_get_data (&list, NULL);
+  g_assert_null (ret);
+
   g_datalist_clear (&list);
 }
 
@@ -531,7 +556,8 @@ main (int argc, char *argv[])
   g_test_add_func ("/dataset/full", test_dataset_full);
   g_test_add_func ("/dataset/foreach", test_dataset_foreach);
   g_test_add_func ("/dataset/destroy", test_dataset_destroy);
-  g_test_add_func ("/datalist/basic", test_datalist_basic);
+  g_test_add_data_func ("/datalist/basic/few", GINT_TO_POINTER (FALSE), test_datalist_basic);
+  g_test_add_data_func ("/datalist/basic/many", GINT_TO_POINTER (TRUE), test_datalist_basic);
   g_test_add_func ("/datalist/id", test_datalist_id);
   g_test_add_func ("/datalist/recursive-clear", test_datalist_clear);
   g_test_add_func ("/datalist/id-remove-multiple", test_datalist_id_remove_multiple);