[resend] [patch] dbus_connection_send_with_reply() leaking DBusPendingCalls

[Havoc Pennington]

Hi,

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.

>        else if (strstr (arg, "--dest=") == arg)
>  	dest = strchr (arg, '=') + 1;
>        else if (strstr (arg, "--type=") == arg)
> @@ -227,6 +227,18 @@
>  	  dbus_message_iter_append_string (&iter, c);
>  	  break;
>  
> +	case DBUS_TYPE_BOOLEAN:
> +          if (strcasecmp(c, "true") == 0)
> +            dbus_message_iter_append_boolean (&iter, TRUE);
> +	  else if (strcasecmp(c, "false") == 0)
> +            dbus_message_iter_append_boolean (&iter, FALSE);

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.

> 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.

I see another possible bug here:

  reply_link = _dbus_list_alloc_link (reply);
  if (!reply)

Probably should be if (reply_link == NULL).

Havoc