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

[enh]

On Sat, Jun 10, 2023 at 1:41 PM Rob Landley <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.


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


> I still have this in pending.h:
>
> // gratuitously memsets ALL the extra space with zeroes (not just a
> terminator)
> // but to make up for it truncating doesn't null terminate the output at
> all.
> // There are occasions to use it, but it is NOT A GENERAL PURPOSE FUNCTION.
> // #define strncpy(...) @@strncpyisbadmmkay@@
> // strncat writes a null terminator one byte PAST the buffer size it's
> given.
> #define strncat(...) strncatisbadmmkay(__VA_ARGS__)
>
> But mostly I tend to use a scripts/findglobals.sh approach. Easy enough to
> just
> search for all the lib/*.c functions that start with x and then grep for
> the
> non-x versions used in the code:
>
> egrep -w "($(sed -n 's/^[^ ].* [^a-zA-Z0-9_]*\(x[^ (]*\)(.*/\1/p' lib/*.c
> | sed
> 's/^x//' | xargs | tr ' ' '|'))[(]" main.c lib/*.c toys/*/*.c | wc -l
> 979
>
> Except 2/3 of that is unprefixed calls to printf(), add a grep -v for that
> and
> it's 233. Much more manageable.
>
> There are still quite a few false positives, the second result is the
> strtol on
> line 304 of lib/args.c which is parsing the "a#<3" numbers in the optstr,
> so the
> suffix handling of xstrtol() would be inappropriate there. And there would
> be a
> bit of whitelisting since xmalloc gets to call malloc and so on... but
> it's not
> an unmanageable audit? Just seemed like more of an "approaching 1.0"
> cleanup...
>
> (For strdup specifically, I see one in patch.c and one in tar.c, oops. No
> uses
> of malloc without the x outside of pending. Oops.)
>
> That said, the only time you should get a NULL return value from an
> allocation
> on a system with mmu is if you've exhausted _virtual_ address space, which
> is
> hard to do on a 64 bit system. (Unless you've set
> https://www.kernel.org/doc/Documentation/vm/overcommit-accounting to 2
> which
> means fork() is going to fail half the time...)
>
> Rob
>
> P.S. Looking at this, there are some design questions. lib/env.c is about
> avoiding memory leaks from the libc environment variable functions, which
> discard the existing data assuming it's environment space (environment
> variables
> started life as a "hidden arguments" hack, see
> https://mstdn.social/@JohnMashey/110506554669692533 ) so if you set the
> same
> variable name multiple times there's a memory leak, and in long-lived
> daemons
> and such that could cause an issue. Of course a possibly better approach is
> using execve() for your children, but libc cares about "TZ" and such in
> annoying
> ways so you do have to modify your environment sometimes... Anyway, yanking
> setenv/unsetenv (and deciding to audit that seperately, with an eye towards
> maybe deleting lib/env.c since I've implemented both httpd and most of sh
> now,
> neither of which use it but both were my big use cases I did it for...)
> doing
> that yanks about another 40 entries from the list...
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.landley.net/pipermail/toybox-landley.net/attachments/20230612/e1669dea/attachment.htm>