On 11/22/2013 12:26:14 AM, Ashwini Sharma wrote:
Hi All,
Due to some issue with my email account, changing my email to
ak.ashwini1...@gmail.com
Finally catching up on my email myself. :)
I came across a bug in pidof command. The issue is
with _-o_ option. callback function returns _1_ for omit
list, to names_to_pid().
Sigh. names_to_pid() is in pending, a command not in pending is calling
it...
(My bad.)
names_to_pid(), breaks the internal _for_ loop, at the same
time it also breaks out of the _while(readdir())_ loop.
Which is causing the issue, as other pids are not searched for.
Indeed, it should return 0 not 1.
for (curname = names; *curname; curname++)
if (**curname == '/' ? !strcmp(cmd, *curname)
: !strcmp(basename(cmd), basename(*curname)))
if (callback(u, *curname)) break;
- if (*curname) break;
}
Why break from _while_ loop, as there can be other processes in the
list
waiting for actioin.
removing this fixes the issue.
Returning nonzero says to abort the loop. We shouldn't return nonzero
if we want additional callbacks callbacks.
_names_to_pid()_ is also used by _killall_. killall also has issues,
1. the exit status is always > 0, thats because of toys.exitval++
in _main() function. Exit status need to be corrected.
Actually it's supposed to exit 1 if no processes were found, and 0 if
it could kill at least one process. I'll have it zero out the exit code
if a signal could successfully be delivered.
2. No error/warning message is displayed for a non-exiting process.
as is one by GNU implementation. instead due to the _toys.exitval++_
its always printing __ No such process __ without any process name.
Actually it prints ("bad %u", pid)?
ret = kill(pid, TT.signum);
if (ret == -1 && !(toys.optflags & FLAG_q))
error_msg("bad %u", (unsigned)pid);
That looks like it should be perror_msg() so it reports the errno
reason.
The "no such process" message was always printed because the exit value
was never cleared.
No, the problem is if you kill things that don't exist in the ubuntu
version:
$ killall whackamole froglegs
whackamole: no process found
froglegs: no process found
You get an error message for each one. And this infrastructure isn't
tracking that. Although to be honest, the upstream infrastructure is
lying:
$ killall init whackamole
init(1): Operation not permitted
init: no process found
whackamole: no process found
And the errno behavior is slightly non-obvious:
$ sleep 9999 &
[1] 8016
$ killall init sleep whackamole
init(1): Operation not permitted
init: no process found
whackamole: no process found
[1]+ Terminated sleep 9999
$ echo $?
1
Rummage, rummage... Ok, if there was a command it couldn't kill
(including "not found"), errno is 1. Order doesn't matter.
Ideally I'd save the errno of each entry so the message was right, but
I don't want to mark up optargs[] (and if I did it'd screw up killing a
second command with the same name), and can't conveniently use toybuf
as a bitfield for two reasons (already used for a message right now,
not necessarily long enough).
Speaking of not necessarily long enough:
if (toys.optflags & FLAG_i) {
sprintf(toybuf, "Signal %s(%d) ?", name, pid);
Bad because toybuf is 4k and name is environment data that could be
128k, so somebody can trivially manipulate a heap overflow out of that.
Right, if I switch that to xsmprintf() I can use toybuf. I can save the
start of the name array into the globals and then do "name-start" to
get the offset into the array. If I use it as a bitfield I can
reproduce the original (broken) behavior, and can track 32768 commands,
which is probably more than anyone will ever use. (With 128k
environment space they'd all have to be 3 character command names to
_fill_ that, you'd need 2 chars to surpass it...)
If I use it as an array to store the errno... does it need to be short
or char?
$ cat arch/*/include/uapi/asm/errno* | awk '{print $3}' | sort -n
...
255
256
257
516
1133
Sigh. Those last few aren't relevant (I grepped, they won't be produced
by kill(2)), but from a complexity level I don't want to have to
explain it in a giant comment, nor am I quite comfortable with 4096
entries as a limit where behavior magically changes.
Screw it: malloc an array.
The next question is, how do I get the position in the array? I was
thinking I could do pointer subtraction but that gives me how many
bytes of string data were used. Hack up names_to_pid() to pass it?
(Only currently two users...) Do a for loop checking?
Eh, I'll just do a for loop. Pathological case is less than 60k command
line, and I don't have to compare strings (just pointers).
In order not to modify the lib function _names_to_pid_ too much, these
killall issues could be fixed from callback function _kill_process()_.
Modifying the callback is the correct approach, yes. The names_to_pid()
function just iterates over the array we give it to find pids, there
shouldn't be any logic about what use we put the results to.
For printing the warning message for non existent processes, can have
a map
for the processes which are signalled thru _kill_process_ and finally
once
_names_to_pid_ returns, print the message for non-signalled processes.
Like to have your opinion.
I implemented the array version mentioned above before I actually read
this far in the message. :)
I need to debug a bit before checking in and it's bus-to-work time, but
I'll see if I can get this checked in tonight.
Rob
_______________________________________________
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net
