Make null byte error a ValueError

Currently we treat paths with null bytes as a TypeError, which is incorrect, and rather inconsistent, as we treat empty paths as ValueError. We do this because the error is generated by zpp and it's easier to always throw TypeError there. This changes the zpp implementation to throw a TypeError only if the type is actually wrong and throw ValueError for null bytes. The error message is also split accordingly, to be more precise. Closes GH-6094.
php · Sep 8, 2020 · 7e339a3 · 7e339a3
1 parent 2386f65
commit 7e339a3
Show file tree

Hide file tree

Showing 59 changed files with 213 additions and 197 deletions.
diff --git a/Zend/zend_API.c b/Zend/zend_API.c
@@ -254,6 +254,12 @@ ZEND_API ZEND_COLD void ZEND_FASTCALL zend_wrong_parameter_type_error(uint32_t n
 		return;
 	}
 
+	if ((expected_type == Z_EXPECTED_PATH || expected_type == Z_EXPECTED_PATH_OR_NULL)
+			&& Z_TYPE_P(arg) == IS_STRING) {
+		zend_argument_value_error(num, "must not contain any null bytes");
+		return;
+	}
+
 	zend_argument_type_error(num, "must be %s, %s given", expected_error[expected_type], zend_zval_type_name(arg));
 }
 /* }}} */
@@ -668,10 +674,12 @@ static const char *zend_parse_arg_impl(zval *arg, va_list *va, const char **spec
 				char **p = va_arg(*va, char **);
 				size_t *pl = va_arg(*va, size_t *);
 				if (!zend_parse_arg_path(arg, p, pl, check_null)) {
-					zend_spprintf(error, 0, "a valid path%s, %s given",
-						check_null ? " or null" : "", zend_zval_type_name(arg)
-					);
-					return "";
+					if (Z_TYPE_P(arg) == IS_STRING) {
+						zend_spprintf(error, 0, "must not contain any null bytes");
+						return "";
+					} else {
+						return check_null ? "?string" : "string";
+					}
 				}
 			}
 			break;
@@ -680,10 +688,12 @@ static const char *zend_parse_arg_impl(zval *arg, va_list *va, const char **spec
 			{
 				zend_string **str = va_arg(*va, zend_string **);
 				if (!zend_parse_arg_path_str(arg, str, check_null)) {
-					zend_spprintf(error, 0, "a valid path%s, %s given",
-						check_null ? " or null" : "", zend_zval_type_name(arg)
-					);
-					return "";
+					if (Z_TYPE_P(arg) == IS_STRING) {
+						zend_spprintf(error, 0, "must not contain any null bytes");
+						return "";
+					} else {
+						return check_null ? "?string" : "string";
+					}
 				}
 			}
 			break;
@@ -762,7 +772,7 @@ static const char *zend_parse_arg_impl(zval *arg, va_list *va, const char **spec
 				if (!zend_parse_arg_object(arg, p, ce, check_null)) {
 					if (ce) {
 						if (check_null) {
-							zend_spprintf(error, 0, "of type ?%s, %s given", ZSTR_VAL(ce->name), zend_zval_type_name(arg));
+							zend_spprintf(error, 0, "must be of type ?%s, %s given", ZSTR_VAL(ce->name), zend_zval_type_name(arg));
 							return "";
 						} else {
 							return ZSTR_VAL(ce->name);
@@ -795,14 +805,14 @@ static const char *zend_parse_arg_impl(zval *arg, va_list *va, const char **spec
 				}
 				if (ce_base) {
 					if ((!*pce || !instanceof_function(*pce, ce_base))) {
-						zend_spprintf(error, 0, "a class name derived from %s%s, %s given",
+						zend_spprintf(error, 0, "must be a class name derived from %s%s, %s given",
 							ZSTR_VAL(ce_base->name), check_null ? " or null" : "", Z_STRVAL_P(arg));
 						*pce = NULL;
 						return "";
 					}
 				}
 				if (!*pce) {
-					zend_spprintf(error, 0, "a valid class name%s, %s given",
+					zend_spprintf(error, 0, "must be a valid class name%s, %s given",
 						check_null ? " or null" : "", Z_STRVAL_P(arg));
 					return "";
 				}
@@ -833,7 +843,7 @@ static const char *zend_parse_arg_impl(zval *arg, va_list *va, const char **spec
 				}
 
 				if (is_callable_error) {
-					zend_spprintf(error, 0, "a valid callback%s, %s", check_null ? " or null" : "", is_callable_error);
+					zend_spprintf(error, 0, "must be a valid callback%s, %s", check_null ? " or null" : "", is_callable_error);
 					efree(is_callable_error);
 					return "";
 				} else {
@@ -874,7 +884,11 @@ static zend_result zend_parse_arg(uint32_t arg_num, zval *arg, va_list *va, cons
 		}
 		if (!(flags & ZEND_PARSE_PARAMS_QUIET) && (*expected_type || error)) {
 			if (error) {
-				zend_argument_type_error(arg_num, "must be %s", error);
+				if (strcmp(error, "must not contain any null bytes") == 0) {
+					zend_argument_value_error(arg_num, "%s", error);
+				} else {
+					zend_argument_type_error(arg_num, "%s", error);
+				}
 				efree(error);
 			} else {
 				zend_argument_type_error(arg_num, "must be of type %s, %s given", expected_type, zend_zval_type_name(arg));

diff --git a/Zend/zend_API.h b/Zend/zend_API.h
@@ -1215,8 +1215,8 @@ static zend_always_inline zval *zend_try_array_init(zval *zv)
 	_(Z_EXPECTED_FUNC_OR_NULL,		"a valid callback or null") \
 	_(Z_EXPECTED_RESOURCE,			"of type resource") \
 	_(Z_EXPECTED_RESOURCE_OR_NULL,	"of type resource or null") \
-	_(Z_EXPECTED_PATH,				"a valid path") \
-	_(Z_EXPECTED_PATH_OR_NULL,		"a valid path or null") \
+	_(Z_EXPECTED_PATH,				"of type string") \
+	_(Z_EXPECTED_PATH_OR_NULL,		"of type ?string") \
 	_(Z_EXPECTED_OBJECT,			"of type object") \
 	_(Z_EXPECTED_OBJECT_OR_NULL,	"of type ?object") \
 	_(Z_EXPECTED_DOUBLE,			"of type float") \

diff --git a/ext/curl/interface.c b/ext/curl/interface.c
@@ -102,7 +102,7 @@ static int php_curl_option_str(php_curl *ch, zend_long option, const char *str,
 	CURLcode error = CURLE_OK;
 
 	if (strlen(str) != len) {
-		zend_type_error("%s(): cURL option cannot contain any null-bytes", get_active_function_name());
+		zend_value_error("%s(): cURL option must not contain any null bytes", get_active_function_name());
 		return FAILURE;
 	}
 

diff --git a/ext/curl/tests/bug68089.phpt b/ext/curl/tests/bug68089.phpt
@@ -12,12 +12,12 @@ $ch = curl_init();
 
 try {
     curl_setopt($ch, CURLOPT_URL, $url);
-} catch (TypeError $exception) {
+} catch (ValueError $exception) {
     echo $exception->getMessage() . "\n";
 }
 
 ?>
 Done
 --EXPECT--
-curl_setopt(): cURL option cannot contain any null-bytes
+curl_setopt(): cURL option must not contain any null bytes
 Done
diff --git a/ext/exif/exif.c b/ext/exif/exif.c
@@ -4541,7 +4541,7 @@ PHP_FUNCTION(exif_read_data)
 		}
 
 		if (CHECK_NULL_PATH(Z_STRVAL_P(stream), Z_STRLEN_P(stream))) {
-			zend_argument_type_error(1, "cannot contain any null-bytes");
+			zend_argument_value_error(1, "must not contain any null bytes");
 			RETURN_THROWS();
 		}
 
@@ -4718,7 +4718,7 @@ PHP_FUNCTION(exif_thumbnail)
 		}
 
 		if (CHECK_NULL_PATH(Z_STRVAL_P(stream), Z_STRLEN_P(stream))) {
-			zend_argument_type_error(1, "cannot contain any null-bytes");
+			zend_argument_value_error(1, "must not contain any null bytes");
 			RETURN_THROWS();
 		}
 

diff --git a/ext/exif/tests/filename_empty.phpt b/ext/exif/tests/filename_empty.phpt
@@ -17,19 +17,19 @@ try {
 
 try {
     exif_read_data("foo\0bar");
-} catch (TypeError $e) {
+} catch (ValueError $e) {
     echo $e->getMessage(), "\n";
 }
 
 try {
     exif_thumbnail("foo\0bar");
-} catch (TypeError $e) {
+} catch (ValueError $e) {
     echo $e->getMessage(), "\n";
 }
 
 ?>
 --EXPECT--
 exif_read_data(): Argument #1 ($filename) cannot be empty
 exif_thumbnail(): Argument #1 ($filename) cannot be empty
-exif_read_data(): Argument #1 ($filename) cannot contain any null-bytes
-exif_thumbnail(): Argument #1 ($filename) cannot contain any null-bytes
+exif_read_data(): Argument #1 ($filename) must not contain any null bytes
+exif_thumbnail(): Argument #1 ($filename) must not contain any null bytes
diff --git a/ext/fileinfo/tests/finfo_open_001.phpt b/ext/fileinfo/tests/finfo_open_001.phpt
@@ -7,7 +7,7 @@ finfo_open(): Testing magic_file names
 
 try {
     var_dump(finfo_open(FILEINFO_MIME, "\0"));
-} catch (TypeError $e) {
+} catch (ValueError $e) {
     echo $e->getMessage(), "\n";
 }
 
@@ -19,7 +19,7 @@ var_dump(finfo_open(FILEINFO_MIME, '/foo/bar/inexistent'));
 
 ?>
 --EXPECTF--
-finfo_open(): Argument #2 ($arg) must be a valid path, string given
+finfo_open(): Argument #2 ($arg) must not contain any null bytes
 resource(%d) of type (file_info)
 resource(%d) of type (file_info)
 

diff --git a/ext/gd/tests/imagegd2_nullbyte_injection.phpt b/ext/gd/tests/imagegd2_nullbyte_injection.phpt
@@ -9,9 +9,9 @@ Testing null byte injection in imagegd2
 $image = imagecreate(1,1);// 1px image
 try {
     imagegd($image, "./foo\0bar");
-} catch (TypeError $e) {
+} catch (ValueError $e) {
     echo $e->getMessage(), "\n";
 }
 ?>
 --EXPECT--
-imagegd(): Argument #2 ($to) must be a valid path, string given
+imagegd(): Argument #2 ($to) must not contain any null bytes
diff --git a/ext/gd/tests/imagegd_nullbyte_injection.phpt b/ext/gd/tests/imagegd_nullbyte_injection.phpt
@@ -9,9 +9,9 @@ Testing null byte injection in imagegd
 $image = imagecreate(1,1);// 1px image
 try {
     imagegd($image, "./foo\0bar");
-} catch (TypeError $e) {
+} catch (ValueError $e) {
     echo $e->getMessage(), "\n";
 }
 ?>
 --EXPECT--
-imagegd(): Argument #2 ($to) must be a valid path, string given
+imagegd(): Argument #2 ($to) must not contain any null bytes
diff --git a/ext/gd/tests/imagexbm_nullbyte_injection.phpt b/ext/gd/tests/imagexbm_nullbyte_injection.phpt
@@ -9,9 +9,9 @@ if(!extension_loaded('gd')) die('skip gd extension not available');
 $image = imagecreate(1,1);// 1px image
 try {
     imagexbm($image, "./foo\0bar");
-} catch (TypeError $e) {
+} catch (ValueError $e) {
     echo $e->getMessage(), "\n";
 }
 ?>
 --EXPECT--
-imagexbm(): Argument #2 ($filename) must be a valid path or null, string given
+imagexbm(): Argument #2 ($filename) must not contain any null bytes
diff --git a/ext/hash/hash.c b/ext/hash/hash.c
@@ -360,7 +360,7 @@ static void php_hash_do_hash(
 	}
 	if (isfilename) {
 		if (CHECK_NULL_PATH(data, data_len)) {
-			zend_argument_type_error(1, "must be a valid path");
+			zend_argument_value_error(1, "must not contain any null bytes");
 			RETURN_THROWS();
 		}
 		stream = php_stream_open_wrapper_ex(data, "rb", REPORT_ERRORS, NULL, FG(default_context));
@@ -499,7 +499,7 @@ static void php_hash_do_hash_hmac(
 
 	if (isfilename) {
 		if (CHECK_NULL_PATH(data, data_len)) {
-			zend_argument_type_error(2, "must be a valid path");
+			zend_argument_value_error(2, "must not contain any null bytes");
 			RETURN_THROWS();
 		}
 		stream = php_stream_open_wrapper_ex(data, "rb", REPORT_ERRORS, NULL, FG(default_context));

diff --git a/ext/hash/tests/hash_hmac_file_error.phpt b/ext/hash/tests/hash_hmac_file_error.phpt
@@ -28,7 +28,7 @@ echo "\n-- Testing hash_hmac_file() function with bad path --\n";
 try {
     var_dump(hash_hmac_file('md5', $file.chr(0).$file, $key, TRUE));
 }
-catch (TypeError $e) {
+catch (ValueError $e) {
     echo $e->getMessage() . "\n";
 }
 
@@ -43,4 +43,4 @@ hash_hmac_file(): Argument #1 ($algo) must be a valid cryptographic hashing algo
 hash_hmac_file(): Argument #1 ($algo) must be a valid cryptographic hashing algorithm
 
 -- Testing hash_hmac_file() function with bad path --
-hash_hmac_file(): Argument #2 ($data) must be a valid path
+hash_hmac_file(): Argument #2 ($data) must not contain any null bytes
diff --git a/ext/phar/tests/badparameters.phpt b/ext/phar/tests/badparameters.phpt
@@ -229,13 +229,13 @@ try {
 ?>
 --EXPECTF--
 Phar::mungServer(): Argument #1 ($munglist) must be of type array, string given
-Phar::createDefaultStub(): Argument #1 ($index) must be a valid path or null, array given
-Phar::loadPhar(): Argument #1 ($filename) must be a valid path, array given
+Phar::createDefaultStub(): Argument #1 ($index) must be of type ?string, array given
+Phar::loadPhar(): Argument #1 ($filename) must be of type string, array given
 Phar::canCompress(): Argument #1 ($method) must be of type int, string given
-Phar::__construct(): Argument #1 ($filename) must be a valid path, array given
+Phar::__construct(): Argument #1 ($filename) must be of type string, array given
 Phar::convertToExecutable(): Argument #1 ($format) must be of type int, array given
 Phar::convertToData(): Argument #1 ($format) must be of type int, array given
-PharData::delete(): Argument #1 ($entry) must be a valid path, array given
+PharData::delete(): Argument #1 ($entry) must be of type string, array given
 Cannot write out phar archive, phar is read-only
 Entry oops does not exist and cannot be deleted
 %sfrontcontroller10.phar
@@ -255,13 +255,13 @@ Phar::compressFiles(): Argument #1 ($compression_type) must be of type int, arra
 Phar is readonly, cannot change compression
 Phar::copy() expects exactly 2 parameters, 1 given
 Cannot copy "a" to "b", phar is read-only
-Phar::offsetExists(): Argument #1 ($entry) must be a valid path, array given
-Phar::offsetGet(): Argument #1 ($entry) must be a valid path, array given
+Phar::offsetExists(): Argument #1 ($entry) must be of type string, array given
+Phar::offsetGet(): Argument #1 ($entry) must be of type string, array given
 Phar::offsetSet() expects exactly 2 parameters, 1 given
-PharData::offsetUnset(): Argument #1 ($entry) must be a valid path, array given
+PharData::offsetUnset(): Argument #1 ($entry) must be of type string, array given
 Write operations disabled by the php.ini setting phar.readonly
-Phar::addEmptyDir(): Argument #1 ($dirname) must be a valid path, array given
-Phar::addFile(): Argument #1 ($filename) must be a valid path, array given
+Phar::addEmptyDir(): Argument #1 ($dirname) must be of type string, array given
+Phar::addFile(): Argument #1 ($filename) must be of type string, array given
 Phar::addFromString() expects exactly 2 parameters, 1 given
 Write operations disabled by the php.ini setting phar.readonly
 Phar::setMetadata() expects exactly 1 parameter, 2 given

diff --git a/ext/phar/tests/bug64931/bug64931.phpt b/ext/phar/tests/bug64931/bug64931.phpt
@@ -38,7 +38,7 @@ try {
 
 try {
     $phar->addFromString(".phar\0", "gotcha");
-} catch (TypeError $e) {
+} catch (ValueError $e) {
     echo "CAUGHT: ". $e->getMessage() ."\n";
 }
 
@@ -53,4 +53,4 @@ CAUGHT: Cannot create any files in magic ".phar" directory
 CAUGHT: Cannot create any files in magic ".phar" directory
 CAUGHT: Cannot create any files in magic ".phar" directory
 CAUGHT: Cannot create any files in magic ".phar" directory
-CAUGHT: Phar::addFromString(): Argument #1 ($localname) must be a valid path, string given
+CAUGHT: Phar::addFromString(): Argument #1 ($localname) must not contain any null bytes
diff --git a/ext/phar/tests/create_path_error.phpt b/ext/phar/tests/create_path_error.phpt
@@ -53,7 +53,7 @@ foreach($checks as $check)
     {
         $phar[$check] = 'error';
     }
-    catch (TypeError $e)
+    catch (ValueError $e)
     {
         echo 'Exception: ' . $e->getMessage() . "\n";
     }
@@ -78,4 +78,4 @@ string(5) "query"
 11:Error: file_put_contents(phar://%s): Failed to open stream: phar error: invalid path "%s" contains illegal character
 12:Error: file_put_contents(phar://%s): Failed to open stream: phar error: invalid path "%s" contains illegal character
 13:Error: file_put_contents(phar://%s): Failed to open stream: phar error: invalid path "%s" contains illegal character
-Exception: Phar::offsetSet(): Argument #1 ($entry) must be a valid path, string given
+Exception: Phar::offsetSet(): Argument #1 ($entry) must not contain any null bytes
diff --git a/ext/phar/tests/fgc_edgecases.phpt b/ext/phar/tests/fgc_edgecases.phpt
@@ -52,7 +52,7 @@ include $pname . '/foo/hi';
 <?php unlink(__DIR__ . '/' . basename(__FILE__, '.clean.php') . '.phar.php'); ?>
 <?php unlink(__DIR__ . '/fgc_edgecases.txt'); ?>
 --EXPECTF--
-file_get_contents(): Argument #1 ($filename) must be a valid path, array given
+file_get_contents(): Argument #1 ($filename) must be of type string, array given
 blah
 <?php
 echo file_get_contents("foo/" . basename(__FILE__));

diff --git a/ext/phar/tests/fopen_edgecases2.phpt b/ext/phar/tests/fopen_edgecases2.phpt
@@ -37,7 +37,7 @@ include $pname . '/foo/hi';
 <?php unlink(__DIR__ . '/' . basename(__FILE__, '.clean.php') . '.phar.php'); ?>
 <?php unlink(__DIR__ . '/fopen_edgecases2.txt'); ?>
 --EXPECTF--
-fopen(): Argument #1 ($filename) must be a valid path, array given
+fopen(): Argument #1 ($filename) must be of type string, array given
 blah
 test
 

diff --git a/ext/phar/tests/opendir_edgecases.phpt b/ext/phar/tests/opendir_edgecases.phpt
@@ -55,7 +55,7 @@ include $pname . '/foo';
 <?php unlink(__DIR__ . '/' . basename(__FILE__, '.clean.php') . '.phar.php'); ?>
 <?php rmdir(__DIR__ . '/opendir_edgecases');
 --EXPECTF--
-opendir(): Argument #1 ($path) must be a valid path, array given
+opendir(): Argument #1 ($path) must be of type string, array given
 .
 ..
 foo

diff --git a/ext/phar/tests/phar_extract.phpt b/ext/phar/tests/phar_extract.phpt
@@ -142,7 +142,7 @@ string(3) "hi3"
 string(3) "hi2"
 bool(false)
 Phar::extractTo(): Argument #2 ($files) must be of type array|string|null, stdClass given
-Phar::extractTo(): Argument #1 ($pathto) must be a valid path, array given
+Phar::extractTo(): Argument #1 ($pathto) must be of type string, array given
 Invalid argument, extraction path must be non-zero length
 Unable to use path "%soops" for extraction, it is a file, must be a directory
 Invalid argument, array of filenames to extract contains non-string value

diff --git a/ext/phar/tests/phar_unlinkarchive.phpt b/ext/phar/tests/phar_unlinkarchive.phpt
@@ -92,7 +92,7 @@ __HALT_COMPILER();
 Unknown phar archive ""
 Unknown phar archive "%sphar_unlinkarchive.phar"
 Unknown phar archive "%sphar_unlinkarchive.phar.tar": internal corruption of phar "%sphar_unlinkarchive.phar.tar" (truncated entry)
-Phar::unlinkArchive(): Argument #1 ($archive) must be a valid path, array given
+Phar::unlinkArchive(): Argument #1 ($archive) must be of type string, array given
 bool(false)
 string(48) "<?php echo "first stub\n"; __HALT_COMPILER(); ?>"
 phar archive "%sphar_unlinkarchive.phar" has open file handles or objects.  fclose() all file handles, and unset() all objects prior to calling unlinkArchive()

diff --git a/ext/phar/tests/pharfileinfo_construct.phpt b/ext/phar/tests/pharfileinfo_construct.phpt
@@ -49,7 +49,7 @@ echo $e->getMessage() . "\n";
 <?php unlink(__DIR__ . '/' . basename(__FILE__, '.clean.php') . '.phar'); ?>
 --EXPECTF--
 Cannot open phar file 'phar://%spharfileinfo_construct.phar/oops': internal corruption of phar "%spharfileinfo_construct.phar" (truncated entry)
-PharFileInfo::__construct(): Argument #1 ($filename) must be a valid path, array given
+PharFileInfo::__construct(): Argument #1 ($filename) must be of type string, array given
 Cannot access phar file entry '%s' in archive '%s'
 Cannot call constructor twice
 '%s' is not a valid phar archive URL (must have at least phar://filename.phar)
diff --git a/ext/spl/tests/bug54291.phpt b/ext/spl/tests/bug54291.phpt
@@ -6,7 +6,7 @@ $dir = new DirectoryIterator("\x00/abc");
 $dir->isFile();
 ?>
 --EXPECTF--
-Fatal error: Uncaught TypeError: DirectoryIterator::__construct(): Argument #1 ($path) must be a valid path, string given in %s:%d
+Fatal error: Uncaught ValueError: DirectoryIterator::__construct(): Argument #1 ($path) must not contain any null bytes in %s:%d
 Stack trace:
 #0 %s(%d): DirectoryIterator->__construct('\x00/abc')
 #1 {main}

diff --git a/ext/spl/tests/bug77431.phpt b/ext/spl/tests/bug77431.phpt
@@ -5,7 +5,7 @@ Bug #77431 (SplFileInfo::__construct() accepts NUL bytes)
 new SplFileInfo("bad\0good");
 ?>
 --EXPECTF--
-Fatal error: Uncaught TypeError: SplFileInfo::__construct(): Argument #1 ($file_name) must be a valid path, string given in %s:%d
+Fatal error: Uncaught ValueError: SplFileInfo::__construct(): Argument #1 ($file_name) must not contain any null bytes in %s:%d
 Stack trace:
 #0 %s(%d): SplFileInfo->__construct('bad\x00good')
 #1 {main}