Re: [PHP-DEV] A patch for HEAD
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
gwy...@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
Re: [PHP-DEV] A patch for HEAD
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 gwy...@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
Re: [PHP-DEV] A patch for HEAD
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 gwy...@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
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] A patch for HEAD
The idea is great. In fact this was in my todo list for php 5.3..
Please give me a few more days to review the patch.
Nuno
P.S.: you can add on more point to your list: you get to know the PID of the
exec'ed process instead of the PID of the shell.
- Original Message -
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
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
[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
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] A patch for HEAD
Gwynne Raskind wrote: 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?). I'm fine with this in HEAD. -Andrei -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] A patch for HEAD
same here :) On Fri, Jun 26, 2009 at 11:32 PM, Andrei Zmievskiand...@gravitonic.com wrote: Gwynne Raskind wrote: 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?). I'm fine with this in HEAD. -Andrei -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php -- Pierre http://blog.thepimp.net | http://www.libgd.org -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
