g_queue_is_empty() internally checks for NULL and return TRUE
in that case.
---
attrib/gattrib.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/attrib/gattrib.c b/attrib/gattrib.c
index 01c19f9..d648b82 100644
--- a/attrib/gattrib.c
+++ b/attrib/gattrib.c
@@ -447,10 +447,8 @@ static gboolean received_data(GIOChannel *io, GIOCondition cond, gpointer data)
status = 0;
done:
- norequests = attrib->requests == NULL ||
- g_queue_is_empty(attrib->requests);
- noresponses = attrib->responses == NULL ||
- g_queue_is_empty(attrib->responses);
+ norequests = g_queue_is_empty(attrib->requests);
+ noresponses = g_queue_is_empty(attrib->responses);
if (cmd) {
if (cmd->func)
--
1.7.9.5
Hi Anderson,
--------------------------------------------------
From: "Anderson Lizardo" <[email protected]>
Sent: Friday, March 08, 2013 5:55 PM
To: "Jaganath Kanakkassery" <[email protected]>
Cc: <[email protected]>; "Vinicius Gomes"
<[email protected]>
Subject: Re: [PATCH 2/3] attrib: Fix use after free of attrib
> Hi Jaganath,
>
> On Fri, Mar 8, 2013 at 8:03 AM, Jaganath Kanakkassery
> <[email protected]> wrote:
>>> I got this issue in BlueZ 4.101 when I called "DiscoverCharacteristics".
>>> "GetProperty" and "SetProperty" of characteristics. Please see the logs
>> [...]
>
> Sorry for the delay. I talked with Vinicius Gomes (who recently worked
> on the GAttrib code), and he agrees this fix is valid.
>
> Were you able to test this change on BlueZ 5, e.g. using gatttool or
> some supported LE profile?
>
I could not test this on BlueZ 5 since we are still using BlueZ 4.101 which
I tested
and is working fine.
Thanks,
Jaganath
Hi Jaganath,
On Fri, Mar 8, 2013 at 8:03 AM, Jaganath Kanakkassery
<[email protected]> wrote:
>> I got this issue in BlueZ 4.101 when I called "DiscoverCharacteristics".
>> "GetProperty" and "SetProperty" of characteristics. Please see the logs
> [...]
Sorry for the delay. I talked with Vinicius Gomes (who recently worked
on the GAttrib code), and he agrees this fix is valid.
Were you able to test this change on BlueZ 5, e.g. using gatttool or
some supported LE profile?
Best Regards,
--
Anderson Lizardo
Instituto Nokia de Tecnologia - INdT
Manaus - Brazil
Hi,
--------------------------------------------------
From: "Jaganath Kanakkassery" <[email protected]>
Sent: Monday, March 04, 2013 7:50 PM
To: "Anderson Lizardo" <[email protected]>
Cc: <[email protected]>
Subject: Re: [PATCH 2/3] attrib: Fix use after free of attrib
> Hi Anderson,
>
> --------------------------------------------------
> From: "Anderson Lizardo" <[email protected]>
> Sent: Monday, March 04, 2013 7:14 PM
> To: "Jaganath Kanakkassery" <[email protected]>
> Cc: <[email protected]>
> Subject: Re: [PATCH 2/3] attrib: Fix use after free of attrib
>
>> Hi Jaganath,
>>
>> On Mon, Mar 4, 2013 at 9:14 AM, Jaganath Kanakkassery
>> <[email protected]> wrote:
>>> If attrib is freed in cmd->func(), then it will be used if either
>>> request or response queue has some data to send.
>>
>> As far as I know, attrib was not supposed to be freed on the
>> cmd->func() callback. Do you have an example/testcase where this is
>> happening? To me, looks like a refcount issue (i.e. g_attrib_unref()
>> is deleting the attrib because a reference was not properly kept where
>> necessary).
>>
>
> I got this issue in BlueZ 4.101 when I called "DiscoverCharacteristics".
> "GetProperty" and "SetProperty" of characteristics. Please see the logs
>
> daemon.debug bluetoothd[4567]: src/adapter.c:adapter_get_device()
> E8:E8:66:E0:C0:01
> daemon.debug bluetoothd[4567]: attrib/gattrib.c:g_attrib_ref() 0xb6f47808:
> ref=1
> daemon.debug bluetoothd[4567]: attrib/gattrib.c:g_attrib_ref() 0xb6f47808:
> ref=2
> daemon.debug bluetoothd[4567]: src/device.c:btd_device_ref() 0xb6f47268:
> ref=9
> daemon.debug bluetoothd[4567]: attrib/gattrib.c:g_attrib_ref() 0xb6f47808:
> ref=3
> daemon.debug bluetoothd[4567]: attrib/gattrib.c:g_attrib_ref() 0xb6f47808:
> ref=4
> daemon.debug bluetoothd[4567]: attrib/gattrib.c:g_attrib_ref() 0xb6f47808:
> ref=5
> daemon.debug bluetoothd[4567]: attrib/gattrib.c:g_attrib_unref()
> 0xb6f47808: ref=4
> daemon.debug bluetoothd[4567]: attrib/client.c:register_characteristic()
> Registered:
> /org/bluez/4567/hci0/dev_E8_E8_66_E0_C0_01/service0012/characteristic0014
> daemon.debug bluetoothd[4567]: attrib/gattrib.c:g_attrib_ref() 0xb6f47808:
> ref=5
> daemon.debug bluetoothd[4567]: attrib/gattrib.c:g_attrib_unref()
> 0xb6f47808: ref=4
> daemon.debug bluetoothd[4567]: attrib/gattrib.c:g_attrib_unref()
> 0xb6f47808: ref=3
> daemon.debug bluetoothd[4567]: gdbus/object.c:generic_message() /:
> org.bluez.Manager.DefaultAdapter()
> daemon.debug bluetoothd[4567]: gdbus/object.c:generic_message()
> /org/bluez/4567/hci0/dev_E8_E8_66_E0_C0_01/service0012/characteristic0014:
> org.bluez.Characteristic.GetProperties()
> daemon.debug bluetoothd[4567]: gdbus/object.c:generic_message() /:
> org.bluez.Manager.DefaultAdapter()
> daemon.debug bluetoothd[4567]: gdbus/object.c:generic_message()
> /org/bluez/4567/hci0/dev_E8_E8_66_E0_C0_01/service0012/characteristic0014:
> org.bluez.Characteristic.SetProperty()
> daemon.debug bluetoothd[4567]: attrib/gattrib.c:g_attrib_ref() 0xb6f47808:
> ref=4
> daemon.debug bluetoothd[4567]: attrib/gattrib.c:g_attrib_unref()
> 0xb6f47808: ref=3
> daemon.debug bluetoothd[4567]: src/device.c:btd_device_unref() 0xb6f47268:
> ref=8
> daemon.debug bluetoothd[4567]: attrib/gattrib.c:g_attrib_unref()
> 0xb6f47808: ref=2
> daemon.debug bluetoothd[4567]: attrib/gattrib.c:g_attrib_unref()
> 0xb6f47808: ref=1
> daemon.debug bluetoothd[4567]: attrib/gattrib.c:g_attrib_unref()
> 0xb6f47808: ref=0
> daemon.debug bluetoothd[4567]: attrib/gattrib.c:g_attrib_ref() 0xb6f47808:
> ref=1
> daemon.err bluetoothd[4567]: crashed [1357010043] processname=bluetoothd,
> pid=4567, tid=4567, signal=11
>
> I did not give this log in commit since it is based on 4.101
>
> What I understood is cmd->func() is doing g_attrib_unref() which frees the
> attrib
> during last unref. Then since data is there in queue it tries to ref in
> wake_up_sender().
>
Ping.
Thanks,
Jaganath
Hi Anderson,
--------------------------------------------------
From: "Anderson Lizardo" <[email protected]>
Sent: Monday, March 04, 2013 7:14 PM
To: "Jaganath Kanakkassery" <[email protected]>
Cc: <[email protected]>
Subject: Re: [PATCH 2/3] attrib: Fix use after free of attrib
> Hi Jaganath,
>
> On Mon, Mar 4, 2013 at 9:14 AM, Jaganath Kanakkassery
> <[email protected]> wrote:
>> If attrib is freed in cmd->func(), then it will be used if either
>> request or response queue has some data to send.
>
> As far as I know, attrib was not supposed to be freed on the
> cmd->func() callback. Do you have an example/testcase where this is
> happening? To me, looks like a refcount issue (i.e. g_attrib_unref()
> is deleting the attrib because a reference was not properly kept where
> necessary).
>
I got this issue in BlueZ 4.101 when I called "DiscoverCharacteristics".
"GetProperty" and "SetProperty" of characteristics. Please see the logs
daemon.debug bluetoothd[4567]: src/adapter.c:adapter_get_device()
E8:E8:66:E0:C0:01
daemon.debug bluetoothd[4567]: attrib/gattrib.c:g_attrib_ref() 0xb6f47808:
ref=1
daemon.debug bluetoothd[4567]: attrib/gattrib.c:g_attrib_ref() 0xb6f47808:
ref=2
daemon.debug bluetoothd[4567]: src/device.c:btd_device_ref() 0xb6f47268:
ref=9
daemon.debug bluetoothd[4567]: attrib/gattrib.c:g_attrib_ref() 0xb6f47808:
ref=3
daemon.debug bluetoothd[4567]: attrib/gattrib.c:g_attrib_ref() 0xb6f47808:
ref=4
daemon.debug bluetoothd[4567]: attrib/gattrib.c:g_attrib_ref() 0xb6f47808:
ref=5
daemon.debug bluetoothd[4567]: attrib/gattrib.c:g_attrib_unref() 0xb6f47808:
ref=4
daemon.debug bluetoothd[4567]: attrib/client.c:register_characteristic()
Registered:
/org/bluez/4567/hci0/dev_E8_E8_66_E0_C0_01/service0012/characteristic0014
daemon.debug bluetoothd[4567]: attrib/gattrib.c:g_attrib_ref() 0xb6f47808:
ref=5
daemon.debug bluetoothd[4567]: attrib/gattrib.c:g_attrib_unref() 0xb6f47808:
ref=4
daemon.debug bluetoothd[4567]: attrib/gattrib.c:g_attrib_unref() 0xb6f47808:
ref=3
daemon.debug bluetoothd[4567]: gdbus/object.c:generic_message() /:
org.bluez.Manager.DefaultAdapter()
daemon.debug bluetoothd[4567]: gdbus/object.c:generic_message()
/org/bluez/4567/hci0/dev_E8_E8_66_E0_C0_01/service0012/characteristic0014:
org.bluez.Characteristic.GetProperties()
daemon.debug bluetoothd[4567]: gdbus/object.c:generic_message() /:
org.bluez.Manager.DefaultAdapter()
daemon.debug bluetoothd[4567]: gdbus/object.c:generic_message()
/org/bluez/4567/hci0/dev_E8_E8_66_E0_C0_01/service0012/characteristic0014:
org.bluez.Characteristic.SetProperty()
daemon.debug bluetoothd[4567]: attrib/gattrib.c:g_attrib_ref() 0xb6f47808:
ref=4
daemon.debug bluetoothd[4567]: attrib/gattrib.c:g_attrib_unref() 0xb6f47808:
ref=3
daemon.debug bluetoothd[4567]: src/device.c:btd_device_unref() 0xb6f47268:
ref=8
daemon.debug bluetoothd[4567]: attrib/gattrib.c:g_attrib_unref() 0xb6f47808:
ref=2
daemon.debug bluetoothd[4567]: attrib/gattrib.c:g_attrib_unref() 0xb6f47808:
ref=1
daemon.debug bluetoothd[4567]: attrib/gattrib.c:g_attrib_unref() 0xb6f47808:
ref=0
daemon.debug bluetoothd[4567]: attrib/gattrib.c:g_attrib_ref() 0xb6f47808:
ref=1
daemon.err bluetoothd[4567]: crashed [1357010043] processname=bluetoothd,
pid=4567, tid=4567, signal=11
I did not give this log in commit since it is based on 4.101
What I understood is cmd->func() is doing g_attrib_unref() which frees the
attrib
during last unref. Then since data is there in queue it tries to ref in
wake_up_sender().
Thanks,
Jaganath
Hi Jaganath,
On Mon, Mar 4, 2013 at 9:14 AM, Jaganath Kanakkassery
<[email protected]> wrote:
> If attrib is freed in cmd->func(), then it will be used if either
> request or response queue has some data to send.
As far as I know, attrib was not supposed to be freed on the
cmd->func() callback. Do you have an example/testcase where this is
happening? To me, looks like a refcount issue (i.e. g_attrib_unref()
is deleting the attrib because a reference was not properly kept where
necessary).
Regards,
--
Anderson Lizardo
Instituto Nokia de Tecnologia - INdT
Manaus - Brazil
Using g_queue_is_empty() directly in if condition looks more
readable.
---
attrib/gattrib.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/attrib/gattrib.c b/attrib/gattrib.c
index 0090027..37581a3 100644
--- a/attrib/gattrib.c
+++ b/attrib/gattrib.c
@@ -394,7 +394,6 @@ static gboolean received_data(GIOChannel *io, GIOCondition cond, gpointer data)
uint8_t buf[512], status;
gsize len;
GIOStatus iostat;
- gboolean norequests, noresponses;
if (attrib->stale)
return FALSE;
@@ -447,10 +446,8 @@ static gboolean received_data(GIOChannel *io, GIOCondition cond, gpointer data)
status = 0;
done:
- norequests = g_queue_is_empty(attrib->requests);
- noresponses = g_queue_is_empty(attrib->responses);
-
- if (!norequests || !noresponses)
+ if (!g_queue_is_empty(attrib->requests) ||
+ !g_queue_is_empty(attrib->responses))
wake_up_sender(attrib);
if (cmd) {
--
1.7.9.5
If attrib is freed in cmd->func(), then it will be used if either
request or response queue has some data to send.
This patch moves calling wake_up_sender() which increases the ref
count of attrib so that it wont get freed in cmd->func().
---
attrib/gattrib.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/attrib/gattrib.c b/attrib/gattrib.c
index d648b82..0090027 100644
--- a/attrib/gattrib.c
+++ b/attrib/gattrib.c
@@ -450,6 +450,9 @@ done:
norequests = g_queue_is_empty(attrib->requests);
noresponses = g_queue_is_empty(attrib->responses);
+ if (!norequests || !noresponses)
+ wake_up_sender(attrib);
+
if (cmd) {
if (cmd->func)
cmd->func(status, buf, len, cmd->user_data);
@@ -457,9 +460,6 @@ done:
command_destroy(cmd);
}
- if (!norequests || !noresponses)
- wake_up_sender(attrib);
-
return TRUE;
}
--
1.7.9.5