[Toybox] [PATCH] killall should kill scripts too.

[enh]

On Thu, Dec 14, 2017 at 6:32 AM, Rob Landley <rob at landley.net> wrote:
> On 12/13/2017 01:48 PM, enh wrote:
>> Found running LTP file system tests on Android.
>
> I have a todo item for this, but I was just going to also check argv[1]
> when argv[0] is an absolute path (starts with '/'), and I should adjust
> pgrep/pkill to do this too.
>
> I'd rather not involve comm because it's capped at 15 chars (arbitrarily
> truncated), I think the "treat absolute path as a potential interpreter"
> logic is simpler and probably less brittle? (The "process could go away
> between the two files we read" race probably isn't an issue here but not
> having to care about that's a bonus.)

i don't like comm truncation either, but it still seems like the
better test. it's more intention-revealing given that we don't have a
canonical list of all valid interpreters.

suppose you set $EDITOR or $PAGER to an absolute path. that shouldn't match.

and, sure, comm is truncated, but argv[1] isn't, and comm is only used
as a hint for "should i be looking at argv[1] rather than argv[0]?".

plus checking comm seems to be consistent with other implementations,
judging by strace.

> (Admittedly this is from somebody who's een doing 'ps ax | grep renderer
> | awk '{print $1}' | xargs kill' for _years_ when chrome's eating too
> much memory, and oddly enough getting away with it. I'm so glad it
> started working again after random chrome update du jour fixed the
> command line truncation bug a few months back :)
>
> Hmmm, and we always need to do the basename() thing if we're going to
> find ./thingy don't we? Hmmm, are we _sure_ libbuf is reliably null
> terminated in the case of a pathological filename (could be
> /big/long/path and sizeof(libbuf) == historical PATH_MAX), so... (It
> hasn't hit us _yet_ because if we're the first user of libbuf it's gotta
> be zeroed because it's in the BSS and the ELF spec says so, but as soon
> as we're NOT the first user Bad Things Happen.)
>
> And we need to getbasename() argv[1] too:
>
>   $ ./sleep.sh &
>   [1] 10840
>   $ ps ax | grep sleep.sh
>   10840 pts/38   S      0:00 /bin/bash ./sleep.sh
>   10843 pts/38   S+     0:00 grep --color=auto sleep.sh
>
> Except... when do you basename the command line argument?
>
>   $ ps ax | grep vlc
>   10881 ?        D      0:00 /usr/bin/vlc --started-from-file
>   10883 pts/42   S+     0:00 grep --color=auto vlc
>   $ killall /not/vlc
>   /not/vlc: No such file or directory
>
> Dear overcomplicated gnu/dammit utility: up yours. Not doing that,
> whatever that is. Sigh, what crazy thing does posix say to do... it
> doens't mention the existence of this utility. Well of course not.
>
> Ok if I have a /path on the command line, don't basename. Otherwise
> match basename. And NOW the fun of testing it! (Fire up qemu because
> this is not the sort of thing I'm comfortable testing on my dev box...)
>
> What new tests did you give me...
>
>>  tests/killall.test | 14 +++++++++++++
>
> Hmmm, I usually use "sleep" for these because it's self cleaning in the
> failure case. :)

(use of `yes` copied from pgrep tests :-) )

> It'd be nice to have an executable test as well, but that's slightly
> more awkward. (What do I expect to have install that's _not_ going to be
> running on the host already? Ah, of course: symlink sleep under a new
> name. :)

`env python` is the other combination that springs to mind if you're
trying for completeness.

> Rob



-- 
Elliott Hughes - http://who/enh - http://jessies.org/~enh/
Android native code/tools questions? Mail me/drop by/add me as a reviewer.