Re: [Toybox] [PATCH] killall should kill scripts too.
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 %*(#(% logic! $ cd sub2 $ pidof ../sub1/sleep $ killall ../sub1/sleep [1]- Terminated sub1/sleep 999 (wd: ~/toybox/clean) (wd now: ~/toybox/clean/sub2) $ So that's nice. The question is which subset of this behavior to implement. I'm leaning towards just keeping what I've already done and calling it good, but I'm trying to write tests that both the host version and the toybox version pass, while testing all the behavioral corner cases I care about, without the script equivalent of #ifdeffery. So how's your holiday going? :) > 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. Did it? I haven't seen this one, and it's the first I remember hearing of it... 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.
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 0x0c3a800e17f0: 00 00 00 00 00 00 00 00 00 00 0
Re: [Toybox] [PATCH] killall should kill scripts too.
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.
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.
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, &st1)) { +char buf[32]; + +sprintf(buf, "/proc/%u/exe"); +if (stat(buf, &st1)) 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, &len))) continue; +// readfile only guarnatees one null terminator and we need
Re: [Toybox] [PATCH] killall should kill scripts too.
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.
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.
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.
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.
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. 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.
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.
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.
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.
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.
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.
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. 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. ___ Toybox mailing list Toybox@lists.landley.net http://lists.landley.net/listinfo.cgi/toybox-landley.net
Re: [Toybox] [PATCH] killall should kill scripts too.
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.) (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. :) 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. :) Rob ___ Toybox mailing list Toybox@lists.landley.net http://lists.landley.net/listinfo.cgi/toybox-landley.net