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.
--
Luiz Augusto von Dentz
