From 7f4a97a0f4d8a53a7b6831f161babff7881da716 Mon Sep 17 00:00:00 2001 From: George Peter Banyard Date: Fri, 31 Jul 2020 15:54:36 +0100 Subject: [PATCH 1/2] Promote warnings to Error in FileInfo extension --- ext/fileinfo/fileinfo.c | 15 ++--- ext/fileinfo/tests/finfo_file_001.phpt | 29 ++++++---- ext/fileinfo/tests/finfo_file_basic.phpt | 15 +++-- ext/fileinfo/tests/mime_content_type_001.phpt | 55 +++++++++++++------ 4 files changed, 73 insertions(+), 41 deletions(-) diff --git a/ext/fileinfo/fileinfo.c b/ext/fileinfo/fileinfo.c index 014c37441aee8..fa40c9a8e2397 100644 --- a/ext/fileinfo/fileinfo.c +++ b/ext/fileinfo/fileinfo.c @@ -376,8 +376,8 @@ static void _php_finfo_get_type(INTERNAL_FUNCTION_PARAMETERS, int mode, int mime break; default: - php_error_docref(NULL, E_WARNING, "Can only process string or stream arguments"); - RETURN_FALSE; + zend_argument_type_error(2, "must be of type string|resource, %s given", zend_zval_type_name(what)); + RETURN_THROWS(); } magic = magic_open(MAGIC_MIME_TYPE); @@ -439,14 +439,12 @@ static void _php_finfo_get_type(INTERNAL_FUNCTION_PARAMETERS, int mode, int mime php_stream_wrapper *wrap; php_stream_statbuf ssb; - if (buffer == NULL || !*buffer) { - php_error_docref(NULL, E_WARNING, "Empty filename or path"); - RETVAL_FALSE; + if (buffer == NULL || buffer_len == 0) { + zend_argument_value_error(1, "cannot be empty"); goto clean; } if (CHECK_NULL_PATH(buffer, buffer_len)) { - php_error_docref(NULL, E_WARNING, "Invalid path"); - RETVAL_FALSE; + zend_argument_type_error(1, "must not contain null bytes"); goto clean; } @@ -487,6 +485,9 @@ static void _php_finfo_get_type(INTERNAL_FUNCTION_PARAMETERS, int mode, int mime default: php_error_docref(NULL, E_WARNING, "Can only process string or stream arguments"); + // TODO Not sure I understand this code flow completely + //zend_argument_type_error(2, "must be of type string|resource, %s given", zend_zval_type_name(group)); + //goto clean; } common: diff --git a/ext/fileinfo/tests/finfo_file_001.phpt b/ext/fileinfo/tests/finfo_file_001.phpt index 3d01c8e88131c..726103e02d666 100644 --- a/ext/fileinfo/tests/finfo_file_001.phpt +++ b/ext/fileinfo/tests/finfo_file_001.phpt @@ -6,22 +6,29 @@ finfo_file(): Testing file names getMessage() . \PHP_EOL; +} +try { + var_dump(finfo_file($fp, '')); +} catch (\ValueError $e) { + echo $e->getMessage() . \PHP_EOL; +} +try { + var_dump(finfo_file($fp, NULL)); +} catch (\ValueError $e) { + echo $e->getMessage() . \PHP_EOL; +} var_dump(finfo_file($fp, '.')); var_dump(finfo_file($fp, '&')); ?> --EXPECTF-- -Warning: finfo_file(): Empty filename or path in %s on line %d -bool(false) - -Warning: finfo_file(): Empty filename or path in %s on line %d -bool(false) - -Warning: finfo_file(): Empty filename or path in %s on line %d -bool(false) +finfo_file(): Argument #1 ($finfo) must not contain null bytes +finfo_file(): Argument #1 ($finfo) cannot be empty +finfo_file(): Argument #1 ($finfo) cannot be empty string(9) "directory" Warning: finfo_file(&): Failed to open stream: No such file or directory in %s on line %d diff --git a/ext/fileinfo/tests/finfo_file_basic.phpt b/ext/fileinfo/tests/finfo_file_basic.phpt index 1851058b131a8..816f826c1b554 100644 --- a/ext/fileinfo/tests/finfo_file_basic.phpt +++ b/ext/fileinfo/tests/finfo_file_basic.phpt @@ -13,14 +13,17 @@ echo "*** Testing finfo_file() : basic functionality ***\n"; var_dump( finfo_file( $finfo, __FILE__) ); var_dump( finfo_file( $finfo, __FILE__, FILEINFO_CONTINUE ) ); var_dump( finfo_file( $finfo, $magicFile ) ); -var_dump( finfo_file( $finfo, $magicFile.chr(0).$magicFile) ); + +try { + var_dump( finfo_file( $finfo, $magicFile.chr(0).$magicFile) ); +} catch (\TypeError $e) { + echo $e->getMessage() . \PHP_EOL; +} ?> ---EXPECTF-- +--EXPECT-- *** Testing finfo_file() : basic functionality *** string(28) "text/x-php; charset=us-ascii" -string(%d) "PHP script, ASCII text%A" +string(22) "PHP script, ASCII text" string(32) "text/plain; charset=unknown-8bit" - -Warning: finfo_file(): Invalid path in %s%efinfo_file_basic.php on line %d -bool(false) +finfo_file(): Argument #1 ($finfo) must not contain null bytes diff --git a/ext/fileinfo/tests/mime_content_type_001.phpt b/ext/fileinfo/tests/mime_content_type_001.phpt index 472d3e84f8125..8a2d41e3013c0 100644 --- a/ext/fileinfo/tests/mime_content_type_001.phpt +++ b/ext/fileinfo/tests/mime_content_type_001.phpt @@ -5,26 +5,47 @@ mime_content_type(): Testing wrong parameters --FILE-- getMessage() . \PHP_EOL; +} +try { + mime_content_type(NULL); +} catch (\TypeError $e) { + echo $e->getMessage() . \PHP_EOL; +} +try { + mime_content_type(new stdclass); +} catch (\TypeError $e) { + echo $e->getMessage() . \PHP_EOL; +} +try { + mime_content_type(array()); +} catch (\TypeError $e) { + echo $e->getMessage() . \PHP_EOL; +} + mime_content_type('foo/inexistent'); -mime_content_type(''); -mime_content_type("\0"); + +try { + mime_content_type(''); +} catch (\ValueError $e) { + echo $e->getMessage() . \PHP_EOL; +} +try { + mime_content_type("\0"); +} catch (\TypeError $e) { + echo $e->getMessage() . \PHP_EOL; +} ?> --EXPECTF-- -Warning: mime_content_type(): Can only process string or stream arguments in %s on line %d - -Warning: mime_content_type(): Can only process string or stream arguments in %s on line %d - -Warning: mime_content_type(): Can only process string or stream arguments in %s on line %d - -Warning: mime_content_type(): Can only process string or stream arguments in %s on line %d +mime_content_type(): Argument #2 must be of type string|resource, int given +mime_content_type(): Argument #2 must be of type string|resource, null given +mime_content_type(): Argument #2 must be of type string|resource, stdClass given +mime_content_type(): Argument #2 must be of type string|resource, array given Warning: mime_content_type(foo/inexistent): Failed to open stream: No such file or directory in %s on line %d - -Warning: mime_content_type(): Empty filename or path in %s on line %d - -Warning: mime_content_type(): Empty filename or path in %s on line %d +mime_content_type(): Argument #1 ($filename) cannot be empty +mime_content_type(): Argument #1 ($filename) must not contain null bytes From 0b54e6118b8e2aa1861dee0e40c4be2bf1f255e9 Mon Sep 17 00:00:00 2001 From: George Peter Banyard Date: Fri, 31 Jul 2020 19:34:15 +0100 Subject: [PATCH 2/2] Review --- ext/fileinfo/fileinfo.c | 9 ++------- ext/fileinfo/tests/mime_content_type_001.phpt | 8 ++++---- 2 files changed, 6 insertions(+), 11 deletions(-) diff --git a/ext/fileinfo/fileinfo.c b/ext/fileinfo/fileinfo.c index fa40c9a8e2397..2538171f25bee 100644 --- a/ext/fileinfo/fileinfo.c +++ b/ext/fileinfo/fileinfo.c @@ -376,7 +376,7 @@ static void _php_finfo_get_type(INTERNAL_FUNCTION_PARAMETERS, int mode, int mime break; default: - zend_argument_type_error(2, "must be of type string|resource, %s given", zend_zval_type_name(what)); + zend_argument_type_error(2, "must be of type resource|string, %s given", zend_zval_type_name(what)); RETURN_THROWS(); } @@ -482,12 +482,7 @@ static void _php_finfo_get_type(INTERNAL_FUNCTION_PARAMETERS, int mode, int mime } break; } - - default: - php_error_docref(NULL, E_WARNING, "Can only process string or stream arguments"); - // TODO Not sure I understand this code flow completely - //zend_argument_type_error(2, "must be of type string|resource, %s given", zend_zval_type_name(group)); - //goto clean; + EMPTY_SWITCH_DEFAULT_CASE() } common: diff --git a/ext/fileinfo/tests/mime_content_type_001.phpt b/ext/fileinfo/tests/mime_content_type_001.phpt index 8a2d41e3013c0..9e3cb603986cd 100644 --- a/ext/fileinfo/tests/mime_content_type_001.phpt +++ b/ext/fileinfo/tests/mime_content_type_001.phpt @@ -41,10 +41,10 @@ try { ?> --EXPECTF-- -mime_content_type(): Argument #2 must be of type string|resource, int given -mime_content_type(): Argument #2 must be of type string|resource, null given -mime_content_type(): Argument #2 must be of type string|resource, stdClass given -mime_content_type(): Argument #2 must be of type string|resource, array given +mime_content_type(): Argument #2 must be of type resource|string, int given +mime_content_type(): Argument #2 must be of type resource|string, null given +mime_content_type(): Argument #2 must be of type resource|string, stdClass given +mime_content_type(): Argument #2 must be of type resource|string, array given Warning: mime_content_type(foo/inexistent): Failed to open stream: No such file or directory in %s on line %d mime_content_type(): Argument #1 ($filename) cannot be empty