| Message ID | 20200305185904.Bluez.v1.1.I6c78c0eb9826eb17c944c4903132ee75c1324136@changeid (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Luiz Von Dentz |
| Headers | show |
| Series | [Bluez,v1] avdtp: Fix crashes in avdtp_abort | expand |
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 >
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
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)