| Message ID | 20200916211122.127881-1-sonnysasaka@chromium.org |
|---|---|
| State | New |
| Headers | show |
| Series | [BlueZ] a2dp: Keep track of ref ownership of a2dp_setup | expand |
This is automated email and please do not reply to this email!
Dear submitter,
Thank you for submitting the patches to the linux bluetooth mailing list.
While we are preparing for reviewing the patches, we found the following
issue/warning.
Test Result:
checkpatch Failed
Outputs:
WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#31:
frame #5: 0xf679523c libglib-2.0.so.0`g_main_context_dispatch at gmain.c:3182
WARNING:BLOCK_COMMENT_STYLE: Block comments use a trailing */ on a separate line
#137: FILE: profiles/audio/avdtp.c:370:
+ * opening is pending */
- total: 0 errors, 2 warnings, 122 lines checked
NOTE: For some of the reported defects, checkpatch may be able to
mechanically convert to the typical style using --fix or --fix-inplace.
Your patch has style problems, please review.
NOTE: Ignored message types: COMMIT_MESSAGE COMPLEX_MACRO CONST_STRUCT FILE_PATH_CHANGES MISSING_SIGN_OFF PREFER_PACKED SPLIT_STRING SSCANF_TO_KSTRTO
NOTE: If any of the errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.
---
Regards,
Linux Bluetooth
This is automated email and please do not reply to this email! Dear submitter, Thank you for submitting the patches to the linux bluetooth mailing list. While we are preparing for reviewing the patches, we found the following issue/warning. Test Result: checkgitlint Failed Outputs: 28: B1 Line exceeds max length (81>80): " frame #5: 0xf679523c libglib-2.0.so.0`g_main_context_dispatch at gmain.c:3182" 32: B1 Line exceeds max length (85>80): " frame #9: 0x0c65a7ea bluetoothd`mainloop_run_with_signal at mainloop-notify.c:201" --- Regards, Linux Bluetooth
Hi Sonny, On Wed, Sep 16, 2020 at 3:31 PM Sonny Sasaka <sonnysasaka@chromium.org> wrote: > > Currently transport_cb and abort_cfm make assumption that they have an > a2dp_setup reference held as a result of open_ind invocation. In the > field this is not always true, for example when the peer device opens an > L2CAP channel for AVDTP transport channel without sending AVDTP_OPEN > request through the AVDTP signaling channel first. Although in this case > the peer device does not behave correctly, we should protect this > possible crash from happening by making sure that transport_cb and > abort_cfm are really holding a reference of a2dp_setup object before > trying to unref them. > > After grabbing a reference, open_ind stores the pointer in > stream->pending_open_data. If this field is set, that means there is a > pending AVDTP_OPEN command and it needs to be unref-fed later once and > only once: when the transport channel is created (transport_cb) or when > the AVDTP_OPEN command is aborted (abort_cfm). If this field is not set, > nothing should unref it. This enforces that the reference counting is > correct regardless of the behavior of the peer device. While I agree that we should handle this properly and never crash I think we can take a more generic approach by attaching a user_data to the stream, see my comments bellow. > A sample crash stack trace from Chrome OS: > * thread #1, stop reason = signal SIGSEGV > * frame #0: 0x0c64f0e8 bluetoothd`queue_remove_all at queue.c:351 > frame #1: 0x0c64f086 bluetoothd`queue_destroy at queue.c:73 > frame #2: 0x0c6022b0 bluetoothd`setup_unref at a2dp.c:222 > frame #3: 0x0c604942 bluetoothd`transport_cb at a2dp.c:2229 > frame #4: 0x0c61e35c bluetoothd`accept_cb at btio.c:203 > frame #5: 0xf679523c libglib-2.0.so.0`g_main_context_dispatch at gmain.c:3182 > frame #6: 0xf67954aa libglib-2.0.so.0`g_main_context_iterate at gmain.c:3920 > frame #7: 0xf679569a libglib-2.0.so.0`g_main_loop_run at gmain.c:4116 > frame #8: 0x0c65a5a0 bluetoothd`mainloop_run at mainloop-glib.c:79 > frame #9: 0x0c65a7ea bluetoothd`mainloop_run_with_signal at mainloop-notify.c:201 > frame #10: 0x0c6477ec bluetoothd`main at main.c:772 > frame #11: 0xf65bc0a2 libc.so.6`__libc_start_main at libc-start.c:308 > > --- > profiles/audio/a2dp.c | 24 ++++++++++++++++-------- > profiles/audio/avdtp.c | 17 +++++++++++++++++ > profiles/audio/avdtp.h | 3 +++ > 3 files changed, 36 insertions(+), 8 deletions(-) > > diff --git a/profiles/audio/a2dp.c b/profiles/audio/a2dp.c > index 2feea66c0..9e96f8c4b 100644 > --- a/profiles/audio/a2dp.c > +++ b/profiles/audio/a2dp.c > @@ -927,10 +927,16 @@ static gboolean open_ind(struct avdtp *session, struct avdtp_local_sep *sep, > else > DBG("Source %p: Open_Ind", sep); > > + if (avdtp_stream_get_pending_open_data(stream)) { > + warn("Pending open data already exists"); > + return FALSE; > + } > + Not really clear why you need to check if there is a setup already in pending state here, if that is the case then that should be fixed in avdtp.c since it is not support to call open again if it is already pending. > setup = a2dp_setup_get(session); > if (!setup) > return FALSE; > > + avdtp_stream_set_pending_open_data(stream, setup); Id like this to be handle generically in with something like: avdtp_stream_set_user_data(session, setup, destroy) setup = avdtp_stream_get_user_data(session) This should avoid lookups in the likes of a2dp_setup_get and ensure that the setup lifetime is tied to the stream. > setup->stream = stream; > > if (!err && setup->chan) > @@ -1285,14 +1291,13 @@ static void abort_cfm(struct avdtp *session, struct avdtp_local_sep *sep, > void *user_data) > { > struct a2dp_sep *a2dp_sep = user_data; > - struct a2dp_setup *setup; > + struct a2dp_setup *setup = avdtp_stream_get_pending_open_data(stream); Abort can be called while in configured state as well, anyway if we go ahead with the idea of attaching the setup to the avdtp_stream then this would be just a matter of calling avdtp_stream_get_user_data. > > if (a2dp_sep->type == AVDTP_SEP_TYPE_SINK) > DBG("Sink %p: Abort_Cfm", sep); > else > DBG("Source %p: Abort_Cfm", sep); > > - setup = find_setup_by_session(session); > if (!setup) > return; > > @@ -1302,6 +1307,7 @@ static void abort_cfm(struct avdtp *session, struct avdtp_local_sep *sep, > } > > setup_unref(setup); > + avdtp_stream_set_pending_open_data(stream, NULL); This would then be avdtp_stream_set_user_data(stream, NULL, NULL), or better yet wait the destroy function to be called which should then do setup_unref to destroy the setup. > } > > static gboolean reconf_ind(struct avdtp *session, struct avdtp_local_sep *sep, > @@ -2216,11 +2222,12 @@ fail: > > static void transport_cb(GIOChannel *io, GError *err, gpointer user_data) > { > - struct a2dp_setup *setup = user_data; > + struct avdtp_stream *stream = user_data; > + struct a2dp_setup *setup = avdtp_stream_get_pending_open_data(stream); > uint16_t omtu, imtu; > > - if (!g_slist_find(setups, setup)) { > - warn("bt_io_accept: setup %p no longer valid", setup); > + if (!setup) { > + warn("transport_cb: pending open data does not exist"); > g_io_channel_shutdown(io, TRUE, NULL); > return; > } > @@ -2238,8 +2245,7 @@ static void transport_cb(GIOChannel *io, GError *err, gpointer user_data) > goto drop; > } > > - if (!avdtp_stream_set_transport(setup->stream, > - g_io_channel_unix_get_fd(io), > + if (!avdtp_stream_set_transport(stream, g_io_channel_unix_get_fd(io), > imtu, omtu)) > goto drop; > > @@ -2249,6 +2255,7 @@ static void transport_cb(GIOChannel *io, GError *err, gpointer user_data) > setup->io = NULL; > > setup_unref(setup); > + avdtp_stream_set_pending_open_data(stream, NULL); Ditto. > return; > > @@ -2297,7 +2304,8 @@ static void confirm_cb(GIOChannel *io, gpointer data) > goto drop; > } > > - if (!bt_io_accept(io, transport_cb, setup, NULL, &err)) { > + if (!bt_io_accept(io, transport_cb, setup->stream, NULL, > + &err)) { > error("bt_io_accept: %s", err->message); > g_error_free(err); > goto drop; > diff --git a/profiles/audio/avdtp.c b/profiles/audio/avdtp.c > index 782268c08..74b9f8dfa 100644 > --- a/profiles/audio/avdtp.c > +++ b/profiles/audio/avdtp.c > @@ -366,6 +366,8 @@ struct avdtp_stream { > GSList *caps; > GSList *callbacks; > struct avdtp_service_capability *codec; > + void *pending_open_data; /* Data when the transport channel > + * opening is pending */ > guint io_id; /* Transport GSource ID */ > guint timer; /* Waiting for other side to close or open > * the transport channel */ > @@ -727,6 +729,11 @@ static void stream_free(void *data) > g_slist_free_full(stream->callbacks, g_free); > g_slist_free_full(stream->caps, g_free); > > + /* pending_open_data must have been unref-ed and unset before freeing > + * avdtp_stream. Otherwise, it is a reference leak bug. > + */ > + assert(!stream->pending_open_data); > + > g_free(stream); > } > > @@ -3147,6 +3154,16 @@ struct avdtp_remote_sep *avdtp_stream_get_remote_sep( > return NULL; > } > > +void avdtp_stream_set_pending_open_data(struct avdtp_stream *stream, void *data) > +{ > + stream->pending_open_data = data; > +} > + > +void *avdtp_stream_get_pending_open_data(struct avdtp_stream *stream) > +{ > + return stream->pending_open_data; > +} > + > gboolean avdtp_stream_set_transport(struct avdtp_stream *stream, int fd, > size_t imtu, size_t omtu) > { > diff --git a/profiles/audio/avdtp.h b/profiles/audio/avdtp.h > index 011fea89e..92ce65ed3 100644 > --- a/profiles/audio/avdtp.h > +++ b/profiles/audio/avdtp.h > @@ -260,6 +260,9 @@ gboolean avdtp_stream_has_capabilities(struct avdtp_stream *stream, > GSList *caps); > struct avdtp_remote_sep *avdtp_stream_get_remote_sep( > struct avdtp_stream *stream); > +void avdtp_stream_set_pending_open_data(struct avdtp_stream *stream, > + void *data); > +void *avdtp_stream_get_pending_open_data(struct avdtp_stream *stream); > > unsigned int avdtp_add_state_cb(struct btd_device *dev, > avdtp_session_state_cb cb, void *user_data); > -- > 2.26.2 > -- Luiz Augusto von Dentz
Hi Luiz, On Wed, Sep 16, 2020 at 4:28 PM Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: > > Hi Sonny, > > On Wed, Sep 16, 2020 at 3:31 PM Sonny Sasaka <sonnysasaka@chromium.org> wrote: > > > > Currently transport_cb and abort_cfm make assumption that they have an > > a2dp_setup reference held as a result of open_ind invocation. In the > > field this is not always true, for example when the peer device opens an > > L2CAP channel for AVDTP transport channel without sending AVDTP_OPEN > > request through the AVDTP signaling channel first. Although in this case > > the peer device does not behave correctly, we should protect this > > possible crash from happening by making sure that transport_cb and > > abort_cfm are really holding a reference of a2dp_setup object before > > trying to unref them. > > > > After grabbing a reference, open_ind stores the pointer in > > stream->pending_open_data. If this field is set, that means there is a > > pending AVDTP_OPEN command and it needs to be unref-fed later once and > > only once: when the transport channel is created (transport_cb) or when > > the AVDTP_OPEN command is aborted (abort_cfm). If this field is not set, > > nothing should unref it. This enforces that the reference counting is > > correct regardless of the behavior of the peer device. > > While I agree that we should handle this properly and never crash I > think we can take a more generic approach by attaching a user_data to > the stream, see my comments bellow. I sent a v2 to change to generic user_data instead of pending_open_data. > > > A sample crash stack trace from Chrome OS: > > * thread #1, stop reason = signal SIGSEGV > > * frame #0: 0x0c64f0e8 bluetoothd`queue_remove_all at queue.c:351 > > frame #1: 0x0c64f086 bluetoothd`queue_destroy at queue.c:73 > > frame #2: 0x0c6022b0 bluetoothd`setup_unref at a2dp.c:222 > > frame #3: 0x0c604942 bluetoothd`transport_cb at a2dp.c:2229 > > frame #4: 0x0c61e35c bluetoothd`accept_cb at btio.c:203 > > frame #5: 0xf679523c libglib-2.0.so.0`g_main_context_dispatch at gmain.c:3182 > > frame #6: 0xf67954aa libglib-2.0.so.0`g_main_context_iterate at gmain.c:3920 > > frame #7: 0xf679569a libglib-2.0.so.0`g_main_loop_run at gmain.c:4116 > > frame #8: 0x0c65a5a0 bluetoothd`mainloop_run at mainloop-glib.c:79 > > frame #9: 0x0c65a7ea bluetoothd`mainloop_run_with_signal at mainloop-notify.c:201 > > frame #10: 0x0c6477ec bluetoothd`main at main.c:772 > > frame #11: 0xf65bc0a2 libc.so.6`__libc_start_main at libc-start.c:308 > > > > --- > > profiles/audio/a2dp.c | 24 ++++++++++++++++-------- > > profiles/audio/avdtp.c | 17 +++++++++++++++++ > > profiles/audio/avdtp.h | 3 +++ > > 3 files changed, 36 insertions(+), 8 deletions(-) > > > > diff --git a/profiles/audio/a2dp.c b/profiles/audio/a2dp.c > > index 2feea66c0..9e96f8c4b 100644 > > --- a/profiles/audio/a2dp.c > > +++ b/profiles/audio/a2dp.c > > @@ -927,10 +927,16 @@ static gboolean open_ind(struct avdtp *session, struct avdtp_local_sep *sep, > > else > > DBG("Source %p: Open_Ind", sep); > > > > + if (avdtp_stream_get_pending_open_data(stream)) { > > + warn("Pending open data already exists"); > > + return FALSE; > > + } > > + > > Not really clear why you need to check if there is a setup already in > pending state here, if that is the case then that should be fixed in > avdtp.c since it is not support to call open again if it is already > pending. It's one thing that avdtp.c should never allow open_ind to be invoked twice. But it's also another thing to ensure that a2dp.c guarantees the correctness of the ref counting of setup object regardless of the broader code area. What we do here is following the contract that stream->user_data being not NULL means that there is a reference taken on a setup object. We should either return early here, or unref that object before continuing to ensure that ref-counting is guaranteed to be correct even without looking/understanding the bigger picture. > > > setup = a2dp_setup_get(session); > > if (!setup) > > return FALSE; > > > > + avdtp_stream_set_pending_open_data(stream, setup); > > Id like this to be handle generically in with something like: > > avdtp_stream_set_user_data(session, setup, destroy) > setup = avdtp_stream_get_user_data(session) > > This should avoid lookups in the likes of a2dp_setup_get and ensure > that the setup lifetime is tied to the stream. I have sent a v2 to use generic user_data with destroy function. However I can't follow exactly what you recommend here because setup still needs to be looked up from a2dp_setup_get because this is the first time we assign stream->user_data. > > > setup->stream = stream; > > > > if (!err && setup->chan) > > @@ -1285,14 +1291,13 @@ static void abort_cfm(struct avdtp *session, struct avdtp_local_sep *sep, > > void *user_data) > > { > > struct a2dp_sep *a2dp_sep = user_data; > > - struct a2dp_setup *setup; > > + struct a2dp_setup *setup = avdtp_stream_get_pending_open_data(stream); > > Abort can be called while in configured state as well, anyway if we go > ahead with the idea of attaching the setup to the avdtp_stream then > this would be just a matter of calling avdtp_stream_get_user_data. > > > > > if (a2dp_sep->type == AVDTP_SEP_TYPE_SINK) > > DBG("Sink %p: Abort_Cfm", sep); > > else > > DBG("Source %p: Abort_Cfm", sep); > > > > - setup = find_setup_by_session(session); > > if (!setup) > > return; > > > > @@ -1302,6 +1307,7 @@ static void abort_cfm(struct avdtp *session, struct avdtp_local_sep *sep, > > } > > > > setup_unref(setup); > > + avdtp_stream_set_pending_open_data(stream, NULL); > > This would then be avdtp_stream_set_user_data(stream, NULL, NULL), or > better yet wait the destroy function to be called which should then do > setup_unref to destroy the setup. > > > } > > > > static gboolean reconf_ind(struct avdtp *session, struct avdtp_local_sep *sep, > > @@ -2216,11 +2222,12 @@ fail: > > > > static void transport_cb(GIOChannel *io, GError *err, gpointer user_data) > > { > > - struct a2dp_setup *setup = user_data; > > + struct avdtp_stream *stream = user_data; > > + struct a2dp_setup *setup = avdtp_stream_get_pending_open_data(stream); > > uint16_t omtu, imtu; > > > > - if (!g_slist_find(setups, setup)) { > > - warn("bt_io_accept: setup %p no longer valid", setup); > > + if (!setup) { > > + warn("transport_cb: pending open data does not exist"); > > g_io_channel_shutdown(io, TRUE, NULL); > > return; > > } > > @@ -2238,8 +2245,7 @@ static void transport_cb(GIOChannel *io, GError *err, gpointer user_data) > > goto drop; > > } > > > > - if (!avdtp_stream_set_transport(setup->stream, > > - g_io_channel_unix_get_fd(io), > > + if (!avdtp_stream_set_transport(stream, g_io_channel_unix_get_fd(io), > > imtu, omtu)) > > goto drop; > > > > @@ -2249,6 +2255,7 @@ static void transport_cb(GIOChannel *io, GError *err, gpointer user_data) > > setup->io = NULL; > > > > setup_unref(setup); > > + avdtp_stream_set_pending_open_data(stream, NULL); > > Ditto. > > > return; > > > > @@ -2297,7 +2304,8 @@ static void confirm_cb(GIOChannel *io, gpointer data) > > goto drop; > > } > > > > - if (!bt_io_accept(io, transport_cb, setup, NULL, &err)) { > > + if (!bt_io_accept(io, transport_cb, setup->stream, NULL, > > + &err)) { > > error("bt_io_accept: %s", err->message); > > g_error_free(err); > > goto drop; > > diff --git a/profiles/audio/avdtp.c b/profiles/audio/avdtp.c > > index 782268c08..74b9f8dfa 100644 > > --- a/profiles/audio/avdtp.c > > +++ b/profiles/audio/avdtp.c > > @@ -366,6 +366,8 @@ struct avdtp_stream { > > GSList *caps; > > GSList *callbacks; > > struct avdtp_service_capability *codec; > > + void *pending_open_data; /* Data when the transport channel > > + * opening is pending */ > > guint io_id; /* Transport GSource ID */ > > guint timer; /* Waiting for other side to close or open > > * the transport channel */ > > @@ -727,6 +729,11 @@ static void stream_free(void *data) > > g_slist_free_full(stream->callbacks, g_free); > > g_slist_free_full(stream->caps, g_free); > > > > + /* pending_open_data must have been unref-ed and unset before freeing > > + * avdtp_stream. Otherwise, it is a reference leak bug. > > + */ > > + assert(!stream->pending_open_data); > > + > > g_free(stream); > > } > > > > @@ -3147,6 +3154,16 @@ struct avdtp_remote_sep *avdtp_stream_get_remote_sep( > > return NULL; > > } > > > > +void avdtp_stream_set_pending_open_data(struct avdtp_stream *stream, void *data) > > +{ > > + stream->pending_open_data = data; > > +} > > + > > +void *avdtp_stream_get_pending_open_data(struct avdtp_stream *stream) > > +{ > > + return stream->pending_open_data; > > +} > > + > > gboolean avdtp_stream_set_transport(struct avdtp_stream *stream, int fd, > > size_t imtu, size_t omtu) > > { > > diff --git a/profiles/audio/avdtp.h b/profiles/audio/avdtp.h > > index 011fea89e..92ce65ed3 100644 > > --- a/profiles/audio/avdtp.h > > +++ b/profiles/audio/avdtp.h > > @@ -260,6 +260,9 @@ gboolean avdtp_stream_has_capabilities(struct avdtp_stream *stream, > > GSList *caps); > > struct avdtp_remote_sep *avdtp_stream_get_remote_sep( > > struct avdtp_stream *stream); > > +void avdtp_stream_set_pending_open_data(struct avdtp_stream *stream, > > + void *data); > > +void *avdtp_stream_get_pending_open_data(struct avdtp_stream *stream); > > > > unsigned int avdtp_add_state_cb(struct btd_device *dev, > > avdtp_session_state_cb cb, void *user_data); > > -- > > 2.26.2 > > > > > -- > Luiz Augusto von Dentz
diff --git a/profiles/audio/a2dp.c b/profiles/audio/a2dp.c index 2feea66c0..9e96f8c4b 100644 --- a/profiles/audio/a2dp.c +++ b/profiles/audio/a2dp.c @@ -927,10 +927,16 @@ static gboolean open_ind(struct avdtp *session, struct avdtp_local_sep *sep, else DBG("Source %p: Open_Ind", sep); + if (avdtp_stream_get_pending_open_data(stream)) { + warn("Pending open data already exists"); + return FALSE; + } + setup = a2dp_setup_get(session); if (!setup) return FALSE; + avdtp_stream_set_pending_open_data(stream, setup); setup->stream = stream; if (!err && setup->chan) @@ -1285,14 +1291,13 @@ static void abort_cfm(struct avdtp *session, struct avdtp_local_sep *sep, void *user_data) { struct a2dp_sep *a2dp_sep = user_data; - struct a2dp_setup *setup; + struct a2dp_setup *setup = avdtp_stream_get_pending_open_data(stream); if (a2dp_sep->type == AVDTP_SEP_TYPE_SINK) DBG("Sink %p: Abort_Cfm", sep); else DBG("Source %p: Abort_Cfm", sep); - setup = find_setup_by_session(session); if (!setup) return; @@ -1302,6 +1307,7 @@ static void abort_cfm(struct avdtp *session, struct avdtp_local_sep *sep, } setup_unref(setup); + avdtp_stream_set_pending_open_data(stream, NULL); } static gboolean reconf_ind(struct avdtp *session, struct avdtp_local_sep *sep, @@ -2216,11 +2222,12 @@ fail: static void transport_cb(GIOChannel *io, GError *err, gpointer user_data) { - struct a2dp_setup *setup = user_data; + struct avdtp_stream *stream = user_data; + struct a2dp_setup *setup = avdtp_stream_get_pending_open_data(stream); uint16_t omtu, imtu; - if (!g_slist_find(setups, setup)) { - warn("bt_io_accept: setup %p no longer valid", setup); + if (!setup) { + warn("transport_cb: pending open data does not exist"); g_io_channel_shutdown(io, TRUE, NULL); return; } @@ -2238,8 +2245,7 @@ static void transport_cb(GIOChannel *io, GError *err, gpointer user_data) goto drop; } - if (!avdtp_stream_set_transport(setup->stream, - g_io_channel_unix_get_fd(io), + if (!avdtp_stream_set_transport(stream, g_io_channel_unix_get_fd(io), imtu, omtu)) goto drop; @@ -2249,6 +2255,7 @@ static void transport_cb(GIOChannel *io, GError *err, gpointer user_data) setup->io = NULL; setup_unref(setup); + avdtp_stream_set_pending_open_data(stream, NULL); return; @@ -2297,7 +2304,8 @@ static void confirm_cb(GIOChannel *io, gpointer data) goto drop; } - if (!bt_io_accept(io, transport_cb, setup, NULL, &err)) { + if (!bt_io_accept(io, transport_cb, setup->stream, NULL, + &err)) { error("bt_io_accept: %s", err->message); g_error_free(err); goto drop; diff --git a/profiles/audio/avdtp.c b/profiles/audio/avdtp.c index 782268c08..74b9f8dfa 100644 --- a/profiles/audio/avdtp.c +++ b/profiles/audio/avdtp.c @@ -366,6 +366,8 @@ struct avdtp_stream { GSList *caps; GSList *callbacks; struct avdtp_service_capability *codec; + void *pending_open_data; /* Data when the transport channel + * opening is pending */ guint io_id; /* Transport GSource ID */ guint timer; /* Waiting for other side to close or open * the transport channel */ @@ -727,6 +729,11 @@ static void stream_free(void *data) g_slist_free_full(stream->callbacks, g_free); g_slist_free_full(stream->caps, g_free); + /* pending_open_data must have been unref-ed and unset before freeing + * avdtp_stream. Otherwise, it is a reference leak bug. + */ + assert(!stream->pending_open_data); + g_free(stream); } @@ -3147,6 +3154,16 @@ struct avdtp_remote_sep *avdtp_stream_get_remote_sep( return NULL; } +void avdtp_stream_set_pending_open_data(struct avdtp_stream *stream, void *data) +{ + stream->pending_open_data = data; +} + +void *avdtp_stream_get_pending_open_data(struct avdtp_stream *stream) +{ + return stream->pending_open_data; +} + gboolean avdtp_stream_set_transport(struct avdtp_stream *stream, int fd, size_t imtu, size_t omtu) { diff --git a/profiles/audio/avdtp.h b/profiles/audio/avdtp.h index 011fea89e..92ce65ed3 100644 --- a/profiles/audio/avdtp.h +++ b/profiles/audio/avdtp.h @@ -260,6 +260,9 @@ gboolean avdtp_stream_has_capabilities(struct avdtp_stream *stream, GSList *caps); struct avdtp_remote_sep *avdtp_stream_get_remote_sep( struct avdtp_stream *stream); +void avdtp_stream_set_pending_open_data(struct avdtp_stream *stream, + void *data); +void *avdtp_stream_get_pending_open_data(struct avdtp_stream *stream); unsigned int avdtp_add_state_cb(struct btd_device *dev, avdtp_session_state_cb cb, void *user_data);
Currently transport_cb and abort_cfm make assumption that they have an a2dp_setup reference held as a result of open_ind invocation. In the field this is not always true, for example when the peer device opens an L2CAP channel for AVDTP transport channel without sending AVDTP_OPEN request through the AVDTP signaling channel first. Although in this case the peer device does not behave correctly, we should protect this possible crash from happening by making sure that transport_cb and abort_cfm are really holding a reference of a2dp_setup object before trying to unref them. After grabbing a reference, open_ind stores the pointer in stream->pending_open_data. If this field is set, that means there is a pending AVDTP_OPEN command and it needs to be unref-fed later once and only once: when the transport channel is created (transport_cb) or when the AVDTP_OPEN command is aborted (abort_cfm). If this field is not set, nothing should unref it. This enforces that the reference counting is correct regardless of the behavior of the peer device. A sample crash stack trace from Chrome OS: * thread #1, stop reason = signal SIGSEGV * frame #0: 0x0c64f0e8 bluetoothd`queue_remove_all at queue.c:351 frame #1: 0x0c64f086 bluetoothd`queue_destroy at queue.c:73 frame #2: 0x0c6022b0 bluetoothd`setup_unref at a2dp.c:222 frame #3: 0x0c604942 bluetoothd`transport_cb at a2dp.c:2229 frame #4: 0x0c61e35c bluetoothd`accept_cb at btio.c:203 frame #5: 0xf679523c libglib-2.0.so.0`g_main_context_dispatch at gmain.c:3182 frame #6: 0xf67954aa libglib-2.0.so.0`g_main_context_iterate at gmain.c:3920 frame #7: 0xf679569a libglib-2.0.so.0`g_main_loop_run at gmain.c:4116 frame #8: 0x0c65a5a0 bluetoothd`mainloop_run at mainloop-glib.c:79 frame #9: 0x0c65a7ea bluetoothd`mainloop_run_with_signal at mainloop-notify.c:201 frame #10: 0x0c6477ec bluetoothd`main at main.c:772 frame #11: 0xf65bc0a2 libc.so.6`__libc_start_main at libc-start.c:308 --- profiles/audio/a2dp.c | 24 ++++++++++++++++-------- profiles/audio/avdtp.c | 17 +++++++++++++++++ profiles/audio/avdtp.h | 3 +++ 3 files changed, 36 insertions(+), 8 deletions(-)