[prev in list] [next in list] [prev in thread] [next in thread] 

List:       freedesktop-dbus
Subject:    Re: glib bindings issues
From:       Robert McQueen <robert.mcqueen () collabora ! co ! uk>
Date:       2006-06-07 0:00:10
Message-ID: 4486170A.7070309 () collabora ! co ! uk
[Download RAW message or body]

Havoc Pennington wrote:
> Hi,

Hola,

> "make check" in glib wasn't working for me, due to GPtrArray append not
> copying the string values passed in. I added more details in a comment
> and also a g_assert_not_reached() to be sure it crashes for everyone
> instead of just corrupting memory, but someone who gets this code needs
> to debug.

Yeah I knew it was broken, that was my bad after adding the freeing
patch. I've fixed it now. Rather than copying everything (the
demarshallers already copy all of the things which are taken from the
messages), and also breaking the API's take semantics for existing
users, I just take a copy if the NOCOPY flag is set (! :D), which seems
to be the right behaviour. I committed the attached patch, which I've
shown round at the office to general agreement. :)

> There's also another likely bug in GSList handling, I added an assertion
> and comment there as well.

Yeah, I cleaned that up too. The GSList code was completely broken in
fact (copying, iterating and appending all screwed), but the code is
hard to reach at the moment because the bindings and binding-tool are
all hardcoded to give you GPtrArray for demarshalling those types.
Something I hope to hack at a bit at GUADEC, making an equivalence
between D-Bus interfaces and GInterfaces on both client and server side,
and using glue and/or introspection as necessary to let people direct
the types that we demarshal to.

> Havoc

Regards,
Rob

["ptrarray-copy-static.patch" (text/x-patch)]

? .dbus-gtype-specialized.h.swp
? .dbus-gvalue-utils.c.swo
? .dbus-gvalue-utils.c.swp
? cscope.out
? ptrarray-copy-static.patch
Index: dbus-gvalue-utils.c
===================================================================
RCS file: /cvs/dbus/dbus/glib/dbus-gvalue-utils.c,v
retrieving revision 1.13
diff -u -p -r1.13 dbus-gvalue-utils.c
--- dbus-gvalue-utils.c	6 Jun 2006 23:07:04 -0000	1.13
+++ dbus-gvalue-utils.c	6 Jun 2006 23:50:47 -0000
@@ -744,6 +744,20 @@ gvalue_take_ptrarray_value (GValue *valu
 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 *dat
     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);



[prev in list] [next in list] [prev in thread] [next in thread]