Original patch by Chan-yeol Park <[email protected]>
Once the headset struct is freed, rfcomm_io_cb would be called and
it might lead to a crash. So when g_io_shutdown rfcomm channel,
additionally we remove g_io_add_watch() source id handle to avoid
unwanted call. And even though rfcomm_io_cb is called and in case
that struct headset is NULL, rfcomm_io_cb would return with FALSE.
---
audio/headset.c | 19 +++++++++++++++++--
1 files changed, 17 insertions(+), 2 deletions(-)
diff --git a/audio/headset.c b/audio/headset.c
index 6aef6a8..77883d2 100644
--- a/audio/headset.c
+++ b/audio/headset.c
@@ -165,6 +165,7 @@ struct headset {
gboolean auto_dc;
guint dc_timer;
+ guint rfcomm_io_id;
gboolean hfp_active;
gboolean search_hfp;
@@ -1280,6 +1281,9 @@ static gboolean rfcomm_io_cb(GIOChannel *chan, GIOCondition cond,
return FALSE;
hs = device->headset;
+ if (!hs)
+ return FALSE;
+
slc = hs->slc;
if (cond & (G_IO_ERR | G_IO_HUP)) {
@@ -1397,8 +1401,9 @@ void headset_connect_cb(GIOChannel *chan, GError *err, gpointer user_data)
else
hs->auto_dc = FALSE;
- g_io_add_watch(chan, G_IO_IN | G_IO_ERR | G_IO_HUP| G_IO_NVAL,
- (GIOFunc) rfcomm_io_cb, dev);
+ hs->rfcomm_io_id = g_io_add_watch(chan,
+ G_IO_IN | G_IO_ERR | G_IO_HUP| G_IO_NVAL,
+ (GIOFunc) rfcomm_io_cb, dev);
DBG("%s: Connected to %s", dev->path, hs_address);
@@ -1619,6 +1624,11 @@ static int rfcomm_connect(struct audio_device *dev, headset_stream_cb_t cb,
DBG("%s: Connecting to %s channel %d", dev->path, address,
hs->rfcomm_ch);
+ if (hs->rfcomm_io_id) {
+ g_source_remove(hs->rfcomm_io_id);
+ hs->rfcomm_io_id = 0;
+ }
+
hs->tmp_rfcomm = bt_io_connect(BT_IO_RFCOMM, headset_connect_cb, dev,
NULL, &err,
BT_IO_OPT_SOURCE_BDADDR, &dev->src,
@@ -2147,6 +2157,11 @@ static int headset_close_rfcomm(struct audio_device *dev)
hs->rfcomm = NULL;
}
+ if (hs->rfcomm_io_id) {
+ g_source_remove(hs->rfcomm_io_id);
+ hs->rfcomm_io_id = 0;
+ }
+
g_free(hs->slc);
hs->slc = NULL;
--
1.7.5.4
Hi Chethan,
On Wed, Dec 28, 2011 at 8:46 AM, Chethan T N <[email protected]> wrote:
> Original patch by Chan-yeol Park <[email protected]>
>
> Once the headset struct is freed, rfcomm_io_cb would be called and
> it might lead to a crash. So when g_io_shutdown rfcomm channel,
> additionally we remove g_io_add_watch() source id handle to avoid
> unwanted call. And even though rfcomm_io_cb is called and in case
> that struct headset is NULL, rfcomm_io_cb would return with FALSE.
I don't think this is true, iirc this will lead to G_IO_NVAL which is
handled in rfcomm_io_cb, so there might be a bug somewhere else if the
callback is still called after headset_close_rfcomm and the condition
is not G_IO_NVAL. Btw it is a good practice to add the backtrace of
the crash you are fixing.
Now holding the source id is probably a good idea since it reduces the
extra call to the callback while disconnecting.
> ?audio/headset.c | ? 19 +++++++++++++++++--
> ?1 files changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/audio/headset.c b/audio/headset.c
> index 6aef6a8..77883d2 100644
> --- a/audio/headset.c
> +++ b/audio/headset.c
> @@ -165,6 +165,7 @@ struct headset {
> ? ? ? ?gboolean auto_dc;
>
> ? ? ? ?guint dc_timer;
> + ? ? ? guint rfcomm_io_id;
>
> ? ? ? ?gboolean hfp_active;
> ? ? ? ?gboolean search_hfp;
> @@ -1280,6 +1281,9 @@ static gboolean rfcomm_io_cb(GIOChannel *chan, GIOCondition cond,
> ? ? ? ? ? ? ? ?return FALSE;
>
> ? ? ? ?hs = device->headset;
> + ? ? ? if (!hs)
> + ? ? ? ? ? ? ? return FALSE;
> +
That is another evidence that there is something else broken, the
callback should not be called after its gsource is removed, so this
prevent the crash but masquerade the real issue.
--
Luiz Augusto von Dentz
Hi Luiz,
--------------------------------------------------
From: "Luiz Augusto von Dentz" <[email protected]>
Sent: Wednesday, December 28, 2011 2:52 PM
To: "Chethan T N" <[email protected]>
Cc: <[email protected]>
Subject: Re: [PATCH] audio: Fix headset crash
> Hi Chethan,
>
> On Wed, Dec 28, 2011 at 8:46 AM, Chethan T N <[email protected]>
> wrote:
>> Original patch by Chan-yeol Park <[email protected]>
>>
>> Once the headset struct is freed, rfcomm_io_cb would be called and
>> it might lead to a crash. So when g_io_shutdown rfcomm channel,
>> additionally we remove g_io_add_watch() source id handle to avoid
>> unwanted call. And even though rfcomm_io_cb is called and in case
>> that struct headset is NULL, rfcomm_io_cb would return with FALSE.
>
> I don't think this is true, iirc this will lead to G_IO_NVAL which is
> handled in rfcomm_io_cb, so there might be a bug somewhere else if the
> callback is still called after headset_close_rfcomm and the condition
> is not G_IO_NVAL. Btw it is a good practice to add the backtrace of
> the crash you are fixing.
>
> Now holding the source id is probably a good idea since it reduces the
> extra call to the callback while disconnecting.
>
>> audio/headset.c | 19 +++++++++++++++++--
>> 1 files changed, 17 insertions(+), 2 deletions(-)
>>
>> diff --git a/audio/headset.c b/audio/headset.c
>> index 6aef6a8..77883d2 100644
>> --- a/audio/headset.c
>> +++ b/audio/headset.c
>> @@ -165,6 +165,7 @@ struct headset {
>> gboolean auto_dc;
>>
>> guint dc_timer;
>> + guint rfcomm_io_id;
>>
>> gboolean hfp_active;
>> gboolean search_hfp;
>> @@ -1280,6 +1281,9 @@ static gboolean rfcomm_io_cb(GIOChannel *chan,
>> GIOCondition cond,
>> return FALSE;
>>
>> hs = device->headset;
>> + if (!hs)
>> + return FALSE;
>> +
>
> That is another evidence that there is something else broken, the
> callback should not be called after its gsource is removed, so this
> prevent the crash but masquerade the real issue.
>
>
> --
> Luiz Augusto von Dentz
Thanks for your valuable inputs.
I will investigate with few more headsets and shall share the information
with logs.
Thanks and Regards
Chethan
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth"
> in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html