ext/pcntl: Refactor pcntl_exec() (php#17172)

* Refactor usage of strlcpy

As we allocate the buffer, we know the string will fit inside the buffer.

* Throw ValueErrors when strings contain null bytes

The underlying C calls work with C strings, which are NULL terminated.

* exec_pcntl() always return false

Thus, update stubs to formally have a return type of `false`.

Author: Girgias

UPGRADING

@@ -42,6 +42,12 @@ PHP 8.5 UPGRADE NOTES
   . ldap_get_option() and ldap_set_option() now throw a ValueError when
     passing an invalid option.
 
+- PCNTL:
+  . pcntl_exec() now throws ValueErrors when entries of the $args parameter
+    contain null bytes.
+  . pcntl_exec() now throws ValueErrors when entries or keys of the
+    $env_vars parameter contain null bytes.
+
 - SPL:
   . ArrayObject no longer accepts enums, as modifying the $name or $value
     properties can break engine assumptions.
@@ -91,6 +97,9 @@ PHP 8.5 UPGRADE NOTES
     gzfile, gzopen and readgzfile functions had been changed
     from int to boolean.
 
+- PCNTL:
+  . pcntl_exec() now has a formal return type of false.
+
 - PDO_PGSQL:
   . PDO::pgsqlCopyFromArray also supports inputs as Iterable.
   . Pdo\Pgsql::setAttribute and Pdo\Pgsql::prepare supports

ext/pcntl/pcntl.c

@@ -620,44 +620,44 @@ PHP_FUNCTION(pcntl_wstopsig)
 /* {{{ Executes specified program in current process space as defined by exec(2) */
 PHP_FUNCTION(pcntl_exec)
 {
-	zval *args = NULL, *envs = NULL;
+	zval *args = NULL;
+	HashTable *env_vars_ht = NULL;
 	zval *element;
-	HashTable *args_hash, *envs_hash;
-	int argc = 0, argi = 0;
-	int envc = 0, envi = 0;
 	char **argv = NULL, **envp = NULL;
-	char **current_arg, **pair;
-	size_t pair_length;
-	zend_string *key;
 	char *path;
 	size_t path_len;
-	zend_ulong key_num;
 
 	ZEND_PARSE_PARAMETERS_START(1, 3)
 		Z_PARAM_PATH(path, path_len)
 		Z_PARAM_OPTIONAL
 		Z_PARAM_ARRAY(args)
-		Z_PARAM_ARRAY(envs)
+		Z_PARAM_ARRAY_HT(env_vars_ht)
 	ZEND_PARSE_PARAMETERS_END();
 
-	if (ZEND_NUM_ARGS() > 1) {
+	if (args != NULL) {
+		// TODO Check array is a list?
 		/* Build argument list */
 		SEPARATE_ARRAY(args);
-		args_hash = Z_ARRVAL_P(args);
-		argc = zend_hash_num_elements(args_hash);
+		const HashTable *args_ht = Z_ARRVAL_P(args);
+		uint32_t argc = zend_hash_num_elements(args_ht);
 
+		/* We want a NULL terminated array of char* with the first entry being the path,
+		 * followed by the arguments */
 		argv = safe_emalloc((argc + 2), sizeof(char *), 0);
-		*argv = path;
-		current_arg = argv+1;
-		ZEND_HASH_FOREACH_VAL(args_hash, element) {
-			if (argi >= argc) break;
+		argv[0] = path;
+		char **current_arg = argv+1;
+		ZEND_HASH_FOREACH_VAL(args_ht, element) {
 			if (!try_convert_to_string(element)) {
 				efree(argv);
 				RETURN_THROWS();
 			}
+			if (zend_str_has_nul_byte(Z_STR_P(element))) {
+				zend_argument_value_error(2, "individual argument must not contain null bytes");
+				efree(argv);
+				RETURN_THROWS();
+			}
 
 			*current_arg = Z_STRVAL_P(element);
-			argi++;
 			current_arg++;
 		} ZEND_HASH_FOREACH_END();
 		*current_arg = NULL;
@@ -667,39 +667,51 @@ PHP_FUNCTION(pcntl_exec)
 		argv[1] = NULL;
 	}
 
-	if ( ZEND_NUM_ARGS() == 3 ) {
+	if (env_vars_ht != NULL) {
 		/* Build environment pair list */
-		SEPARATE_ARRAY(envs);
-		envs_hash = Z_ARRVAL_P(envs);
-		envc = zend_hash_num_elements(envs_hash);
+		char **pair;
+		zend_ulong key_num;
+		zend_string *key;
 
-		size_t envp_len = (envc + 1);
+		/* We want a NULL terminated array of char* */
+		size_t envp_len = zend_hash_num_elements(env_vars_ht) + 1;
 		pair = envp = safe_emalloc(envp_len, sizeof(char *), 0);
 		memset(envp, 0, sizeof(char *) * envp_len);
-		ZEND_HASH_FOREACH_KEY_VAL(envs_hash, key_num, key, element) {
-			if (envi >= envc) break;
-			if (!key) {
-				key = zend_long_to_str(key_num);
-			} else {
-				zend_string_addref(key);
+		ZEND_HASH_FOREACH_KEY_VAL(env_vars_ht, key_num, key, element) {
+			zend_string *element_str = zval_try_get_string(element);
+			if (element_str == NULL) {
+				goto cleanup_env_vars;
 			}
 
-			if (!try_convert_to_string(element)) {
-				zend_string_release(key);
+			if (zend_str_has_nul_byte(element_str)) {
+				zend_argument_value_error(3, "value for environment variable must not contain null bytes");
+				zend_string_release_ex(element_str, false);
 				goto cleanup_env_vars;
 			}
 
+			/* putenv() allows integer environment variables */
+			if (!key) {
+				key = zend_long_to_str((zend_long) key_num);
+			} else {
+				if (zend_str_has_nul_byte(key)) {
+					zend_argument_value_error(3, "name for environment variable must not contain null bytes");
+					zend_string_release_ex(element_str, false);
+					goto cleanup_env_vars;
+				}
+				zend_string_addref(key);
+			}
+
 			/* Length of element + equal sign + length of key + null */
-			ZEND_ASSERT(Z_STRLEN_P(element) < SIZE_MAX && ZSTR_LEN(key) < SIZE_MAX);
-			*pair = safe_emalloc(Z_STRLEN_P(element) + 1, sizeof(char), ZSTR_LEN(key) + 1);
-			pair_length = Z_STRLEN_P(element) + ZSTR_LEN(key) + 2;
-			strlcpy(*pair, ZSTR_VAL(key), ZSTR_LEN(key) + 1);
-			strlcat(*pair, "=", pair_length);
-			strlcat(*pair, Z_STRVAL_P(element), pair_length);
+			*pair = safe_emalloc(ZSTR_LEN(element_str) + 1, sizeof(char), ZSTR_LEN(key) + 1);
+			/* Copy key=element + final null byte into buffer */
+			memcpy(*pair, ZSTR_VAL(key), ZSTR_LEN(key));
+			(*pair)[ZSTR_LEN(key)] = '=';
+			/* Copy null byte */
+			memcpy(*pair + ZSTR_LEN(key) + 1, ZSTR_VAL(element_str), ZSTR_LEN(element_str) + 1);
 
 			/* Cleanup */
-			zend_string_release_ex(key, 0);
-			envi++;
+			zend_string_release_ex(key, false);
+			zend_string_release_ex(element_str, false);
 			pair++;
 		} ZEND_HASH_FOREACH_END();
 		*(pair) = NULL;

ext/pcntl/pcntl.stub.php

@@ -1055,7 +1055,7 @@ function pcntl_wtermsig(int $status): int|false {}
 
     function pcntl_wstopsig(int $status): int|false {}
 
-    function pcntl_exec(string $path, array $args = [], array $env_vars = []): bool {}
+    function pcntl_exec(string $path, array $args = [], array $env_vars = []): false {}
 
     function pcntl_alarm(int $seconds): int {}
 

ext/pcntl/pcntl_arginfo.h

@@ -0,0 +1,36 @@
+--TEST--
+pcntl_exec(): Test cleanup after value that contain null bytes has been encountered for $args and $env_vars.
+--EXTENSIONS--
+pcntl
+--FILE--
+<?php
+try {
+    pcntl_exec('cmd', ['-n', "value\0null\0byte"]);
+} catch (Throwable $e) {
+    echo $e::class, ': ', $e->getMessage(), "\n";
+}
+
+try {
+    pcntl_exec(
+        'cmd',
+        ['-n'],
+        ['var1' => 'value1', 'var2' => "value\0null\0byte"],
+    );
+} catch (Throwable $e) {
+    echo $e::class, ': ', $e->getMessage(), "\n";
+}
+
+try {
+    pcntl_exec(
+        'cmd',
+        ['-n'],
+        ['var1' => 'value1', "key\0null\0byte" => "value2"],
+    );
+} catch (Throwable $e) {
+    echo $e::class, ': ', $e->getMessage(), "\n";
+}
+?>
+--EXPECT--
+ValueError: pcntl_exec(): Argument #2 ($args) individual argument must not contain null bytes
+ValueError: pcntl_exec(): Argument #3 ($env_vars) value for environment variable must not contain null bytes
+ValueError: pcntl_exec(): Argument #3 ($env_vars) name for environment variable must not contain null bytes