2005-05-06 Robert McQueen <robot101@debian.org>

	* glib/dbus-gvalue-utils.c: Fix the failing test where static string
	  pointers were put into a GPtrArray-based specialised collection, and
	  then freed along with the array. GValues which you add into
	  collections or maps which have the NOCOPY flag set are assumed to not
	  belong to the caller, so rather than the existing pointer-stealing
	  semantics, they are copied instead. Given that the main consumers of
	  this abstraction are the bindings themselves, I don't think this is
	  too bad, but others should watch their choice of take vs set_static.

diff --git a/ChangeLog b/ChangeLog
index 68aa136d7e87f8912c8a26451d1d09de022b8d3e..ddb650cb7e803bd6dcfc2268051889052fb72c31 100644 (file)
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,14 @@
+2005-05-06  Robert McQueen  <robot101@debian.org>
+
+       * glib/dbus-gvalue-utils.c: Fix the failing test where static string
+         pointers were put into a GPtrArray-based specialised collection, and
+         then freed along with the array. GValues which you add into
+         collections or maps which have the NOCOPY flag set are assumed to not
+         belong to the caller, so rather than the existing pointer-stealing
+         semantics, they are copied instead. Given that the main consumers of
+         this abstraction are the bindings themselves, I don't think this is
+         too bad, but others should watch their choice of take vs set_static.
+
 2005-05-06  Robert McQueen  <robot101@debian.org>
 
        * glib/dbus-gvalue-utils.c: Spotted a warning about the return value

diff --git a/glib/dbus-gvalue-utils.c b/glib/dbus-gvalue-utils.c
index 83e9557247b88e02c7c33339d413ad8ff98cf4d0..89ff16d95672e8ee7e065f3454da2ceacfc880fc 100644 (file)
--- a/glib/dbus-gvalue-utils.c
+++ b/glib/dbus-gvalue-utils.c
@@ -744,6 +744,20 @@ gvalue_take_ptrarray_value (GValue *value, gpointer instance)
 static gpointer
 ptrarray_value_from_gvalue (const GValue *value)
 {
+  GValue tmp = {0, };
+
+  /* if the NOCOPY flag is set, then value was created via set_static and hence
+   * is not owned by us. in order to preserve the "take" semantics that the API
+   * has in general (which avoids copying in the common case), we must copy any
+   * static values so that we can indiscriminately free the entire collection
+   * later. */
+  if (value->data[1].v_uint & G_VALUE_NOCOPY_CONTENTS)
+    {
+      g_value_init (&tmp, G_VALUE_TYPE (value));
+      g_value_copy (value, &tmp);
+      value = &tmp;
+    }
+
   switch (g_type_fundamental (G_VALUE_TYPE (value)))
     {
     case G_TYPE_STRING:
@@ -1315,17 +1329,7 @@ _dbus_gvalue_utils_test (const char *datadir)
     g_assert (!strcmp ("bar", g_ptr_array_index (instance, 1)));
     g_assert (!strcmp ("baz", g_ptr_array_index (instance, 2)));
 
-    /* FIXME this crashes, I believe because ptrarray_append
-     * doesn't copy the incoming static string, then ptrarray_free
-     * tries to free it; looks to me like always copying appended
-     * values would be the only working approach.
-     */
     g_value_unset (&val);
-    /* FIXME make sure this test fails for everyone, since
-     * apparently people didn't see it, the bad free
-     * maybe didn't crash everywhere
-     */
-    g_assert_not_reached();
   }
 
   type = dbus_g_type_get_struct ("GValueArray", G_TYPE_STRING, G_TYPE_UINT, DBUS_TYPE_G_OBJECT_PATH, G_TYPE_INVALID);