| Message ID | 20220202114204.31918-1-joel@jms.id.au |
|---|---|
| State | New |
| Headers | show |
| Series | [libgpiod] core: Fix line_bulk_foreach_line invalid memory access | expand |
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
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
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@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? > 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.
On Fri, Feb 25, 2022 at 4:08 PM Kent Gibson <warthog618@gmail.com> wrote: > [snip] > > > > > > 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. > Yes and no. I'm not very happy about making the very command-line users that we managed to convince to switch away from sysfs to using gpio-tools angry again with totally incompatible changes in v2. > 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? > We cannot make that assumptions and I would prefer to stay both compatible AND explicit here. As in: work with offsets by default and with names as an option. On the other hand - if we specify --by-name, I'm fine with skipping the chip parameter and let the program look up the line among all chips. > The sets should also accept a set of true/false variants, such as the > on/off, active/inactive in the examples above. Why though? What do we gain from accepting all kinds of different strings? IMO it just makes the interface less clear. > The gets should return active/inactive to make it clear they refer to > logical values, not physical values. > What's wrong with 1/0? > I am also wondering why the tools are separate, instead of being merged > into a single tool. Is there a reason for that? > You mean like busybox' single executable with multiple links under different names internally redirecting to different main() functions or really a single tool with multiple commands? The reason for having separate tools is that they really are tiny, the little code they share and statically link to is negligible and it's simply clearer as to which tool does what. I didn't want the tools to be this swiss-knife do-it-all program that requires studying the README for a long time. > 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. > gpioset --interactive is definitely something I'm interested in. > 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. > While the total overhaul of the library is understandable, I would prefer to keep the tools mostly backward compatible. I plan on adding gpiowatch for watching info events but that's it. Do you see any things that are obviously wrong in how the tools work that would justify the sweeping changing you mention? Bart > Cheers, > Kent.
On Fri, Feb 25, 2022 at 10:55:25PM +0100, Bartosz Golaszewski wrote: > On Fri, Feb 25, 2022 at 4:08 PM Kent Gibson <warthog618@gmail.com> wrote: > > > > [snip] > > > > > > > > > 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. > > > > Yes and no. I'm not very happy about making the very command-line > users that we managed to convince to switch away from sysfs to using > gpio-tools angry again with totally incompatible changes in v2. > Oh, I know - I've made that same point myself. But it cuts both ways - if you don't fix things now you never will. Make current users angry, or make future users confused and angry. Your call. Of course my prefered option is an interface that will satisfy both current and future users, but I seriously doubt we can get there if we maintain backward compatibility for the sake of current users. > > 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? > > > > We cannot make that assumptions and I would prefer to stay both > compatible AND explicit here. As in: work with offsets by default and > with names as an option. On the other hand - if we specify --by-name, > I'm fine with skipping the chip parameter and let the program look up > the line among all chips. > I'm thinking that most users would prefer to work with names. And that preference will only grow stronger over time. I'm starting to lean that way myself. So you are happy with positional chip by default, but optional when --by-name is set? That is going to make command line parsing even more fun, but ok. So your acceptible forms would be (using get for brevity): gpioget gpiochip0 16 17 23 gpioget --by-name GPIO16 GPIO17 GPIO23 gpioget --by-name -c gpiochip0 GPIO16 GPIO17 GPIO23 ? > > The sets should also accept a set of true/false variants, such as the > > on/off, active/inactive in the examples above. > > Why though? What do we gain from accepting all kinds of different > strings? IMO it just makes the interface less clear. > All kinds? I suggested two variants. That is all, not the whole thesaurus. I explicitly avoid 0/1 and low/high, cos line levels. Could be convinces to accept 0/1 for backward compatibility. Users can then use a nomenclature that best suits their application. That makes their scripts more readable. And much more meaningful than 0/1. I mean 0/1 is literally the least amount of information you could possibly provide. While the active-low option exists, setting line=1 will always be confusing. Dropping the active-low option for set would mean it operates with physical levels, while the other commands use logical, so don't really want to go there either.... (I have also wondered if having the active-low option in the uAPI is actually beneficial in general, but that bird has flown.) > > The gets should return active/inactive to make it clear they refer to > > logical values, not physical values. > > > > What's wrong with 1/0? > As stated, that can easily be misinterpreted as meaning physical line levels. And the logical and physical levels are not the same if active-low, and the get command gives no indication if the active-low is set or not. > > I am also wondering why the tools are separate, instead of being merged > > into a single tool. Is there a reason for that? > > > > You mean like busybox' single executable with multiple links under > different names internally redirecting to different main() functions > or really a single tool with multiple commands? > One advantage of a single tool is that when you add a new one, say watch, it just gets added to the tool and pops up in the tool help. So it aids discoverabilty. And adding commands doesn't automatically require an extra binary to be built and installed - just upgrading the one. The busy-box command-link approach is an option, with the link name determining the subcommand. Or it can be called directly with subcommands. e.g. gpiodctl set gpiochip0 16=0 Wrt backward compatibility, perhaps add an option to behave as per v1, so current users can just add that until they migrate to the v2 command line? When called directly it could use the v2 command line, but when called via a link, it could enable the v1 option and support the v1 command line. Or is that just confusing too? > The reason for having separate tools is that they really are tiny, the > little code they share and statically link to is negligible and it's > simply clearer as to which tool does what. I didn't want the tools to > be this swiss-knife do-it-all program that requires studying the > README for a long time. > Who needs the README - the tool help should be sufficient. If not then fix the help. They might be tiny, but collectively they can't be smaller than a unified executable. Are there cases where only a subset of commands is deployed? Though even that could be addressed with build options to drop subcommands from the tool. > > 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. > > > > gpioset --interactive is definitely something I'm interested in. > Yeah, it also solves most of my discomfort with the named option - it only does the find once and caches the offset for subsequent sets. The best of both worlds. Would adding a find ioctl to the uAPI be worthwhile? > > 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. > > > > While the total overhaul of the library is understandable, I would > prefer to keep the tools mostly backward compatible. I plan on adding > gpiowatch for watching info events but that's it. Do you see any > things that are obviously wrong in how the tools work that would > justify the sweeping changing you mention? > Other than the named mode... The set is the one that bothers me the most. The --mode is a bit confusing. Most of them map to the same thing, and I can get the same effect with --interactive and/or a hold period. (the hold period being a forced sleep after a set. That comes in handy in the interactive mode where you can just hammer out sets and the hold period defines the rate they will be applied, so blinking or even bit-bashing becomes super easy) Time periods should be a single option that includes the time units, e.g. -p 10ms is more readable than -u 10000000 (how many zeros is that??) And I'm pondering a toggle/blink mode, which would toggle the requested lines at the rate defined by the hold period. So a simple blinker would be: gpioset --by-name --blink -p 1s LED=on And a corresponding toggle command for the interactive mode. (which could specify a subset of lines to toggle, or toggle the whole set) If you want to retain backward compatibility then adding these features, without removing the old, will produce a cluttered, messy and really confusing gpioset interface. And that is the tool most likely to see use :(. Wrt gpioinfo, I'm not sure the quotes on the name and consumer in the output helps. And I'd restrict the chip option to a single chip, else all chips. No subset option, to be consistent with other tools and to allow the option of selecting the line or a subset of lines, either by offset or name. e.g. who is consuming the LED line? gpioinfo --by-name LED gpiofind should have a chip option to limit searching (names are only unique within chips, right, or is even that still not guaranteed?), and an option to provide the info for found lines - since we have it in hand anyway. gpiodetect could get a chip option, so only get the info for the chip of interest (to ignore those orphaned gpio-sims ;-). gpiomon should provide sequence numbers. Maybe an option to return the whole event in JSON? Anyway, in short, if backward compatibility is a requirement then I'm really not sure how best to proceed - except now I need another coffee. Cheers, Kent.
On Sat, Feb 26, 2022 at 5:01 AM Kent Gibson <warthog618@gmail.com> wrote: > > On Fri, Feb 25, 2022 at 10:55:25PM +0100, Bartosz Golaszewski wrote: > > On Fri, Feb 25, 2022 at 4:08 PM Kent Gibson <warthog618@gmail.com> wrote: > > > > > > > [snip] > > > > > > > > > > > > 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. > > > > > > > Yes and no. I'm not very happy about making the very command-line > > users that we managed to convince to switch away from sysfs to using > > gpio-tools angry again with totally incompatible changes in v2. > > > > Oh, I know - I've made that same point myself. > But it cuts both ways - if you don't fix things now you never will. > Make current users angry, or make future users confused and angry. > Your call. > > Of course my prefered option is an interface that will satisfy both > current and future users, but I seriously doubt we can get there if we > maintain backward compatibility for the sake of current users. > You opened a can of worms below. :) > > > 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? > > > > > > > We cannot make that assumptions and I would prefer to stay both > > compatible AND explicit here. As in: work with offsets by default and > > with names as an option. On the other hand - if we specify --by-name, > > I'm fine with skipping the chip parameter and let the program look up > > the line among all chips. > > > > I'm thinking that most users would prefer to work with names. > And that preference will only grow stronger over time. > I'm starting to lean that way myself. > Ok, I may get convinced but we'd need something that behaves consistently and predictably. A GPIO line name can still parse as an unsigned integer so we cannot make assumptions here. Maybe --by-name or --by-offset should always be required? > So you are happy with positional chip by default, but optional when > --by-name is set? That is going to make command line parsing even more > fun, but ok. > > So your acceptible forms would be (using get for brevity): > gpioget gpiochip0 16 17 23 > gpioget --by-name GPIO16 GPIO17 GPIO23 > gpioget --by-name -c gpiochip0 GPIO16 GPIO17 GPIO23 > > ? > > > > The sets should also accept a set of true/false variants, such as the > > > on/off, active/inactive in the examples above. > > > > Why though? What do we gain from accepting all kinds of different > > strings? IMO it just makes the interface less clear. > > > > All kinds? I suggested two variants. That is all, not the whole thesaurus. > I explicitly avoid 0/1 and low/high, cos line levels. > Could be convinces to accept 0/1 for backward compatibility. > > Users can then use a nomenclature that best suits their application. > That makes their scripts more readable. And much more meaningful than > 0/1. I mean 0/1 is literally the least amount of information you could > possibly provide. > Do you think we should also use this naming in the library? As in: enum { GPIOD_LINE_STATE_ACTIVE = 1, GPIOD_LINE_STATE_INACTIVE = 0 }; And corresponding scoped enums in C++ and Python? > While the active-low option exists, setting line=1 will always be > confusing. Dropping the active-low option for set would mean it > operates with physical levels, while the other commands use logical, > so don't really want to go there either.... > > (I have also wondered if having the active-low option in the uAPI > is actually beneficial in general, but that bird has flown.) > > > > The gets should return active/inactive to make it clear they refer to > > > logical values, not physical values. > > > > > > > What's wrong with 1/0? > > > > As stated, that can easily be misinterpreted as meaning physical line levels. > And the logical and physical levels are not the same if active-low, > and the get command gives no indication if the active-low is set or not. > Right. > > > I am also wondering why the tools are separate, instead of being merged > > > into a single tool. Is there a reason for that? > > > > > > > You mean like busybox' single executable with multiple links under > > different names internally redirecting to different main() functions > > or really a single tool with multiple commands? > > > > One advantage of a single tool is that when you add a new one, say > watch, it just gets added to the tool and pops up in the tool help. > So it aids discoverabilty. > > And adding commands doesn't automatically require an extra binary to be > built and installed - just upgrading the one. > > The busy-box command-link approach is an option, with the link name > determining the subcommand. Or it can be called directly with subcommands. > e.g. gpiodctl set gpiochip0 16=0 > I can work with that. > Wrt backward compatibility, perhaps add an option to behave as per v1, > so current users can just add that until they migrate to the v2 command > line? > When called directly it could use the v2 command line, but when called > via a link, it could enable the v1 option and support the v1 command line. > Or is that just confusing too? > > > The reason for having separate tools is that they really are tiny, the > > little code they share and statically link to is negligible and it's > > simply clearer as to which tool does what. I didn't want the tools to > > be this swiss-knife do-it-all program that requires studying the > > README for a long time. > > > > Who needs the README - the tool help should be sufficient. > If not then fix the help. > > They might be tiny, but collectively they can't be smaller than a unified > executable. > Are there cases where only a subset of commands is deployed? > Though even that could be addressed with build options to drop subcommands > from the tool. > Indeed. > > > 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. > > > > > > > gpioset --interactive is definitely something I'm interested in. > > > > Yeah, it also solves most of my discomfort with the named option - it > only does the find once and caches the offset for subsequent sets. > The best of both worlds. > > Would adding a find ioctl to the uAPI be worthwhile? > I don't think so and the reason for that is this: if you want maximum performance, you should be ready to spend time and use the C API. The command line tools aren't meant to be fast. I suppose you spend more time forking and execing than looking up names frankly. The DBus API and gpioset --interactive will most likely solve this issue anyway. > > > 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. > > > Ok, let's think about what we must do before v2 and what can be implemented later without breaking compatibility again. > > > > While the total overhaul of the library is understandable, I would > > prefer to keep the tools mostly backward compatible. I plan on adding > > gpiowatch for watching info events but that's it. Do you see any > > things that are obviously wrong in how the tools work that would > > justify the sweeping changing you mention? > > > > Other than the named mode... > > The set is the one that bothers me the most. > The --mode is a bit confusing. Most of them map to the same thing, > and I can get the same effect with --interactive and/or a hold period. > (the hold period being a forced sleep after a set. That comes in handy > in the interactive mode where you can just hammer out sets and the hold > period defines the rate they will be applied, so blinking or even > bit-bashing becomes super easy) Given that - as someone pointed out recently - some users find it difficult to "gpioget `gpiofind GPIO0`" (hence the whole --by-name idea) I expect they would find it even harder to fiddle with pipes and other shellnanigans. > > Time periods should be a single option that includes the time units, > e.g. -p 10ms is more readable than -u 10000000 (how many zeros is that??) > > And I'm pondering a toggle/blink mode, which would toggle the requested > lines at the rate defined by the hold period. So a simple blinker would > be: > gpioset --by-name --blink -p 1s LED=on > > And a corresponding toggle command for the interactive mode. > (which could specify a subset of lines to toggle, or toggle the whole > set) > > If you want to retain backward compatibility then adding these > features, without removing the old, will produce a cluttered, messy > and really confusing gpioset interface. And that is the tool most > likely to see use :(. > Doesn't really look like it to me for the three examples above, but I'm not entirely against non-compatible changes. I would just prefer to keep a similar look and feel. > Wrt gpioinfo, I'm not sure the quotes on the name and consumer in the > output helps. You mean for programmatic parsing? For a human reader this doesn't really matter, does it? > And I'd restrict the chip option to a single chip, else > all chips. No subset option, to be consistent with other tools > and to allow the option of selecting the line or a subset of lines, > either by offset or name. > e.g. who is consuming the LED line? > gpioinfo --by-name LED > > gpiofind should have a chip option to limit searching (names are only > unique within chips, right, or is even that still not guaranteed?), > and an option to provide the info for found lines - since we have it in > hand anyway. > > gpiodetect could get a chip option, so only get the info for the chip > of interest (to ignore those orphaned gpio-sims ;-). > Touché. > gpiomon should provide sequence numbers. Maybe an option to return the > whole event in JSON? > Sure, sequence numbers are already on my TODO list just like any other new information that's passed over v2 ABI. > Anyway, in short, if backward compatibility is a requirement then I'm > really not sure how best to proceed - except now I need another coffee. > Let's say that I'm open to breaking changes but I would like to limit them wherever we can unlike the library. I won't get to it though before we merge v2 API into master, so going back to writing C++ tests. :) Bart > Cheers, > Kent.
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) {
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(-)