[Toybox] [PATCH] gpiod: fix foreach_chip crashes.

[enh]

On Mon, Jun 12, 2023 at 12:55 PM Rob Landley <rob at landley.net> wrote:

> On 6/12/23 09:32, enh wrote:> On Sat, Jun 10, 2023 at 1:41 PM Rob Landley
> <rob at landley.net
> > <mailto:rob at landley.net>> wrote:
> >
> >     On 6/10/23 00:05, enh via Toybox wrote:
> >     > This was segfaulting because the loop copying the chips into the
> array
> >     > was corrupting the list, and because the llist_traverse was being
> given
> >     > the wrong pointer.
> >
> >     Huh, I wonder how that segfaults. I still don't have a test
> environment for
> >     this, but foreach_chip() is called exactly once (from three
> different command
> >     main functions) so consuming the list in the one and only pass seemed
> >     reasonable? I see I forgot to remove the free before exit but the
> only way there
> >     should be anything left is if TT.chip_count was < than the number of
> entries in
> >     the list, and even then the remains of the list should still be in
> their
> >     original order...?
> >
> > oh, yeah, just not trying to free the now-corrupted list would have been
> fine
> > too. (really there were two unrelated bugs: the traversal meant that
> there was a
> > leak, and the free was using the wrong pointer, which was the segfault.
> just
> > fixing the segfault didn't make any sense on its own.)
> >
> >     *shrug* Applied your patch as-is, I shouldn't poke at code I can't
> test...
> >
> > i can send you a tested patch for not freeing if you prefer.
>
> Six of one... I was just saying "when I changed this bit I forgot to
> change that
> bit".
>

so my argument for preferring to not corrupt the list was "one could
imagine calling foreach_chip() more than once in a toy; gpiomon, say [which
i haven't actually written because i've never used it, and the people
who've asked for the gpio tools have also denied wanting that one, and it
has a lot of options and i don't want to guess at the useful subset without
a user], might do this between each sleep". in which case, the freeing
would be a _bug_.

but that's arguable enough (and a problem that my first attempt to run
gpiomod would demonstrate via crash) that i won't send you the one-line
patch, even though i think removing the llist_traverse() line would be fine.


> In theory making the free() optional reduces the size of the binary, and
> various
> embedded environments still use brk() under the covers, meaning to free the
> entire heap at exit() they just have to move one pointer. A lot of "what
> can we
> do in 64k of ram" embedded environments look back at what Unix v6 and
> friends
> _did_ in similarly sized environments, and it still works. If you want a
> posix
> programming environment in tiny RTOS, what's old is often new again. And
> yes, we
> have reached the "on-chip battery" stage of embedded chicanery:
>
>
> https://www.independent.co.uk/tech/battery-smallest-computer-wearables-b2021494.html
>
> >     > The strdup wasn't failing, but should be an xstrdup anyway. (I
> think
> >     > all the strdups in toys/ should actually be xstrdups. Maybe we
> should
> >     > even poison strdup, or just `#define strdup xstrdup`?)
> >
> >     I've historically done some poisoning, but it tended to cause weird
> build breaks
> >     with macros in header files and so on?
> >
> >
> > there's a #pragma `poison` in both gcc and clang (specifically for this
> > problem). as long as we're building with those compilers, that should
> prevent
> > any uses creeping in?
>
> Is there a list of clang pragmas anywhere? The llvm documentation at
> https://clang.llvm.org/docs/UsersManual.html does not seem to have one...


yeah, don't get me started ... afaict even our own llvm people either look
embarrassed and back away or recommend you read the gcc docs instead. or
that old classic, "check the source". which doesn't work so well for
projects on the scale of gcc or clang, but the llvm source does include a
test for `#pragma GCC poison rindex`.


>

Rob
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.landley.net/pipermail/toybox-landley.net/attachments/20230612/324d6abb/attachment.htm>