[Toybox] [TOYBOX] [PATCH] pidof ignores full pathname

[Rob Landley]

Lukasz Skalski <l.skalski at partner.samsung.com> said:
> Description of problem:
> 
> Pidof can't get the pid when using full path.

Sigh. lib/pending.c is the place for library code I'm still not  
comfortable with. Let's see...

The guts of this function are reimplementing readfile(), which implies  
readfile isn't flexible enough and should be able to take a buffer the  
way readline() can. (And then malloc its own buffer if that buffer is  
NULL.) I don't hugely care about it returning the length (it's a null  
terminated string, don't use it for stuff that can have another null in  
it).

The name of the function is way too long, let's just call it  
name_to_pid() and rename the two existing users...

I'm a bit uncomfortable declaring a 4k buffer on the stack. I'm aware  
linux userspace generally has plenty of space for this, but a lot of  
environments don't (kernel, u-boot, thread stacks, nommu systems) and  
I'm trying to be agnostic about where this code gets reused from. (This  
is exactly the sort of thing toybuf is for, except that's left free for  
command code to use, so library functions can't use it. Possibly I  
should look to see if a libbuf would make sense?)

> Manpage says:
> 
> "[...] When pidof is invoked with a full pathname to the program it  
> should
> find the pid of, it is reasonably safe. Otherwise it is possible that  
> it
> returns pids of running programs that happen to have the same name as  
> the
> program you're after but are actually other programs [...]"
> 
> This patch fix above problem in for_each_pid_with_name_in() function  
> (lib/pending.c).

Doesn't calling basename on both entries defeat the whole "reasonably  
safe" bit you just quoted?

Ok, I can compare the first char to '/' to find absolute paths, but  
what do I do with a relative path? It's still specifying a specific  
file, but we have to normalize it against cwd of the process to get the  
absolute path. Also, what about "/usr/../bin/ls"? Should we require the  
caller to supply normalized absolute paths, or convert them on the fly  
here? If we do convert them, is storing the pointer in the original  
array acceptable, or should I malloc a new one? Hmmm... I should _not_  
do realpath if the kernel doesn't, because busybox cares what the  
symlink was named.

Ah, the old tension between "do it properly" and "be simple". I guess  
for the moment starting with / means compare the whole thing, not  
starting with / means compare.

As long as I'm renaming the function: system calls return 0 for success  
and -1 for failure, let's make 0 the default return so we have the  
option in future of specifying different non-default behaviors (or at  
least making the return code mean something specific).

While adjusting the callers: killall.c resets toys.exitval to 0 each  
time it tries to kill something. This means its return code can only be  
failure if the _last_ thing it tried to kill didn't work, that's  
probably wrong. It defaults to 0, so let's just yank that line...

Also, use toybox's error_msg() instead of libc's perror(), and print  
the error before the success message in the -v case (calling printf  
changes errno)...

Right, let's check this in before going _too_ far down that tangent.

Rob
 1378248714.0