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

On Wed, 2026-08-05 at 13:12 -0400, Luiz Augusto von Dentz wrote:
> 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.

That would be nice follow-up work to be done, but this patch is
specifically about being able to test the out-of-bounds access caused
by those 2 portions of code in patch #2.

Then the fix is applied in patch #3.

Finally, we could optimise/clean this up and remove that parsing code,
but that would be in a 4th patch.