From 32b6154a61fae820386527f3019f8c5937fc5d27 Mon Sep 17 00:00:00 2001 From: Adam Baratz Date: Mon, 10 Oct 2016 18:10:37 -0400 Subject: [PATCH] Fix #73234: Emulated statements let value dictate parameter type The prepared statement emulator (pdo_sql_parser.*) figures out how to quote each query parameter. The intended type is specified by the PDO::PARAM_* consts, but this direction wasn't always followed. In practice, queries could work as expected, but subtle errors could result. For example, a numeric string bound as PDO::PARAM_INT would be sent to a driver's quote function. While these functions are told which type is expected, they generally assume values are being quoted as strings. This can result in implicit casts, which are bad for performance. This commit includes the following changes: - Cast values marked as bool/int/null to the appropriate type and bypass the driver's quote function. - Save some memory by dropping the temporary zval used for casting. - Avoid a memory leak if the driver's quote function produces an error. - Appropriate test suite updates. --- NEWS | 4 ++++ ext/pdo/pdo_sql_parser.c | 46 +++++++++++++++++++++--------------- ext/pdo/pdo_sql_parser.re | 46 +++++++++++++++++++++--------------- ext/pdo/tests/bug_65946.phpt | 4 +--- ext/pdo/tests/bug_73234.phpt | 43 +++++++++++++++++++++++++++++++++ ext/pdo/tests/pdo_018.phpt | 18 +++++--------- ext/pdo/tests/pdo_024.phpt | 2 +- 7 files changed, 109 insertions(+), 54 deletions(-) create mode 100644 ext/pdo/tests/bug_73234.phpt diff --git a/NEWS b/NEWS index fba9c1c4b2fee..df57efc45561a 100644 --- a/NEWS +++ b/NEWS @@ -33,5 +33,9 @@ PHP NEWS . Added array input support to mb_convert_encoding(). (Yasuo) . Added array input support to mb_check_encoding(). (Yasuo) +- PDO_DBlib: + . Fixed bug #73234 (Emulated statements let value dictate parameter type). + (Adam Baratz) + <<< NOTE: Insert NEWS from last stable release here prior to actual release! >>> diff --git a/ext/pdo/pdo_sql_parser.c b/ext/pdo/pdo_sql_parser.c index 573d5865a0dfa..1156d9fe03d92 100644 --- a/ext/pdo/pdo_sql_parser.c +++ b/ext/pdo/pdo_sql_parser.c @@ -554,40 +554,48 @@ PDO_API int pdo_parse_params(pdo_stmt_t *stmt, char *inquery, size_t inquery_len } plc->freeq = 1; } else { - zval tmp_param; - ZVAL_DUP(&tmp_param, parameter); - switch (Z_TYPE(tmp_param)) { - case IS_NULL: - plc->quoted = "NULL"; - plc->qlen = sizeof("NULL")-1; + zend_string *buf = NULL; + + switch (param->param_type) { + case PDO_PARAM_BOOL: + plc->quoted = zend_is_true(parameter) ? "1" : "0"; + plc->qlen = sizeof("1")-1; plc->freeq = 0; break; - case IS_FALSE: - case IS_TRUE: - convert_to_long(&tmp_param); - /* fall through */ - case IS_LONG: - case IS_DOUBLE: - convert_to_string(&tmp_param); - plc->qlen = Z_STRLEN(tmp_param); - plc->quoted = estrdup(Z_STRVAL(tmp_param)); + case PDO_PARAM_INT: + buf = zend_long_to_str(zval_get_long(parameter)); + + plc->qlen = ZSTR_LEN(buf); + plc->quoted = estrdup(ZSTR_VAL(buf)); plc->freeq = 1; break; + case PDO_PARAM_NULL: + plc->quoted = "NULL"; + plc->qlen = sizeof("NULL")-1; + plc->freeq = 0; + break; + default: - convert_to_string(&tmp_param); - if (!stmt->dbh->methods->quoter(stmt->dbh, Z_STRVAL(tmp_param), - Z_STRLEN(tmp_param), &plc->quoted, &plc->qlen, + buf = zval_get_string(parameter); + if (!stmt->dbh->methods->quoter(stmt->dbh, ZSTR_VAL(buf), + ZSTR_LEN(buf), &plc->quoted, &plc->qlen, param->param_type)) { /* bork */ ret = -1; strncpy(stmt->error_code, stmt->dbh->error_code, 6); + if (buf) { + zend_string_release(buf); + } goto clean_up; } plc->freeq = 1; } - zval_dtor(&tmp_param); + + if (buf) { + zend_string_release(buf); + } } } else { zval *parameter; diff --git a/ext/pdo/pdo_sql_parser.re b/ext/pdo/pdo_sql_parser.re index 20b9976850c44..7528830dae412 100644 --- a/ext/pdo/pdo_sql_parser.re +++ b/ext/pdo/pdo_sql_parser.re @@ -240,40 +240,48 @@ safe: } plc->freeq = 1; } else { - zval tmp_param; - ZVAL_DUP(&tmp_param, parameter); - switch (Z_TYPE(tmp_param)) { - case IS_NULL: - plc->quoted = "NULL"; - plc->qlen = sizeof("NULL")-1; + zend_string *buf = NULL; + + switch (param->param_type) { + case PDO_PARAM_BOOL: + plc->quoted = zend_is_true(parameter) ? "1" : "0"; + plc->qlen = sizeof("1")-1; plc->freeq = 0; break; - case IS_FALSE: - case IS_TRUE: - convert_to_long(&tmp_param); - /* fall through */ - case IS_LONG: - case IS_DOUBLE: - convert_to_string(&tmp_param); - plc->qlen = Z_STRLEN(tmp_param); - plc->quoted = estrdup(Z_STRVAL(tmp_param)); + case PDO_PARAM_INT: + buf = zend_long_to_str(zval_get_long(parameter)); + + plc->qlen = ZSTR_LEN(buf); + plc->quoted = estrdup(ZSTR_VAL(buf)); plc->freeq = 1; break; + case PDO_PARAM_NULL: + plc->quoted = "NULL"; + plc->qlen = sizeof("NULL")-1; + plc->freeq = 0; + break; + default: - convert_to_string(&tmp_param); - if (!stmt->dbh->methods->quoter(stmt->dbh, Z_STRVAL(tmp_param), - Z_STRLEN(tmp_param), &plc->quoted, &plc->qlen, + buf = zval_get_string(parameter); + if (!stmt->dbh->methods->quoter(stmt->dbh, ZSTR_VAL(buf), + ZSTR_LEN(buf), &plc->quoted, &plc->qlen, param->param_type)) { /* bork */ ret = -1; strncpy(stmt->error_code, stmt->dbh->error_code, 6); + if (buf) { + zend_string_release(buf); + } goto clean_up; } plc->freeq = 1; } - zval_dtor(&tmp_param); + + if (buf) { + zend_string_release(buf); + } } } else { zval *parameter; diff --git a/ext/pdo/tests/bug_65946.phpt b/ext/pdo/tests/bug_65946.phpt index c636db52043f7..76a4e8db89e43 100644 --- a/ext/pdo/tests/bug_65946.phpt +++ b/ext/pdo/tests/bug_65946.phpt @@ -18,9 +18,7 @@ $db->exec('CREATE TABLE test(id int)'); $db->exec('INSERT INTO test VALUES(1)'); switch ($db->getAttribute(PDO::ATTR_DRIVER_NAME)) { case 'dblib': - // if :limit is used, the value will be quoted as '1', which is invalid syntax - // this is a bug, to be addressed separately from adding these tests to pdo_dblib - $sql = 'SELECT TOP 1 * FROM test'; + $sql = 'SELECT TOP :limit * FROM test'; break; case 'firebird': $sql = 'SELECT FIRST :limit * FROM test'; diff --git a/ext/pdo/tests/bug_73234.phpt b/ext/pdo/tests/bug_73234.phpt new file mode 100644 index 0000000000000..43b484e9b26e0 --- /dev/null +++ b/ext/pdo/tests/bug_73234.phpt @@ -0,0 +1,43 @@ +--TEST-- +PDO Common: Bug #73234 (Emulated statements let value dictate parameter type) +--SKIPIF-- + +--FILE-- +setAttribute(PDO::ATTR_EMULATE_PREPARES, true); +$db->exec('CREATE TABLE test(id INT NULL)'); + +$stmt = $db->prepare('INSERT INTO test VALUES(:value)'); + +$stmt->bindValue(':value', 0, PDO::PARAM_NULL); +$stmt->execute(); + +$stmt->bindValue(':value', null, PDO::PARAM_NULL); +$stmt->execute(); + +$stmt = $db->query('SELECT * FROM test'); +var_dump($stmt->fetchAll(PDO::FETCH_ASSOC)); +?> +--EXPECT-- +array(2) { + [0]=> + array(1) { + ["id"]=> + NULL + } + [1]=> + array(1) { + ["id"]=> + NULL + } +} diff --git a/ext/pdo/tests/pdo_018.phpt b/ext/pdo/tests/pdo_018.phpt index 80e34532872f2..9bb0d1c7a1521 100644 --- a/ext/pdo/tests/pdo_018.phpt +++ b/ext/pdo/tests/pdo_018.phpt @@ -106,22 +106,16 @@ unset($stmt); echo "===INSERT===\n"; $stmt = $db->prepare('INSERT INTO test VALUES(:id, :classtype, :val)'); -$stmt->bindParam(':id', $idx); -$stmt->bindParam(':classtype', $ctype); -$stmt->bindParam(':val', $val); foreach($objs as $idx => $obj) { $ctype = $ctypes[get_class($obj)]; - if (method_exists($obj, 'serialize')) - { - $val = $obj->serialize(); - } - else - { - $val = ''; - } - $stmt->execute(); + + $stmt->bindValue(':id', $idx); + $stmt->bindValue(':classtype', $ctype, $ctype === null ? PDO::PARAM_NULL : PDO::PARAM_INT); + $stmt->bindValue(':val', method_exists($obj, 'serialize') ? $obj->serialize() : ''); + + $stmt->execute(); } unset($stmt); diff --git a/ext/pdo/tests/pdo_024.phpt b/ext/pdo/tests/pdo_024.phpt index 571ea732868d8..a70e924d49a8e 100644 --- a/ext/pdo/tests/pdo_024.phpt +++ b/ext/pdo/tests/pdo_024.phpt @@ -19,7 +19,7 @@ $db->exec('create table test (id int, name varchar(10) null)'); $stmt = $db->prepare('insert into test (id, name) values(0, :name)'); $name = NULL; $before_bind = $name; -$stmt->bindParam(':name', $name); +$stmt->bindParam(':name', $name, PDO::PARAM_NULL); if ($name !== $before_bind) { echo "bind: fail\n"; } else {