On Sat, Jun 10, 2023 at 1:41 PM Rob Landley <r...@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... >
_______________________________________________ Toybox mailing list Toybox@lists.landley.net http://lists.landley.net/listinfo.cgi/toybox-landley.net
