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

[Kent Gibson <warthog618@xxxxxxxxx>]

On Thu, Feb 24, 2022 at 03:50:18PM +0100, Bartosz Golaszewski wrote:
> On Thu, Feb 24, 2022 at 2:15 AM Joel Stanley <joel@xxxxxxxxx> wrote:
> >
> > 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.
> >
> 
> 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?
> 

I'm thinking that we should be re-visting the tools as part of the
switch to libgpiod v2, as a major version bump is our only opportunity
to make sweeping changes.

I have to admit I was not initially in favour of the by-name option, as
it is hideously inefficient compared to the offset version.  What was 
one or two ioctl calls could now be dozens, if not more.
And the thought of that happening everytime a user wants to toggle a
single line makes my skin crawl.

However, in light of our recent discussions, I think we need it as an
option.  But I would prefer to revise the tool command lines so lines
can be identified by name or offset.  The named option should be the
simplest, and so not require a --by-name flag.
My current thinking is that the chip should become an optional arg,
rather than a positional arg.
So [-c <chip>] <line>...
e.g.
    gpioset GPIO17=active GPIO22=1
or
    gpioset -c gpiochip0 17=1 LED=off

similarly get:

    gpioget GPIO17 GPIO22
or
    gpioget -c gpiochip0 17 LED

If the chip is not specified then the line identifier must be a name.
If the chip is specified then the line identifier is interpreted as an
offset if it parses as an int, else a name.
Either way, if multiple lines are provided then they must be on the one
chip.
That all hinges on the assumption that line names are never simply
stringified integers, or at least if they are then it matches the
offset.  Is that viable?

The sets should also accept a set of true/false variants, such as the
on/off, active/inactive in the examples above.
The gets should return active/inactive to make it clear they refer to
logical values, not physical values.

I am also wondering why the tools are separate, instead of being merged
into a single tool.  Is there a reason for that?

I've got a bunch of other minor changes that I've been trialing in my
Rust library.  So I have a working prototype of the set --interactive
that I had mentioned.  I scrapped the batch option - it doesn't
add much that the interactive mode and a named pipe doesn't already give
you.

But I digress.  The main thing I want to achieve here is to determine
where you want to go with the tools for v2, and what any contraints
might be.  Then we can take it from there.

Cheers,
Kent.