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

[Rob Landley]

Blah, I got deep enough into
https://github.com/landley/mkroot/commits/master I forgot to check my
mail for a couple days...

On 12/14/2017 08:01 PM, enh wrote:
> 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.

An interpreter run from #! will have a path in argv[0], a command run
from $PATH won't have a path in the name. Given that tinycc's -run mode
is technically an interpreter, there can't really _be_ a canonical list.
So it's going to be some sort of heuristic, the question is what.

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

Hmmm...

  $ /bin/less filename.txt
  $ killall filename.txt

I can see a case for it. (Especially since killall filename without the
extension wouldn't match.)

We're not going to get perfect behavior out of this:

  $ time sleep 100 &
  [1] 19950
  $ killall time
  time: no process found

(It's a shell builtin. Yes, you can background a shell builtin. I assume
there's an automatic subshell in there somewhere. On a related note,
ctrl-z to suspend "read" turns out to be... problematic.)

> 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]?".

Reading two files, more complex parsing (the only way to tell whether )
is part of the filename or not is to continue to the _last_ ) which
means strrchr() over the whole string you read...)

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

Except gnu is often crazy and busybox has a tendency to blindly copy it.
And whatever we do there are still questionable cases:

  $ echo sleep 100 > thingy.sh
  $ bash thingy.sh &
  [1] 19893
  $ killall thingy.sh
  thingy.sh: no process found
  $

What's the _right_ thing to do there? Tying "killall bash" is going to
leave the system really unhappy. My "path might mean interpreter"
heuristic wouldn't catch this one either, but it's guaranteed to catch
anything run from #! because that has to be abspath.

Well, ok, not quite _guaranteed_:

  $ cd ~
  $ ln -s /bin/bash rutabaga
  $ echo -e '#!rutabaga\necho ha" > thingy.sh
  $ chmod +x thingy.sh
  $ ./thingy.sh
  ha
  $

But that's fairly deep voodoo nobody should ever do. :)

>>>  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 :-) )

I have not actually met Divya Kothari.

>> 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.

Would somebody please explain to me why they trust env to exist at a
specific absolute path but don't trust python to? I've never understood
this.

Anyway, I did several hours of work on this back around wednesday, and
then context switched away and never got back to it. (Unprecedented, I
know.) Lemme try to finish that up and check it in...

Rob