ext/standard: Refactor exec.c public APIs to use zend_string pointers (#14353)

Girgias · web-flow · commit 48d5ae98e74d · 2024-05-29T10:59:17.000+01:00
* Pull zend_string* from INI directive

* Ensure that mail.force_extra_parameters INI directive does not have any nul bytes

* ext/standard: Make php_escape_shell_cmd() take a zend_string* instead of char*

This saves on an expensive strlen() computation

* Convert E_ERROR to ValueError in php_escape_shell_cmd()

* ext/standard: Make php_escape_shell_arg() take a zend_string* instead of char*

This saves on an expensive strlen() computation

* Convert E_ERROR to ValueError in php_escape_shell_arg()
diff --git a/UPGRADING.INTERNALS b/UPGRADING.INTERNALS
@@ -246,6 +246,12 @@ PHP 8.4 INTERNALS UPGRADE NOTES
  g. ext/standard
    - Added the php_base64_encode_ex() API with flag parameters, value can be
      PHP_BASE64_NO_PADDING to encode without the padding character '='.
+   - The php_escape_shell_cmd() now takes a zend_string* instead of a char*
+     Moreover, providing it with a binary safe string is the responsibility of
+     the caller now.
+   - The php_escape_shell_arg() now takes a zend_string* instead of a char*
+     Moreover, providing it with a binary safe string is the responsibility of
+     the caller now.
 
 ========================
 4. OpCode changes
diff --git a/ext/mbstring/mbstring.c b/ext/mbstring/mbstring.c
@@ -4435,7 +4435,6 @@ PHP_FUNCTION(mb_send_mail)
 	zend_string *str_headers = NULL;
 	size_t i;
 	char *to_r = NULL;
-	char *force_extra_parameters = INI_STR("mail.force_extra_parameters");
 	bool suppress_content_type = false;
 	bool suppress_content_transfer_encoding = false;
 
@@ -4653,10 +4652,11 @@ PHP_FUNCTION(mb_send_mail)
 
 	str_headers = smart_str_extract(&str);
 
+	zend_string *force_extra_parameters = zend_ini_str_ex("mail.force_extra_parameters", strlen("mail.force_extra_parameters"), false, NULL);
 	if (force_extra_parameters) {
 		extra_cmd = php_escape_shell_cmd(force_extra_parameters);
 	} else if (extra_cmd) {
-		extra_cmd = php_escape_shell_cmd(ZSTR_VAL(extra_cmd));
+		extra_cmd = php_escape_shell_cmd(extra_cmd);
 	}
 
 	RETVAL_BOOL(php_mail(to_r, ZSTR_VAL(subject), message, ZSTR_VAL(str_headers), extra_cmd ? ZSTR_VAL(extra_cmd) : NULL));
diff --git a/ext/standard/exec.c b/ext/standard/exec.c
@@ -279,19 +279,23 @@ PHP_FUNCTION(passthru)
 
    *NOT* safe for binary strings
 */
-PHPAPI zend_string *php_escape_shell_cmd(const char *str)
+PHPAPI zend_string *php_escape_shell_cmd(const zend_string *unescaped_cmd)
 {
 	size_t x, y;
-	size_t l = strlen(str);
-	uint64_t estimate = (2 * (uint64_t)l) + 1;
 	zend_string *cmd;
 #ifndef PHP_WIN32
 	char *p = NULL;
 #endif
 
+	ZEND_ASSERT(ZSTR_LEN(unescaped_cmd) == strlen(ZSTR_VAL(unescaped_cmd)) && "Must be a binary safe string");
+	size_t l = ZSTR_LEN(unescaped_cmd);
+	const char *str = ZSTR_VAL(unescaped_cmd);
+
+	uint64_t estimate = (2 * (uint64_t)l) + 1;
+
 	/* max command line length - two single quotes - \0 byte length */
 	if (l > cmd_max_len - 2 - 1) {
-		php_error_docref(NULL, E_ERROR, "Command exceeds the allowed length of %zu bytes", cmd_max_len);
+		zend_value_error("Command exceeds the allowed length of %zu bytes", cmd_max_len);
 		return ZSTR_EMPTY_ALLOC();
 	}
 
@@ -367,7 +371,7 @@ PHPAPI zend_string *php_escape_shell_cmd(const char *str)
 	ZSTR_VAL(cmd)[y] = '\0';
 
 	if (y > cmd_max_len + 1) {
-		php_error_docref(NULL, E_ERROR, "Escaped command exceeds the allowed length of %zu bytes", cmd_max_len);
+		zend_value_error("Escaped command exceeds the allowed length of %zu bytes", cmd_max_len);
 		zend_string_release_ex(cmd, 0);
 		return ZSTR_EMPTY_ALLOC();
 	}
@@ -385,16 +389,20 @@ PHPAPI zend_string *php_escape_shell_cmd(const char *str)
 /* }}} */
 
 /* {{{ php_escape_shell_arg */
-PHPAPI zend_string *php_escape_shell_arg(const char *str)
+PHPAPI zend_string *php_escape_shell_arg(const zend_string *unescaped_arg)
 {
 	size_t x, y = 0;
-	size_t l = strlen(str);
 	zend_string *cmd;
+
+	ZEND_ASSERT(ZSTR_LEN(unescaped_arg) == strlen(ZSTR_VAL(unescaped_arg)) && "Must be a binary safe string");
+	size_t l = ZSTR_LEN(unescaped_arg);
+	const char *str = ZSTR_VAL(unescaped_arg);
+
 	uint64_t estimate = (4 * (uint64_t)l) + 3;
 
 	/* max command line length - two single quotes - \0 byte length */
 	if (l > cmd_max_len - 2 - 1) {
-		php_error_docref(NULL, E_ERROR, "Argument exceeds the allowed length of %zu bytes", cmd_max_len);
+		zend_value_error("Argument exceeds the allowed length of %zu bytes", cmd_max_len);
 		return ZSTR_EMPTY_ALLOC();
 	}
 
@@ -453,7 +461,7 @@ PHPAPI zend_string *php_escape_shell_arg(const char *str)
 	ZSTR_VAL(cmd)[y] = '\0';
 
 	if (y > cmd_max_len + 1) {
-		php_error_docref(NULL, E_ERROR, "Escaped argument exceeds the allowed length of %zu bytes", cmd_max_len);
+		zend_value_error("Escaped argument exceeds the allowed length of %zu bytes", cmd_max_len);
 		zend_string_release_ex(cmd, 0);
 		return ZSTR_EMPTY_ALLOC();
 	}
@@ -471,18 +479,13 @@ PHPAPI zend_string *php_escape_shell_arg(const char *str)
 /* {{{ Escape shell metacharacters */
 PHP_FUNCTION(escapeshellcmd)
 {
-	char *command;
-	size_t command_len;
+	zend_string *command;
 
 	ZEND_PARSE_PARAMETERS_START(1, 1)
-		Z_PARAM_STRING(command, command_len)
+		Z_PARAM_PATH_STR(command)
 	ZEND_PARSE_PARAMETERS_END();
 
-	if (command_len) {
-		if (command_len != strlen(command)) {
-			zend_argument_value_error(1, "must not contain any null bytes");
-			RETURN_THROWS();
-		}
+	if (ZSTR_LEN(command)) {
 		RETVAL_STR(php_escape_shell_cmd(command));
 	} else {
 		RETVAL_EMPTY_STRING();
@@ -493,18 +496,12 @@ PHP_FUNCTION(escapeshellcmd)
 /* {{{ Quote and escape an argument for use in a shell command */
 PHP_FUNCTION(escapeshellarg)
 {
-	char *argument;
-	size_t argument_len;
+	zend_string *argument;
 
 	ZEND_PARSE_PARAMETERS_START(1, 1)
-		Z_PARAM_STRING(argument, argument_len)
+		Z_PARAM_PATH_STR(argument)
 	ZEND_PARSE_PARAMETERS_END();
 
-	if (argument_len != strlen(argument)) {
-		zend_argument_value_error(1, "must not contain any null bytes");
-		RETURN_THROWS();
-	}
-
 	RETVAL_STR(php_escape_shell_arg(argument));
 }
 /* }}} */
diff --git a/ext/standard/exec.h b/ext/standard/exec.h
@@ -20,8 +20,8 @@
 PHP_MINIT_FUNCTION(proc_open);
 PHP_MINIT_FUNCTION(exec);
 
-PHPAPI zend_string *php_escape_shell_cmd(const char *str);
-PHPAPI zend_string *php_escape_shell_arg(const char *str);
+PHPAPI zend_string *php_escape_shell_cmd(const zend_string *unescaped_cmd);
+PHPAPI zend_string *php_escape_shell_arg(const zend_string *unescaped_arg);
 PHPAPI int php_exec(int type, const char *cmd, zval *array, zval *return_value);
 
 #endif /* EXEC_H */
diff --git a/ext/standard/mail.c b/ext/standard/mail.c
@@ -247,7 +247,6 @@ PHP_FUNCTION(mail)
 	HashTable *headers_ht = NULL;
 	size_t to_len, message_len;
 	size_t subject_len, i;
-	char *force_extra_parameters = INI_STR("mail.force_extra_parameters");
 	char *to_r, *subject_r;
 
 	ZEND_PARSE_PARAMETERS_START(3, 5)
@@ -312,10 +311,11 @@ PHP_FUNCTION(mail)
 		subject_r = subject;
 	}
 
+	zend_string *force_extra_parameters = zend_ini_str_ex("mail.force_extra_parameters", strlen("mail.force_extra_parameters"), false, NULL);
 	if (force_extra_parameters) {
 		extra_cmd = php_escape_shell_cmd(force_extra_parameters);
 	} else if (extra_cmd) {
-		extra_cmd = php_escape_shell_cmd(ZSTR_VAL(extra_cmd));
+		extra_cmd = php_escape_shell_cmd(extra_cmd);
 	}
 
 	if (php_mail(to_r, subject_r, message, headers_str && ZSTR_LEN(headers_str) ? ZSTR_VAL(headers_str) : NULL, extra_cmd ? ZSTR_VAL(extra_cmd) : NULL)) {
diff --git a/ext/standard/tests/general_functions/escapeshellarg_bug71270.phpt b/ext/standard/tests/general_functions/escapeshellarg_bug71270.phpt
@@ -4,9 +4,15 @@ Test escapeshellarg() allowed argument length
 <?php
 ini_set('memory_limit', -1);
 $var_2  = str_repeat('A', 1024*1024*64);
-escapeshellarg($var_2);
+
+try {
+    escapeshellarg($var_2);
+} catch (Throwable $e) {
+    echo $e::class, ': ', $e->getMessage(), PHP_EOL;
+}
 
 ?>
 ===DONE===
 --EXPECTF--
-Fatal error: escapeshellarg(): Argument exceeds the allowed length of %d bytes in %s on line %d
+ValueError: Argument exceeds the allowed length of %d bytes
+===DONE===
diff --git a/ext/standard/tests/general_functions/escapeshellcmd_bug71270.phpt b/ext/standard/tests/general_functions/escapeshellcmd_bug71270.phpt
@@ -4,9 +4,15 @@ Test escapeshellcmd() allowed argument length
 <?php
 ini_set('memory_limit', -1);
 $var_2  = str_repeat('A', 1024*1024*64);
-escapeshellcmd($var_2);
+
+try {
+    escapeshellcmd($var_2);
+} catch (Throwable $e) {
+    echo $e::class, ': ', $e->getMessage(), PHP_EOL;
+}
 
 ?>
 ===DONE===
 --EXPECTF--
-Fatal error: escapeshellcmd(): Command exceeds the allowed length of %d bytes in %s on line %d
+ValueError: Command exceeds the allowed length of %d bytes
+===DONE===
diff --git a/main/main.c b/main/main.c
@@ -652,6 +652,11 @@ static PHP_INI_MH(OnUpdateMailLog)
 /* {{{ PHP_INI_MH */
 static PHP_INI_MH(OnChangeMailForceExtra)
 {
+	/* Check that INI setting does not have any nul bytes */
+	if (new_value && ZSTR_LEN(new_value) != strlen(ZSTR_VAL(new_value))) {
+		/* TODO Emit warning? */
+		return FAILURE;
+	}
 	/* Don't allow changing it in htaccess */
 	if (stage == PHP_INI_STAGE_HTACCESS) {
 			return FAILURE;
