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

[Rob Landley]

On 12/20/2017 12:34 PM, enh wrote:
> thanks. though you have though managed to convince me that the
> situation is already a mess.

It keeps getting messier. I'm trying to write proper test cases for
this, and it's being... Sigh.

  $ mkdir sub1 sub2
  $ cp "$(which sleep)" sub1/sleep
  $ ln sub1/sleep sub2/sleep
  $ sub1/sleep 999 &
  [1] 2096
  $ sub2/sleep 998 &
  [2] 2099
  $ pidof sub1/sleep
  2096
  $ pidof $PWD/sub1/sleep
  2099 2096

So the ubuntu version is matching argv[0] for a relative path and inodes
for an absolute path. How do I know the absolute path is matching inodes?

  $ rm sub2/sleep
  $ cp sub1/sleep sub2
  $ sub1/sleep 999 & sub2/sleep 998 &
  [1] 2295
  [2] 2296
  $ pidof $PWD/sub1/sleep
  2295

And the sad part is: ubuntu's pidof and ubuntu's killall aren't using
the same 

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

[enh]

On Wed, Dec 20, 2017 at 10:34 AM, enh  wrote:
> On Tue, Dec 19, 2017 at 2:35 PM, Rob Landley  wrote:
>>
>>
>> On 12/18/2017 11:15 PM, enh wrote:
>>> On Mon, Dec 18, 2017 at 5:07 PM, enh  wrote:
> Having killall need to do similar grinding over a large number of
> processes seems unnecessary. That said, it looks like to match ubuntu's
> killall we would need to read two files _and_ stat /proc/$$/exe.

 that's still a lot lighter weight than all the work pgrep/pkill have
 to do, and it's what everyone's already living with anyway...
>>>
>>> (and toybox lsof is still 10x faster than FSF lsof on my machines.
>>
>> $ sleep 999 > test3 &
>> $ time lsof test3
>> COMMAND   PIDUSER   FD   TYPE DEVICE SIZE/OFF NODE NAME
>> sleep   31117 landley1w   REG8,10 11944128 test3
>>
>> real0m3.134s
>> user0m0.988s
>> sys 0m2.084s
>> $ time ./lsof ../toy3/test3
>> COMMAND PID   USER   FD  TYPE DEVICE  SIZE/OFF
>> NODE NAME
>> sleep 31117landley1w  REG8,1 0
>> 11944128 /home/landley/toybox/toy3/test3
>>
>> real0m3.279s
>> user0m1.032s
>> sys 0m2.000s
>>
>> $ ps ax |wc
>> 3763041   98906
>>
>> Same 3 seconds to iterate over 376 processes in the simple case. (lsof
>> -i takes 6 seconds, toybox doesn't have -i yet...)
>
> i think we have this same conversation every few months :-)
>
> we should probably work out what's different. maybe because my
> machines have lots of processes and few network connections?
> (profiling confirms that most of lsof time goes to parsing
> /proc/PID/maps for me.)
>
>>> don't think lsof is high on the list of things to worry about. even
>>> for top, i'm more worried about the fact that it crashes if you leave
>>> it running long enough, or the broken ps -AT...)
>>
>> I have a tab open for ps -AT, but I thought I'd gotten all the ps/top
>> crashes out of the way? Is that still a thing?
>
> i think the ps "directory disappeared from under me" crashes are
> fixed. i haven't seen a report for some time.
>
> i don't think the top "memory corruption if left running long enough"
> crashes are fixed, but i've only ever seen a few crashes. i've just
> built a ToT toybox with asan for my laptop and i'll leave it running
> during the day.

took a long time (wasn't paying attention, so don't know how long),
but (formatting screwed up by virtue of coming from the middle of top
output) the top crash is still there:

=hrome --type=r+

==16990==ERROR: AddressSanitizer: heap-buffer-overflow on address
0x61d00074c178 at pc 0x005505c6 bp 0x7ffdfbb29190 sp
0x7ffdfbb29188
  READ of size 8 at
0x61d00074c178 thread T0
#0 0x5505c5  (/home/enh/toybox-asan/toybox+0x5505c5)
#1
0x54dff2  (/home/enh/toybox-asan/toybox+0x54dff2)
#2 0x4f96bb
(/home/enh/toybox-asan/toybox+0x4f96bb)
#3 0x4f8aff  (/home/enh/toybox-asan/toybox+0x4f8aff)

 #4 0x4f96bb  (/home/enh/toybox-asan/toybox+0x4f96bb)
#5 0x4f8aff
(/home/enh/toybox-asan/toybox+0x4f8aff)
#6 0x4f9892  (/home/enh/toybox-asan/toybox+0x4f9892)

 #7 0x7fc5fda712b0  (/lib/x86_64-linux-gnu/libc.so.6+0x202b0)
#8
0x41b0d9  (/home/enh/toybox-asan/toybox+0x41b0d9)

0x61d00074c178 is located 0 bytes
to the right of 2296-byte region [0x61d00074b880,0x61d00074c178)
  allocated by
thread T0 here:
  #0 0x4b9518  (/home/enh/toybox-asan/toybox+0x4b9518)
  #1
0x4f5fa4  (/home/enh/toybox-asan/toybox+0x4f5fa4)
  #2 0x54ebcd
(/home/enh/toybox-asan/toybox+0x54ebcd)
  #3 0x54dff2  (/home/enh/toybox-asan/toybox+0x54dff2)

#4 0x4f96bb  (/home/enh/toybox-asan/toybox+0x4f96bb)
  #5 0x4f8aff
(/home/enh/toybox-asan/toybox+0x4f8aff)
  #6 0x4f96bb  (/home/enh/toybox-asan/toybox+0x4f96bb)

   #7 0x4f8aff  (/home/enh/toybox-asan/toybox+0x4f8aff)
  #8 0x4f9892
(/home/enh/toybox-asan/toybox+0x4f9892)
  #9 0x7fc5fda712b0
(/lib/x86_64-linux-gnu/libc.so.6+0x202b0)

  SUMMARY: AddressSanitizer: heap-buffer-overflow
(/home/enh/toybox-asan/toybox+0x5505c5)
  Shadow bytes around the buggy address:

0x0c3a800e17d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
   0x0c3a800e17e0: 00 00 00 00
00 00 00 00 00 00 00 00 00 00 00 00
   

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

[enh]

On Tue, Dec 19, 2017 at 2:35 PM, Rob Landley  wrote:
>
>
> On 12/18/2017 11:15 PM, enh wrote:
>> On Mon, Dec 18, 2017 at 5:07 PM, enh  wrote:
 Having killall need to do similar grinding over a large number of
 processes seems unnecessary. That said, it looks like to match ubuntu's
 killall we would need to read two files _and_ stat /proc/$$/exe.
>>>
>>> that's still a lot lighter weight than all the work pgrep/pkill have
>>> to do, and it's what everyone's already living with anyway...
>>
>> (and toybox lsof is still 10x faster than FSF lsof on my machines.
>
> $ sleep 999 > test3 &
> $ time lsof test3
> COMMAND   PIDUSER   FD   TYPE DEVICE SIZE/OFF NODE NAME
> sleep   31117 landley1w   REG8,10 11944128 test3
>
> real0m3.134s
> user0m0.988s
> sys 0m2.084s
> $ time ./lsof ../toy3/test3
> COMMAND PID   USER   FD  TYPE DEVICE  SIZE/OFF
> NODE NAME
> sleep 31117landley1w  REG8,1 0
> 11944128 /home/landley/toybox/toy3/test3
>
> real0m3.279s
> user0m1.032s
> sys 0m2.000s
>
> $ ps ax |wc
> 3763041   98906
>
> Same 3 seconds to iterate over 376 processes in the simple case. (lsof
> -i takes 6 seconds, toybox doesn't have -i yet...)

i think we have this same conversation every few months :-)

we should probably work out what's different. maybe because my
machines have lots of processes and few network connections?
(profiling confirms that most of lsof time goes to parsing
/proc/PID/maps for me.)

>> don't think lsof is high on the list of things to worry about. even
>> for top, i'm more worried about the fact that it crashes if you leave
>> it running long enough, or the broken ps -AT...)
>
> I have a tab open for ps -AT, but I thought I'd gotten all the ps/top
> crashes out of the way? Is that still a thing?

i think the ps "directory disappeared from under me" crashes are
fixed. i haven't seen a report for some time.

i don't think the top "memory corruption if left running long enough"
crashes are fixed, but i've only ever seen a few crashes. i've just
built a ToT toybox with asan for my laptop and i'll leave it running
during the day.

> 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
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net

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

[enh]

On Tue, Dec 19, 2017 at 1:38 PM, Rob Landley  wrote:
>
>
> On 12/18/2017 07:07 PM, enh wrote:
>> On Sun, Dec 17, 2017 at 9:40 AM, Rob Landley  wrote:
>>> On 12/17/2017 11:20 AM, Rob Landley wrote:
 Sigh. You argument that "we can't just do something easily explained but
 must copy ubuntu's magic edge cases exactly" implies I need to
 understand ubuntu's magic edge cases, which are nonobvious.
>>>
>>> I should clarify: what I'm uncomfortable with is the need to read
>>> multiple /proc files per process, because the speed of lsof is just
>>> _sad_ and the cpu usage of top is unpleasant. (Part of the reason I
>>> haven't finished cleaning up lsof yet is I still hope to be able to
>>> speed it up somehow, which is a largeish time sink every time I turn my
>>> attention to it.)
>>>
>>> Having killall need to do similar grinding over a large number of
>>> processes seems unnecessary. That said, it looks like to match ubuntu's
>>> killall we would need to read two files _and_ stat /proc/$$/exe.
>>
>> that's still a lot lighter weight than all the work pgrep/pkill have
>> to do, and it's what everyone's already living with anyway...
>
> That and the "it doesn't have to be right or match a spec, it has to
> match the other command's behavior", which is a nonobvious hairball once
> I started looking at it:
>
>   $ echo -e '#!/bin/bash\nsleep "$@"' > spleep
>   $ chmod +x spleep
>   $ spleep 123 &
>   [1] 28391
>   $ killall $PWD/spleep
>   /home/landley/toybox/toy3/sub2/spleep: no process found
>
> Because no process has /proc/*/exe pointing to that file's dev:inode
> pair. The existing lib/ plumbing for this is _only_ checking names, not
> checking this inode stuff. But this is in lib because it's shared
> plumbing, pidof also uses it and the host version of that has -s to
> check for scripts...
>
> Sigh. Ok, it's been long enough I'm invoking the "apply the other
> person's patch then fiddle on top of it" rule, but this absolute path
> business is not what the other one's doing...

thanks. though you have though managed to convince me that the
situation is already a mess.

i think i might only have tested against busybox killall, not GNU
killall (because the original bug report said "busybox works" and
didn't mention GNU), and i just assumed busybox and GNU matched.

> 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
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net

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

[Rob Landley]

On 12/19/2017 03:38 PM, Rob Landley wrote:
> Sigh. Ok, it's been long enough I'm invoking the "apply the other
> person's patch then fiddle on top of it" rule, but this absolute path
> business is not what the other one's doing...

Darn it, in the attached patch I'm breaking out of the loop when a name
matches and calling callback() if the loop didn't exhaust the name list,
and I got to the "why does the callback have a return value", and
killall never does but pidof does and then I went:

  $ sleep 999 &
  [1] 3449
  $ pidof sleep sleep
  3449 3449

So nope, I can't break then handle. It's entirely possible I need a goto.

While I'm at it, I tried to write a one or two line comment for this and
just couldn't get it down to a reasonable size:

  if (!strcmp(bb, basename(cmd)) break;
  if (bb!=*cur && !strcmp(bb, basename(cmd+strlen(cmd)+1))) break;

The bb != *cur test is to make sure there's a path in argv[0], and I'm
not sure if I need it? Although #!blah without a path basically never
happens (but it works, runs an interpreter in the current directory) at
that point in the code we've already matched comm against basename (or
we would have continued at the top of the loop, we only call it a
_success_ if there's no path and strlen<16 but it's a _failure_ if it
doesn't match).

So the false positive case of not having that test would be something
like this passing:

  comm = thisisaverylong
  cmdline = thisisaverylongotherfile /usr/bin/thisisaverylongname

Which A) probably never happens, B) well what if that first one _was_ an
interpreter, C) how would only false positiving for
/opt/thisisaverylongotherfile be an improvement, D) exec can just make
_up_ the contents of argv[0] and argv[1] anyway so who am I kidding?

So I'm not sure what the heuristic accomplishes, but the _common_case_
is /bin/sh with an absolute path, so it _seems_ like something I should
check? (I suppose I could check for **cmd == '/' instead...)

(If you recall my discomfort with "just do what the other one does",
especially if I'm not reading any gnu/dammit code for
copyright/licensing reasons... What I care about is doing the right
thing. It's not always clear to me what that _is_. Consistency with
other implementation(s) is a strong weighting factor, but not the only one.)

> Rob

Seriously, my failure mode these days is writing something like this
patch and then throwing it away twice daily, because if I got distracted
from it now without doing the level of writeup and come back to look at
it in a week, all I'd remember is there were unresolved issues and then
I'd spent an hour staring at it trying to figure out what they were
without ever quite being comfortable with the result.

(And yes, the tiny things that don't matter are still the ones I get
hung up on, precisely because there's no clear winner.)
diff --git a/lib/lib.c b/lib/lib.c
index 44a7cc7..fe9c5fc 100644
--- a/lib/lib.c
+++ b/lib/lib.c
@@ -1022,25 +1022,52 @@ void names_to_pid(char **names, int (*callback)(pid_t pid, char *name))
   DIR *dp;
   struct dirent *entry;
 
-  if (!(dp = opendir("/proc"))) perror_exit("opendir");
+  if (!(dp = opendir("/proc"))) perror_exit("no /proc");
 
   while ((entry = readdir(dp))) {
-unsigned u;
-char *cmd, *comm, **cur;
+unsigned u = atoi(entry->d_name);
+char *cmd = 0, *comm, **cur;
 
-if (!(u = atoi(entry->d_name))) continue;
+if (!u) continue;
 
-// For a script, comm and argv[1] will match (argv[0] will be the interp).
+// Comm is original name of executable (argv[0] could be #! interpreter)
+// but it's limited to 15 characters
 sprintf(libbuf, "/proc/%u/comm", u);
 if (!(comm = readfile(libbuf, libbuf, sizeof(libbuf continue;
-sprintf(libbuf+16, "/proc/%u/cmdline", u);
-if (!(cmd = readfile(libbuf+16, libbuf+16, sizeof(libbuf)-16))) continue;
-
-for (cur = names; *cur; cur++)
-  if (argv0_match(cmd, *cur) ||
-  (!strncmp(comm, *cur, 15) && argv0_match(cmd+strlen(cmd)+1, *cur)))
-if (callback(u, *cur)) break;
-if (*cur) break;
+
+for (cur = names; *cur; cur++) {
+  struct stat st1, st2;
+  char *bb = basename(*cur);
+  off_t len;
+
+  // fast path: only matching a filename (no path) that fits in comm
+  if (strncmp(comm, bb, 15)) continue;
+  len = strlen(bb);
+  if (bb==*cur && len<16) break;
+
+  // If we have a path to existing file only match if same inode
+  if (bb!=cur && !stat(*cur, )) {
+char buf[32];
+
+sprintf(buf, "/proc/%u/exe");
+if (stat(buf, )) continue;
+if (st1.st_dev != st2.st_dev || st1.st_ino != st2.st_ino) continue;
+break;
+  }
+
+  // Nope, gotta read command line to confirm
+  if (!cmd) {
+sprintf(cmd = libbuf+16, "/proc/%u/cmdline", u);
+len = sizeof(libbuf)-17;
+if (!(cmd = readfileat(AT_FDCWD, cmd, cmd, ))) continue;
+// readfile only guarnatees one null terminator and we need two
+   

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

[Rob Landley]

On 12/18/2017 11:15 PM, enh wrote:
> On Mon, Dec 18, 2017 at 5:07 PM, enh  wrote:
>>> Having killall need to do similar grinding over a large number of
>>> processes seems unnecessary. That said, it looks like to match ubuntu's
>>> killall we would need to read two files _and_ stat /proc/$$/exe.
>>
>> that's still a lot lighter weight than all the work pgrep/pkill have
>> to do, and it's what everyone's already living with anyway...
> 
> (and toybox lsof is still 10x faster than FSF lsof on my machines.

$ sleep 999 > test3 &
$ time lsof test3
COMMAND   PIDUSER   FD   TYPE DEVICE SIZE/OFF NODE NAME
sleep   31117 landley1w   REG8,10 11944128 test3

real0m3.134s
user0m0.988s
sys 0m2.084s
$ time ./lsof ../toy3/test3
COMMAND PID   USER   FD  TYPE DEVICE  SIZE/OFF
NODE NAME
sleep 31117landley1w  REG8,1 0
11944128 /home/landley/toybox/toy3/test3

real0m3.279s
user0m1.032s
sys 0m2.000s

$ ps ax |wc
3763041   98906

Same 3 seconds to iterate over 376 processes in the simple case. (lsof
-i takes 6 seconds, toybox doesn't have -i yet...)

> don't think lsof is high on the list of things to worry about. even
> for top, i'm more worried about the fact that it crashes if you leave
> it running long enough, or the broken ps -AT...)

I have a tab open for ps -AT, but I thought I'd gotten all the ps/top
crashes out of the way? Is that still a thing?

Rob
___
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net

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

[Rob Landley]

On 12/18/2017 07:07 PM, enh wrote:
> On Sun, Dec 17, 2017 at 9:40 AM, Rob Landley  wrote:
>> On 12/17/2017 11:20 AM, Rob Landley wrote:
>>> Sigh. You argument that "we can't just do something easily explained but
>>> must copy ubuntu's magic edge cases exactly" implies I need to
>>> understand ubuntu's magic edge cases, which are nonobvious.
>>
>> I should clarify: what I'm uncomfortable with is the need to read
>> multiple /proc files per process, because the speed of lsof is just
>> _sad_ and the cpu usage of top is unpleasant. (Part of the reason I
>> haven't finished cleaning up lsof yet is I still hope to be able to
>> speed it up somehow, which is a largeish time sink every time I turn my
>> attention to it.)
>>
>> Having killall need to do similar grinding over a large number of
>> processes seems unnecessary. That said, it looks like to match ubuntu's
>> killall we would need to read two files _and_ stat /proc/$$/exe.
> 
> that's still a lot lighter weight than all the work pgrep/pkill have
> to do, and it's what everyone's already living with anyway...

That and the "it doesn't have to be right or match a spec, it has to
match the other command's behavior", which is a nonobvious hairball once
I started looking at it:

  $ echo -e '#!/bin/bash\nsleep "$@"' > spleep
  $ chmod +x spleep
  $ spleep 123 &
  [1] 28391
  $ killall $PWD/spleep
  /home/landley/toybox/toy3/sub2/spleep: no process found

Because no process has /proc/*/exe pointing to that file's dev:inode
pair. The existing lib/ plumbing for this is _only_ checking names, not
checking this inode stuff. But this is in lib because it's shared
plumbing, pidof also uses it and the host version of that has -s to
check for scripts...

Sigh. Ok, it's been long enough I'm invoking the "apply the other
person's patch then fiddle on top of it" rule, but this absolute path
business is not what the other one's doing...

Rob
___
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net

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

[enh]

On Mon, Dec 18, 2017 at 5:07 PM, enh  wrote:
> On Sun, Dec 17, 2017 at 9:40 AM, Rob Landley  wrote:
>> On 12/17/2017 11:20 AM, Rob Landley wrote:
>>> Sigh. You argument that "we can't just do something easily explained but
>>> must copy ubuntu's magic edge cases exactly" implies I need to
>>> understand ubuntu's magic edge cases, which are nonobvious.
>>
>> I should clarify: what I'm uncomfortable with is the need to read
>> multiple /proc files per process, because the speed of lsof is just
>> _sad_ and the cpu usage of top is unpleasant. (Part of the reason I
>> haven't finished cleaning up lsof yet is I still hope to be able to
>> speed it up somehow, which is a largeish time sink every time I turn my
>> attention to it.)
>>
>> Having killall need to do similar grinding over a large number of
>> processes seems unnecessary. That said, it looks like to match ubuntu's
>> killall we would need to read two files _and_ stat /proc/$$/exe.
>
> that's still a lot lighter weight than all the work pgrep/pkill have
> to do, and it's what everyone's already living with anyway...

(and toybox lsof is still 10x faster than FSF lsof on my machines. i
don't think lsof is high on the list of things to worry about. even
for top, i'm more worried about the fact that it crashes if you leave
it running long enough, or the broken ps -AT...)

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



-- 
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
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net

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

[enh]

On Sun, Dec 17, 2017 at 9:40 AM, Rob Landley  wrote:
> On 12/17/2017 11:20 AM, Rob Landley wrote:
>> Sigh. You argument that "we can't just do something easily explained but
>> must copy ubuntu's magic edge cases exactly" implies I need to
>> understand ubuntu's magic edge cases, which are nonobvious.
>
> I should clarify: what I'm uncomfortable with is the need to read
> multiple /proc files per process, because the speed of lsof is just
> _sad_ and the cpu usage of top is unpleasant. (Part of the reason I
> haven't finished cleaning up lsof yet is I still hope to be able to
> speed it up somehow, which is a largeish time sink every time I turn my
> attention to it.)
>
> Having killall need to do similar grinding over a large number of
> processes seems unnecessary. That said, it looks like to match ubuntu's
> killall we would need to read two files _and_ stat /proc/$$/exe.

that's still a lot lighter weight than all the work pgrep/pkill have
to do, and it's what everyone's already living with anyway...

> 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
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net

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

[Rob Landley]

On 12/16/2017 09:43 PM, enh wrote:
> I don't think the world is helped by having two different imperfect
> heuristics, and I do think it will cause bug reports for us.

We _do_ have two imperfect heuristics. Checking the patch you sent
you're checking an exact match for argv[0] before looking at comm, and
your basename() logic only kicks in for absolute paths. So a command run
at a relative path won't do what ubuntu's does:

  $ ln -s $(which sleep) sleep
  $ sleep 999 &
  [1] 19487
  $ ./sleep 1000 &
  [2] 19490
  $ killall ./sleep
  [1]-  Terminated  sleep 999
  [2]+  Terminated  ./sleep 1000

Hmmm. What _is_ it doing?

  $ $PWD/sleep 998 &
  [1] 19916
  $ sleep 1001 &
  [2] 19917
  $ killall $PWD/sleep
  [1]-  Terminated  $PWD/sleep 998
  [2]+  Terminated  sleep 1001

Yeah, it doesn't look like it's ever taking the $PATH into account? But
then why...

  $ $PWD/sleep 998 &
  [1] 19945
  $ sleep 1001 &
  [2] 19946
  $ $PWD/../sleep 998 &
  bash: /home/landley/toybox/toy3/../sleep: No such file or directory

It's because it's a symlink, isn't it?

  $ killall sleep
  [1]-  Terminated  $PWD/sleep 998
  [2]+  Terminated  sleep 1001
  $ sleep 1001 &
  [1] 21634
  $ $PWD/sleep 998 &
  [2] 21637
  $ killall $PWD/sleep
  [2]+  Terminated  $PWD/sleep 998

Yes it is. It's comparing the inode.

  $ sleep 1001 &
  [1] 21738
  $ ./sleep 999 &
  [2] 21739
  $ killall sleep
  [1]-  Terminated  sleep 1001
  [2]+  Terminated  ./sleep 999

But ONLY for paths, not for simple names.

  $ sleep 1001 & ./sleep 999 &
  [1] 21758
  [2] 21759
  $ killall ./sleep
  [2]+  Terminated  ./sleep 999

Including relative paths, so the x[0] == '/' test is still wrong.

Sigh. You argument that "we can't just do something easily explained but
must copy ubuntu's magic edge cases exactly" implies I need to
understand ubuntu's magic edge cases, which are nonobvious.

Rob
___
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net

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

[enh]

I don't think the world is helped by having two different imperfect
heuristics, and I do think it will cause bug reports for us.

I also think if anyone wants sensible behavior (or more control) they
should probably use pgrep/pkill instead of killall... (Where the multi
argument semantics are already consistent across implementations.)

(Though that reminds me that our pgrep/pkill -a behaves differently to the
others, and -a doesn't show up in the help. But that hasn't broken anyone's
scripting that I'm aware of yet, so that can wait for another day...)


On Sat, Dec 16, 2017, 17:40 Rob Landley  wrote:

> On 12/16/2017 06:59 PM, enh wrote:
> > That's my point: there's no way to implement this correctly, so the
> > least worst choice is to just implement it the same as everyone else.
> > Whatever we do will be surprising in some instances, but "the same
> > surprises as the other systems" is at least manageable. I don't see how
> > anyone wins from toybox having a different but also broken heuristic.
>
> Grumble grumble wanna decent spec.
>
> Sigh. I'm annoyed by having to read two different files to do one thing
> but if you feel strongly about it...
>
> I still need to fix the basename() stuff and add lots of tests. I'll try
> to get something checked in tomorrow.
>
> Rob
>
___
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net

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

[Rob Landley]

On 12/16/2017 06:59 PM, enh wrote:
> That's my point: there's no way to implement this correctly, so the
> least worst choice is to just implement it the same as everyone else.
> Whatever we do will be surprising in some instances, but "the same
> surprises as the other systems" is at least manageable. I don't see how
> anyone wins from toybox having a different but also broken heuristic. 

Grumble grumble wanna decent spec.

Sigh. I'm annoyed by having to read two different files to do one thing
but if you feel strongly about it...

I still need to fix the basename() stuff and add lots of tests. I'll try
to get something checked in tomorrow.

Rob
___
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net

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

[enh]

That's my point: there's no way to implement this correctly, so the least
worst choice is to just implement it the same as everyone else. Whatever we
do will be surprising in some instances, but "the same surprises as the
other systems" is at least manageable. I don't see how anyone wins from
toybox having a different but also broken heuristic.

On Sat, Dec 16, 2017, 15:13 Rob Landley  wrote:

> 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  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
>
___
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net

Re: [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  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
___
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net

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

[enh]

Found running LTP file system tests on Android.

Bug: http://b/70627145
---
 lib/lib.c  | 28 +++-
 tests/killall.test | 14 ++
 2 files changed, 33 insertions(+), 9 deletions(-)
 create mode 100644 tests/killall.test
From 8ed2c141c3ef516ba0698d2a771e0869df09e276 Mon Sep 17 00:00:00 2001
From: Elliott Hughes 
Date: Wed, 13 Dec 2017 11:47:08 -0800
Subject: [PATCH] killall should kill scripts too.

Found running LTP file system tests on Android.

Bug: http://b/70627145
---
 lib/lib.c  | 28 +++-
 tests/killall.test | 14 ++
 2 files changed, 33 insertions(+), 9 deletions(-)
 create mode 100644 tests/killall.test

diff --git a/lib/lib.c b/lib/lib.c
index 651593e..44a7cc7 100644
--- a/lib/lib.c
+++ b/lib/lib.c
@@ -1010,6 +1010,12 @@ char *getbasename(char *name)
   return name;
 }
 
+static int argv0_match(char *cmd, char *name)
+{
+  return (*name == '/' ? !strcmp(cmd, name)
+  : !strcmp(getbasename(cmd), getbasename(name)));
+}
+
 // Execute a callback for each PID that matches a process name from a list.
 void names_to_pid(char **names, int (*callback)(pid_t pid, char *name))
 {
@@ -1020,17 +1026,21 @@ void names_to_pid(char **names, int (*callback)(pid_t pid, char *name))
 
   while ((entry = readdir(dp))) {
 unsigned u;
-char *cmd, **curname;
+char *cmd, *comm, **cur;
 
 if (!(u = atoi(entry->d_name))) continue;
-sprintf(libbuf, "/proc/%u/cmdline", u);
-if (!(cmd = readfile(libbuf, libbuf, sizeof(libbuf continue;
-
-for (curname = names; *curname; curname++)
-  if (**curname == '/' ? !strcmp(cmd, *curname)
-  : !strcmp(getbasename(cmd), getbasename(*curname)))
-if (callback(u, *curname)) break;
-if (*curname) break;
+
+// For a script, comm and argv[1] will match (argv[0] will be the interp).
+sprintf(libbuf, "/proc/%u/comm", u);
+if (!(comm = readfile(libbuf, libbuf, sizeof(libbuf continue;
+sprintf(libbuf+16, "/proc/%u/cmdline", u);
+if (!(cmd = readfile(libbuf+16, libbuf+16, sizeof(libbuf)-16))) continue;
+
+for (cur = names; *cur; cur++)
+  if (argv0_match(cmd, *cur) ||
+  (!strncmp(comm, *cur, 15) && argv0_match(cmd+strlen(cmd)+1, *cur)))
+if (callback(u, *cur)) break;
+if (*cur) break;
   }
   closedir(dp);
 }
diff --git a/tests/killall.test b/tests/killall.test
new file mode 100644
index 000..100ac7c
--- /dev/null
+++ b/tests/killall.test
@@ -0,0 +1,14 @@
+#!/bin/bash
+
+[ -f testing.sh ] && . testing.sh
+
+#testing "name" "command" "result" "infile" "stdin"
+
+echo "#!/bin/sh
+yes > /dev/null" > toybox.killall.test.script
+chmod a+x toybox.killall.test.script
+
+./toybox.killall.test.script &
+testing "script" "killall toybox.killall.test.script && echo killed ; pgrep -l toybox.killall.test.script || echo really" "killed\nreally\n" "" ""
+
+rm -f toybox.killall.test.script
-- 
2.15.1.504.g5279b80103-goog

___
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net