Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:44996
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: error (pb1.pair.com: domain darkrainfall.org from 208.97.132.81 cause and error)
To: Nuno Lopes <nlopess@php.net>
In-Reply-To: <8B30398EC4FD40AC9A8FE70E8B0E114C@PC3EE1F19287>
References: <E2310D92-1B79-428A-ABB0-918E9488C21E@darkrainfall.org> <8B30398EC4FD40AC9A8FE70E8B0E114C@PC3EE1F19287>
Message-ID: <48A0063A-8A7E-4A37-BCD2-31FA8374EC89@darkrainfall.org>
Content-Type: text/plain; charset=US-ASCII; format=flowed; delsp=yes
Content-Transfer-Encoding: 7bit
Mime-Version: 1.0 (Apple Message framework v935.3)
Date: Thu, 16 Jul 2009 18:23:41 -0400
Cc: "PHP internals" <internals@lists.php.net>
Subject: Re: [PHP-DEV] A patch for HEAD
From: gwynne@darkrainfall.org (Gwynne Raskind)

Alright, I've implemented pretty much every single suggestion you  
made :). Note that _php_array_to_envp() actually suffers the same  
"modify in place" problem, but because it iterates the array twice (is  
that REALLY so much better than allocating a few extra pointers or  
calling perealloc() a few times?), the fix for it isn't trivial, so I  
left it alone. The updated patch is (again) posted at:

http://darkrainfall.org/php-5.3-shellbypass.patch

On Jul 15, 2009, at 4:20 PM, Nuno Lopes wrote:
> Hi,
>
> So the patch looks generally good. Here are some minor comments  
> about it:
>
> - I believe _php_array_to_argv() doesn't need TSRMLS_DC. If that's  
> the case, please remove it.
> - in _php_array_to_argv() you modify the input array destructively  
> (when calling convert_to_string_ex). You should not modify the input.
> - in _php_array_to_argv(), HASH_OF will never return NULL (because  
> we already know that it's an array).
> - apparently the check 'if (cnt < 1) {' will never be true, as the  
> array will always have one element. please verify that this special  
> case does what you want (maybe change to 'cnt <= 1')
> - argvs with length==0 are perfectly valid, and so you shouldn't  
> skip them.
> - the api is a little inconsistent, as you use the idx 0 to retrieve  
> the command name, but then you use the array order to retrieve the  
> rest of the elements (and thus if the idx 0 doesn't appear in the  
> beginning your code will fail). I would just stick with the array  
> order. e.g. array(1=>'/usr/bin/echo', 0=>'foo') or similar should  
> print 'foo'.
> - in exit_fail you can remove the check for bypass_shell, as  
> _php_free_argv() will check the argument against NULL.
> - the line 'proc->argv = (bypass_shell ? child_argv : NULL);' can be  
> simplified to 'proc->argv = child_argv;' since child_argv is already  
> initialized to NULL
>
> I think that's it :)  It's only minor things, I guess.
> As soon as you fix these things, please go ahead and commit the  
> patch, or mail it back to the ML in case you need me to do it.
>
> Nuno
>
>
> ----- Original Message ----- From: "Gwynne Raskind" <gwynne@darkrainfall.org 
> >
> To: "PHP internals" <internals@lists.php.net>
> Sent: Friday, June 26, 2009 10:20 PM
> Subject: [PHP-DEV] A patch for HEAD
>
>
>> I've just finished making this patch for my own use (diffed  
>> against  5.3 CVS):
>>
>> http://darkrainfall.org/php-5.3-shellbypass.patch
>>
>> In short, what it does is make proc_open()'s shell_bypass option  
>> available to UNIX systems. This is accomplished by allowing the   
>> "command" parameter to proc_open() to be an array of arguments to  
>> pass  to execv[e](). I've included a few tests to check the  
>> functionality.
>>
>> (A few more tests could be devised to, for example, check that the  
>> correct warning is issued if you pass an array without  
>> bypass_shell  set, or a string with it set, etc.)
>>
>> The exact behavior of the argument array is:
>> 1) The array must contain at least one element, at index 0.
>> 2) The element at index 0 is always the exact command path passed  
>> to execv[e]() (after being filtered through any safe_mode  
>> restrictions,  as with the normal behavior of proc_open()).
>> 3) Any other elements form the argv array passed to execv[e](). By  
>> convention the first of these arguments (argv[0] in the child  
>> process)  is the same as the command path, however my patch does  
>> NOT enforce or  assume this; it simply calls execv[e] 
>> ($argument_array[0], array_slice($argument_array, 1)).
>>
>> This patch currently provides the only useful way to fork a process  
>> without running a shell (pcntl_fork() + pcntl_exec() are useless  
>> since there's no pcntl_dup2() to control the descriptors of the  
>> child).
>>
>> Why would you want to avoid the shell?
>>
>> - Efficiency. The shell is an extra, often unnecessary process,  
>> which must parse the commandline given to it into individual  
>> arguments according to all its various rules. Not to mention the  
>> overhead of setting up another entire process just to run a third  
>> process.
>>
>> - Resource control. The shell is an extra process. If you don't  
>> need  it, and your system is tight on process space, best to avoid  
>> it.
>>
>> - Sanity. Correctly quoting arguments to a shell command ranges  
>> from mildly annoying (escapeshellarg() in simple cases) to  
>> nightmarish  (manual parsing of a string in some edge cases).  
>> Passing arguments  directly completely bypasses this, quite  
>> possibly saving you quite a  bit of string parsing time if you were  
>> doing something like  "$shell_args = implode(' ',  
>> array_map('escapeshellarg', $raw_args));".
>>
>> - Oddly enough, security. Since there's no shell, it's more  
>> difficult  to subvert the child process to do other things than the  
>> coder  intended (unless of course, said coder executes a shell this  
>> way).
>>
>> This patch does nothing on Windows, since the option was already  
>> implemented there. It also does nothing on Netware, since from what  
>> I could see in the code, Netware doesn't have a shell in the first  
>> place.
>>
>> I'm proposing the inclusion of this patch in HEAD (which I'll port  
>> it  to if I get a thumbs-up here), and possibly 5.3.2. Criticism  
>> and angry flames welcome. Constructive critcism and good-natured  
>> comments will  be ignored ;) (just kidding... or am I?).
>>
>> -- Gwynne 
>

-- Gwynne