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

[Rob Landley]

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?

Thanks,

Rob

 1436559674.0