On Friday 03 May 2019 11:55:38 Luiz Augusto von Dentz wrote:
> Hi Pali,
>
> On Fri, May 3, 2019 at 12:10 AM Pali Rohár <pali.rohar@xxxxxxxxx> wrote:
> >
> > On Thursday 02 May 2019 22:58:36 Pali Rohár wrote:
> > > On Thursday 02 May 2019 22:53:49 Pali Rohár wrote:
> > > > On Thursday 02 May 2019 22:41:55 Pali Rohár wrote:
> > > > > On Monday 29 April 2019 18:39:16 Pali Rohár wrote:
> > > > > > On Monday 29 April 2019 19:35:24 Luiz Augusto von Dentz wrote:
> > > > > > > Hi Pali,
> > > > > > >
> > > > > > > On Mon, Apr 29, 2019 at 3:03 PM Luiz Augusto von Dentz
> > > > > > > <luiz.dentz@xxxxxxxxx> wrote:
> > > > > > > >
> > > > > > > > From: Luiz Augusto von Dentz <luiz.von.dentz@xxxxxxxxx>
> > > > > > > >
> > > > > > > > Endpoint may not be able to select a valid configuration but there could
> > > > > > > > be other endpoints available that could be used, so instead of just
> > > > > > > > using the first match this collects all the matching endpoints and put
> > > > > > > > them into a queue (ordered by priority) then proceed to next endpoint
> > > > > > > > only failing if none of the available endpoits can select a valid
> > > > > > > > configuration.
> > > > > > > > ---
> > > > > > > > profiles/audio/a2dp.c | 77 ++++++++++++++++++++++++++++---------------
> > > > > > > > 1 file changed, 50 insertions(+), 27 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/profiles/audio/a2dp.c b/profiles/audio/a2dp.c
> > > > > > > > index a23abdd97..4d378a91a 100644
> > > > > > > > --- a/profiles/audio/a2dp.c
> > > > > > > > +++ b/profiles/audio/a2dp.c
> > > > > > > > @@ -105,6 +105,7 @@ struct a2dp_setup_cb {
> > > > > > > > struct a2dp_setup {
> > > > > > > > struct a2dp_channel *chan;
> > > > > > > > struct avdtp *session;
> > > > > > > > + struct queue *eps;
> > > > > > > > struct a2dp_sep *sep;
> > > > > > > > struct a2dp_remote_sep *rsep;
> > > > > > > > struct avdtp_stream *stream;
> > > > > > > > @@ -2406,23 +2407,44 @@ void a2dp_remove_sep(struct a2dp_sep *sep)
> > > > > > > >
> > > > > > > > static void select_cb(struct a2dp_setup *setup, void *ret, int size)
> > > > > > > > {
> > > > > > > > - if (size < 0) {
> > > > > > > > - DBG("Endpoint replied an invalid configuration");
> > > > > > > > + struct avdtp_service_capability *service;
> > > > > > > > + struct avdtp_media_codec_capability *codec;
> > > > > > > > + int err;
> > > > > > > > +
> > > > > > > > + if (size) {
> > > > > > > > + caps_add_codec(&setup->caps, setup->sep->codec, ret, size);
> > > > > > > > goto done;
> > > > > > > > }
> > > > > > > >
> > > > > > > > - caps_add_codec(&setup->caps, setup->sep->codec, ret, size);
> > > > > > > > + setup->sep = queue_pop_head(setup->eps);
> > > > > > > > + if (!setup->sep) {
> > > > > > > > + error("Unable to select a valid configuration");
> > > > > > > > + queue_destroy(setup->eps, NULL);
> > > > > > > > + goto done;
> > > > > > > > + }
> > > > > > > > +
> > > > > > > > + setup->rsep = find_remote_sep(setup->chan, setup->sep);
> > > > > > > > + service = avdtp_get_codec(setup->rsep->sep);
> > > > > > > > + codec = (struct avdtp_media_codec_capability *) service->data;
> > > > > > > > +
> > > > > > > > + err = setup->sep->endpoint->select_configuration(setup->sep,
> > > > > > > > + codec->data,
> > > > > > > > + service->length - sizeof(*codec),
> > > > > > > > + setup_ref(setup),
> > > > > > > > + select_cb, setup->sep->user_data);
> > > > > > > > + if (err == 0)
> > > > > > > > + return;
> > > > > > > >
> > > > > > > > done:
> > > > > > > > finalize_select(setup);
> > > > > > > > setup_unref(setup);
> > > > > > > > }
> > > > > > > >
> > > > > > > > -static struct a2dp_sep *a2dp_find_sep(struct avdtp *session, GSList *list,
> > > > > > > > +static struct queue *a2dp_find_eps(struct avdtp *session, GSList *list,
> > > > > > > > const char *sender)
> > > > > > > > {
> > > > > > > > - struct a2dp_sep *first = NULL;
> > > > > > > > struct a2dp_channel *chan = find_channel(session);
> > > > > > > > + struct queue *seps = NULL;
> > > > > > > >
> > > > > > > > for (; list; list = list->next) {
> > > > > > > > struct a2dp_sep *sep = list->data;
> > > > > > > > @@ -2444,26 +2466,25 @@ static struct a2dp_sep *a2dp_find_sep(struct avdtp *session, GSList *list,
> > > > > > > > if (!rsep)
> > > > > > > > continue;
> > > > > > > >
> > > > > > > > - /* Locate last used if set */
> > > > > > > > - if (chan->last_used) {
> > > > > > > > - if (chan->last_used->sep == rsep)
> > > > > > > > - return sep;
> > > > > > > > - first = sep;
> > > > > > > > - continue;
> > > > > > > > - }
> > > > > > > > + if (!seps)
> > > > > > > > + seps = queue_new();
> > > > > > > >
> > > > > > > > - return sep;
> > > > > > > > + /* Prepend last used so it is preferred over others */
> > > > > > > > + if (chan->last_used && chan->last_used->sep == rsep)
> > > > > > > > + queue_push_head(seps, sep);
> > > > > > > > + else
> > > > > > > > + queue_push_tail(seps, sep);
> > > > > > > >
> > > > > > > > }
> > > > > > > >
> > > > > > > > - return first;
> > > > > > > > + return seps;
> > > > > > > > }
> > > > > > > >
> > > > > > > > -static struct a2dp_sep *a2dp_select_sep(struct avdtp *session, uint8_t type,
> > > > > > > > +static struct queue *a2dp_select_eps(struct avdtp *session, uint8_t type,
> > > > > > > > const char *sender)
> > > > > > > > {
> > > > > > > > struct a2dp_server *server;
> > > > > > > > - struct a2dp_sep *sep;
> > > > > > > > + struct queue *seps;
> > > > > > > > GSList *l;
> > > > > > > >
> > > > > > > > server = find_server(servers, avdtp_get_adapter(session));
> > > > > > > > @@ -2473,11 +2494,11 @@ static struct a2dp_sep *a2dp_select_sep(struct avdtp *session, uint8_t type,
> > > > > > > > l = type == AVDTP_SEP_TYPE_SINK ? server->sources : server->sinks;
> > > > > > > >
> > > > > > > > /* Check sender's seps first */
> > > > > > > > - sep = a2dp_find_sep(session, l, sender);
> > > > > > > > - if (sep != NULL)
> > > > > > > > - return sep;
> > > > > > > > + seps = a2dp_find_eps(session, l, sender);
> > > > > > > > + if (seps != NULL)
> > > > > > > > + return seps;
> > > > > > > >
> > > > > > > > - return a2dp_find_sep(session, l, NULL);
> > > > > > > > + return a2dp_find_eps(session, l, NULL);
> > > > > > > > }
> > > > > > > >
> > > > > > > > static void store_remote_sep(void *data, void *user_data)
> > > > > > > > @@ -2580,13 +2601,13 @@ unsigned int a2dp_select_capabilities(struct avdtp *session,
> > > > > > > > {
> > > > > > > > struct a2dp_setup *setup;
> > > > > > > > struct a2dp_setup_cb *cb_data;
> > > > > > > > - struct a2dp_sep *sep;
> > > > > > > > + struct queue *eps;
> > > > > > > > struct avdtp_service_capability *service;
> > > > > > > > struct avdtp_media_codec_capability *codec;
> > > > > > > > int err;
> > > > > > > >
> > > > > > > > - sep = a2dp_select_sep(session, type, sender);
> > > > > > > > - if (!sep) {
> > > > > > > > + eps = a2dp_select_eps(session, type, sender);
> > > > > > > > + if (!eps) {
> > > > > > > > error("Unable to select SEP");
> > > > > > > > return 0;
> > > > > > > > }
> > > > > > > > @@ -2599,8 +2620,9 @@ unsigned int a2dp_select_capabilities(struct avdtp *session,
> > > > > > > > cb_data->select_cb = cb;
> > > > > > > > cb_data->user_data = user_data;
> > > > > > > >
> > > > > > > > - setup->sep = sep;
> > > > > > > > - setup->rsep = find_remote_sep(setup->chan, sep);
> > > > > > > > + setup->eps = eps;
> > > > > > > > + setup->sep = queue_pop_head(eps);
> > > > > > > > + setup->rsep = find_remote_sep(setup->chan, setup->sep);
> > > > > > > >
> > > > > > > > if (setup->rsep == NULL) {
> > > > > > > > error("Could not find remote sep");
> > > > > > > > @@ -2610,10 +2632,11 @@ unsigned int a2dp_select_capabilities(struct avdtp *session,
> > > > > > > > service = avdtp_get_codec(setup->rsep->sep);
> > > > > > > > codec = (struct avdtp_media_codec_capability *) service->data;
> > > > > > > >
> > > > > > > > - err = sep->endpoint->select_configuration(sep, codec->data,
> > > > > > > > + err = setup->sep->endpoint->select_configuration(setup->sep,
> > > > > > > > + codec->data,
> > > > > > > > service->length - sizeof(*codec),
> > > > > > > > setup_ref(setup),
> > > > > > > > - select_cb, sep->user_data);
> > > > > > > > + select_cb, setup->sep->user_data);
> > > > > > > > if (err == 0)
> > > > > > > > return cb_data->id;
> > > > > > > >
> > > > > > > > --
> > > > > > > > 2.20.1
> > > > > > >
> > > > > > > Le me know if you find any problem with this change, my headsets seems
> > > > > > > to always succeed the first try so I cannot really reproduce the
> > > > > > > problem.
> > > > > >
> > > > > > Ok, I will try it in next days. I can register PA endpoints which always
> > > > > > "fails" so I can test if this is working correctly as expected.
> > > > >
> > > > > Nope, this your patch does not work. It cause segfaulting of bluetoothd
> > > > > daemon itself. Here is stacktrace of current git master branch (where is
> > > > > your patch too):
> > > > >
> > > > > Program received signal SIGSEGV, Segmentation fault.
> > > > > __memmove_avx_unaligned_erms () at ../sysdeps/x86_64/multiarch/memmove-vec-unaligned-erms.S:416
> > > > > 416 ../sysdeps/x86_64/multiarch/memmove-vec-unaligned-erms.S: Adresár alebo súbor neexistuje.
> > > > > (gdb) bt
> > > > > #0 __memmove_avx_unaligned_erms () at ../sysdeps/x86_64/multiarch/memmove-vec-unaligned-erms.S:416
> > > > > #1 0x0000557d2d2f3164 in caps_add_codec (l=0x557d2f1a9708, codec=<optimized out>, caps=0x0, size=18446744073709551615) at profiles/audio/a2dp.c:1519
> > > > > #2 0x0000557d2d2f626a in select_cb (setup=0x557d2f1a96c0, ret=<optimized out>, size=<optimized out>) at profiles/audio/a2dp.c:2422
> > > > > #3 0x0000557d2d2fe016 in endpoint_reply (call=<optimized out>, user_data=0x557d2f187810) at profiles/audio/media.c:316
> > > > > #4 0x00007fd2dc6aa002 in ?? () from /lib/x86_64-linux-gnu/libdbus-1.so.3
> > > > > #5 0x00007fd2dc6ada5f in dbus_connection_dispatch () from /lib/x86_64-linux-gnu/libdbus-1.so.3
> > > > > #6 0x0000557d2d36f320 in message_dispatch (data=0x557d2f184560) at gdbus/mainloop.c:72
> > > > > #7 0x00007fd2dc9326aa in g_main_context_dispatch () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
> > > > > #8 0x00007fd2dc932a60 in ?? () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
> > > > > #9 0x00007fd2dc932d82 in g_main_loop_run () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
> > > > > #10 0x0000557d2d384a45 in mainloop_run () at src/shared/mainloop-glib.c:79
> > > > > #11 0x0000557d2d384e0f in mainloop_run_with_signal (func=<optimized out>, user_data=0x0) at src/shared/mainloop-notify.c:201
> > > > > #12 0x0000557d2d2eb0a8 in main (argc=<optimized out>, argv=<optimized out>) at src/main.c:729
> > > > >
> > > > > In syslog for bluetoothd is this last line:
> > > > > bluetoothd[12605]: Endpoint replied with an error: org.bluez.Error.InvalidArguments
> > > > >
> > > > > pulseaudio refused only one SelectConfiguration call.
> > > >
> > > > In profiles/audio/a2dp.c is:
> > > >
> > > > static void select_cb(struct a2dp_setup *setup, void *ret, int size)
> > > > {
> > > > ...
> > > > if (size) {
> > > > caps_add_codec(&setup->caps, setup->sep->codec, ret, size);
> > > > ...
> > > >
> > > > and then there is:
> > > >
> > > > tatic void caps_add_codec(GSList **l, uint8_t codec, uint8_t *caps,
> > > > size_t size)
> > > > {
> > > > ...
> > > > memcpy(cap->data, caps, size);
> > > > ...
> > > >
> > > > When select_cb() is called with size=-1 then is calls caps_add_codec()
> > > > as size is non-zero and cast signed 32bit int -1 to unsigned 64bit int
> > > > as value 2^64-1. Later is called memcpy with size 2^64-1 and crash is
> > > > there. Moreover caps is NULLL pointer, therefore void *ret in select_cb
> > > > is NULL too. Seems like wrong error handling in select_cb() function.
> > >
> > > And seems that ret=NULL and size=-1 comes from media.c endpoint_reply()
> > > functions. As those values are defaults... Maybe it is a bug here?
> >
> > Here is patch which fixes above problem with crashing:
> >
> > diff --git a/profiles/audio/a2dp.c b/profiles/audio/a2dp.c
> > index d0676b577..b06eafae0 100644
> > --- a/profiles/audio/a2dp.c
> > +++ b/profiles/audio/a2dp.c
> > @@ -1703,7 +1703,7 @@ static DBusMessage *set_configuration(DBusConnection *conn, DBusMessage *msg,
> > if (dbus_message_iter_get_arg_type(&props) != DBUS_TYPE_DICT_ENTRY)
> > return btd_error_invalid_args(msg);
> >
> > - if (parse_properties(&props, &caps, &size) < 0)
> > + if (parse_properties(&props, &caps, &size) < 0 || size <= 0)
> > return btd_error_invalid_args(msg);
> >
> > err = a2dp_reconfig(chan, sender, lsep, rsep, caps, size,
> > @@ -2418,7 +2418,7 @@ static void select_cb(struct a2dp_setup *setup, void *ret, int size)
> > struct avdtp_media_codec_capability *codec;
> > int err;
> >
> > - if (size) {
> > + if (ret && size > 0) {
> > caps_add_codec(&setup->caps, setup->sep->codec, ret, size);
> > goto done;
> > }
>
> Ive sent a similar fix, there is also a patch to abort if stream
> cannot be closed when switching streams. Regarding the endpoint
> selection it seems to be working correctly at least it does choose the
> endpoints in the order they are registered.
Ok, seems that crashing with those 3 patches on ML is fixed.
--
Pali Rohár
pali.rohar@xxxxxxxxx
Attachment:
signature.asc
Description: PGP signature
