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... _______________________________________________ Toybox mailing list Toybox@lists.landley.net http://lists.landley.net/listinfo.cgi/toybox-landley.net
