dbus_connection_send_with_reply_and_block and errors

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

dbus_connection_send_with_reply_and_block and errors

Timo Teräs
Hi,

I just noticed that DBusError might refer to freed memory after some D-BUS
API calls. E.g.:

dbus_set_error() never duplicates the error.name field; it just does a
pointer assignment.

dbus_set_error_from_message() does dbus_set_error() with name pointer passed
from dbus_message_get_error_name(). So the DBusError is only valid as long
as the original DBusMessage object exists.

dbus_connection_send_with_reply_and_block() in cases on error does
dbus_set_error_from_message() and immediately dbus_message_unref(), thus all
  errors returned by _send_with_reply_and_block() will refer to already
freed memory. Most propably dbus_set_error_from_message() is misused in a
lot of other places too (e.g. applications using libdbus).

How this should be fixed? Modify DBusError to duplicate/free the .name field
as well? Or maybe dbus_set_error_from_message() could add a refcount to the
message?

Cheers,
   Timo

--
dbus mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/dbus
Reply | Threaded
Open this post in threaded view
|

Re: dbus_connection_send_with_reply_and_block and errors

Havoc Pennington
On Tue, 2005-08-09 at 19:02 +0300, Timo Teräs wrote:
>
> dbus_set_error() never duplicates the error.name field; it just does a
> pointer assignment.
>

I think this is the bug (I'm not sure why it is like this to be honest,
seems like a pretty bad idea, though it looks like it was deliberate)

Havoc


--
dbus mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/dbus
Reply | Threaded
Open this post in threaded view
|

Re: dbus_connection_send_with_reply_and_block and errors

Timo Teräs
ext Havoc Pennington wrote:
> On Tue, 2005-08-09 at 19:02 +0300, Timo Teräs wrote:
>>dbus_set_error() never duplicates the error.name field; it just does a
>>pointer assignment.
>
> I think this is the bug (I'm not sure why it is like this to be honest,
> seems like a pretty bad idea, though it looks like it was deliberate)

I guess in most cases the error.name is a constant and can be safely
assigned. This makes sense since it saves memory and CPU cycles. But in case
of dbus_set_error_from_message() it is just broken.

So, shall I write a patch to do duplication of .name in dbus_set_error() and
do a free() in dbus_error_free()?

Or would it make any sense to add a reference count for the message in
dbus_set_error_from_message() and release it in dbus_error_free()? This
would be a bit more CPU friendly solution. This would need changing of
DBusErrorReal: we could use the padding1 field for the DBusMessage pointer.

Cheers,
   Timo
--
dbus mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/dbus
Reply | Threaded
Open this post in threaded view
|

Re: dbus_connection_send_with_reply_and_block and errors

Colin Walters
On Tue, 2005-08-09 at 19:55 +0300, Timo Teräs wrote:

> ext Havoc Pennington wrote:
> > On Tue, 2005-08-09 at 19:02 +0300, Timo Teräs wrote:
> >>dbus_set_error() never duplicates the error.name field; it just does a
> >>pointer assignment.
> >
> > I think this is the bug (I'm not sure why it is like this to be honest,
> > seems like a pretty bad idea, though it looks like it was deliberate)
>
> I guess in most cases the error.name is a constant and can be safely
> assigned. This makes sense since it saves memory and CPU cycles. But in case
> of dbus_set_error_from_message() it is just broken.
>
> So, shall I write a patch to do duplication of .name in dbus_set_error() and
> do a free() in dbus_error_free()?
Hm...doesn't this make OOM handling significantly more complicated if
setting an error can require memory allocation?  

Perhaps instead of the reference counting (which sounds complicated) we
could simply to add a "dbus_bool_t free_name" member;
dbus_set_error_from_message sets this (and dups the name), and
dbus_error_free frees the name if it's set.  Wouldn't that work?



--
dbus mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/dbus

signature.asc (196 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: dbus_connection_send_with_reply_and_block and errors

Timo Teräs
ext Colin Walters wrote:

> On Tue, 2005-08-09 at 19:55 +0300, Timo Teräs wrote:
>
>>ext Havoc Pennington wrote:
>>
>>>On Tue, 2005-08-09 at 19:02 +0300, Timo Teräs wrote:
>>>
>>>>dbus_set_error() never duplicates the error.name field; it just does a
>>>>pointer assignment.
>>>
>>>I think this is the bug (I'm not sure why it is like this to be honest,
>>>seems like a pretty bad idea, though it looks like it was deliberate)
>>
>>I guess in most cases the error.name is a constant and can be safely
>>assigned. This makes sense since it saves memory and CPU cycles. But in case
>>of dbus_set_error_from_message() it is just broken.
>>
>>So, shall I write a patch to do duplication of .name in dbus_set_error() and
>>do a free() in dbus_error_free()?
>
> Hm...doesn't this make OOM handling significantly more complicated if
> setting an error can require memory allocation?  

Not sure about this. But dbus_set_error does already allocate memory for
the message field. I guess in OOM situation dbus_set_error_const()
should be used without exception.

> Perhaps instead of the reference counting (which sounds complicated) we
> could simply to add a "dbus_bool_t free_name" member;
> dbus_set_error_from_message sets this (and dups the name), and
> dbus_error_free frees the name if it's set.  Wouldn't that work?

Yes. Now that I thinked more about this, it might be good to do the
change in dbus_set_error() since most people will propably assume that
it dups both fields. Atleast I thought so until I read the docs in more
detail. So I agree with Havoc that doing assignment of .name in
dbus_set_error() is a bad idea.

I'll do a patch for this and post it asap for review.

Cheers,
  Timo
--
dbus mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/dbus
Reply | Threaded
Open this post in threaded view
|

Re: dbus_connection_send_with_reply_and_block and errors

Colin Walters
On Tue, 2005-08-09 at 21:02 +0300, Timo Teräs wrote:

> Not sure about this. But dbus_set_error does already allocate memory for
> the message field.

You're right, looking at the code dbus_set_error just transforms the
error to OOM in that case, which surprised me at first but I guess it
makes sense.  Sorry about commenting without looking at that code.

> Yes. Now that I thinked more about this, it might be good to do the
> change in dbus_set_error() since most people will propably assume that
> it dups both fields. Atleast I thought so until I read the docs in more
> detail. So I agree with Havoc that doing assignment of .name in
> dbus_set_error() is a bad idea.
>
> I'll do a patch for this and post it asap for review.

Ok.


--
dbus mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/dbus

signature.asc (196 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: dbus_connection_send_with_reply_and_block and errors

Timo Teräs
ext Colin Walters wrote:

> On Tue, 2005-08-09 at 21:02 +0300, Timo Teräs wrote:
>>Yes. Now that I thinked more about this, it might be good to do the
>>change in dbus_set_error() since most people will propably assume that
>>it dups both fields. Atleast I thought so until I read the docs in more
>>detail. So I agree with Havoc that doing assignment of .name in
>>dbus_set_error() is a bad idea.
>>
>>I'll do a patch for this and post it asap for review.
>
> Ok.
Here you go. Passes 'make check'.

Cheers,
  Timo

Index: dbus/dbus-errors.c
===================================================================
RCS file: /cvs/dbus/dbus/dbus/dbus-errors.c,v
retrieving revision 1.27
diff -u -r1.27 dbus-errors.c
--- dbus/dbus-errors.c 10 Aug 2004 03:06:59 -0000 1.27
+++ dbus/dbus-errors.c 9 Aug 2005 18:56:18 -0000
@@ -180,7 +180,10 @@
   real = (DBusRealError *)error;
 
   if (!real->const_message)
-    dbus_free (real->message);
+    {
+      dbus_free (real->name);
+      dbus_free (real->message);
+    }
 
   dbus_error_init (error);
 }
@@ -306,7 +309,7 @@
  * @todo should be called dbus_error_set()
  *
  * @param error the error.
- * @param name the error name (not copied!!!)
+ * @param name the error name
  * @param format printf-style format string.
  */
 void
@@ -359,12 +362,17 @@
       _dbus_string_free (&str);
       goto nomem;
     }
+  _dbus_string_free (&str);
   
-  real->name = name;
+  real->name = _dbus_strdup(name);
+  if (real->name == NULL)
+    {
+      dbus_free(real->message);
+      real->message = NULL;
+      goto nomem;
+    }
   real->const_message = FALSE;
 
-  _dbus_string_free (&str);
-
   return;
   
  nomem:

--
dbus mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/dbus
Reply | Threaded
Open this post in threaded view
|

Re: dbus_connection_send_with_reply_and_block and errors

Havoc Pennington
Hi,

Thanks, this looks good, missing a couple spaces-before-parens:

On Tue, 2005-08-09 at 21:58 +0300, Timo Teräs wrote:
> +  real->name = _dbus_strdup(name);

space

> +      dbus_free(real->message);

space
 
> -  _dbus_string_free (&str);

Sure you should remove this?

Havoc


--
dbus mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/dbus
Reply | Threaded
Open this post in threaded view
|

Re: dbus_connection_send_with_reply_and_block and errors

Timo Teräs
ext Havoc Pennington wrote:
> Hi,
>
> Thanks, this looks good, missing a couple spaces-before-parens:

Oh yeah, forgot that. I usually code kernel style. Fixed that.

>>-  _dbus_string_free (&str);
>
> Sure you should remove this?

I just moved it a bit upwards to handle propely the OOM case of
_dbus_strdup().


- Timo



Index: dbus/dbus-errors.c
===================================================================
RCS file: /cvs/dbus/dbus/dbus/dbus-errors.c,v
retrieving revision 1.27
diff -u -r1.27 dbus-errors.c
--- dbus/dbus-errors.c 10 Aug 2004 03:06:59 -0000 1.27
+++ dbus/dbus-errors.c 9 Aug 2005 18:56:18 -0000
@@ -180,7 +180,10 @@
   real = (DBusRealError *)error;
 
   if (!real->const_message)
-    dbus_free (real->message);
+    {
+      dbus_free (real->name);
+      dbus_free (real->message);
+    }
 
   dbus_error_init (error);
 }
@@ -306,7 +309,7 @@
  * @todo should be called dbus_error_set()
  *
  * @param error the error.
- * @param name the error name (not copied!!!)
+ * @param name the error name
  * @param format printf-style format string.
  */
 void
@@ -359,12 +362,17 @@
       _dbus_string_free (&str);
       goto nomem;
     }
+  _dbus_string_free (&str);
   
-  real->name = name;
+  real->name = _dbus_strdup (name);
+  if (real->name == NULL)
+    {
+      dbus_free (real->message);
+      real->message = NULL;
+      goto nomem;
+    }
   real->const_message = FALSE;
 
-  _dbus_string_free (&str);
-
   return;
   
  nomem:

--
dbus mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/dbus