From ff76978e13a0119d41a6b7efa8db8a000e9989a1 Mon Sep 17 00:00:00 2001 From: Jordi Domenech Bonilla Date: Sun, 16 Aug 2020 07:31:10 +0200 Subject: [PATCH 1/3] Fix #79969: Wrong error message in strict mode in ReflectionMethod::__construct The ReflectionMethod constructor can be called by using 1 or 2 parameters. The parameter parsing was checking first if the two parameters were correct quietly and, in case they weren't, it would try again but considering one parameter, this time throwing errors if it was not correct. As checking for the parameter type was performed in the first check (with two parameters), the type error was muted and replaced instead with the errors for the check with one parameter. --- ext/reflection/php_reflection.c | 17 ++++++++++++----- .../ReflectionMethod_constructor_error2.phpt | 2 +- ext/reflection/tests/bug79969.phpt | 14 ++++++++++++++ 3 files changed, 27 insertions(+), 6 deletions(-) create mode 100644 ext/reflection/tests/bug79969.phpt diff --git a/ext/reflection/php_reflection.c b/ext/reflection/php_reflection.c index 3fa4533e4e093..b14d9226fb6a7 100644 --- a/ext/reflection/php_reflection.c +++ b/ext/reflection/php_reflection.c @@ -2954,7 +2954,17 @@ ZEND_METHOD(reflection_method, __construct) size_t name_len, tmp_len; zval ztmp; - if (zend_parse_parameters_ex(ZEND_PARSE_PARAMS_QUIET, ZEND_NUM_ARGS(), "zs", &classname, &name_str, &name_len) == FAILURE) { + if (ZEND_NUM_ARGS() == 2) { + if (zend_parse_parameters_throw(ZEND_NUM_ARGS(), "zs", &classname, &name_str, &name_len) == FAILURE) { + return; + } + + if (Z_TYPE_P(classname) == IS_OBJECT) { + orig_obj = classname; + } else { + orig_obj = NULL; + } + } else { if (zend_parse_parameters_throw(ZEND_NUM_ARGS(), "s", &name_str, &name_len) == FAILURE) { return; } @@ -2964,16 +2974,13 @@ ZEND_METHOD(reflection_method, __construct) "Invalid method name %s", name_str); return; } + classname = &ztmp; tmp_len = tmp - name_str; ZVAL_STRINGL(classname, name_str, tmp_len); name_len = name_len - (tmp_len + 2); name_str = tmp + 2; orig_obj = NULL; - } else if (Z_TYPE_P(classname) == IS_OBJECT) { - orig_obj = classname; - } else { - orig_obj = NULL; } object = getThis(); diff --git a/ext/reflection/tests/ReflectionMethod_constructor_error2.phpt b/ext/reflection/tests/ReflectionMethod_constructor_error2.phpt index 723ab00f84f22..ae376132de091 100644 --- a/ext/reflection/tests/ReflectionMethod_constructor_error2.phpt +++ b/ext/reflection/tests/ReflectionMethod_constructor_error2.phpt @@ -58,4 +58,4 @@ Too many arguments: Ok - ReflectionMethod::__construct() expects exactly 1 parameter, 3 given Ok - Class InvalidClassName does not exist Ok - The parameter class is expected to be either a string or an object -Ok - ReflectionMethod::__construct() expects exactly 1 parameter, 2 given +Ok - ReflectionMethod::__construct() expects parameter 2 to be string, array given diff --git a/ext/reflection/tests/bug79969.phpt b/ext/reflection/tests/bug79969.phpt new file mode 100644 index 0000000000000..c0e6bb54d56c4 --- /dev/null +++ b/ext/reflection/tests/bug79969.phpt @@ -0,0 +1,14 @@ +--TEST-- +Bug #79969: ReflectionMethod::__construct throws incorrect exception when 2nd parameter is int +--FILE-- + +--EXPECTF-- +Fatal error: Uncaught TypeError: ReflectionMethod::__construct() expects parameter 2 to be string, int given in %s +Stack trace: +#0 %s(%d): ReflectionMethod->__construct('xxx', 1) +#1 {main} + thrown in %s on line %d From 8bdd5c9803ad338ad63d98f6c3a566c9f925def9 Mon Sep 17 00:00:00 2001 From: Jordi Domenech Bonilla Date: Sun, 16 Aug 2020 15:01:28 +0200 Subject: [PATCH 2/3] Improved correction for fix #79969 after feedback --- ext/reflection/php_reflection.c | 54 +++++++++++-------- .../tests/ReflectionMethod_006.phpt | 18 +++---- .../ReflectionMethod_constructor_error2.phpt | 27 ++++------ 3 files changed, 47 insertions(+), 52 deletions(-) diff --git a/ext/reflection/php_reflection.c b/ext/reflection/php_reflection.c index b14d9226fb6a7..6e2d3a4ec92cc 100644 --- a/ext/reflection/php_reflection.c +++ b/ext/reflection/php_reflection.c @@ -2954,33 +2954,41 @@ ZEND_METHOD(reflection_method, __construct) size_t name_len, tmp_len; zval ztmp; - if (ZEND_NUM_ARGS() == 2) { - if (zend_parse_parameters_throw(ZEND_NUM_ARGS(), "zs", &classname, &name_str, &name_len) == FAILURE) { - return; - } + switch (ZEND_NUM_ARGS()) { + case 2: + if (zend_parse_parameters_throw(ZEND_NUM_ARGS(), "zs", &classname, &name_str, &name_len) == FAILURE) { + return; + } - if (Z_TYPE_P(classname) == IS_OBJECT) { - orig_obj = classname; - } else { + if (Z_TYPE_P(classname) == IS_OBJECT) { + orig_obj = classname; + } else { + orig_obj = NULL; + } + + break; + + case 1: + if (zend_parse_parameters_throw(ZEND_NUM_ARGS(), "s", &name_str, &name_len) == FAILURE) { + return; + } + + if ((tmp = strstr(name_str, "::")) == NULL) { + zend_throw_exception_ex(reflection_exception_ptr, 0, "Invalid method name %s", name_str); + return; + } + + classname = &ztmp; + tmp_len = tmp - name_str; + ZVAL_STRINGL(classname, name_str, tmp_len); + name_len = name_len - (tmp_len + 2); + name_str = tmp + 2; orig_obj = NULL; - } - } else { - if (zend_parse_parameters_throw(ZEND_NUM_ARGS(), "s", &name_str, &name_len) == FAILURE) { - return; - } - if ((tmp = strstr(name_str, "::")) == NULL) { - zend_throw_exception_ex(reflection_exception_ptr, 0, - "Invalid method name %s", name_str); - return; - } + break; - classname = &ztmp; - tmp_len = tmp - name_str; - ZVAL_STRINGL(classname, name_str, tmp_len); - name_len = name_len - (tmp_len + 2); - name_str = tmp + 2; - orig_obj = NULL; + default: + ZEND_WRONG_PARAM_COUNT(); } object = getThis(); diff --git a/ext/reflection/tests/ReflectionMethod_006.phpt b/ext/reflection/tests/ReflectionMethod_006.phpt index 627dc96f32ee6..b76ffc2e893c1 100644 --- a/ext/reflection/tests/ReflectionMethod_006.phpt +++ b/ext/reflection/tests/ReflectionMethod_006.phpt @@ -6,16 +6,9 @@ Steve Seear --FILE-- getMessage().PHP_EOL; -} -try { - new ReflectionMethod('a', 'b', 'c'); -} catch (TypeError $re) { - echo "Ok - ".$re->getMessage().PHP_EOL; -} +new ReflectionMethod(); +new ReflectionMethod('a', 'b', 'c'); + class C { public function f() {} @@ -43,8 +36,9 @@ var_dump($rm->getName(1)); ?> --EXPECTF-- -Ok - ReflectionMethod::__construct() expects exactly 1 parameter, 0 given -Ok - ReflectionMethod::__construct() expects exactly 1 parameter, 3 given +Warning: Wrong parameter count for ReflectionMethod::__construct() in %s on line %d + +Warning: Wrong parameter count for ReflectionMethod::__construct() in %s on line %d Warning: ReflectionMethod::isFinal() expects exactly 0 parameters, 1 given in %s on line %d NULL diff --git a/ext/reflection/tests/ReflectionMethod_constructor_error2.phpt b/ext/reflection/tests/ReflectionMethod_constructor_error2.phpt index ae376132de091..971681227f5ac 100644 --- a/ext/reflection/tests/ReflectionMethod_constructor_error2.phpt +++ b/ext/reflection/tests/ReflectionMethod_constructor_error2.phpt @@ -12,20 +12,11 @@ class TestClass } } +echo "Too few arguments:\n"; +$methodInfo = new ReflectionMethod(); -try { - echo "Too few arguments:\n"; - $methodInfo = new ReflectionMethod(); -} catch (TypeError $re) { - echo "Ok - ".$re->getMessage().PHP_EOL; -} -try { - echo "\nToo many arguments:\n"; - $methodInfo = new ReflectionMethod("TestClass", "foo", true); -} catch (TypeError $re) { - echo "Ok - ".$re->getMessage().PHP_EOL; -} - +echo "\nToo many arguments:\n"; +$methodInfo = new ReflectionMethod("TestClass", "foo", true); try { //invalid class @@ -50,12 +41,14 @@ try{ } ?> ---EXPECT-- +--EXPECTF-- Too few arguments: -Ok - ReflectionMethod::__construct() expects exactly 1 parameter, 0 given + +Warning: Wrong parameter count for ReflectionMethod::__construct() in %s on line %d Too many arguments: -Ok - ReflectionMethod::__construct() expects exactly 1 parameter, 3 given + +Warning: Wrong parameter count for ReflectionMethod::__construct() in %s on line %d Ok - Class InvalidClassName does not exist Ok - The parameter class is expected to be either a string or an object -Ok - ReflectionMethod::__construct() expects parameter 2 to be string, array given +Ok - ReflectionMethod::__construct() expects parameter 2 to be string, array given \ No newline at end of file From 897b4918ace5ae6004aa390a502e4727d1f4449a Mon Sep 17 00:00:00 2001 From: Jordi Domenech Bonilla Date: Mon, 17 Aug 2020 12:16:05 +0200 Subject: [PATCH 3/3] Improved fix #79969 given feedback by assuring parameter types --- ext/reflection/php_reflection.c | 57 +++++++++++-------- ext/reflection/tests/008.phpt | 2 +- .../tests/ReflectionMethod_006.phpt | 39 +++++++++++-- .../ReflectionMethod_constructor_error1.phpt | 8 +-- .../ReflectionMethod_constructor_error2.phpt | 20 +++++-- 5 files changed, 85 insertions(+), 41 deletions(-) diff --git a/ext/reflection/php_reflection.c b/ext/reflection/php_reflection.c index 6e2d3a4ec92cc..4d851f436c1ff 100644 --- a/ext/reflection/php_reflection.c +++ b/ext/reflection/php_reflection.c @@ -2953,42 +2953,49 @@ ZEND_METHOD(reflection_method, __construct) char *name_str, *tmp; size_t name_len, tmp_len; zval ztmp; + const char *space; - switch (ZEND_NUM_ARGS()) { - case 2: - if (zend_parse_parameters_throw(ZEND_NUM_ARGS(), "zs", &classname, &name_str, &name_len) == FAILURE) { - return; - } + if (zend_parse_parameters_throw(ZEND_NUM_ARGS(), "z|s!", &classname, &name_str, &name_len) == FAILURE) { + return; + } - if (Z_TYPE_P(classname) == IS_OBJECT) { - orig_obj = classname; - } else { - orig_obj = NULL; + switch (Z_TYPE_P(classname)) { + case IS_STRING: + orig_obj = NULL; + + if (ZEND_NUM_ARGS() == 1) { + name_str = Z_STRVAL_P(classname); + name_len = Z_STRLEN_P(classname); + + if ((tmp = strstr(name_str, "::")) == NULL) { + zend_throw_exception_ex(reflection_exception_ptr, 0, "Invalid method name %s", name_str); + return; + } + + classname = &ztmp; + tmp_len = tmp - name_str; + ZVAL_STRINGL(classname, name_str, tmp_len); + name_len = name_len - (tmp_len + 2); + name_str = tmp + 2; } break; - case 1: - if (zend_parse_parameters_throw(ZEND_NUM_ARGS(), "s", &name_str, &name_len) == FAILURE) { - return; - } + case IS_OBJECT: + if (ZEND_NUM_ARGS() == 1 || !name_str) { + tmp = get_active_class_name(&space); - if ((tmp = strstr(name_str, "::")) == NULL) { - zend_throw_exception_ex(reflection_exception_ptr, 0, "Invalid method name %s", name_str); - return; + zend_type_error("%s%s%s() expects parameter %d to be %s, %s given", + tmp, space, get_active_function_name(), 2, "string", "null"); + return; } - classname = &ztmp; - tmp_len = tmp - name_str; - ZVAL_STRINGL(classname, name_str, tmp_len); - name_len = name_len - (tmp_len + 2); - name_str = tmp + 2; - orig_obj = NULL; + orig_obj = classname; break; default: - ZEND_WRONG_PARAM_COUNT(); + _DO_THROW("The parameter class is expected to be either a string or an object"); } object = getThis(); @@ -3017,8 +3024,8 @@ ZEND_METHOD(reflection_method, __construct) if (classname == &ztmp) { zval_ptr_dtor_str(&ztmp); } - _DO_THROW("The parameter class is expected to be either a string or an object"); - /* returns out of this function */ + + return; } if (classname == &ztmp) { diff --git a/ext/reflection/tests/008.phpt b/ext/reflection/tests/008.phpt index 80a4a39218e05..f54c8862cb314 100644 --- a/ext/reflection/tests/008.phpt +++ b/ext/reflection/tests/008.phpt @@ -28,7 +28,7 @@ echo "Done\n"; ?> --EXPECT-- string(20) "Invalid method name " -string(21) "Invalid method name 1" +string(66) "The parameter class is expected to be either a string or an object" string(21) "Class does not exist" string(22) "Class a does not exist" string(21) "Class does not exist" diff --git a/ext/reflection/tests/ReflectionMethod_006.phpt b/ext/reflection/tests/ReflectionMethod_006.phpt index b76ffc2e893c1..56de7d36a76ff 100644 --- a/ext/reflection/tests/ReflectionMethod_006.phpt +++ b/ext/reflection/tests/ReflectionMethod_006.phpt @@ -6,14 +6,41 @@ Steve Seear --FILE-- getMessage()); +} + +try { + new ReflectionMethod('a', 'b', 'c'); +} catch (Throwable $e) { + var_dump($e->getMessage()); +} class C { public function f() {} } +try { + new ReflectionMethod(new C); +} catch (Throwable $e) { + var_dump($e->getMessage()); +} + +try { + new ReflectionMethod(new C, null); +} catch (Throwable $e) { + var_dump($e->getMessage()); +} + +try { + new ReflectionMethod(new C, ""); +} catch (Throwable $e) { + var_dump($e->getMessage()); +} + $rm = new ReflectionMethod('C', 'f'); var_dump($rm->isFinal(1)); @@ -36,9 +63,11 @@ var_dump($rm->getName(1)); ?> --EXPECTF-- -Warning: Wrong parameter count for ReflectionMethod::__construct() in %s on line %d - -Warning: Wrong parameter count for ReflectionMethod::__construct() in %s on line %d +string(69) "ReflectionMethod::__construct() expects at least 1 parameter, 0 given" +string(69) "ReflectionMethod::__construct() expects at most 2 parameters, 3 given" +string(76) "ReflectionMethod::__construct() expects parameter 2 to be string, null given" +string(76) "ReflectionMethod::__construct() expects parameter 2 to be string, null given" +string(27) "Method C::() does not exist" Warning: ReflectionMethod::isFinal() expects exactly 0 parameters, 1 given in %s on line %d NULL diff --git a/ext/reflection/tests/ReflectionMethod_constructor_error1.phpt b/ext/reflection/tests/ReflectionMethod_constructor_error1.phpt index 5c980536b9f9b..e90bea879cdf8 100644 --- a/ext/reflection/tests/ReflectionMethod_constructor_error1.phpt +++ b/ext/reflection/tests/ReflectionMethod_constructor_error1.phpt @@ -65,14 +65,14 @@ try { ?> --EXPECTF-- Wrong type of argument (bool): -ReflectionException: Invalid method name 1 in %s +ReflectionException: The parameter class is expected to be either a string or an object in %s:%d Stack trace: -#0 %s ReflectionMethod->__construct('1') +#0 %s ReflectionMethod->__construct(true) #1 {main} Wrong type of argument (int): -ReflectionException: Invalid method name 3 in %s +ReflectionException: The parameter class is expected to be either a string or an object in %s:%d Stack trace: -#0 %s ReflectionMethod->__construct('3') +#0 %s ReflectionMethod->__construct(3) #1 {main} Wrong type of argument (bool, string): ReflectionException: The parameter class is expected to be either a string or an object in %s diff --git a/ext/reflection/tests/ReflectionMethod_constructor_error2.phpt b/ext/reflection/tests/ReflectionMethod_constructor_error2.phpt index 971681227f5ac..aa164ee64d49c 100644 --- a/ext/reflection/tests/ReflectionMethod_constructor_error2.phpt +++ b/ext/reflection/tests/ReflectionMethod_constructor_error2.phpt @@ -13,10 +13,20 @@ class TestClass } echo "Too few arguments:\n"; -$methodInfo = new ReflectionMethod(); + +try { + $methodInfo = new ReflectionMethod(); +} catch (Throwable $e) { + var_dump($e->getMessage()); +} echo "\nToo many arguments:\n"; -$methodInfo = new ReflectionMethod("TestClass", "foo", true); + +try { + $methodInfo = new ReflectionMethod("TestClass", "foo", true); +} catch (Throwable $e) { + var_dump($e->getMessage()); +} try { //invalid class @@ -43,12 +53,10 @@ try{ ?> --EXPECTF-- Too few arguments: - -Warning: Wrong parameter count for ReflectionMethod::__construct() in %s on line %d +string(69) "ReflectionMethod::__construct() expects at least 1 parameter, 0 given" Too many arguments: - -Warning: Wrong parameter count for ReflectionMethod::__construct() in %s on line %d +string(69) "ReflectionMethod::__construct() expects at most 2 parameters, 3 given" Ok - Class InvalidClassName does not exist Ok - The parameter class is expected to be either a string or an object Ok - ReflectionMethod::__construct() expects parameter 2 to be string, array given \ No newline at end of file