On Fri, Mar 15, 2024 at 1:46 PM Rob Landley <[email protected]> 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 >
_______________________________________________ Toybox mailing list [email protected] http://lists.landley.net/listinfo.cgi/toybox-landley.net
