bluetoothd bug

bluetoothd bug

[Claudio Takahasi]

Hi Johan/Luiz,

I found an error "invalid read". The pastebin log shows a PBAP
connection in the RFCOMM channel 15. See line 195 of the first log,
when the connection is refused the "temporary" device is being
removed, however it seems that the "reverse" SDP query still active.
PTS is establishing RFCOMM connection before "Encrypt Change" event,
but this wrong procedure should not "harm" bluetoothd. In some cases I
noticed segmentation fault, probably due this error.

http://pastebin.com/m308f85ef
http://pastebin.com/m2d80dd71

How to reproduce:
PTS v 3.3.1.4
Simple pairing enabled
Test Case: TC_PSE_SSM_BV_08_I (phonebook)

Regards,
Claudio.

-- 
--
Claudio Takahasi
Instituto Nokia de Tecnologia
Recife - Pernambuco - Brasil
+55 81 30879999

Re: bluetoothd bug

[Luiz Augusto von Dentz]

Hi Claudio,

On Mon, Jan 25, 2010 at 8:14 PM, Claudio Takahasi
<claudio.takahasi@openbossa.org> wrote:
> Hi Johan/Luiz,
>
> I found an error "invalid read". The pastebin log shows a PBAP
> connection in the RFCOMM channel 15. See line 195 of the first log,
> when the connection is refused the "temporary" device is being
> removed, however it seems that the "reverse" SDP query still active.
> PTS is establishing RFCOMM connection before "Encrypt Change" event,
> but this wrong procedure should not "harm" bluetoothd. In some cases I
> noticed segmentation fault, probably due this error.
>
> http://pastebin.com/m308f85ef
> http://pastebin.com/m2d80dd71

Does this fix the problem:

diff --git a/src/device.c b/src/device.c
index a2730ab..dfdacd8 100644
--- a/src/device.c
+++ b/src/device.c
@@ -239,6 +239,9 @@ static void device_free(gpointer user_data)
        if (device->disconn_timer)
                g_source_remove(device->disconn_timer);

+       if (device->discov_timer)
+               g_source_remove(device->discov_timer);
+
        debug("device_free(%p)", device);

        g_free(device->authr);


-- 
Luiz Augusto von Dentz
Engenheiro de Computação

Re: bluetoothd bug

[Claudio Takahasi]

On Tue, Jan 26, 2010 at 9:09 AM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> Hi Claudio,
>
> On Mon, Jan 25, 2010 at 8:14 PM, Claudio Takahasi
> <claudio.takahasi@openbossa.org> wrote:
>> Hi Johan/Luiz,
>>
>> I found an error "invalid read". The pastebin log shows a PBAP
>> connection in the RFCOMM channel 15. See line 195 of the first log,
>> when the connection is refused the "temporary" device is being
>> removed, however it seems that the "reverse" SDP query still active.
>> PTS is establishing RFCOMM connection before "Encrypt Change" event,
>> but this wrong procedure should not "harm" bluetoothd. In some cases I
>> noticed segmentation fault, probably due this error.
>>
>> http://pastebin.com/m308f85ef
>> http://pastebin.com/m2d80dd71
>
> Does this fix the problem:
>
> diff --git a/src/device.c b/src/device.c
> index a2730ab..dfdacd8 100644
> --- a/src/device.c
> +++ b/src/device.c
> @@ -239,6 +239,9 @@ static void device_free(gpointer user_data)
>        if (device->disconn_timer)
>                g_source_remove(device->disconn_timer);
>
> +       if (device->discov_timer)
> +               g_source_remove(device->discov_timer);
> +
>        debug("device_free(%p)", device);
>
>        g_free(device->authr);
>
>
> --
> Luiz Augusto von Dentz
> Engenheiro de Computação
>

Hi Luiz,

yep, it is working perfectly: no warning/no segmentation fault.

Claudio.

Re: bluetoothd bug

[Luiz Augusto von Dentz]

[-- Attachment #1: Type: text/plain, Size: 1987 bytes --]

Hi Claudio,

On Tue, Jan 26, 2010 at 8:33 PM, Claudio Takahasi
<claudio.takahasi@openbossa.org> wrote:
> On Tue, Jan 26, 2010 at 9:09 AM, Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
>> Hi Claudio,
>>
>> On Mon, Jan 25, 2010 at 8:14 PM, Claudio Takahasi
>> <claudio.takahasi@openbossa.org> wrote:
>>> Hi Johan/Luiz,
>>>
>>> I found an error "invalid read". The pastebin log shows a PBAP
>>> connection in the RFCOMM channel 15. See line 195 of the first log,
>>> when the connection is refused the "temporary" device is being
>>> removed, however it seems that the "reverse" SDP query still active.
>>> PTS is establishing RFCOMM connection before "Encrypt Change" event,
>>> but this wrong procedure should not "harm" bluetoothd. In some cases I
>>> noticed segmentation fault, probably due this error.
>>>
>>> http://pastebin.com/m308f85ef
>>> http://pastebin.com/m2d80dd71
>>
>> Does this fix the problem:
>>
>> diff --git a/src/device.c b/src/device.c
>> index a2730ab..dfdacd8 100644
>> --- a/src/device.c
>> +++ b/src/device.c
>> @@ -239,6 +239,9 @@ static void device_free(gpointer user_data)
>>        if (device->disconn_timer)
>>                g_source_remove(device->disconn_timer);
>>
>> +       if (device->discov_timer)
>> +               g_source_remove(device->discov_timer);
>> +
>>        debug("device_free(%p)", device);
>>
>>        g_free(device->authr);
>>
>>
>> --
>> Luiz Augusto von Dentz
>> Engenheiro de Computação
>>
>
> Hi Luiz,
>
> yep, it is working perfectly: no warning/no segmentation fault.
>
> Claudio.
>

Great, not sure how PTS was able to trigger it since it seems to be
passing bonding phase without resetting temporary flag which would
mean bonding fails and would not even schedule the reversal service
discovery, but a segfault is always a bug no matter what stupid things
PTS is doing.

Proper git patch attached.

-- 
Luiz Augusto von Dentz
Engenheiro de Computação

[-- Attachment #2: 0001-Fix-possible-invalid-read-when-removing-a-temporary-.patch --]
[-- Type: text/x-patch, Size: 934 bytes --]

From be0c1373499bbc480298429109124efbbe49f15a Mon Sep 17 00:00:00 2001
From: Luiz Augusto Von Dentz <luiz.dentz-von@nokia.com>
Date: Tue, 26 Jan 2010 16:17:21 +0200
Subject: [PATCH 1/6] Fix possible invalid read when removing a temporary device

An invalid read may happens when doing the reverse service discovery but
for some reason the device disconnects with the temporary flag set which
will make the device to be removed without cancelling the service.
---
 src/device.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/src/device.c b/src/device.c
index a2730ab..dfdacd8 100644
--- a/src/device.c
+++ b/src/device.c
@@ -239,6 +239,9 @@ static void device_free(gpointer user_data)
 	if (device->disconn_timer)
 		g_source_remove(device->disconn_timer);
 
+	if (device->discov_timer)
+		g_source_remove(device->discov_timer);
+
 	debug("device_free(%p)", device);
 
 	g_free(device->authr);
-- 
1.6.3.3

Re: bluetoothd bug

[Johan Hedberg]

Hi Luiz,

On Wed, Jan 27, 2010, Luiz Augusto von Dentz wrote:
> From be0c1373499bbc480298429109124efbbe49f15a Mon Sep 17 00:00:00 2001
> From: Luiz Augusto Von Dentz <luiz.dentz-von@nokia.com>
> Date: Tue, 26 Jan 2010 16:17:21 +0200
> Subject: [PATCH 1/6] Fix possible invalid read when removing a temporary device
> 
> An invalid read may happens when doing the reverse service discovery but
> for some reason the device disconnects with the temporary flag set which
> will make the device to be removed without cancelling the service.
> ---
>  src/device.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)

The patch has been pushed upstream. Thanks for the fix!

Johan

Re: bluetoothd bug

[Marcel Holtmann]

Hi Luiz,

> Great, not sure how PTS was able to trigger it since it seems to be
> passing bonding phase without resetting temporary flag which would
> mean bonding fails and would not even schedule the reversal service
> discovery, but a segfault is always a bug no matter what stupid things
> PTS is doing.

as I explained to Claudio, the PTS guys didn't even read their own
specification. With Simple Pairing you are required to establish the
encryption for everything except PSM 1. And the kernel will reject
everything that tries to establish a L2CAP channel before encryption is
in place.

Regards

Marcel