Re: [PECL-DEV] safe_shell_exec extension for consideration

Stig Bakken wrote:

> Hehe, alright.  I just have a thing about choosing descriptive names 
> that do not promise too much or little :-)

I definitely lose on all counts then.  While what I wrote is safer than 
involving a shell, it's not "safe" in that a caller could still use it 
to accomplish something dangerous.  Further, it explicitly doesn't 
involve a shell and I stuck the word "shell" in there. 
somewhat_safer_non_shell_exec() may have been more accurate :)

...snipped...

> Actually, system() and/or proc_open() would be better PHP functions to 
> mimic/overload, since there would be no shell involved here.

If you have the time, check out the stuff I wrote.  Notice I also allow 
an optional "flags" parameter that a caller can pass as either 
MERGE_STDERR or DISCARD_STDERR.  This was to allow for existing code 
that was calling shell_exec() and depending on shell I/O redirection 
(2>&1 or 2>/dev/null, respectively).  Would you want these options also 
added to proc_open() but only used with the code I add?  If I overload 
proc_open(), would I make the array arguments only valid if bypass_shell 
were set, or would that remain a Windows-only option?

> 
> I think it makes sense for you to write this code as an extension (to 
> make it available for everyone), but at the same make a patch for PHP's 
> built-in function(s).

I'm embarrassed to admit that I don't know how this would work.  Will 
nothing break if I patch PHP's built-in functions but also provide the 
same interface via an extension?


> Using an array parameter is definitely the most 
> intuitive approach IMHO.
> 

I'm glad you agree on this.  It took me a while to come back around to 
this mindset, but having implemented and modified a bunch of existing 
code to work with it, it's much nicer.  Also, I fixed a couple 
pre-existing bugs in the PHP code we're installing where they weren't 
properly escaping whitespace so filenames with spaces would break.  My 
initial code that accepted a string and then parsed it to create an 
*argv[] was working for the code I had to run through it, but I could 
see it breaking if set loose in the wild.


> The code would be a bunch of parameter conversions (to turn the string 
> array into va_arg()), a fork() and execv() call, some file descriptor 
> fiddling to create a php stream from the forked process's standard 
> output, and a waitpid() call collecting exit status.

Again, take a look at what I already wrote.  This is pretty much all 
there, with the exception of a few terminology differences that might 
just be typos on your part.

execvp() takes an argument vector as its 2nd argument and doesn't use 
the va_args interface, so building this array was fairly simple.  I have 
a statically allocated *char[] and I copy all of the char * address 
pointers from the array zval into my array which I pass to execvp().

The file descriptor fiddling is already in place, but it has no 
knowledge of a PHP stream.  All I did was to open a pipe from the 
parent, close the write end in the parent, close the read end in the 
child and then dup2 stdout/stderr in the child to the write end of the 
pipe.  This is where my MERGE_STDERR and DISCARD_STDERR flags can be used.

The waitpid is there to prevent any zombie accumulation, but because I 
copied the notion of shell_exec() returning the command's output as a 
string, it has no way to provide the exit status to the caller and does 
nothing with it.

Considering that I already have all of this implemented, it wouldn't be 
a ton of work for me to merge it into some existing function or 
extension.  As you can tell from all of my questions, I'm going to need 
a little direction on exactly where you want it to go and how you want 
the interface to work.  I suspect we'll need a bit more back and forth 
before we decide on the details.

Thanks for your feedback and your willingness to assist with this.  If 
we get too mired in details, you can take things off-list until we get 
something solid in place.  I'll leave that up to you.

Dave