[PATCH] Fix double free on AVDTP Abort response

[PATCH] Fix double free on AVDTP Abort response

[Daniel Örstadius]

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

With the patch I submitted some time ago
http://git.kernel.org/?p=bluetooth/bluez.git;a=commit;h=e9b1a8f7266d0674b1ea068a5bb5698e9ee424c9
there is a code path leading to a double free:
session_cb -> avdtp_parse_resp -> avdtp_abort_resp ->
avdtp_sep_set_state(..., AVDTP_STATE_IDLE) -> handle_unanswered_req

A response to AVDTP Abort could lead to the pending request being
freed both in session_cb and handle_unanswered_req.

This patch avoids doing it in the latter function. The primary purpose
of adding handle_unanswered_req was to trigger responses on the Audio
API (it was based on avdtp.c:request_timeout). AFAIU, AVDTP Abort
doesn't lead to an API response and will be freed elsewhere
(session_cb or avdtp_unref).

/Daniel

[-- Attachment #2: 0001-Fix-double-free-on-AVDTP-Abort-response.patch --]
[-- Type: text/x-patch, Size: 963 bytes --]

From 803637bc0e452392498714cd8245a06f5aea2edc Mon Sep 17 00:00:00 2001
From: Daniel Orstadius <daniel.orstadius@gmail.com>
Date: Fri, 19 Feb 2010 17:51:48 +0200
Subject: [PATCH] Fix double free on AVDTP Abort response

The pending request might be freed twice when receiving an Abort
response, in handle_unanswered_req and session_cb. Avoid freeing
it in handle_unanswered_req.
---
 audio/avdtp.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/audio/avdtp.c b/audio/avdtp.c
index 2591845..ae7c88e 100644
--- a/audio/avdtp.c
+++ b/audio/avdtp.c
@@ -905,6 +905,13 @@ static void handle_unanswered_req(struct avdtp *session,
 	struct avdtp_local_sep *lsep;
 	struct avdtp_error err;
 
+	if (session->req == AVDTP_ABORT) {
+		/* Avoid freeing the Abort request here */
+		debug("handle_unanswered_req: Abort req, returning");
+		session->req->stream = NULL;
+		return;
+	}
+
 	req = session->req;
 	session->req = NULL;
 
-- 
1.6.0.4

Re: [PATCH] Fix double free on AVDTP Abort response

[Johan Hedberg]

Hi Daniel,

On Fri, Feb 19, 2010, Daniel Örstadius wrote:
> +	if (session->req == AVDTP_ABORT) {
> +		/* Avoid freeing the Abort request here */
> +		debug("handle_unanswered_req: Abort req, returning");
> +		session->req->stream = NULL;
> +		return;
> +	}

Please try to at least do a compile check before you submit patches.
This one gives the following error:
audio/avdtp.c: In function ‘handle_unanswered_req’:
audio/avdtp.c:908: error: comparison between pointer and integer

What you probably want is session->req->signal_id == AVDTP_ABORT.

Johan

Re: [PATCH] Fix double free on AVDTP Abort response

[Daniel Örstadius]

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

On Fri, Feb 19, 2010 at 7:32 PM, Johan Hedberg <johan.hedberg@gmail.com> wrote:

> Please try to at least do a compile check before you submit patches.
> This one gives the following error:
> audio/avdtp.c: In function ‘handle_unanswered_req’:
> audio/avdtp.c:908: error: comparison between pointer and integer
>
> What you probably want is session->req->signal_id == AVDTP_ABORT.
>

Sorry. The attached patch compiles without warnings on "./configure && make".
(new dependency to a capabilities lib prevents ./bootstrap-configure atm)

Br,
Daniel

[-- Attachment #2: 0001-Fix-double-free-on-AVDTP-Abort-response.patch --]
[-- Type: text/x-patch, Size: 974 bytes --]

From f45006ec75c23b55470e8088fe64e8f0b6ab6404 Mon Sep 17 00:00:00 2001
From: Daniel Orstadius <daniel.orstadius@gmail.com>
Date: Sun, 21 Feb 2010 14:39:49 +0200
Subject: [PATCH] Fix double free on AVDTP Abort response

The pending request might be freed twice when receiving an Abort
response, in handle_unanswered_req and session_cb. Avoid freeing
it in handle_unanswered_req.
---
 audio/avdtp.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/audio/avdtp.c b/audio/avdtp.c
index 2591845..add08f1 100644
--- a/audio/avdtp.c
+++ b/audio/avdtp.c
@@ -905,6 +905,13 @@ static void handle_unanswered_req(struct avdtp *session,
 	struct avdtp_local_sep *lsep;
 	struct avdtp_error err;
 
+	if (session->req->signal_id == AVDTP_ABORT) {
+		/* Avoid freeing the Abort request here */
+		debug("handle_unanswered_req: Abort req, returning");
+		session->req->stream = NULL;
+		return;
+	}
+
 	req = session->req;
 	session->req = NULL;
 
-- 
1.6.0.4

Re: [PATCH] Fix double free on AVDTP Abort response

[Marcel Holtmann]

Hi Daniel,

> > Please try to at least do a compile check before you submit patches.
> > This one gives the following error:
> > audio/avdtp.c: In function ‘handle_unanswered_req’:
> > audio/avdtp.c:908: error: comparison between pointer and integer
> >
> > What you probably want is session->req->signal_id == AVDTP_ABORT.
> >
> 
> Sorry. The attached patch compiles without warnings on "./configure && make".
> (new dependency to a capabilities lib prevents ./bootstrap-configure atm)

you can use ./bootstrap-configure --disable-capng.

Regards

Marcel

Re: [PATCH] Fix double free on AVDTP Abort response

[Johan Hedberg]

Hi Daniel,

On Sun, Feb 21, 2010, Daniel Örstadius wrote:
> > Please try to at least do a compile check before you submit patches.
> > This one gives the following error:
> > audio/avdtp.c: In function ‘handle_unanswered_req’:
> > audio/avdtp.c:908: error: comparison between pointer and integer
> >
> > What you probably want is session->req->signal_id == AVDTP_ABORT.
> >
> 
> Sorry. The attached patch compiles without warnings on "./configure && make".
> (new dependency to a capabilities lib prevents ./bootstrap-configure atm)

You can avoid that with "./bootstrap-configure --disable-capng"

> The pending request might be freed twice when receiving an Abort
> response, in handle_unanswered_req and session_cb. Avoid freeing
> it in handle_unanswered_req.
> ---
>  audio/avdtp.c |    7 +++++++
>  1 files changed, 7 insertions(+), 0 deletions(-)

Thanks, the patch is now upstream.

Johan

Re: [PATCH] Fix double free on AVDTP Abort response

[Luiz Augusto von Dentz]

Hi Johan,

On Sun, Feb 21, 2010 at 3:46 PM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> Hi Daniel,
>
> On Sun, Feb 21, 2010, Daniel Örstadius wrote:
>> > Please try to at least do a compile check before you submit patches.
>> > This one gives the following error:
>> > audio/avdtp.c: In function ‘handle_unanswered_req’:
>> > audio/avdtp.c:908: error: comparison between pointer and integer
>> >
>> > What you probably want is session->req->signal_id == AVDTP_ABORT.
>> >
>>
>> Sorry. The attached patch compiles without warnings on "./configure && make".
>> (new dependency to a capabilities lib prevents ./bootstrap-configure atm)
>
> You can avoid that with "./bootstrap-configure --disable-capng"
>
>> The pending request might be freed twice when receiving an Abort
>> response, in handle_unanswered_req and session_cb. Avoid freeing
>> it in handle_unanswered_req.
>> ---
>>  audio/avdtp.c |    7 +++++++
>>  1 files changed, 7 insertions(+), 0 deletions(-)
>
> Thanks, the patch is now upstream.

While discussing this problem we find out that our timeout doesn't
really work as we imagine due to the possibility of abort being
rejected, so either we don't call any callback on timeout and do it on
abort response while changing to aborting state (which probably means
4-8 sec before we are really able to abort/cancel) or we disconnect as
the other end seems to not cooperate on aborting.

Btw, we probably should have a smaller timeout for avdtp_abort anyway
and make sure we call it upon RequestDisconnect not avdtp_close which
has no priority over the others commands.

Regards,

-- 
Luiz Augusto von Dentz
Computer Engineer

Re: [PATCH] Fix double free on AVDTP Abort response

[Johan Hedberg]

Hi Luiz,

On Sun, Feb 21, 2010, Luiz Augusto von Dentz wrote:
> While discussing this problem we find out that our timeout doesn't
> really work as we imagine due to the possibility of abort being
> rejected, so either we don't call any callback on timeout and do it on
> abort response while changing to aborting state (which probably means
> 4-8 sec before we are really able to abort/cancel) or we disconnect as
> the other end seems to not cooperate on aborting.
> 
> Btw, we probably should have a smaller timeout for avdtp_abort anyway
> and make sure we call it upon RequestDisconnect not avdtp_close which
> has no priority over the others commands.

Makes sense. Feel free to propose a patch (or multiple patches) for all
this ;)

Johan