Skip to content

Add $result_code parameter to shell_exec #7663

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Add $result_code parameter to shell_exec #7663

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
763deb4
Add $result_code parameter to shell_exec
l-pt Nov 18, 2021
717a4f6
add test case for shell_exec() with $result_code set
l-pt Nov 18, 2021
eac1217
make shell_exec() stub consistent with system, passthru, exec and reg…
l-pt Nov 18, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions ext/standard/basic_functions.stub.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1004,8 +1004,11 @@ function escapeshellcmd(string $command): string {}
/** @refcount 1 */
function escapeshellarg(string $arg): string {}

/** @refcount 1 */
function shell_exec(string $command): string|false|null {}
/**
* @param int $result_code
* @refcount 1
*/
function shell_exec(string $command, &$result_code = null): string|false|null {}

#ifdef HAVE_NICE
function proc_nice(int $priority): bool {}
Expand Down
3 changes: 2 additions & 1 deletion ext/standard/basic_functions_arginfo.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/* This is a generated file, edit the .stub.php file instead.
* Stub hash: 96393b26861733e5c7fc9a12095a29c07ed73928 */
* Stub hash: 57b39034cfe4b7421db6a6cc09ba4b086e95d646 */

ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(arginfo_set_time_limit, 0, 1, _IS_BOOL, 0)
ZEND_ARG_TYPE_INFO(0, seconds, IS_LONG, 0)
Expand Down Expand Up @@ -1166,6 +1166,7 @@ ZEND_END_ARG_INFO()

ZEND_BEGIN_ARG_WITH_RETURN_TYPE_MASK_EX(arginfo_shell_exec, 0, 1, MAY_BE_STRING|MAY_BE_FALSE|MAY_BE_NULL)
ZEND_ARG_TYPE_INFO(0, command, IS_STRING, 0)
ZEND_ARG_INFO_WITH_DEFAULT_VALUE(1, result_code, "null")
ZEND_END_ARG_INFO()

#if defined(HAVE_NICE)
Expand Down
11 changes: 9 additions & 2 deletions ext/standard/exec.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -515,11 +515,15 @@ PHP_FUNCTION(shell_exec)
FILE *in;
char *command;
size_t command_len;
zval *ret_code = NULL;
zend_string *ret;
php_stream *stream;
int ret_pclose;

ZEND_PARSE_PARAMETERS_START(1, 1)
ZEND_PARSE_PARAMETERS_START(1, 2)
Z_PARAM_STRING(command, command_len)
Z_PARAM_OPTIONAL
Z_PARAM_ZVAL(ret_code)
ZEND_PARSE_PARAMETERS_END();

if (!command_len) {
Expand All @@ -542,8 +546,11 @@ PHP_FUNCTION(shell_exec)

stream = php_stream_fopen_from_pipe(in, "rb");
ret = php_stream_copy_to_mem(stream, PHP_STREAM_COPY_ALL, 0);
php_stream_close(stream);
ret_pclose = php_stream_close(stream);

if (ret_code) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't this evaluate to false if ret_code === 0?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If so, is exec not also affected by this?

php-src/ext/standard/exec.c

Lines 246 to 248 in 41d2785

if (ret_code) {
ZEND_TRY_ASSIGN_REF_LONG(ret_code, ret);
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TL;DR: this code is fine.
[First, that's C not PHP, so there's no "===" operator; second,] ret_code is a zval * i.e. a pointer to zval, so if (ret_code) means if (ret_code != NULL) i.e. (here) the PHP function was called with a 2nd argument (a variable into which to copy the value of ret_pclose).

ZEND_TRY_ASSIGN_REF_LONG(ret_code, ret_pclose);
}
if (ret && ZSTR_LEN(ret) > 0) {
RETVAL_STR(ret);
}
Expand Down
27 changes: 27 additions & 0 deletions ext/standard/tests/misc/shell_exec_variation.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
--TEST--
shell_exec() with exit code setting
--SKIPIF--
<?php
if (!is_executable("/bin/true")) echo "skip";
if (!function_exists("shell_exec")) echo "skip shell_exec() is not available";
?>
--FILE--
<?php
$retcode = null;

$commands = [
"/bin/true",
"non-existent-command > /dev/null 2>&1"
];

foreach ($commands as $command) {
shell_exec($command, $retcode);
var_dump($retcode);
}

echo "Done\n";
?>
--EXPECT--
int(0)
int(127)
Copy link
Contributor

@divinity76 divinity76 Nov 27, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't this 127 on unix-like OS's (Linux, MacOS, BSD),
but just 1 on Windows ? then again i wouldn't expect to find /bin/true on Windows

it is possible to make /bin/true executable on windows tho:

C:\>mkdir bin

C:\>cd bin

C:\bin>copy C:\Windows\System32\PING.EXE .
        1 file(s) copied.

C:\bin>move PING.EXE true.exe
        1 file(s) moved.

C:\bin>php -r "var_dump(is_executable('/bin/true'));"
bool(false)

C:\bin>move true.exe true
        1 file(s) moved.

C:\bin>php -r "var_dump(is_executable('/bin/true'));"
bool(true)

Done