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

[Rob Landley]

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

*shrug* Applied your patch as-is, I shouldn't poke at code I can't test...

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