Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:45033
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: unknown (pb1.pair.com: domain php.net does not designate 212.55.154.26 as permitted sender)
Message-ID: <57BB047CFDBE494FBAC57E993B455B7F@PC3EE1F19287>
To: "Gwynne Raskind" <gwynne@darkrainfall.org>
Cc: "PHP internals" <internals@lists.php.net>
References: <E2310D92-1B79-428A-ABB0-918E9488C21E@darkrainfall.org> <8B30398EC4FD40AC9A8FE70E8B0E114C@PC3EE1F19287> <48A0063A-8A7E-4A37-BCD2-31FA8374EC89@darkrainfall.org>
Date: Sun, 19 Jul 2009 19:13:28 +0100
MIME-Version: 1.0
Content-Type: text/plain;
	format=flowed;
	charset="iso-8859-1";
	reply-type=response
Content-Transfer-Encoding: 7bit
Subject: Re: [PHP-DEV] A patch for HEAD
From: nlopess@php.net ("Nuno Lopes")

Commited!

Thanks,
Nuno

----- Original Message -----
> 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