[Toybox] [PATCH] readelf: fix -n for x86-64 ibt/shstk notes.

[Rob Landley]

Applied, but I don't have a local way to reproduce the issue nor did it come
with a test.

On 3/14/24 19:08, enh via Toybox wrote:
> I couldn't work out how to get gcc to actually produce such a thing, but
> /bin/dbxtool on my debian box right now has them.

I don't seem to have that command. What package does dpkg-query -S /bin/dbxtool
say it's in on your system?

Also:

-    unsigned namesz=elf_int(&p), descsz=elf_int(&p), type=elf_int(&p), j=0;
+    unsigned namesz=elf_int(&p),descsz=elf_int(&p),type=elf_int(&p),j=0;

Is there a reason for that? Spaces after commas is the usual in the codebase, I
think? (Both in declaration lists and function arguments. With the occasional
cheat to fit in 80 columns but that doesn't apply here. I mean, I applied it
anyway, but...?)

And then you add more spaces in:

-        while (descsz-j > 8) { // Ignore 0-padding at the end.
+        while (descsz - j > 0) {

That one's not a strong thing, but why add the spaces around the minus there?

The "x = y;" spaces around assignment operators got drilled into me back on
busybox, along with the spaces in flow control statements that would otherwise
look like functions, ala if (potato) vs if(potato). And I mentioned the spaces
after commas.

For most of the rest of them, descsz-j>8 works just as well for me? Especially
with syntax highlighting. I lean towards smaller representations where possible,
although "consistent" beats "correct" so I have it do what the nearby code is
doing unless I'm cleaning up a function at a time.

In while (descsz-j > 8) the spaces are separating the math from the comparison
so almost earn their keep, but if you add more spaces it equally looks like
descsz - (j > 0) so I dunno what the spaces are supposed to accomplish?

As I said, not a strong thing. Just curious why the change. At that level it's
more aesthetics than policy, but I wonder if you've got a mental rule I'm missing...

> The big mistake here is that GNU property notes' data is always 8-byte
> aligned, so we needed to skip that. That lets us get rid of the existing
> loop termination hack to skip padding.
> 
> While I'm here -- since the symptom was running off the end of the file --
> I've also added a bounds check in the property dumping loop.
> 
> I wish I had fuzzing infrastructure to run AFL++ against this every time
> it changes... In lieu of that I do wonder whether we should add `readelf
> -aW /bin/* > /dev/null` as a smoke test that "at least it works for all
> the _valid_ binaries on the system you're testing on". That would have
> caught this sooner.

At a design level the test suite tries to be self-contained and deterministic.
Even the TEST_HOST option to sanity check the tests' validity against the host
commands behavior is annoyingly made of heisenbugs. (Plus the filesystem you run
it on can change behavior enough to break things, we've had kernel version skew
at least once, and the bsd tests are handwaves at best.)

A test where you honestly don't know what input data you're running it on
wouldn't really belong in "make tests", that would be some other test. Maybe a
scripts/probes/test-readelf or something? (It sort of conceptually fits with
GLOBALS and findglobals. Questions I occasionally want to ask, but have not yet
tried to automate understanding of the results.)

I could see having such a test as part of the automated LFS build, and I could
see running it against the results of an android build. But neither use case
seems a good fit for "cd toybox; make tests" being the delivery mechanism.

Also, if you're gonna do it, traverse $PATH instead of a hardwired /bin. There's
a script snippet to do that in mkroot/record-commands already:

  tr : '\n' <<< "$PATH" | while read i; do
    find "$i" -type f,l -maxdepth 1 -executable -exec basename {} \; | \
      while read FILE; do ln -s logpath "$WRAPDIR/$FILE" 2>/dev/null; done
  done

Which now that I look at it should probably have ${FILE##*/} instead of the
calls to basename. (Noticeably faster not to spawn a hundred child processes...)

Rob