<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Mar 15, 2024 at 1:46 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">Applied, but I don't have a local way to reproduce the issue nor did it come<br>
with a test.<br></blockquote><div><br></div><div>yeah, i explained a bit in the commit message --- i have since managed to produce a file with a note that has the thing we weren't decoding, but _not_ with a note that contains two things (which was the "real" bug that made things go off the rails). i'll keep prodding the toolchain folks for ideas, and i'll let you know if i actually manage.</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">
On 3/14/24 19:08, enh via Toybox wrote:<br>
> I couldn't work out how to get gcc to actually produce such a thing, but<br>
> /bin/dbxtool on my debian box right now has them.<br>
<br>
I don't seem to have that command. What package does dpkg-query -S /bin/dbxtool<br>
say it's in on your system?<br></blockquote><div><br></div><div>i have no idea, because dpkg doesn't know. i'm assuming it's <a href="https://github.com/rhboot/dbxtool">https://github.com/rhboot/dbxtool</a> which funnily enough says it's been replaced by the only other two binaries in my /bin that seem to have been built in this way. (but, you know, fair enough: ideally i'd have all the binaries built like this, but if it's only going to be a few, those are good places to start.)</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">
Also:<br>
<br>
-    unsigned namesz=elf_int(&p), descsz=elf_int(&p), type=elf_int(&p), j=0;<br>
+    unsigned namesz=elf_int(&p),descsz=elf_int(&p),type=elf_int(&p),j=0;<br>
<br>
Is there a reason for that? Spaces after commas is the usual in the codebase, I<br>
think? (Both in declaration lists and function arguments. With the occasional<br>
cheat to fit in 80 columns but that doesn't apply here. I mean, I applied it<br>
anyway, but...?)<br></blockquote><div><br></div><div>oh, sorry --- this is noise from a bit that got reverted. i needed an extra variable telling me whether i'd seen a gnu property note --- until i worked out that actually it's just the _content_ of the gnu property note that needs special handling and deleted [not quite] all that.</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">
And then you add more spaces in:<br>
<br>
-        while (descsz-j > 8) { // Ignore 0-padding at the end.<br>
+        while (descsz - j > 0) {<br>
<br>
That one's not a strong thing, but why add the spaces around the minus there?<br></blockquote><div><br></div><div>because i thought i'd seen you say recently that you only remove the spaces when you need to do so to fit on the line?</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 "x = y;" spaces around assignment operators got drilled into me back on<br>
busybox, along with the spaces in flow control statements that would otherwise<br>
look like functions, ala if (potato) vs if(potato). And I mentioned the spaces<br>
after commas.<br>
<br>
For most of the rest of them, descsz-j>8 works just as well for me? Especially<br>
with syntax highlighting. I lean towards smaller representations where possible,<br>
although "consistent" beats "correct" so I have it do what the nearby code is<br>
doing unless I'm cleaning up a function at a time.<br>
<br>
In while (descsz-j > 8) the spaces are separating the math from the comparison<br>
so almost earn their keep, but if you add more spaces it equally looks like<br>
descsz - (j > 0) so I dunno what the spaces are supposed to accomplish?<br>
<br>
As I said, not a strong thing. Just curious why the change. At that level it's<br>
more aesthetics than policy, but I wonder if you've got a mental rule I'm missing...<br></blockquote><div><br></div><div>oh, _my_ rule is "everywhere i've worked has wanted all the spaces, all the time". unfortunately that collides with me not having a good model of the toybox heuristics, and there's no .clang-format file so i can just let the computer fix it. so i do my best to guess what i think it's supposed to look like. sometimes i do okay, days like yesterday not so much :-(</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 big mistake here is that GNU property notes' data is always 8-byte<br>
> aligned, so we needed to skip that. That lets us get rid of the existing<br>
> loop termination hack to skip padding.<br>
> <br>
> While I'm here -- since the symptom was running off the end of the file --<br>
> I've also added a bounds check in the property dumping loop.<br>
> <br>
> I wish I had fuzzing infrastructure to run AFL++ against this every time<br>
> it changes... In lieu of that I do wonder whether we should add `readelf<br>
> -aW /bin/* > /dev/null` as a smoke test that "at least it works for all<br>
> the _valid_ binaries on the system you're testing on". That would have<br>
> caught this sooner.<br>
<br>
At a design level the test suite tries to be self-contained and deterministic.<br>
Even the TEST_HOST option to sanity check the tests' validity against the host<br>
commands behavior is annoyingly made of heisenbugs. (Plus the filesystem you run<br>
it on can change behavior enough to break things, we've had kernel version skew<br>
at least once, and the bsd tests are handwaves at best.)<br>
<br>
A test where you honestly don't know what input data you're running it on<br>
wouldn't really belong in "make tests", that would be some other test. Maybe a<br>
scripts/probes/test-readelf or something? (It sort of conceptually fits with<br>
GLOBALS and findglobals. Questions I occasionally want to ask, but have not yet<br>
tried to automate understanding of the results.)<br>
<br>
I could see having such a test as part of the automated LFS build, and I could<br>
see running it against the results of an android build. But neither use case<br>
seems a good fit for "cd toybox; make tests" being the delivery mechanism.<br>
<br>
Also, if you're gonna do it, traverse $PATH instead of a hardwired /bin. There's<br>
a script snippet to do that in mkroot/record-commands already:<br>
<br>
  tr : '\n' <<< "$PATH" | while read i; do<br>
    find "$i" -type f,l -maxdepth 1 -executable -exec basename {} \; | \<br>
      while read FILE; do ln -s logpath "$WRAPDIR/$FILE" 2>/dev/null; done<br>
  done<br>
<br>
Which now that I look at it should probably have ${FILE##*/} instead of the<br>
calls to basename. (Noticeably faster not to spawn a hundred child processes...)<br></blockquote><div><br></div><div>interesting, i hadn't thought of $PATH... i was thinking it's either / or /bin and the former's unbounded, so "/bin it is" but, yeah, $PATH is probably a good compromise.</div><div><br></div><div>but i'm still hopeful we can get a (sensibly tiny!) example we can check in.</div><div><br></div><div>i wish i'd kept notes about when i used AFL++ to test readelf (or was it file?) a few years back. `make fuzz` is a lot more "legit" than "at least do a quick check there's nothing on the host you're running on that would choke you". the trouble with AFL++ is it just keeps running until you tell it to stop, and there's no obvious way to know when enough is enough.</div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Rob<br>
</blockquote></div></div>