[resend] [patch] dbus_connection_send_with_reply() leaking DBusPendingCalls

[Timo Teräs]

Hiya,

Havoc Pennington wrote:
> Thanks for tracking down these issues, some comments follow.
> 
> On Wed, 2004-03-24 at 07:45, Timo Teräs wrote:
>>-        print_reply = TRUE;
>>+        print_reply = TRUE, message_type = DBUS_MESSAGE_TYPE_METHOD_CALL;
> I'd rather have two lines and avoid comma operator.

Done.

> Not sure strcasecmp is portable, but in any case its behavior is
> locale-specific which isn't desirable in this context - you'd want the
> equivalent of g_ascii_strcasecmp instead.
> 
> However, a simpler solution I think is:
>  strcmp (c, "true) == 0 || strcmp (c, "True") == 0 || strcmp (c, "TRUE")
> == 0
> 
> or better, factor out a parse_boolean function that does that.
> Supporting tRue and truE is pretty weird anyway.

I forgot all about the locale stuff. Hmm, I think strcmping agains 'true' is
enough, after all the all the other switches are lower case and case sensitive
as well.

>>diff -u -r1.76 dbus-connection.c
>>--- dbus/dbus-connection.c	8 Mar 2004 10:59:20 -0000	1.76
>>+++ dbus/dbus-connection.c	24 Mar 2004 12:36:49 -0000
>>@@ -1787,10 +1787,9 @@
>>     }
>> 
>>   if (pending_return)
>>-    {
>>-      dbus_pending_call_ref (pending);
>>-      *pending_return = pending;
>>-    }
>>+    *pending_return = pending;
>>+  else
>>+    dbus_pending_call_unref (pending);
>> 
> 
> 
> There's an additional memory leak right before this it looks like:
> 
>   if (!_dbus_connection_send_unlocked (connection, message, NULL))
>     {
>       _dbus_connection_detach_pending_call_and_unlock (connection,
>                                                        pending);
>       return FALSE;
>     }
> 
> Maybe a better patch would refactor the function to have an "out:" block
> and "goto out" when cleanup is required.

Polished patch attached. I used "error" instead since it was used also somewhere
else in the code. Also the error checking of "reply_link == NULL" could be simplified
by one line if the "reply" can be unreferenced before unlocking the connection, but
I don't really know the lockings that well. So I left it as it was.

> I see another possible bug here:
> 
>   reply_link = _dbus_list_alloc_link (reply);
>   if (!reply)
> 
> Probably should be if (reply_link == NULL).

Most likely, fixed that also.

- Timo
-------------- next part --------------
A non-text attachment was scrubbed...
Name: dbus_send-2.diff
Type: text/x-patch
Size: 1222 bytes
Desc: not available
Url : http://freedesktop.org/pipermail/dbus/attachments/20040325/a760eff5/dbus_send-2.bin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pending_call_fix-2.diff
Type: text/x-patch
Size: 1996 bytes
Desc: not available
Url : http://freedesktop.org/pipermail/dbus/attachments/20040325/a760eff5/pending_call_fix-2.bin