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

[Bartosz Golaszewski <brgl@bgdev.pl>]

On Thu, Feb 24, 2022 at 2:15 AM Joel Stanley <joel@jms.id.au> wrote:
>
> On Fri, 18 Feb 2022 at 18:42, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> >
> > On Fri, Feb 18, 2022 at 7:38 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > >
> > > On Wed, Feb 2, 2022 at 12:42 PM Joel Stanley <joel@jms.id.au> 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@jms.id.au>
> > > > ---
> > > >  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.
>

I know and I'm sorry. I admit that this is not the best time to try to
get new features in.

> 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?

I can't just yet. I want to keep the codebase bisectable and only
merge the new API into master once it's complete for the C, C++ and
Python parts. The branch called next/libgpiod-2.0 contains the WIP
changes but they are not complete yet. I just posted the test suite
for C and plan on posting the complete C++ bindings soon.

In fact - we discussed it with Kent and Linus and I expect to be able
to release the v2 in around two months and merge the new API into
master in a month.

You can base your work on next/libgpiod-2.0 but could you just hold
off the new features until after the new API is in master?

Thank you for your understanding,
Bart