From 0e0325b7c5df93f7852a18358ef94d4d3e251cce Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?M=C3=A1t=C3=A9=20Kocsis?= Date: Tue, 18 Aug 2020 13:38:18 +0200 Subject: [PATCH 1/2] Promote warnings to exceptions in ext/phar --- ext/phar/func_interceptors.c | 4 +-- ext/phar/phar_object.c | 12 ++++++--- ext/phar/phar_object.stub.php | 4 +-- ext/phar/phar_object_arginfo.h | 4 +-- ext/phar/tests/fgc_edgecases.phpt | 27 ++++++++++++++++----- ext/phar/tests/tar/phar_setdefaultstub.phpt | 20 ++++++++++----- ext/phar/tests/zip/phar_setdefaultstub.phpt | 18 ++++++++++---- 7 files changed, 62 insertions(+), 27 deletions(-) diff --git a/ext/phar/func_interceptors.c b/ext/phar/func_interceptors.c index b3eea43ec8ace..eca7a11d8b52f 100644 --- a/ext/phar/func_interceptors.c +++ b/ext/phar/func_interceptors.c @@ -137,8 +137,8 @@ PHAR_FUNC(phar_file_get_contents) /* {{{ */ if (ZEND_NUM_ARGS() == 5 && maxlen < 0) { efree(arch); - php_error_docref(NULL, E_WARNING, "Length must be greater than or equal to zero"); - RETURN_FALSE; + zend_argument_value_error(5, "must be greater than or equal to 0"); + RETURN_THROWS(); } /* retrieving a file defaults to within the current directory, so use this if possible */ diff --git a/ext/phar/phar_object.c b/ext/phar/phar_object.c index 97be3e900c117..a861f101fea36 100644 --- a/ext/phar/phar_object.c +++ b/ext/phar/phar_object.c @@ -2918,7 +2918,7 @@ PHP_METHOD(Phar, setDefaultStub) size_t index_len = 0, webindex_len = 0; int created_stub = 0; - if (zend_parse_parameters(ZEND_NUM_ARGS(), "|s!s", &index, &index_len, &webindex, &webindex_len) == FAILURE) { + if (zend_parse_parameters(ZEND_NUM_ARGS(), "|s!s!", &index, &index_len, &webindex, &webindex_len) == FAILURE) { RETURN_THROWS(); } @@ -2935,9 +2935,13 @@ PHP_METHOD(Phar, setDefaultStub) RETURN_THROWS(); } - if (ZEND_NUM_ARGS() > 0 && (phar_obj->archive->is_tar || phar_obj->archive->is_zip)) { - php_error_docref(NULL, E_WARNING, "Method accepts no arguments for a tar- or zip-based phar stub, %d given", ZEND_NUM_ARGS()); - RETURN_FALSE; + if ((index || webindex) && (phar_obj->archive->is_tar || phar_obj->archive->is_zip)) { + const char *space; + zend_value_error( + "%s::setDefaultStub(): Argument #%d ($%s) must be null for a tar- or zip-based phar stub, string given", + get_active_class_name(&space), index ? 1 : 2, index ? "index" : "webindex" + ); + RETURN_THROWS(); } if (PHAR_G(readonly)) { diff --git a/ext/phar/phar_object.stub.php b/ext/phar/phar_object.stub.php index 7f0eca1034f8f..d9b69f784f4d2 100644 --- a/ext/phar/phar_object.stub.php +++ b/ext/phar/phar_object.stub.php @@ -125,7 +125,7 @@ public function offsetUnset($entry) {} public function setAlias(string $alias) {} /** @return bool */ - public function setDefaultStub(?string $index = null, string $webindex = UNKNOWN) {} + public function setDefaultStub(?string $index = null, ?string $webindex = null) {} /** * @param mixed $metadata @@ -398,7 +398,7 @@ public function setAlias(string $alias) {} * @return bool * @alias Phar::setDefaultStub */ - public function setDefaultStub(?string $index = null, string $webindex = UNKNOWN) {} + public function setDefaultStub(?string $index = null, ?string $webindex = null) {} /** * @param mixed $metadata diff --git a/ext/phar/phar_object_arginfo.h b/ext/phar/phar_object_arginfo.h index 67b5ba558029f..95799bd487822 100644 --- a/ext/phar/phar_object_arginfo.h +++ b/ext/phar/phar_object_arginfo.h @@ -1,5 +1,5 @@ /* This is a generated file, edit the .stub.php file instead. - * Stub hash: f25efd47b496a7d06a30c77911a565a49e383bce */ + * Stub hash: be0f8bcd0ef8fdac59700160dff7d0beb210aa48 */ ZEND_BEGIN_ARG_INFO_EX(arginfo_class_Phar___construct, 0, 0, 1) ZEND_ARG_TYPE_INFO(0, filename, IS_STRING, 0) @@ -125,7 +125,7 @@ ZEND_END_ARG_INFO() ZEND_BEGIN_ARG_INFO_EX(arginfo_class_Phar_setDefaultStub, 0, 0, 0) ZEND_ARG_TYPE_INFO_WITH_DEFAULT_VALUE(0, index, IS_STRING, 1, "null") - ZEND_ARG_TYPE_INFO(0, webindex, IS_STRING, 0) + ZEND_ARG_TYPE_INFO_WITH_DEFAULT_VALUE(0, webindex, IS_STRING, 1, "null") ZEND_END_ARG_INFO() ZEND_BEGIN_ARG_INFO_EX(arginfo_class_Phar_setMetadata, 0, 0, 1) diff --git a/ext/phar/tests/fgc_edgecases.phpt b/ext/phar/tests/fgc_edgecases.phpt index a902b669c9aa7..38ff03cce8b0e 100644 --- a/ext/phar/tests/fgc_edgecases.phpt +++ b/ext/phar/tests/fgc_edgecases.phpt @@ -29,7 +29,11 @@ mkdir($pname . '/oops'); file_put_contents($pname . '/foo/hi', 'getMessage() . "\n"; +} echo file_get_contents("fgc_edgecases.txt"); set_include_path("' . addslashes(__DIR__) . '"); echo file_get_contents("fgc_edgecases.txt", true); @@ -53,7 +57,11 @@ blah getMessage() . "\n"; +} echo file_get_contents("fgc_edgecases.txt"); set_include_path("%stests"); echo file_get_contents("fgc_edgecases.txt", true); @@ -63,14 +71,17 @@ echo file_get_contents("./hi", 0, $context, 50000); echo file_get_contents("./hi"); echo file_get_contents("./hi", 0, $context, 0, 0); ?> - -Warning: file_get_contents(): Length must be greater than or equal to zero in phar://%sfgc_edgecases.phar.php/foo/hi on line %d +file_get_contents(): Argument #5 ($maxlen) must be greater than or equal to 0 test test getMessage() . "\n"; +} echo file_get_contents("fgc_edgecases.txt"); set_include_path("%stests"); echo file_get_contents("fgc_edgecases.txt", true); @@ -87,7 +98,11 @@ Warning: file_get_contents(): Failed to seek to position 50000 in the stream in getMessage() . "\n"; +} echo file_get_contents("fgc_edgecases.txt"); set_include_path("%stests"); echo file_get_contents("fgc_edgecases.txt", true); diff --git a/ext/phar/tests/tar/phar_setdefaultstub.phpt b/ext/phar/tests/tar/phar_setdefaultstub.phpt index 6811c4c59dd26..7404a5851eb89 100644 --- a/ext/phar/tests/tar/phar_setdefaultstub.phpt +++ b/ext/phar/tests/tar/phar_setdefaultstub.phpt @@ -33,18 +33,28 @@ echo "========================================================================== try { $phar->setDefaultStub('my/custom/thingy.php'); +} catch(ValueError $e) { + echo $e->getMessage(). "\n"; +} + +try { $phar->stopBuffering(); } catch(Exception $e) { echo $e->getMessage(). "\n"; } - var_dump($phar->getStub()); echo "============================================================================\n"; echo "============================================================================\n"; + try { $phar->setDefaultStub('my/custom/thingy.php', 'the/web.php'); +} catch(ValueError $e) { + echo $e->getMessage(). "\n"; +} + +try { $phar->stopBuffering(); } catch(Exception $e) { echo $e->getMessage(). "\n"; @@ -57,7 +67,7 @@ var_dump($phar->getStub()); ---EXPECTF-- +--EXPECT-- string(51) " " ============================================================================ @@ -66,13 +76,11 @@ string(60) "setDefaultStub('my/custom/thingy.php'); +} catch(Error $e) { + echo $e->getMessage(). "\n"; +} + +try { $phar->stopBuffering(); } catch(Exception $e) { echo $e->getMessage(). "\n"; @@ -45,6 +50,11 @@ echo "========================================================================== try { $phar->setDefaultStub('my/custom/thingy.php', 'the/web.php'); +} catch(ValueError $e) { + echo $e->getMessage(). "\n"; +} + +try { $phar->stopBuffering(); } catch(Exception $e) { echo $e->getMessage(). "\n"; @@ -57,7 +67,7 @@ var_dump($phar->getStub()); ---EXPECTF-- +--EXPECT-- string(51) " " ============================================================================ @@ -66,13 +76,11 @@ string(60) " Date: Mon, 24 Aug 2020 20:08:45 +0200 Subject: [PATCH 2/2] Address code review --- ext/phar/func_interceptors.c | 11 ++++++++--- ext/phar/phar_object.c | 6 +----- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/ext/phar/func_interceptors.c b/ext/phar/func_interceptors.c index eca7a11d8b52f..f7cfe2249f777 100644 --- a/ext/phar/func_interceptors.c +++ b/ext/phar/func_interceptors.c @@ -97,7 +97,8 @@ PHAR_FUNC(phar_file_get_contents) /* {{{ */ zend_bool use_include_path = 0; php_stream *stream; zend_long offset = -1; - zend_long maxlen = PHP_STREAM_COPY_ALL; + zend_long maxlen; + zend_bool maxlen_is_null = 1; zval *zcontext = NULL; if (!PHAR_G(intercepted)) { @@ -110,10 +111,14 @@ PHAR_FUNC(phar_file_get_contents) /* {{{ */ } /* Parse arguments */ - if (zend_parse_parameters_ex(ZEND_PARSE_PARAMS_QUIET, ZEND_NUM_ARGS(), "p|br!ll", &filename, &filename_len, &use_include_path, &zcontext, &offset, &maxlen) == FAILURE) { + if (zend_parse_parameters_ex(ZEND_PARSE_PARAMS_QUIET, ZEND_NUM_ARGS(), "p|br!ll!", &filename, &filename_len, &use_include_path, &zcontext, &offset, &maxlen, &maxlen_is_null) == FAILURE) { goto skip_phar; } + if (maxlen_is_null) { + maxlen = (ssize_t) PHP_STREAM_COPY_ALL; + } + if (use_include_path || (!IS_ABSOLUTE_PATH(filename, filename_len) && !strstr(filename, "://"))) { char *arch, *entry, *fname; zend_string *entry_str = NULL; @@ -135,7 +140,7 @@ PHAR_FUNC(phar_file_get_contents) /* {{{ */ /* fopen within phar, if :// is not in the url, then prepend phar:/// */ entry_len = filename_len; - if (ZEND_NUM_ARGS() == 5 && maxlen < 0) { + if (!maxlen_is_null && maxlen < 0) { efree(arch); zend_argument_value_error(5, "must be greater than or equal to 0"); RETURN_THROWS(); diff --git a/ext/phar/phar_object.c b/ext/phar/phar_object.c index a861f101fea36..efb7048cf6ab5 100644 --- a/ext/phar/phar_object.c +++ b/ext/phar/phar_object.c @@ -2936,11 +2936,7 @@ PHP_METHOD(Phar, setDefaultStub) } if ((index || webindex) && (phar_obj->archive->is_tar || phar_obj->archive->is_zip)) { - const char *space; - zend_value_error( - "%s::setDefaultStub(): Argument #%d ($%s) must be null for a tar- or zip-based phar stub, string given", - get_active_class_name(&space), index ? 1 : 2, index ? "index" : "webindex" - ); + zend_argument_value_error(index ? 1 : 2, "must be null for a tar- or zip-based phar stub, string given"); RETURN_THROWS(); }