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

[enh]

On Fri, Mar 15, 2024 at 1:46 PM Rob Landley <rob at landley.net> wrote:

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

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.


> 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?
>

i have no idea, because dpkg doesn't know. i'm assuming it's
https://github.com/rhboot/dbxtool 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.)


> 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...?)
>

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.


> 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?
>

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?


> 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...
>

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 :-(


> > 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...)
>

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.

but i'm still hopeful we can get a (sensibly tiny!) example we can check in.

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.


> Rob
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.landley.net/pipermail/toybox-landley.net/attachments/20240315/66160688/attachment.htm>