On Fri, Jul 10, 2015 at 1:21 PM, Rob Landley <[email protected]> wrote: > On 07/07/2015 06:00 PM, enh wrote: >> rob: preferred fix here so we can provide you with the patch you want? > > I'm still trying to unravel the top posting here... > > Let's see... > >> <[email protected]> wrote: >> >> Hi, >> >> I was trying toybox in nexus 9 android M preview and saw >> that killall tries to kill "all" pid and pidof >> whatever_you_type_without_a_slash_at_the_beginning answers >> with a list containing all existing pids. >> I am not sure if it belongs here or to the android bugtracker. > > killall ignoring its command line arguments would be a bug, yes. (I'm > not sure what the pidof bit means, is this a separate bug in pidof or > something about killall misparsing its output...?) > > $ ./pidof bash > 1812 2316 2992 3327 3715 3787 4331 7102 7477 7520 7547 8637 8682 9209 > 9328 11539 11582 11616 11696 12066 16227 17118 17173 17230 18230 18329 > 18523 18634 18780 19151 22171 22311 22844 22960 23001 24135 24805 24895 > 26464 26560 27307 27606 28126 28156 28179 28313 28390 28911 30396 30418 > 30647 30688 30722 30791 31537 32328 32424 > $ ./pidof init > 1 1195 1864 > > It seems like toybox pidof is being selective without an absolute path...? > >> (this came up independently today when an OEM was surprised to find that >> toybox's "killall" is rather too literal :-) ) > > Rather too broken, sounds like. (Pretty sure it worked when I tested it, > although I might only have tested it against uclibc because I do that > sort of thing in aboriginal under qemu, not on my host system...) > >> My findings: >> >> in lib.c line 819 >> : !strcmp(basename(cmd), basename(*curname))) >> >> call to basename in both strcmp arguments does not work as >> expected since the bionic basename gives back always the >> same address for the returned char* >> >> I was not able to find a reliable source for the expected >> behavior of baseline concerning the return buffer address >> and allocation. > > Sigh. Posix warns that some implementations are... they're too polite to > say "damn stupid" but: > > http://pubs.opengroup.org/onlinepubs/9699919799/functions/basename.html > > And of course we've been wrestling with this in commits 90e8605ea587 and > 41ed97934989 and 7d64dae54bde and de699accf680 and e910826c812f and > 468f155ecefe... (It SEEMS to do something so obvious! But unfortunately, > due to the way posix defined it, it doesn't actually.) > > Grumble grumble basename_r() grumble. (Why would the gnu/dammit guys > change the behavior of an existing function in a nonconforming way that > varies based on which header you include first rather than doing the _r > thing that 8 gazillion other non-threadsafe functions have done? How > does that make sense?) > > Alright, I think I understand the problem now. And it being in a common > lib.c function would explain pidof getting affected. And I think the fix > is to just implement a basename_r() that returns "" when you have a > trailing slash because _this_ is _crazy_. > > I checked in a wild guess at it, which seems to work for pidof. (Did > pidof "" always list kernel threads? It does now. The other pidof lists > nothing, but doesn't consider it an error, so...) > > Let me know if I missed anything important in the rest of the inside-out > top posted message?
lgtm. > Thanks, > > Rob -- Elliott Hughes - http://who/enh - http://jessies.org/~enh/ Android native code/tools questions? Mail me/drop by/add me as a reviewer. _______________________________________________ Toybox mailing list [email protected] http://lists.landley.net/listinfo.cgi/toybox-landley.net
