Skip to content

Commit

Permalink
Fix #73234: Emulated statements let value dictate parameter type
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@adambaratz
adambaratz committed Oct 10, 2016
1 parent f51405c commit 32b6154
Show file tree
Hide file tree
Showing 7 changed files with 109 additions and 54 deletions.
4 changes: 4 additions & 0 deletions NEWS
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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! >>>

46 changes: 27 additions & 19 deletions ext/pdo/pdo_sql_parser.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
46 changes: 27 additions & 19 deletions ext/pdo/pdo_sql_parser.re
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
4 changes: 1 addition & 3 deletions ext/pdo/tests/bug_65946.phpt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down
43 changes: 43 additions & 0 deletions ext/pdo/tests/bug_73234.phpt
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
--TEST--
PDO Common: Bug #73234 (Emulated statements let value dictate parameter type)
--SKIPIF--
<?php
if (!extension_loaded('pdo')) die('skip');
$dir = getenv('REDIR_TEST_DIR');
if (false == $dir) die('skip no driver');
require_once $dir . 'pdo_test.inc';
PDOTest::skip();
?>
--FILE--
<?php
if (getenv('REDIR_TEST_DIR') === false) putenv('REDIR_TEST_DIR='.dirname(__FILE__) . '/../../pdo/tests/');
require_once getenv('REDIR_TEST_DIR') . 'pdo_test.inc';

$db = PDOTest::factory();
$db->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
}
}
18 changes: 6 additions & 12 deletions ext/pdo/tests/pdo_018.phpt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
2 changes: 1 addition & 1 deletion ext/pdo/tests/pdo_024.phpt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down

8 comments on commit 32b6154

@madorin
Copy link
Contributor

@madorin madorin commented on 32b6154 Jan 2, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Adam,

First:
I think PDO::PARAM_NULL should be removed/deprecated at all, because NULL is a state not a type as expected here nor a value.
It is wrong to write $stmt->bindParam(':name', $value, PDO::PARAM_NULL); along with PDO::PARAM_INT.

Second:
Your bug test #73234 is not SQL standard compliant and many DBMS that does not emulate parameters fail this test. Parameter type in that query is unknown at prepare stage.

@adambaratz
Copy link
Contributor Author

@adambaratz adambaratz commented on 32b6154 Jan 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. True as that may be, that change would require an RFC. Definitely feels out-of-scope for a bug fix.
  2. Is it the INSERTs that aren't compliant? There are other PDO tests with similar queries, so I'm wondering what's different here.

FYI, this change has been committed to master, but I'll certainly clean up the test as needed. I'll leave the PR open until we resolve this point.

@madorin
Copy link
Contributor

@madorin madorin commented on 32b6154 Jan 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adam,

  1. Then why to improve this fail?
    Expecting $stmt->bindValue(':value', 0, PDO::PARAM_NULL); to put a NULL there is not OK.
    0 is not NULL. What it should put there if we pass 1 or "A"? put there also NULL?

  2. Here, 18th line:
    32b6154#diff-3470eab08d851fd4571a17610980860aR18

Should be: $db->exec('CREATE TABLE test(id INT)');
By default, field can be null, just NOT NULL is required, when it is the case.

And it of course Firebird implementation fail it. ZERO != NULL.

========OUT========
array(2) {
[0]=>
array(1) {
["id"]=>
string(1) "0"
}
[1]=>
array(1) {
["id"]=>
NULL
}
}
========DONE========

========EXP========
array(2) {
[0]=>
array(1) {
["id"]=>
NULL
}
[1]=>
array(1) {
["id"]=>
NULL
}
}
========DONE========

@adambaratz
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Since PHP is dynamically typed, it seemed appropriate to treat this as a cast, especially since casts result from using the other types. What behavior would you expect if PDO::PARAM_NULL is given as the type, but the value !== null?
  2. T-SQL is a little different here. See 9fb94f0, which refers to https://msdn.microsoft.com/en-us/library/ms174979.aspx. Are those tests failing with pdo_firebird?

An easy fix would be to add a special case so these tests use different SQL with pdo_firebird. If that works for you, I can put the patch together.

@madorin
Copy link
Contributor

@madorin madorin commented on 32b6154 Jan 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adam,

  1. We can't cast some variable to NULL. Even more, in SQL world NULL = NULL evaluate to false.
    Because NULL means unknown, missing value. 0 is a KNOWN value. It is wrong to cast it as NULL.
    0 may be cased as false for a boolean field, but not as null. False is not null as true is not null.
    What's the problem, really? PHP allow NULLs. You may do bindValue('param', NULL, PDO::PARAM_INT);

What behavior would you expect if PDO::PARAM_NULL is given as the type

As I said PDO::PARAM_NULL is NOT a type. And my suggestion is to deprecate it.
For now i'd put a NULL for any value you pass there if PDO::PARAM_NULL is passed.

  1. From T-SQL docs I see it is allowed to omit it:
NULL | NOT NULL
Determine whether null values are allowed in the column.
NULL is not strictly a constraint but can be specified just like NOT NULL.

I think we should follow SQL standard in a multiple dbmses condition, and adjust it for specific DBs that does not support it.

http://www.w3schools.com/sql/sql_notnull.asp
By default, a table column can hold NULL values.

@madorin
Copy link
Contributor

@madorin madorin commented on 32b6154 Jan 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, about this 9fb94f0, I fixed 018, 024 pdo tests in master some days ago because they fail on Firebird :)

@adambaratz
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should've been more specific with my commit comment. See this part of that (long) doc:
https://msdn.microsoft.com/en-us/library/ms174979.aspx#Nullability Rules Within a Table Definition

Environment settings can influence how the table is created if you're not specific.

I'll patch pdo_018.phpt so it continues to run reliably with MSSQL. Please be on the lookout for DB-specific nuances in these tests. I understand they might not play nice with all DBs, but please try to preserve them for the DBs that need them.

@madorin
Copy link
Contributor

@madorin madorin commented on 32b6154 Jan 9, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adam, it seems then you have "ANSI_NULL_DEFAULT_OFF = ON" or (ANSI_NULL_DEFAULT_ON = OFF") in your test database, right? Which seems to be a non-standard behavior, at least for most DBMSes. Sorry, i'm not an expert in T-SQL.

Please sign in to comment.