[PATCH] Fix crash when GetProperties req is received before any adapters are set up

[PATCH] Fix crash when GetProperties req is received before any adapters are set up

[ext-tommi.keisala]

In-Reply-To: 


We have observed bluetoothd crashing on startup and I traced the issue being dict_append_array call with empty array but elements_n being 1.

[PATCH] Fix crash when GetProperties req is received before any adapters are set up

[ext-tommi.keisala]

From: Tommi Keisala <ext-tommi.keisala@nokia.com>

This patch avoids a crash when org.bluez.Manager GetProperties request is
received and there is not yet any adapters ready. Happens often for example
when bluetoothd and ofonod is started next ot each other.

Signed-off-by: Tommi Keisala <ext-tommi.keisala@nokia.com>
---
 src/manager.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/src/manager.c b/src/manager.c
index aff069c..8967691 100644
--- a/src/manager.c
+++ b/src/manager.c
@@ -184,6 +184,7 @@ static DBusMessage *get_properties(DBusConnection *conn,
 	GSList *list;
 	char **array;
 	int i;
+	int n_elements;
 
 	reply = dbus_message_new_method_return(msg);
 	if (!reply)
@@ -197,6 +198,7 @@ static DBusMessage *get_properties(DBusConnection *conn,
 			DBUS_DICT_ENTRY_END_CHAR_AS_STRING, &dict);
 
 	array = g_new0(char *, g_slist_length(adapters) + 1);
+	n_elements = 0;
 	for (i = 0, list = adapters; list; list = list->next, i++) {
 		struct btd_adapter *adapter = list->data;
 
@@ -204,8 +206,10 @@ static DBusMessage *get_properties(DBusConnection *conn,
 			continue;
 
 		array[i] = (char *) adapter_get_path(adapter);
+		n_elements++;
 	}
-	dict_append_array(&dict, "Adapters", DBUS_TYPE_OBJECT_PATH, &array, i);
+	dict_append_array(&dict, "Adapters", DBUS_TYPE_OBJECT_PATH, &array,
+			n_elements);
 	g_free(array);
 
 	dbus_message_iter_close_container(&iter, &dict);
-- 
1.7.0.4

Re: [PATCH] Fix crash when GetProperties req is received before any adapters are set up

[Johan Hedberg]

Hi Tommi,

On Fri, Oct 22, 2010, ext-tommi.keisala@nokia.com wrote:
> This patch avoids a crash when org.bluez.Manager GetProperties request is
> received and there is not yet any adapters ready. Happens often for example
> when bluetoothd and ofonod is started next ot each other.
> 
> Signed-off-by: Tommi Keisala <ext-tommi.keisala@nokia.com>
> ---
>  src/manager.c |    6 +++++-
>  1 files changed, 5 insertions(+), 1 deletions(-)

Looks like a bugfix is definitely in order here, but why introduce a new
variable to the function? Wouldn't something like the folling be good
enough:

--- a/src/manager.c
+++ b/src/manager.c
@@ -197,13 +197,14 @@ static DBusMessage *get_properties(DBusConnection *conn,
                        DBUS_DICT_ENTRY_END_CHAR_AS_STRING, &dict);
 
        array = g_new0(char *, g_slist_length(adapters) + 1);
-       for (i = 0, list = adapters; list; list = list->next, i++) {
+       for (i = 0, list = adapters; list; list = list->next) {
                struct btd_adapter *adapter = list->data;
 
                if (!adapter_is_ready(adapter))
                        continue;
 
                array[i] = (char *) adapter_get_path(adapter);
+               i++;
        }
        dict_append_array(&dict, "Adapters", DBUS_TYPE_OBJECT_PATH, &array, i);
        g_free(array);

Could you send a new patch which doesn't introduce a new variable? Also,
please leave out the signed-off-by from the commit message since it's
not used for userspace bluez patches. Thanks.

Johan

Re: [PATCH] Fix crash when GetProperties req is received before any adapters are set up

[Luiz Augusto von Dentz]

Hi,

On Fri, Oct 22, 2010 at 4:12 PM, Johan Hedberg <johan.hedberg@nokia.com> wrote:
> Hi Tommi,
>
> On Fri, Oct 22, 2010, ext-tommi.keisala@nokia.com wrote:
>> This patch avoids a crash when org.bluez.Manager GetProperties request is
>> received and there is not yet any adapters ready. Happens often for example
>> when bluetoothd and ofonod is started next ot each other.
>>
>> Signed-off-by: Tommi Keisala <ext-tommi.keisala@nokia.com>
>> ---
>>  src/manager.c |    6 +++++-
>>  1 files changed, 5 insertions(+), 1 deletions(-)
>
> Looks like a bugfix is definitely in order here, but why introduce a new
> variable to the function? Wouldn't something like the folling be good
> enough:
>
> --- a/src/manager.c
> +++ b/src/manager.c
> @@ -197,13 +197,14 @@ static DBusMessage *get_properties(DBusConnection *conn,
>                        DBUS_DICT_ENTRY_END_CHAR_AS_STRING, &dict);
>
>        array = g_new0(char *, g_slist_length(adapters) + 1);
> -       for (i = 0, list = adapters; list; list = list->next, i++) {
> +       for (i = 0, list = adapters; list; list = list->next) {
>                struct btd_adapter *adapter = list->data;
>
>                if (!adapter_is_ready(adapter))
>                        continue;
>
>                array[i] = (char *) adapter_get_path(adapter);
> +               i++;
>        }
>        dict_append_array(&dict, "Adapters", DBUS_TYPE_OBJECT_PATH, &array, i);
>        g_free(array);
>
> Could you send a new patch which doesn't introduce a new variable? Also,
> please leave out the signed-off-by from the commit message since it's
> not used for userspace bluez patches. Thanks.

Maybe we should also add a check here to omit adapters if we don't
have at least one to append, it should be fine to have empty
containers but I don't think this is very useful for the application
since they have to iterate in the container to find out there is in
fact nothing there.


-- 
Luiz Augusto von Dentz
Computer Engineer

Re: [PATCH] Fix crash when GetProperties req is received before any adapters are set up

[Johan Hedberg]

Hi Luiz,

On Fri, Oct 22, 2010, Luiz Augusto von Dentz wrote:
> Maybe we should also add a check here to omit adapters if we don't
> have at least one to append, it should be fine to have empty
> containers but I don't think this is very useful for the application
> since they have to iterate in the container to find out there is in
> fact nothing there.

Wouldn't it complicate e.g. python code in that you'd need

if manager_props.has_key('Adapters'):
	for adapter in manager_props['Adapters']:
		...

to avoid triggering a KeyError exception which could happen if you try
to access manager_props['Adapters'] directly.

Johan