Re: [libgpiod PATCH] core: Fix line_bulk_foreach_line invalid memory access

[Joel Stanley <joel@xxxxxxxxx>]

On Fri, 18 Feb 2022 at 18:42, Bartosz Golaszewski <brgl@xxxxxxxx> wrote:
>
> On Fri, Feb 18, 2022 at 7:38 PM Bartosz Golaszewski <brgl@xxxxxxxx> wrote:
> >
> > On Wed, Feb 2, 2022 at 12:42 PM Joel Stanley <joel@xxxxxxxxx> wrote:
> > >
> > > Running libgpiod applications under valgrind results in the following
> > > warning:
> > >
> > > ==3006== Invalid read of size 8
> > > ==3006==    at 0x10C867: line_request_values (core.c:711)
> > > ==3006==    by 0x10CDA6: gpiod_line_request_bulk (core.c:849)
> > > ==3006==    by 0x10AE27: main (gpioset.c:323)
> > > ==3006==  Address 0x4a4d370 is 0 bytes after a block of size 16 alloc'd
> > > ==3006==    at 0x483F790: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
> > > ==3006==    by 0x10B884: gpiod_line_bulk_new (core.c:109)
> > > ==3006==    by 0x10DBB0: gpiod_chip_get_lines (helpers.c:24)
> > > ==3006==    by 0x10ADC3: main (gpioset.c:313)
> > >
> > > This is because the foreach loop reads the next value before checking
> > > that index is still in bounds.
> > >
> > > Add a test to avoid reading past the end of the allocation.
> > >
> > > This bug is not present a released version of libgpiod.
> > >
> > > Fixes: 2b02d7ae1aa6 ("treewide: rework struct gpiod_line_bulk")
> > > Signed-off-by: Joel Stanley <joel@xxxxxxxxx>
> > > ---
> > >  lib/core.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/lib/core.c b/lib/core.c
> > > index 6ef09baec0f5..4463a7014776 100644
> > > --- a/lib/core.c
> > > +++ b/lib/core.c
> > > @@ -178,7 +178,8 @@ GPIOD_API void gpiod_line_bulk_foreach_line(struct gpiod_line_bulk *bulk,
> > >  #define line_bulk_foreach_line(bulk, line, index)                      \
> > >         for ((index) = 0, (line) = (bulk)->lines[0];                    \
> > >              (index) < (bulk)->num_lines;                               \
> > > -            (index)++, (line) = (bulk)->lines[(index)])
> > > +            (index)++,                                                 \
> > > +            (line) = (index) < (bulk)->num_lines ? (bulk)->lines[(index)] : NULL)
> > >
> > >  GPIOD_API bool gpiod_is_gpiochip_device(const char *path)
> > >  {
> > > --
> > > 2.34.1
> > >
> >
> > I'll skip this because this entire struct is going away in v2 and the
> > bug is not present in v1.6.x.
> >
> > Bart
>
> Ugh actually all three patches fix issues in the master branch that
> have never been nor will be released.
>
> I'm not sure if I made myself clear on that - the changes in the
> master branch are going away and the de facto new API is in
> next/libgpiod-2.0. I already pushed the other two so I'll leave them
> there but please take a look at the next branch so that you know how
> the upcoming API will work. That's also applicable to the patches
> adding the by-name option to the tools - I think it would be better to
> base them on that branch right away.

That's a bit frustrating.

Perhaps you could make the master branch contain the code you're
working on (instead of next), if you plan on abandoning the current
code base?