[Bluez PATCH v1] avdtp: Fix crashes in avdtp_abort

[Bluez PATCH v1] avdtp: Fix crashes in avdtp_abort

[Howard Chung]

Initialized avdtp_local_sep later since stream could be NULL.
---

 profiles/audio/avdtp.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/profiles/audio/avdtp.c b/profiles/audio/avdtp.c
index 0e075f9ff..12d984866 100644
--- a/profiles/audio/avdtp.c
+++ b/profiles/audio/avdtp.c
@@ -3566,7 +3566,7 @@ int avdtp_abort(struct avdtp *session, struct avdtp_stream *stream)
 {
 	struct seid_req req;
 	int ret;
-	struct avdtp_local_sep *sep = stream->lsep;
+	struct avdtp_local_sep *sep;
 
 	if (!stream && session->discover) {
 		/* Don't call cb since it being aborted */
@@ -3581,6 +3581,7 @@ int avdtp_abort(struct avdtp *session, struct avdtp_stream *stream)
 	if (stream->lsep->state == AVDTP_STATE_ABORTING)
 		return -EINVAL;
 
+	sep = stream->lsep;
 	avdtp_sep_set_state(session, sep, AVDTP_STATE_ABORTING);
 
 	if (session->req && stream == session->req->stream)
-- 
2.25.0.265.gbab2e86ba0-goog

Re: [Bluez PATCH v1] avdtp: Fix crashes in avdtp_abort

[Luiz Augusto von Dentz]

Hi Howard,

On Thu, Mar 5, 2020 at 3:06 AM Howard Chung <howardchung@google.com> wrote:
>
> Initialized avdtp_local_sep later since stream could be NULL.
> ---
>
>  profiles/audio/avdtp.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/profiles/audio/avdtp.c b/profiles/audio/avdtp.c
> index 0e075f9ff..12d984866 100644
> --- a/profiles/audio/avdtp.c
> +++ b/profiles/audio/avdtp.c
> @@ -3566,7 +3566,7 @@ int avdtp_abort(struct avdtp *session, struct avdtp_stream *stream)
>  {
>         struct seid_req req;
>         int ret;
> -       struct avdtp_local_sep *sep = stream->lsep;
> +       struct avdtp_local_sep *sep;
>
>         if (!stream && session->discover) {
>                 /* Don't call cb since it being aborted */
> @@ -3581,6 +3581,7 @@ int avdtp_abort(struct avdtp *session, struct avdtp_stream *stream)
>         if (stream->lsep->state == AVDTP_STATE_ABORTING)
>                 return -EINVAL;

I suspect there i something else going on then just the lsep being
NULL since we do check it on the line above it would have crashed
anyway, is this perhaps the result of lsep being unregistered before
the avdtp_abort is called?

> +       sep = stream->lsep;
>         avdtp_sep_set_state(session, sep, AVDTP_STATE_ABORTING);
>
>         if (session->req && stream == session->req->stream)
> --
> 2.25.0.265.gbab2e86ba0-goog
>


-- 
Luiz Augusto von Dentz

Re: [Bluez PATCH v1] avdtp: Fix crashes in avdtp_abort

[Luiz Augusto von Dentz]

Hi Howard,

On Mon, Mar 23, 2020 at 2:43 AM Yun-hao Chung <howardchung@google.com> wrote:
>
> Hi Luiz,
>
> I can trigger the crash by running a connect disconnect loop. What I've found is that crashes always happened when sep is assigned to stream->lsep but stream is NULL. If I apply the change above, there are no such crashes anymore. So I'm still believing it is caused by stream being NULL instead of stream->lsep being NULL. Do I miss something?
>
> One thing to add:
> Perhaps it would be more clear if we use stream->lsep directly when calling avdtp_sep_set_state.
>
> Here is the stack trace:
>
> Crash reason:  SIGSEGV /0x00000000
> Crash address: 0x18
> Process uptime: not available
>
> Thread 0 (crashed)
>  0  bluetoothd!avdtp_abort [avdtp.c : 3487 + 0x0]
>  1  bluetoothd!a2dp_cancel [a2dp.c : 2180 + 0x5]
>  2  bluetoothd!sink_disconnect [sink.c : 404 + 0x5]
>  3  bluetoothd!btd_service_disconnect [service.c : 293 + 0x5]
>  4  libglib-2.0.so.0!g_list_foreach [glist.c : 1013 + 0x6]
>  5  bluetoothd!device_request_disconnect [device.c : 1500 + 0xe]
>  6  bluetoothd!dev_disconnect [device.c : 1652 + 0xb]
>  7  bluetoothd!generic_message [object.c : 259 + 0x8]
>  8  libdbus-1.so.3!_dbus_object_tree_dispatch_and_unlock [dbus-object-tree.c : 1020 + 0x5]
>  9  libdbus-1.so.3!dbus_connection_dispatch [dbus-connection.c : 4750 + 0x8]
> 10  bluetoothd!message_dispatch [mainloop.c : 72 + 0x8]
> 11  libglib-2.0.so.0!g_main_context_dispatch [gmain.c : 3182 + 0x2]
> 12  libglib-2.0.so.0!g_main_context_iterate [gmain.c : 3920 + 0x8]
> 13  libglib-2.0.so.0!g_main_loop_run [gmain.c : 4116 + 0xf]
> 14  bluetoothd!main [main.c : 800 + 0x5]
> 15  libc.so.6!__libc_start_main [libc-start.c : 308 + 0x1a]
> 16  bluetoothd!_start + 0x2a

I see so the setup->stream is stil NULL in this case so trying to
access stream->lsep will fail.

>
> Thanks,
> Howard
>
> On Fri, Mar 6, 2020 at 3:17 AM Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote:
>>
>> Hi Howard,
>>
>> On Thu, Mar 5, 2020 at 3:06 AM Howard Chung <howardchung@google.com> wrote:
>> >
>> > Initialized avdtp_local_sep later since stream could be NULL.
>> > ---
>> >
>> >  profiles/audio/avdtp.c | 3 ++-
>> >  1 file changed, 2 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/profiles/audio/avdtp.c b/profiles/audio/avdtp.c
>> > index 0e075f9ff..12d984866 100644
>> > --- a/profiles/audio/avdtp.c
>> > +++ b/profiles/audio/avdtp.c
>> > @@ -3566,7 +3566,7 @@ int avdtp_abort(struct avdtp *session, struct avdtp_stream *stream)
>> >  {
>> >         struct seid_req req;
>> >         int ret;
>> > -       struct avdtp_local_sep *sep = stream->lsep;
>> > +       struct avdtp_local_sep *sep;

Lets just remove this variable for here.

>> >         if (!stream && session->discover) {
>> >                 /* Don't call cb since it being aborted */
>> > @@ -3581,6 +3581,7 @@ int avdtp_abort(struct avdtp *session, struct avdtp_stream *stream)
>> >         if (stream->lsep->state == AVDTP_STATE_ABORTING)
>> >                 return -EINVAL;
>>
>> I suspect there i something else going on then just the lsep being
>> NULL since we do check it on the line above it would have crashed
>> anyway, is this perhaps the result of lsep being unregistered before
>> the avdtp_abort is called?
>>
>> > +       sep = stream->lsep;
>> >         avdtp_sep_set_state(session, sep, AVDTP_STATE_ABORTING);

Just use stream->lsep here since at this point we already verified
that stream != NULL and even attempted to read its state.

>> >
>> >         if (session->req && stream == session->req->stream)
>> > --
>> > 2.25.0.265.gbab2e86ba0-goog
>> >
>>
>>
>> --
>> Luiz Augusto von Dentz



-- 
Luiz Augusto von Dentz