Re: [BlueZ, v2 1/3] avrcp: Split off name parsing from parse_*_element()

Hi Bastien,

On Tue, Aug 4, 2026 at 10:31 AM Bastien Nocera <[email protected]> wrote:
>
> This will allow us to use the name extraction code in
> parse_media_element() and parse_folder_element() separately, such
> as in tests.
> ---
>  Makefile.plugins             |  1 +
>  profiles/audio/avrcp-parse.c | 47 ++++++++++++++++++++++++++++++++++++
>  profiles/audio/avrcp-parse.h | 19 +++++++++++++++
>  profiles/audio/avrcp.c       | 26 +++++---------------
>  4 files changed, 73 insertions(+), 20 deletions(-)
>  create mode 100644 profiles/audio/avrcp-parse.c
>  create mode 100644 profiles/audio/avrcp-parse.h
>
> diff --git a/Makefile.plugins b/Makefile.plugins
> index ac667beda847..a505fcd6691f 100644
> --- a/Makefile.plugins
> +++ b/Makefile.plugins
> @@ -37,6 +37,7 @@ builtin_modules += avrcp
>  builtin_sources += profiles/audio/control.h profiles/audio/control.c \
>                         profiles/audio/avctp.h profiles/audio/avctp.c \
>                         profiles/audio/avrcp.h profiles/audio/avrcp.c \
> +                       profiles/audio/avrcp-parse.h profiles/audio/avrcp-parse.c \
>                         profiles/audio/avrcp-player.c
>  endif
>
> diff --git a/profiles/audio/avrcp-parse.c b/profiles/audio/avrcp-parse.c
> new file mode 100644
> index 000000000000..d3d0a070a4da
> --- /dev/null
> +++ b/profiles/audio/avrcp-parse.c
> @@ -0,0 +1,47 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + *
> + *  BlueZ - Bluetooth protocol stack for Linux
> + *
> + *  Copyright (C) 2026  Red Hat Inc.
> + *
> + *
> + */
> +
> +#include "avrcp-parse.h"
> +#include "src/shared/util.h"
> +
> +gboolean parse_media_element_name(uint8_t *operands, uint16_t len,
> +                                        char *name, uint16_t *namesize)
> +{
> +       uint16_t namelen;
> +
> +       if (len < 13)
> +               return FALSE;
> +
> +       memset(name, 0, NAME_MAX_LEN);
> +       *namesize = get_be16(&operands[11]);
> +       namelen = MIN(*namesize, NAME_MAX_LEN - 1);
> +       if (namelen > 0) {
> +               memcpy(name, &operands[13], namelen);
> +               strtoutf8(name, namelen);
> +       }
> +
> +       return TRUE;
> +}
> +
> +gboolean parse_media_folder_name(uint8_t *operands, uint16_t len,
> +                                       char *name)
> +{
> +       uint16_t namelen;
> +
> +       if (len < 12)
> +               return FALSE;
> +
> +       memset(name, 0, NAME_MAX_LEN);
> +       namelen = MIN(get_be16(&operands[12]), NAME_MAX_LEN - 1);
> +       if (namelen > 0)
> +               memcpy(name, &operands[14], namelen);
> +
> +       return TRUE;
> +}

Rather than creating yet another file how about hosting this under
shared/util.h directly? It already depends on it anyway, we could got
with something like strntoutf8 or a similar function that checks the
length, etc, actually be maybe better to do it under
util_iov_pull_utf8(iov, len, str, str_max_len) so we can load the pdu
into the iov then use iov_pull_mem, etc, to verify that we have enough
bytes in a generic manner.

> diff --git a/profiles/audio/avrcp-parse.h b/profiles/audio/avrcp-parse.h
> new file mode 100644
> index 000000000000..f7a33c854442
> --- /dev/null
> +++ b/profiles/audio/avrcp-parse.h
> @@ -0,0 +1,19 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + *
> + *  BlueZ - Bluetooth protocol stack for Linux
> + *
> + *  Copyright (C) 2026  Red Hat Inc.
> + *
> + *
> + */
> +
> +#include <glib.h>
> +#include <inttypes.h>
> +
> +#define NAME_MAX_LEN 255
> +
> +gboolean parse_media_element_name(uint8_t *operands, uint16_t len,
> +                                        char *name, uint16_t *namesize);
> +gboolean parse_media_folder_name(uint8_t *operands, uint16_t len,
> +                                       char *name);
> diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
> index 2194a913580f..af3c72174764 100644
> --- a/profiles/audio/avrcp.c
> +++ b/profiles/audio/avrcp.c
> @@ -52,6 +52,7 @@
>
>  #include "avctp.h"
>  #include "avrcp.h"
> +#include "avrcp-parse.h"
>  #include "control.h"
>  #include "media.h"
>  #include "player.h"
> @@ -2614,24 +2615,15 @@ static struct media_item *parse_media_element(struct avrcp *session,
>         struct avrcp_player *player;
>         struct media_player *mp;
>         struct media_item *item;
> -       uint16_t namelen, namesize;
> -       char name[255];
> +       uint16_t namesize;
> +       char name[NAME_MAX_LEN];
>         uint64_t uid;
>         uint8_t count;
>
> -       if (len < 13)
> +       if (!parse_media_element_name(operands, len, name, &namesize))
>                 return NULL;
>
>         uid = get_be64(&operands[0]);
> -
> -       memset(name, 0, sizeof(name));
> -       namesize = get_be16(&operands[11]);
> -       namelen = MIN(namesize, sizeof(name) - 1);
> -       if (namelen > 0) {
> -               memcpy(name, &operands[13], namelen);
> -               strtoutf8(name, namelen);
> -       }
> -
>         count = operands[13 + namesize];
>
>         player = session->controller->player;
> @@ -2655,24 +2647,18 @@ static struct media_item *parse_media_folder(struct avrcp *session,
>         struct avrcp_player *player = session->controller->player;
>         struct media_player *mp = player->user_data;
>         struct media_item *item;
> -       uint16_t namelen;
> -       char name[255];
> +       char name[NAME_MAX_LEN];
>         uint64_t uid;
>         uint8_t type;
>         uint8_t playable;
>
> -       if (len < 12)
> +       if (!parse_media_folder_name(operands, len, name))
>                 return NULL;
>
>         uid = get_be64(&operands[0]);
>         type = operands[8];
>         playable = operands[9];
>
> -       memset(name, 0, sizeof(name));
> -       namelen = MIN(get_be16(&operands[12]), sizeof(name) - 1);
> -       if (namelen > 0)
> -               memcpy(name, &operands[14], namelen);
> -
>         item = media_player_create_folder(mp, name, type, uid);
>         if (!item)
>                 return NULL;
> --
> 2.55.0
>
>


-- 
Luiz Augusto von Dentz