[PATCH] avdtp: Fix crash on connection lost

[PATCH] avdtp: Fix crash on connection lost

[Szymon Janc]

This fix avdtp session reference counting. avdtp_unref has a special
case when ref==1 and due to nested calls session could be free in
consecutive call resulting in accesing already freed memory.

Instead of having extra free lock keep extra local reference. This also
make avdtp_unref easier to undertand as there is no double ref
decrement inside one unref call.

avdtp_unref will set session state to disconnected when reference count
drops to 1 so there is no need to explicite set it in connection_lost.

bluetoothd[29474]: audio/avdtp.c:avdtp_ref() 0x555555856aa0: ref=2
bluetoothd[29474]: audio/source.c:source_connect() stream creation in progress
bluetoothd[29474]: src/mgmt.c:mgmt_event() cond 1
bluetoothd[29474]: src/mgmt.c:mgmt_event() Received 31 bytes from management socket
bluetoothd[29474]: src/mgmt.c:mgmt_device_connected() hci0 device 84:00:D2:DB:F7:8C connected eir_len 12
bluetoothd[29474]: src/adapter.c:adapter_get_device() 84:00:D2:DB:F7:8C
bluetoothd[29474]: src/mgmt.c:mgmt_event() cond 1
bluetoothd[29474]: src/mgmt.c:mgmt_event() Received 14 bytes from management socket
bluetoothd[29474]: src/mgmt.c:mgmt_auth_failed() hci0 auth failed status 6
bluetoothd[29474]: src/device.c:device_bonding_complete() bonding (nil) status 0x06
bluetoothd[29474]: connect error: Invalid exchange (52)
bluetoothd[29474]: audio/avdtp.c:connection_lost() Disconnected from 84:00:D2:DB:F7:8C
bluetoothd[29474]: audio/avdtp.c:avdtp_unref() 0x555555856aa0: ref=1
bluetoothd[29474]: audio/avdtp.c:avdtp_unref() 0x555555856aa0: ref=0
bluetoothd[29474]: audio/avdtp.c:avdtp_unref() 0x555555856aa0: freeing session and removing from list
bluetoothd[29474]: audio/avdtp.c:avdtp_unref() 0x555555856aa0: freeing session and removing from list

Program received signal SIGSEGV, Segmentation fault.
avdtp_unref (session=0x555555856aa0) at audio/avdtp.c:1239
1239            server->sessions = g_slist_remove(server->sessions, session);

---
 audio/avdtp.c |   10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/audio/avdtp.c b/audio/avdtp.c
index bd91cb6..4109be9 100644
--- a/audio/avdtp.c
+++ b/audio/avdtp.c
@@ -388,7 +388,6 @@ struct avdtp_stream {
 
 struct avdtp {
 	int ref;
-	int free_lock;
 
 	uint16_t version;
 
@@ -1158,14 +1157,14 @@ static void connection_lost(struct avdtp *session, int err)
 	if (err != EACCES)
 		avdtp_cancel_authorization(session);
 
-	session->free_lock = 1;
+	avdtp_ref(session);
 
 	finalize_discovery(session, err);
 
 	g_slist_foreach(session->streams, (GFunc) release_stream, session);
 	session->streams = NULL;
 
-	session->free_lock = 0;
+	avdtp_unref(session);
 
 	if (session->io) {
 		g_io_channel_shutdown(session->io, FALSE, NULL);
@@ -1173,8 +1172,6 @@ static void connection_lost(struct avdtp *session, int err)
 		session->io = NULL;
 	}
 
-	avdtp_set_state(session, AVDTP_SESSION_STATE_DISCONNECTED);
-
 	if (session->io_id) {
 		g_source_remove(session->io_id);
 		session->io_id = 0;
@@ -1221,9 +1218,6 @@ void avdtp_unref(struct avdtp *session)
 
 		if (session->io)
 			set_disconnect_timer(session);
-		else if (!session->free_lock) /* Drop the local ref if we
-						 aren't connected */
-			session->ref--;
 	}
 
 	if (session->ref > 0)
-- 
1.7.9.5

Re: [PATCH] avdtp: Fix crash on connection lost

[Luiz Augusto von Dentz]

Hi Szymon,

On Tue, Oct 16, 2012 at 2:37 PM, Szymon Janc <szymon.janc@tieto.com> wrote:
> This fix avdtp session reference counting. avdtp_unref has a special
> case when ref==1 and due to nested calls session could be free in
> consecutive call resulting in accesing already freed memory.
>
> Instead of having extra free lock keep extra local reference. This also
> make avdtp_unref easier to undertand as there is no double ref
> decrement inside one unref call.
>
> avdtp_unref will set session state to disconnected when reference count
> drops to 1 so there is no need to explicite set it in connection_lost.
>
> bluetoothd[29474]: audio/avdtp.c:avdtp_ref() 0x555555856aa0: ref=2
> bluetoothd[29474]: audio/source.c:source_connect() stream creation in progress
> bluetoothd[29474]: src/mgmt.c:mgmt_event() cond 1
> bluetoothd[29474]: src/mgmt.c:mgmt_event() Received 31 bytes from management socket
> bluetoothd[29474]: src/mgmt.c:mgmt_device_connected() hci0 device 84:00:D2:DB:F7:8C connected eir_len 12
> bluetoothd[29474]: src/adapter.c:adapter_get_device() 84:00:D2:DB:F7:8C
> bluetoothd[29474]: src/mgmt.c:mgmt_event() cond 1
> bluetoothd[29474]: src/mgmt.c:mgmt_event() Received 14 bytes from management socket
> bluetoothd[29474]: src/mgmt.c:mgmt_auth_failed() hci0 auth failed status 6
> bluetoothd[29474]: src/device.c:device_bonding_complete() bonding (nil) status 0x06
> bluetoothd[29474]: connect error: Invalid exchange (52)
> bluetoothd[29474]: audio/avdtp.c:connection_lost() Disconnected from 84:00:D2:DB:F7:8C
> bluetoothd[29474]: audio/avdtp.c:avdtp_unref() 0x555555856aa0: ref=1
> bluetoothd[29474]: audio/avdtp.c:avdtp_unref() 0x555555856aa0: ref=0
> bluetoothd[29474]: audio/avdtp.c:avdtp_unref() 0x555555856aa0: freeing session and removing from list
> bluetoothd[29474]: audio/avdtp.c:avdtp_unref() 0x555555856aa0: freeing session and removing from list
>
> Program received signal SIGSEGV, Segmentation fault.
> avdtp_unref (session=0x555555856aa0) at audio/avdtp.c:1239
> 1239            server->sessions = g_slist_remove(server->sessions, session);
>
> ---
>  audio/avdtp.c |   10 ++--------
>  1 file changed, 2 insertions(+), 8 deletions(-)
>
> diff --git a/audio/avdtp.c b/audio/avdtp.c
> index bd91cb6..4109be9 100644
> --- a/audio/avdtp.c
> +++ b/audio/avdtp.c
> @@ -388,7 +388,6 @@ struct avdtp_stream {
>
>  struct avdtp {
>         int ref;
> -       int free_lock;
>
>         uint16_t version;
>
> @@ -1158,14 +1157,14 @@ static void connection_lost(struct avdtp *session, int err)
>         if (err != EACCES)
>                 avdtp_cancel_authorization(session);
>
> -       session->free_lock = 1;
> +       avdtp_ref(session);
>
>         finalize_discovery(session, err);
>
>         g_slist_foreach(session->streams, (GFunc) release_stream, session);
>         session->streams = NULL;
>
> -       session->free_lock = 0;
> +       avdtp_unref(session);
>
>         if (session->io) {
>                 g_io_channel_shutdown(session->io, FALSE, NULL);
> @@ -1173,8 +1172,6 @@ static void connection_lost(struct avdtp *session, int err)
>                 session->io = NULL;
>         }
>
> -       avdtp_set_state(session, AVDTP_SESSION_STATE_DISCONNECTED);

This is actually the one that we need in case we got disconnect by
remote after discovery completes. avctp.c has a similar structure to
handler such states, but without the refcounting all is much simpler.

>         if (session->io_id) {
>                 g_source_remove(session->io_id);
>                 session->io_id = 0;
> @@ -1221,9 +1218,6 @@ void avdtp_unref(struct avdtp *session)
>
>                 if (session->io)
>                         set_disconnect_timer(session);
> -               else if (!session->free_lock) /* Drop the local ref if we
> -                                                aren't connected */
> -                       session->ref--;
>         }
>
>         if (session->ref > 0)
> --
> 1.7.9.5

We should probably get rid of the free_lock as well.

-- 
Luiz Augusto von Dentz

Re: [PATCH] avdtp: Fix crash on connection lost

[Luiz Augusto von Dentz]

Hi again,

On Tue, Oct 16, 2012 at 7:34 PM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> Hi Szymon,
>
> On Tue, Oct 16, 2012 at 2:37 PM, Szymon Janc <szymon.janc@tieto.com> wrote:
>> This fix avdtp session reference counting. avdtp_unref has a special
>> case when ref==1 and due to nested calls session could be free in
>> consecutive call resulting in accesing already freed memory.
>>
>> Instead of having extra free lock keep extra local reference. This also
>> make avdtp_unref easier to undertand as there is no double ref
>> decrement inside one unref call.
>>
>> avdtp_unref will set session state to disconnected when reference count
>> drops to 1 so there is no need to explicite set it in connection_lost.
>>
>> bluetoothd[29474]: audio/avdtp.c:avdtp_ref() 0x555555856aa0: ref=2
>> bluetoothd[29474]: audio/source.c:source_connect() stream creation in progress
>> bluetoothd[29474]: src/mgmt.c:mgmt_event() cond 1
>> bluetoothd[29474]: src/mgmt.c:mgmt_event() Received 31 bytes from management socket
>> bluetoothd[29474]: src/mgmt.c:mgmt_device_connected() hci0 device 84:00:D2:DB:F7:8C connected eir_len 12
>> bluetoothd[29474]: src/adapter.c:adapter_get_device() 84:00:D2:DB:F7:8C
>> bluetoothd[29474]: src/mgmt.c:mgmt_event() cond 1
>> bluetoothd[29474]: src/mgmt.c:mgmt_event() Received 14 bytes from management socket
>> bluetoothd[29474]: src/mgmt.c:mgmt_auth_failed() hci0 auth failed status 6
>> bluetoothd[29474]: src/device.c:device_bonding_complete() bonding (nil) status 0x06
>> bluetoothd[29474]: connect error: Invalid exchange (52)
>> bluetoothd[29474]: audio/avdtp.c:connection_lost() Disconnected from 84:00:D2:DB:F7:8C
>> bluetoothd[29474]: audio/avdtp.c:avdtp_unref() 0x555555856aa0: ref=1
>> bluetoothd[29474]: audio/avdtp.c:avdtp_unref() 0x555555856aa0: ref=0
>> bluetoothd[29474]: audio/avdtp.c:avdtp_unref() 0x555555856aa0: freeing session and removing from list
>> bluetoothd[29474]: audio/avdtp.c:avdtp_unref() 0x555555856aa0: freeing session and removing from list
>>
>> Program received signal SIGSEGV, Segmentation fault.
>> avdtp_unref (session=0x555555856aa0) at audio/avdtp.c:1239
>> 1239            server->sessions = g_slist_remove(server->sessions, session);
>>
>> ---
>>  audio/avdtp.c |   10 ++--------
>>  1 file changed, 2 insertions(+), 8 deletions(-)
>>
>> diff --git a/audio/avdtp.c b/audio/avdtp.c
>> index bd91cb6..4109be9 100644
>> --- a/audio/avdtp.c
>> +++ b/audio/avdtp.c
>> @@ -388,7 +388,6 @@ struct avdtp_stream {
>>
>>  struct avdtp {
>>         int ref;
>> -       int free_lock;
>>
>>         uint16_t version;
>>
>> @@ -1158,14 +1157,14 @@ static void connection_lost(struct avdtp *session, int err)
>>         if (err != EACCES)
>>                 avdtp_cancel_authorization(session);
>>
>> -       session->free_lock = 1;
>> +       avdtp_ref(session);
>>
>>         finalize_discovery(session, err);
>>
>>         g_slist_foreach(session->streams, (GFunc) release_stream, session);
>>         session->streams = NULL;
>>
>> -       session->free_lock = 0;
>> +       avdtp_unref(session);
>>
>>         if (session->io) {
>>                 g_io_channel_shutdown(session->io, FALSE, NULL);
>> @@ -1173,8 +1172,6 @@ static void connection_lost(struct avdtp *session, int err)
>>                 session->io = NULL;
>>         }
>>
>> -       avdtp_set_state(session, AVDTP_SESSION_STATE_DISCONNECTED);
>
> This is actually the one that we need in case we got disconnect by
> remote after discovery completes. avctp.c has a similar structure to
> handler such states, but without the refcounting all is much simpler.
>
>>         if (session->io_id) {
>>                 g_source_remove(session->io_id);
>>                 session->io_id = 0;
>> @@ -1221,9 +1218,6 @@ void avdtp_unref(struct avdtp *session)
>>
>>                 if (session->io)
>>                         set_disconnect_timer(session);
>> -               else if (!session->free_lock) /* Drop the local ref if we
>> -                                                aren't connected */
>> -                       session->ref--;
>>         }
>>
>>         if (session->ref > 0)
>> --
>> 1.7.9.5
>
> We should probably get rid of the free_lock as well.

I just remembered the reason we have this strange refcount 1, it was
to keep the session up while the unix client/alsa client reconnects. I
think we can drop this altogether now that we don't support such bogus
clients, this also means dropping dc_timer and which should fix the
refcount for good.

-- 
Luiz Augusto von Dentz

Re: [PATCH] avdtp: Fix crash on connection lost

[Szymon Janc]

On Tuesday 16 of October 2012 20:34:47 Luiz Augusto von Dentz wrote:
> Hi Szymon,

Hi Luiz,

> >         if (session->io) {
> >                 g_io_channel_shutdown(session->io, FALSE, NULL);
> > @@ -1173,8 +1172,6 @@ static void connection_lost(struct avdtp *session, int err)
> >                 session->io = NULL;
> >         }
> >
> > -       avdtp_set_state(session, AVDTP_SESSION_STATE_DISCONNECTED);
> 
> This is actually the one that we need in case we got disconnect by
> remote after discovery completes. avctp.c has a similar structure to
> handler such states, but without the refcounting all is much simpler.

OK

> 
> >         if (session->io_id) {
> >                 g_source_remove(session->io_id);
> >                 session->io_id = 0;
> > @@ -1221,9 +1218,6 @@ void avdtp_unref(struct avdtp *session)
> >
> >                 if (session->io)
> >                         set_disconnect_timer(session);
> > -               else if (!session->free_lock) /* Drop the local ref if we
> > -                                                aren't connected */
> > -                       session->ref--;
> >         }
> >
> >         if (session->ref > 0)
> > --
> > 1.7.9.5
> 
> We should probably get rid of the free_lock as well.

Well, this is what this patch does, doesn't it?:)

-- 
BR
Szymon Janc

Re: [PATCH] avdtp: Fix crash on connection lost

[Luiz Augusto von Dentz]

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

Hi Szymon,

On Wed, Oct 17, 2012 at 9:28 AM, Szymon Janc <szymon.janc@tieto.com> wrote:
> On Tuesday 16 of October 2012 20:34:47 Luiz Augusto von Dentz wrote:
>> Hi Szymon,
>
> Hi Luiz,
>
>> >         if (session->io) {
>> >                 g_io_channel_shutdown(session->io, FALSE, NULL);
>> > @@ -1173,8 +1172,6 @@ static void connection_lost(struct avdtp *session, int err)
>> >                 session->io = NULL;
>> >         }
>> >
>> > -       avdtp_set_state(session, AVDTP_SESSION_STATE_DISCONNECTED);
>>
>> This is actually the one that we need in case we got disconnect by
>> remote after discovery completes. avctp.c has a similar structure to
>> handler such states, but without the refcounting all is much simpler.
>
> OK
>
>>
>> >         if (session->io_id) {
>> >                 g_source_remove(session->io_id);
>> >                 session->io_id = 0;
>> > @@ -1221,9 +1218,6 @@ void avdtp_unref(struct avdtp *session)
>> >
>> >                 if (session->io)
>> >                         set_disconnect_timer(session);
>> > -               else if (!session->free_lock) /* Drop the local ref if we
>> > -                                                aren't connected */
>> > -                       session->ref--;
>> >         }
>> >
>> >         if (session->ref > 0)
>> > --
>> > 1.7.9.5
>>
>> We should probably get rid of the free_lock as well.
>
> Well, this is what this patch does, doesn't it?:)

What about the attached patch, it should fix the references for good
by using the dc_timer when the reference drops to 0, I just need to
check if 1 second is enough to cover all cases including the ones
where the remote device connects but doesn't setup any stream, with
the devices I have 1 second seems enough.

-- 
Luiz Augusto von Dentz

[-- Attachment #2: 0001-AVDTP-Do-not-keep-a-internal-reference.patch --]
[-- Type: application/octet-stream, Size: 5527 bytes --]

From 39312cf09fac700505deb7babd129bec1b4e26dc Mon Sep 17 00:00:00 2001
From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
Date: Tue, 16 Oct 2012 23:09:53 +0300
Subject: [PATCH BlueZ] AVDTP: Do not keep a internal reference

Don't initialize reference with 1, instead always start disconnect timer
when reference drops to 0, so in case nobody reclaims the session it
automatically disconnect after 1 second and frees the memory.
---
 audio/avdtp.c | 114 ++++++++++++++++++++++++++--------------------------------
 1 file changed, 50 insertions(+), 64 deletions(-)

diff --git a/audio/avdtp.c b/audio/avdtp.c
index bd91cb6..e35b324 100644
--- a/audio/avdtp.c
+++ b/audio/avdtp.c
@@ -388,7 +388,6 @@ struct avdtp_stream {
 
 struct avdtp {
 	int ref;
-	int free_lock;
 
 	uint16_t version;
 
@@ -780,8 +779,9 @@ static void avdtp_set_state(struct avdtp *session,
 	}
 }
 
-static void stream_free(struct avdtp_stream *stream)
+static void stream_free(void *data)
 {
+	struct avdtp_stream *stream = data;
 	struct avdtp_remote_sep *rsep;
 
 	stream->lsep->info.inuse = 0;
@@ -1148,33 +1148,30 @@ static int avdtp_cancel_authorization(struct avdtp *session)
 	return 0;
 }
 
-static void connection_lost(struct avdtp *session, int err)
+static void sep_free(gpointer data)
 {
-	char address[18];
-
-	ba2str(&session->dst, address);
-	DBG("Disconnected from %s", address);
+	struct avdtp_remote_sep *sep = data;
 
-	if (err != EACCES)
-		avdtp_cancel_authorization(session);
+	g_slist_free_full(sep->caps, g_free);
+	g_free(sep);
+}
 
-	session->free_lock = 1;
+static void avdtp_free(struct avdtp *session)
+{
+	struct avdtp_server *server;
 
-	finalize_discovery(session, err);
+	DBG("%p", session);
 
-	g_slist_foreach(session->streams, (GFunc) release_stream, session);
-	session->streams = NULL;
+	g_slist_free_full(session->streams, stream_free);
 
-	session->free_lock = 0;
+	if (session->state != AVDTP_SESSION_STATE_DISCONNECTED)
+		avdtp_set_state(session, AVDTP_SESSION_STATE_DISCONNECTED);
 
 	if (session->io) {
 		g_io_channel_shutdown(session->io, FALSE, NULL);
 		g_io_channel_unref(session->io);
-		session->io = NULL;
 	}
 
-	avdtp_set_state(session, AVDTP_SESSION_STATE_DISCONNECTED);
-
 	if (session->io_id) {
 		g_source_remove(session->io_id);
 		session->io_id = 0;
@@ -1183,69 +1180,58 @@ static void connection_lost(struct avdtp *session, int err)
 	if (session->dc_timer)
 		remove_disconnect_timer(session);
 
-	if (session->ref != 1)
-		error("connection_lost: ref count not 1 after all callbacks");
-	else
-		avdtp_unref(session);
-}
+	avdtp_cancel_authorization(session);
 
-static void sep_free(gpointer data)
-{
-	struct avdtp_remote_sep *sep = data;
+	server = session->server;
 
-	g_slist_free_full(sep->caps, g_free);
-	g_free(sep);
-}
+	server->sessions = g_slist_remove(server->sessions, session);
 
-void avdtp_unref(struct avdtp *session)
-{
-	struct avdtp_server *server;
+	if (session->req)
+		pending_req_free(session->req);
 
-	if (!session)
-		return;
+	g_slist_free_full(session->seps, sep_free);
 
-	session->ref--;
+	g_free(session->buf);
 
-	DBG("%p: ref=%d", session, session->ref);
+	g_free(session);
+}
 
-	if (session->ref == 1) {
-		if (session->state == AVDTP_SESSION_STATE_CONNECTING &&
-								session->io) {
-			avdtp_cancel_authorization(session);
-			g_io_channel_shutdown(session->io, TRUE, NULL);
-			g_io_channel_unref(session->io);
-			session->io = NULL;
-			avdtp_set_state(session,
-					AVDTP_SESSION_STATE_DISCONNECTED);
-		}
+static void connection_lost(struct avdtp *session, int err)
+{
+	char address[18];
 
-		if (session->io)
-			set_disconnect_timer(session);
-		else if (!session->free_lock) /* Drop the local ref if we
-						 aren't connected */
-			session->ref--;
-	}
+	ba2str(&session->dst, address);
+	DBG("Disconnected from %s", address);
 
-	if (session->ref > 0)
+	if (err != EACCES)
+		avdtp_cancel_authorization(session);
+
+	if (session->ref == 0) {
+		avdtp_free(session);
 		return;
+	}
 
-	server = session->server;
+	g_slist_foreach(session->streams, (GFunc) release_stream, session);
+	session->streams = NULL;
 
-	DBG("%p: freeing session and removing from list", session);
+	finalize_discovery(session, err);
 
-	if (session->dc_timer)
-		remove_disconnect_timer(session);
+	avdtp_set_state(session, AVDTP_SESSION_STATE_DISCONNECTED);
+}
 
-	server->sessions = g_slist_remove(server->sessions, session);
+void avdtp_unref(struct avdtp *session)
+{
+	if (!session)
+		return;
 
-	if (session->req)
-		pending_req_free(session->req);
+	session->ref--;
 
-	g_slist_free_full(session->seps, sep_free);
+	DBG("%p: ref=%d", session, session->ref);
 
-	g_free(session->buf);
+	if (session->ref > 0)
+		return;
 
-	g_free(session);
+	set_disconnect_timer(session);
 }
 
 struct avdtp *avdtp_ref(struct avdtp *session)
@@ -2231,7 +2217,7 @@ static gboolean session_cb(GIOChannel *chan, GIOCondition cond,
 			goto failed;
 		}
 
-		if (session->ref == 1 && !session->streams && !session->req)
+		if (session->ref == 0 && !session->streams && !session->req)
 			set_disconnect_timer(session);
 
 		if (session->streams && session->dc_timer)
@@ -2383,7 +2369,7 @@ static struct avdtp *avdtp_get_internal(const bdaddr_t *src, const bdaddr_t *dst
 
 	session->server = server;
 	bacpy(&session->dst, dst);
-	session->ref = 1;
+	set_disconnect_timer(session);
 	/* We don't use avdtp_set_state() here since this isn't a state change
 	 * but just setting of the initial state */
 	session->state = AVDTP_SESSION_STATE_DISCONNECTED;
-- 
1.7.11.4

Re: [PATCH] avdtp: Fix crash on connection lost

[Szymon Janc]

On Wednesday 17 of October 2012 11:48:36 Luiz Augusto von Dentz wrote:
> Hi Szymon,

Hi Luiz,
 
> >> We should probably get rid of the free_lock as well.
> >
> > Well, this is what this patch does, doesn't it?:)
> 
> What about the attached patch, it should fix the references for good
> by using the dc_timer when the reference drops to 0, I just need to
> check if 1 second is enough to cover all cases including the ones
> where the remote device connects but doesn't setup any stream, with
> the devices I have 1 second seems enough.

Yeap, this fix crash I've observed.

However, avdtp_get_internal() now returns newly created session with ref=0
and in avdtp_confirm_cb() there is unref called when btd_request_authorization
failed. Due to ref being signed int (why?) this could lead to not-nice-to-debug
bug if that session is reclaimed before dc timer fires (ref would go from -1 to
0).

Also I'm having trouble with tracking lifetime of session object.. I think
only avdtp_ref/unref function should really care about ref count. That would
require other function to only use ref/unref to get/free session object.
e.g. avdtp_free should be only called from disconnect_timeout() and
connection_lost() should only trigger other session users to drop their
references. Same goes to set_disconnect_timer() which should only be called
from avdtp_unref. There should be no need to call connection_lost from
disconnect_timeout as this timer should only run when there is no references
held to that session, right?

Or do I miss something?:)
-- 
BR
Szymon Janc

Re: [PATCH] avdtp: Fix crash on connection lost

[Luiz Augusto von Dentz]

Hi Szymon,

On Wed, Oct 17, 2012 at 12:17 PM, Szymon Janc <szymon.janc@tieto.com> wrote:
> On Wednesday 17 of October 2012 11:48:36 Luiz Augusto von Dentz wrote:
>> Hi Szymon,
>
> Hi Luiz,
>
>> >> We should probably get rid of the free_lock as well.
>> >
>> > Well, this is what this patch does, doesn't it?:)
>>
>> What about the attached patch, it should fix the references for good
>> by using the dc_timer when the reference drops to 0, I just need to
>> check if 1 second is enough to cover all cases including the ones
>> where the remote device connects but doesn't setup any stream, with
>> the devices I have 1 second seems enough.
>
> Yeap, this fix crash I've observed.
>
> However, avdtp_get_internal() now returns newly created session with ref=0
> and in avdtp_confirm_cb() there is unref called when btd_request_authorization
> failed. Due to ref being signed int (why?) this could lead to not-nice-to-debug
> bug if that session is reclaimed before dc timer fires (ref would go from -1 to
> 0).

Good catch, I should be able to fix this, perhaps we can free
immediately or let the timeout handle this as well.

> Also I'm having trouble with tracking lifetime of session object.. I think
> only avdtp_ref/unref function should really care about ref count. That would
> require other function to only use ref/unref to get/free session object.
> e.g. avdtp_free should be only called from disconnect_timeout() and
> connection_lost() should only trigger other session users to drop their
> references. Same goes to set_disconnect_timer() which should only be called
> from avdtp_unref. There should be no need to call connection_lost from
> disconnect_timeout as this timer should only run when there is no references
> held to that session, right?

Well that involves changing some other logic behind the timeout, such as this:

		if (session->ref == 0 && !session->streams && !session->req)
			set_disconnect_timer(session);

		if (session->streams && session->dc_timer)
			remove_disconnect_timer(session);

IMO those should be removed in favor of the ref/unref handling if the
disconnect timer should be started/removed, if there is a request the
user should be holding a reference and the same for the stream, this
should actually simplify even further the code. Also the reason I did
avdtp_free in the connection_lost was not to much too much code around
as avdtp_free call a lot of other functions, but you are probably
right and this should actually be called from disconnect_timeout.

-- 
Luiz Augusto von Dentz