PDO Unconditional errors by Girgias · Pull Request #6212 · php/php-src

Girgias · 2020-09-25T04:02:53Z

A couple of unrelated refactoring changes that I made should be able to land on their own if it makes the PR too noisy.

Used only PDO MySQL for tests so hopefully not too many more drivers complain.

nikic

Can you please apply the bug fix separately (together with corresponding test changes)?

nikic · 2020-09-25T08:24:34Z

And please also apply at least "Inverse logic in do_fetch() to reduce a level of indentation" separately, as the indentation changes makes it hard to understand the diff.

nikic

First pass ... this is huge

Girgias · 2020-09-28T10:37:34Z

Pushing review changed before the ZPP variadic revert which I'm doing now

pdo_stmt_verify_mode() never returns FAILURE (aka -1) and thus this path was never hit

Promote a couple of confirmed cases to unconditional error Fix error messages

… the ODBC driver And ammend PDO::quote() test to take into account drivers which do not support the feature

nikic

This looks pretty good

nikic · 2020-09-28T13:11:57Z

I'm not confident it can't fail, e.g. >&- is used.

nikic · 2020-09-28T13:20:00Z

So this happens if you use an out of bounds parameter number? I think generally it would make sense for things like this to throw, but we need to be careful that native & emulated prepared statements have the same behavior. I don't know whether both go through this codepath.

I think those trigger if the param name/number is not present. I would imagine they both go through this code path as this is something internal to PDO which is not exposed, as all PDO bind param/value methods go through this. Or do emulated prepared statements somehow override the PDO methods?

Indentation Error message Use variadic number position for pdo_stmt_setup_fetch_mode() param cehcks Use values from *args directly instead of preassigning them in pdo_stmt_setup_fetch_mode() Drop todo comment

Indent (again) Drop unnecessary cehck Improve title + SKIPIF for PDO SQLite test

nikic · 2020-09-28T14:45:30Z

- );
- PDO_HANDLE_DBH_ERR();
- RETURN_FALSE;
+ zend_type_error("PDO::ATTR_STATEMENT_CLASS ctor_args must be ?array, %s given",


Suggested change
zend_type_error("PDO::ATTR_STATEMENT_CLASS ctor_args must be ?array, %s given",
zend_type_error("PDO::ATTR_STATEMENT_CLASS ctor_args must be of type ?array, %s given",

nikic · 2020-09-28T14:46:35Z

- "ctor_args must be an array"
- );
- PDO_HANDLE_DBH_ERR();
+ zend_type_error("PDO::ATTR_STATEMENT_CLASS ctor_args must be ?array, %s given",


Suggested change
zend_type_error("PDO::ATTR_STATEMENT_CLASS ctor_args must be ?array, %s given",
zend_type_error("PDO::ATTR_STATEMENT_CLASS ctor_args must be of type ?array, %s given",

nikic · 2020-09-28T14:49:30Z

+ /* Figure out correct class */
+ if (arg2) {
+ if (Z_TYPE_P(arg2) != IS_STRING) {
+ zend_argument_type_error(2, "must be string, %s given", zend_zval_type_name(arg2));


Suggested change
zend_argument_type_error(2, "must be string, %s given", zend_zval_type_name(arg2));
zend_argument_type_error(2, "must be of type string, %s given", zend_zval_type_name(arg2));

nikic · 2020-09-28T14:49:57Z

+ if (arg2) {
+ // Reuse convert_to_long(arg2); ?
+ if (Z_TYPE_P(arg2) != IS_LONG) {
+ zend_argument_type_error(2, "must be int, %s given", zend_zval_type_name(arg2));


Suggested change
zend_argument_type_error(2, "must be int, %s given", zend_zval_type_name(arg2));
zend_argument_type_error(2, "must be of type int, %s given", zend_zval_type_name(arg2));

nikic · 2020-09-28T14:50:31Z

+ return false;
+ }
+ if (Z_TYPE(args[0]) != IS_LONG) {
+ zend_argument_type_error(arg1_arg_num, "must be int, %s given", zend_zval_type_name(&args[0]));


Suggested change
zend_argument_type_error(arg1_arg_num, "must be int, %s given", zend_zval_type_name(&args[0]));
zend_argument_type_error(arg1_arg_num, "must be of type int, %s given", zend_zval_type_name(&args[0]));

nikic · 2020-09-28T14:50:56Z

+ return false;
+ }
+ if (Z_TYPE(args[0]) != IS_STRING) {
+ zend_argument_type_error(arg1_arg_num, "must be string, %s given", zend_zval_type_name(&args[0]));


Suggested change
zend_argument_type_error(arg1_arg_num, "must be string, %s given", zend_zval_type_name(&args[0]));
zend_argument_type_error(arg1_arg_num, "must be of type string, %s given", zend_zval_type_name(&args[0]));

nikic · 2020-09-28T14:51:10Z

- retval = FAILURE;
- } else if (Z_TYPE(args[1]) == IS_ARRAY && zend_hash_num_elements(Z_ARRVAL(args[1]))) {
- ZVAL_ARR(&stmt->fetch.cls.ctor_args, zend_array_dup(Z_ARRVAL(args[1])));
+ zend_argument_type_error(constructor_arg_num, "must be ?array, %s given",


Suggested change
zend_argument_type_error(constructor_arg_num, "must be ?array, %s given",
zend_argument_type_error(constructor_arg_num, "must be of type ?array, %s given",

nikic · 2020-09-28T14:51:21Z

-if (SUCCESS == retval) {
-ZVAL_COPY(&stmt->fetch.into, &args[0]);
+ if (Z_TYPE(args[0]) != IS_OBJECT) {
+	zend_argument_type_error(arg1_arg_num, "must be object, %s given", zend_zval_type_name(&args[0]));


Suggested change
zend_argument_type_error(arg1_arg_num, "must be object, %s given", zend_zval_type_name(&args[0]));
zend_argument_type_error(arg1_arg_num, "must be object, %s given", zend_zval_type_name(&args[0]));
Suggested change
zend_argument_type_error(arg1_arg_num, "must be object, %s given", zend_zval_type_name(&args[0]));
zend_argument_type_error(arg1_arg_num, "must be of type object, %s given", zend_zval_type_name(&args[0]));

nikic · 2020-09-28T14:51:41Z

 	zval *args = NULL;
 	uint32_t num_args = 0;

+	/* Support null for constructor arguments for BC */


Comment now a bit out of place

nikic · 2020-09-28T14:52:25Z

 	if (Z_TYPE_P(value) != IS_LONG && Z_TYPE_P(value) != IS_STRING && Z_TYPE_P(value) != IS_FALSE && Z_TYPE_P(value) != IS_TRUE) { \
- pdo_raise_impl_error(dbh, NULL, "HY000", "attribute value must be an integer"); \
- PDO_HANDLE_DBH_ERR(); \
+ zend_type_error("Attribute value must be int for selected attribute, %s given", zend_zval_type_name(value)); \


Suggested change
zend_type_error("Attribute value must be int for selected attribute, %s given", zend_zval_type_name(value)); \
zend_type_error("Attribute value must be of type int for selected attribute, %s given", zend_zval_type_name(value)); \

nikic

LG assuming CI happy.

Girgias requested a review from kocsismate September 25, 2020 04:03

kocsismate mentioned this pull request Sep 25, 2020
Warning to Error (TypeError/ValueError) in extensions php/php-tasks#18
Closed
71 tasks

nikic reviewed Sep 25, 2020
View reviewed changes

Comment threadext/pdo/pdo_stmt.c Outdated

Girgias force-pushed the pdo-unconditional-error branch from 19bc7ab to 1f276daCompare September 25, 2020 13:48

Girgias mentioned this pull request Sep 25, 2020
Fix logic error in PDO #6215
Closed

Girgias force-pushed the pdo-unconditional-error branch 5 times, most recently from ebdc73b to 9d79ad6Compare September 26, 2020 17:50

nikic reviewed Sep 27, 2020
View reviewed changes

Comment threadext/pdo/pdo_dbh.stub.php Outdated

Girgias force-pushed the pdo-unconditional-error branch from b03e59e to 32b0b47Compare September 27, 2020 22:37

nikic reviewed Sep 28, 2020
View reviewed changes

Girgias force-pushed the pdo-unconditional-error branch from 5e67d6d to 0ae6ecbCompare September 28, 2020 12:05

Girgias added 14 commits September 28, 2020 14:10

Fix a logical bug in PDO
2435c87
pdo_stmt_verify_mode() never returns FAILURE (aka -1) and thus this path was never hit

Column index must be >= 0 (just 1 case) and todo comment.
e280e5e

Refactor PDO setFetchMode
96770fa

Drop ZPP check in PDO Mysqli test...
c5a4c3e

Make certain error conditions always error in PDO::fetchAll()
4c75cae

Unconditonal error for values less than a minimum
2cf849b

Add todo comments
220d89f

ValueError for negative column index
b2bd8b6

Assertion + always throw on invalid Fetch mode
156ec13

ValueError for incorrect Fetch Flag usage
7bcf852

Make invalid user callable throw a type error
d92274a

More TODOs
4028d9f

Todo everything...
1615a93

ValueError for empty query in PDO::exec()
2f9bb0e

Girgias added 15 commits September 28, 2020 14:10

Redfine todo comment
8e9fa15

Introduce ValueError for empty bind param names
1725e31

Fix stubs after making uninitialized object return Error
79e9444

Introduce ValueError for empty string arguments
64349d3

Revert ValueError for empty username
a52ce29

Make fetch argument variadic
d94521b

Fix test after making fetch args variadic
ee37a1d

Review for pdo_dbh.c
89b233c
Promote a couple of confirmed cases to unconditional error Fix error messages

Drop todo comments in pdo_sql_parser.re
978b43d

Revert + add test for empty string in PDO::qutoe()
909651e

Drop ValueError for empty lastInsertId()
86959ee

Accept strings values again for integer attribute values
8944242

Review for pdo_stmt.c
a0eedd8
Promote a couple of confirmed cases to unconditional error Fix error messages

Revert ZPP to use variadics
2fd259f

Do not advertise support for a PDO feature if it's not implemented by…
c0d2d8b
… the ODBC driver And ammend PDO::quote() test to take into account drivers which do not support the feature

nikic reviewed Sep 28, 2020
View reviewed changes

Comment threadext/pdo_mysql/tests/pdo_mysql_stmt_nextrowset.phpt Outdated

nikic reviewed Sep 28, 2020
View reviewed changes

Comment threadext/pdo_sqlite/tests/bug_44159_sqlite_version.phpt Outdated

nikic reviewed Sep 28, 2020
View reviewed changes

Another round of review
e4a13d0
Indentation Error message Use variadic number position for pdo_stmt_setup_fetch_mode() param cehcks Use values from *args directly instead of preassigning them in pdo_stmt_setup_fetch_mode() Drop todo comment

Girgias force-pushed the pdo-unconditional-error branch from 0ae6ecb to e4a13d0Compare September 28, 2020 13:43

nikic reviewed Sep 28, 2020
View reviewed changes

Comment threadext/pdo_sqlite/tests/bug_44159_sqlite_version.phpt Outdated

Minor review
080465f
Indent (again) Drop unnecessary cehck Improve title + SKIPIF for PDO SQLite test

nikic reviewed Sep 28, 2020
View reviewed changes

Fix type error messages
c949e24

nikic approved these changes Sep 28, 2020
View reviewed changes

php-pulls closed this in a5cf828Sep 28, 2020

Girgias deleted the pdo-unconditional-error branch September 28, 2020 17:52

Girgias mentioned this pull request Sep 28, 2020
Another pass making some failure states unconditional erros in PDO #6227
Merged

	zend_type_error("PDO::ATTR_STATEMENT_CLASS ctor_args must be ?array, %s given",
	zend_type_error("PDO::ATTR_STATEMENT_CLASS ctor_args must be of type ?array, %s given",

	zend_argument_type_error(2, "must be string, %s given", zend_zval_type_name(arg2));
	zend_argument_type_error(2, "must be of type string, %s given", zend_zval_type_name(arg2));

	zend_argument_type_error(2, "must be int, %s given", zend_zval_type_name(arg2));
	zend_argument_type_error(2, "must be of type int, %s given", zend_zval_type_name(arg2));

	zend_argument_type_error(arg1_arg_num, "must be int, %s given", zend_zval_type_name(&args[0]));
	zend_argument_type_error(arg1_arg_num, "must be of type int, %s given", zend_zval_type_name(&args[0]));

	zend_argument_type_error(constructor_arg_num, "must be ?array, %s given",
	zend_argument_type_error(constructor_arg_num, "must be of type ?array, %s given",

	zend_type_error("Attribute value must be int for selected attribute, %s given", zend_zval_type_name(value)); \
	zend_type_error("Attribute value must be of type int for selected attribute, %s given", zend_zval_type_name(value)); \

Uh oh!

Conversation

Girgias commented Sep 25, 2020

Uh oh!

nikic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nikic commented Sep 25, 2020

Uh oh!

Uh oh!

nikic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Girgias commented Sep 28, 2020

Uh oh!

nikic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nikic left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants