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

On Thu, 2026-08-06 at 10:19 -0400, Luiz Augusto von Dentz wrote:
> Hi Bastien,
> 
> On Thu, Aug 6, 2026 at 5:59 AM Bastien Nocera <[email protected]>
> wrote:
> > 
> > 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 sounds like some pretty big code changes that are beyond what I
have the time to do right now.

> > 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.
> 
> I don't think this is the best approach because test-avrcp is testing
> the wrong implementation, so that needs rework.
> 
>  I'd go straight to
> shared/util helper function which would allow us to create test cases
> immediately without changing a bunch of things and removing
> test-avrcp.c in the process.

But then you're not actually testing that your fix actually fixed the
security issue / bug...

I'll re-send a better version of the fix without tests.

Cheers