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

[Bartosz Golaszewski <brgl@xxxxxxxx>]

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.

Bart