<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sat, Jun 10, 2023 at 1:41 PM Rob Landley <<a href="mailto:rob@landley.net">rob@landley.net</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On 6/10/23 00:05, enh via Toybox wrote:<br>
> This was segfaulting because the loop copying the chips into the array<br>
> was corrupting the list, and because the llist_traverse was being given<br>
> the wrong pointer.<br>
<br>
Huh, I wonder how that segfaults. I still don't have a test environment for<br>
this, but foreach_chip() is called exactly once (from three different command<br>
main functions) so consuming the list in the one and only pass seemed<br>
reasonable? I see I forgot to remove the free before exit but the only way there<br>
should be anything left is if TT.chip_count was < than the number of entries in<br>
the list, and even then the remains of the list should still be in their<br>
original order...?<br></blockquote><div><br></div><div>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.)</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
*shrug* Applied your patch as-is, I shouldn't poke at code I can't test...<br></blockquote><div><br></div><div>i can send you a tested patch for not freeing if you prefer.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> The strdup wasn't failing, but should be an xstrdup anyway. (I think<br>
> all the strdups in toys/ should actually be xstrdups. Maybe we should<br>
> even poison strdup, or just `#define strdup xstrdup`?)<br>
<br>
I've historically done some poisoning, but it tended to cause weird build breaks<br>
with macros in header files and so on? </blockquote><div><br></div><div>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?</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">I still have this in pending.h:<br>
<br>
// gratuitously memsets ALL the extra space with zeroes (not just a terminator)<br>
// but to make up for it truncating doesn't null terminate the output at all.<br>
// There are occasions to use it, but it is NOT A GENERAL PURPOSE FUNCTION.<br>
// #define strncpy(...) @@strncpyisbadmmkay@@<br>
// strncat writes a null terminator one byte PAST the buffer size it's given.<br>
#define strncat(...) strncatisbadmmkay(__VA_ARGS__)<br>
<br>
But mostly I tend to use a scripts/findglobals.sh approach. Easy enough to just<br>
search for all the lib/*.c functions that start with x and then grep for the<br>
non-x versions used in the code:<br>
<br>
egrep -w "($(sed -n 's/^[^ ].* [^a-zA-Z0-9_]*\(x[^ (]*\)(.*/\1/p' lib/*.c | sed<br>
's/^x//' | xargs | tr ' ' '|'))[(]" main.c lib/*.c toys/*/*.c | wc -l<br>
979<br>
<br>
Except 2/3 of that is unprefixed calls to printf(), add a grep -v for that and<br>
it's 233. Much more manageable.<br>
<br>
There are still quite a few false positives, the second result is the strtol on<br>
line 304 of lib/args.c which is parsing the "a#<3" numbers in the optstr, so the<br>
suffix handling of xstrtol() would be inappropriate there. And there would be a<br>
bit of whitelisting since xmalloc gets to call malloc and so on... but it's not<br>
an unmanageable audit? Just seemed like more of an "approaching 1.0" cleanup...<br>
<br>
(For strdup specifically, I see one in patch.c and one in tar.c, oops. No uses<br>
of malloc without the x outside of pending. Oops.)<br>
<br>
That said, the only time you should get a NULL return value from an allocation<br>
on a system with mmu is if you've exhausted _virtual_ address space, which is<br>
hard to do on a 64 bit system. (Unless you've set<br>
<a href="https://www.kernel.org/doc/Documentation/vm/overcommit-accounting" rel="noreferrer" target="_blank">https://www.kernel.org/doc/Documentation/vm/overcommit-accounting</a> to 2 which<br>
means fork() is going to fail half the time...)<br>
<br>
Rob<br>
<br>
P.S. Looking at this, there are some design questions. lib/env.c is about<br>
avoiding memory leaks from the libc environment variable functions, which<br>
discard the existing data assuming it's environment space (environment variables<br>
started life as a "hidden arguments" hack, see<br>
<a href="https://mstdn.social/@JohnMashey/110506554669692533" rel="noreferrer" target="_blank">https://mstdn.social/@JohnMashey/110506554669692533</a> ) so if you set the same<br>
variable name multiple times there's a memory leak, and in long-lived daemons<br>
and such that could cause an issue. Of course a possibly better approach is<br>
using execve() for your children, but libc cares about "TZ" and such in annoying<br>
ways so you do have to modify your environment sometimes... Anyway, yanking<br>
setenv/unsetenv (and deciding to audit that seperately, with an eye towards<br>
maybe deleting lib/env.c since I've implemented both httpd and most of sh now,<br>
neither of which use it but both were my big use cases I did it for...) doing<br>
that yanks about another 40 entries from the list...<br>
</blockquote></div></div>