[PATCH] dbus timeout handling

[PATCH] dbus timeout handling

[Daniel Örstadius]

Hi,

When receiving a file it's possible to crash obexd by letting the
"org.openobex.Error.Rejected" reply from the call to
request_authorization and the obexd timeout for the response occur at
roughly the same time.

No crash seen if the obexd timeout reaches function "agent_reply"
first. But if the agent's response arrives just before the obexd
timeout, the timeout will still be dispatched causing what looks like
an issue in the dbus-obexd integration. Added my own debug output to
show the situation:

obexd[8454]: entering remove_timeout
obexd[8454]: entering timeout_handler_dispatch
obexd[8454]: entering remove_timeout
obexd[8454]: entering timeout_handler_free
obexd[8454]: entering agent_reply
obexd[8454]: Agent replied with an error: org.bluez.Error.Rejected,
request rejected
obexd[8454]: after g_main_context_iteration in request_authorization
[segmentation fault, valgrind complains on two locations in
/lib/libdbus-1.so.3.4.0]

To fix this, there seems to be a chance to  avoid dispatching the
timeout in callback function mainloop.c:remove_timeout (this function
is currently empty, but maybe for good reason?)

After having moved the g_source_remove call on the timer to
"remove_timeout" from "timeout_handler_free" the issue was not
reproduced, although I can't claim to understand the dbus mainloop
integration code completely. It might be a bit awkward to break up the
g_source_remove and g_free of the timer into different callbacks.

Is this an acceptable solution for removing the crash?

Best regards,
Daniel

-----

For reference, the authorization method used to get the crash:

QString Widget::Authorize(QDBusObjectPath transger,QString bdaddr,QString name,
			QString type, int length, int time, QDBusMessage msg)
{	
	QTest::qWait(60000);

	QDBusConnection conn(QDBusConnection::sessionBus());
	QDBusMessage reply(msg.createErrorReply("org.bluez.Error.Rejected","request
rejected"));
	conn.send(reply);
	
	return QString();	
}


------

>From 1ec3f03d3c0d3fa6011db95e329b03e163bd1524 Mon Sep 17 00:00:00 2001
From: Daniel Orstadius <daniel.orstadius@gmail.com>
Date: Fri, 11 Dec 2009 09:50:23 +0200
Subject: [PATCH] dbus timeout handling

---
 gdbus/mainloop.c |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/gdbus/mainloop.c b/gdbus/mainloop.c
index bd775f8..b583ace 100644
--- a/gdbus/mainloop.c
+++ b/gdbus/mainloop.c
@@ -183,7 +183,6 @@ static void timeout_handler_free(void *data)
 	if (!handler)
 		return;

-	g_source_remove(handler->id);
 	g_free(handler);
 }

@@ -207,6 +206,14 @@ static dbus_bool_t add_timeout(DBusTimeout
*timeout, void *data)

 static void remove_timeout(DBusTimeout *timeout, void *data)
 {
+        timeout_handler_t *handler;
+
+        handler = dbus_timeout_get_data(timeout);
+
+        if (!handler)
+                return;
+
+        g_source_remove(handler->id);
 }

 static void timeout_toggled(DBusTimeout *timeout, void *data)
-- 
1.6.0.4

Re: [PATCH] dbus timeout handling

[Daniel Örstadius]

On Fri, Dec 11, 2009 at 10:25 AM, Daniel Örstadius
<daniel.orstadius@gmail.com> wrote:

> When receiving a file it's possible to crash obexd by letting the
> "org.openobex.Error.Rejected" reply from the call to
> request_authorization and the obexd timeout for the response occur at
> roughly the same time.
>

Making a new patch proposal for this after discussion with Johan.

The patch concerns how dbus timeouts are stopped in obexd in the
functions remove_timeout and timeout_handler_free. Without this patch
it is all done in the latter. Using the patch it is done in both, with
the freeing being made in timeout_handler_free.

The reason for making this modification is that in at least one corner
case it looks to be possible for obexd to dispatch (by
timeout_handler_dispatch) a timeout on which dbus has called
remove_timeout (which is currently empty).

Compared the obexd-dbus mainloop integration with the dbus-glib
implementation at
http://cgit.freedesktop.org/dbus/dbus-glib/tree/dbus/dbus-gmain.c

Its corresponding callbacks for obexd's remove_timeout and
timeout_handler_free both do the same thing as far as I understood the
code.

>From the dbus-glib code:

dbus_connection_set_timeout_functions (.., remove_timeout,..)
leading to

remove_timeout ->
connection_setup_remove_timeout ->
timeout_handler_destroy_source ->
g_source_destroy

and
add_timeout -> dbus_timeout_set_data (.., timeout_handler_timeout_freed)
leading to

timeout_handler_timeout_freed ->
timeout_handler_destroy_source ->
g_source_destroy

They both stop the timeout, but do not free the timeout_handler
structure. This is done in timeout_handler_source_finalized which is
set via

g_source_set_callback (.., timeout_handler_source_finalized) in add_timeout.

obexd does not use this kind of finalize callback. Other than this,
the intention is for the patch to map to the dbus-glib implementation
by removing the timer in both callbacks.

(I'm not an expert in dbus nor glib, there could be pitfalls with this
modification)

/Daniel

---------

>From 70cc29c724c089e1468a6670c53e1f77d5b3ebcc Mon Sep 17 00:00:00 2001
From: Daniel Orstadius <daniel.orstadius@gmail.com>
Date: Wed, 16 Dec 2009 11:23:46 +0200
Subject: [PATCH] dbus timeout handling

---
 gdbus/mainloop.c |   17 ++++++++++++++++-
 1 files changed, 16 insertions(+), 1 deletions(-)

diff --git a/gdbus/mainloop.c b/gdbus/mainloop.c
index bd775f8..f808f5d 100644
--- a/gdbus/mainloop.c
+++ b/gdbus/mainloop.c
@@ -183,7 +183,11 @@ static void timeout_handler_free(void *data)
 	if (!handler)
 		return;

-	g_source_remove(handler->id);
+	if (handler->id != 0) {
+		g_source_remove(handler->id);
+		handler->id = 0;
+	}
+
 	g_free(handler);
 }

@@ -207,6 +211,17 @@ static dbus_bool_t add_timeout(DBusTimeout
*timeout, void *data)

 static void remove_timeout(DBusTimeout *timeout, void *data)
 {
+        timeout_handler_t *handler;
+
+        handler = dbus_timeout_get_data(timeout);
+
+        if (!handler)
+                return;
+
+        if (handler->id != 0) {
+                g_source_remove(handler->id);
+                handler->id = 0;
+        }
 }

 static void timeout_toggled(DBusTimeout *timeout, void *data)
-- 
1.6.0.4

Re: [PATCH] dbus timeout handling

[Johan Hedberg]

Hi Daniel,

On Wed, Dec 16, 2009, Daniel Örstadius wrote:
> From 70cc29c724c089e1468a6670c53e1f77d5b3ebcc Mon Sep 17 00:00:00 2001
> From: Daniel Orstadius <daniel.orstadius@gmail.com>
> Date: Wed, 16 Dec 2009 11:23:46 +0200
> Subject: [PATCH] dbus timeout handling
> 
> ---
>  gdbus/mainloop.c |   17 ++++++++++++++++-
>  1 files changed, 16 insertions(+), 1 deletions(-)

Thanks for the patch. I've pushed it upstream with one minor coding
style change: "> 0" instead of "!= 0" (Marcel requested this).

Johan