[Toybox] names_to_pid() and behavior for killall and pidof commands on android

[enh]

On Fri, Jul 10, 2015 at 1:21 PM, Rob Landley <rob at landley.net> 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...
>
>>   <nicolas.noury at gmail.com> 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.

 1436594845.0