Skip to content

Commit 03c997a

Browse files
iliaaliliaal
committed
ext/pdo: Clear bindings when execute() fails to bind its array
PDOStatement::execute() destroyed bound_params before registering the entries of its array argument, so a failure part-way through (a value whose string conversion throws) left the entries registered so far in place, and a following execute() without arguments ran with them. Drop the partial table on failure instead. really_register_bound_param() now looks the table up only after converting the value, and raises the PDO_PARAM_EVT_ALLOC error after undoing its insert, so a __toString() or error handler that re-executes the statement no longer frees the table in use.
1 parent 83e2394 commit 03c997a

4 files changed

Lines changed: 128 additions & 14 deletions

File tree

‎NEWS‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,8 @@ PHP NEWS
4343
that is not in the result set. (Ilia Alshanetsky)
4444
. Fixed bug GH-23962 (Destroying a persistent PDO instance rolls back a
4545
transaction still in use by another instance). (Lazizbek Ergashev)
46+
. Fixed PDOStatement::execute() leaving a partial set of bindings in place
47+
when binding its array argument fails. (Ilia Alshanetsky, Kamil Tekiela)
4648

4749
- PGSQL:
4850
. Fixed pg_lo_write() rejecting data containing null bytes. (Ilia Alshanetsky)

‎ext/pdo/pdo_stmt.c‎

Lines changed: 17 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -249,19 +249,6 @@ static bool really_register_bound_param(struct pdo_bound_param_data *param, pdo_
249249
zval *parameter;
250250
struct pdo_bound_param_data *pparam = NULL;
251251

252-
hash = is_param ? stmt->bound_params : stmt->bound_columns;
253-
254-
if (!hash) {
255-
ALLOC_HASHTABLE(hash);
256-
zend_hash_init(hash, 13, NULL, param_dtor, 0);
257-
258-
if (is_param) {
259-
stmt->bound_params = hash;
260-
} else {
261-
stmt->bound_columns = hash;
262-
}
263-
}
264-
265252
if (!Z_ISREF(param->parameter)) {
266253
parameter = &param->parameter;
267254
} else {
@@ -346,6 +333,17 @@ static bool really_register_bound_param(struct pdo_bound_param_data *param, pdo_
346333
/* delete any other parameter registered with this number.
347334
* If the parameter is named, it will be removed and correctly
348335
* disposed of by the hash_update call that follows */
336+
hash = is_param ? stmt->bound_params : stmt->bound_columns;
337+
if (!hash) {
338+
ALLOC_HASHTABLE(hash);
339+
zend_hash_init(hash, 13, NULL, param_dtor, 0);
340+
if (is_param) {
341+
stmt->bound_params = hash;
342+
} else {
343+
stmt->bound_columns = hash;
344+
}
345+
}
346+
349347
if (param->paramno >= 0) {
350348
zend_hash_index_del(hash, param->paramno);
351349
}
@@ -360,7 +358,6 @@ static bool really_register_bound_param(struct pdo_bound_param_data *param, pdo_
360358
/* tell the driver we just created a parameter */
361359
if (stmt->methods->param_hook) {
362360
if (!stmt->methods->param_hook(stmt, pparam, PDO_PARAM_EVT_ALLOC)) {
363-
PDO_HANDLE_STMT_ERR();
364361
/* undo storage allocation; the hash will free the parameter
365362
* name if required */
366363
if (pparam->name) {
@@ -370,6 +367,7 @@ static bool really_register_bound_param(struct pdo_bound_param_data *param, pdo_
370367
}
371368
/* param->parameter is freed by hash dtor */
372369
ZVAL_UNDEF(&param->parameter);
370+
PDO_HANDLE_STMT_ERR();
373371
return false;
374372
}
375373
}
@@ -423,6 +421,11 @@ PHP_METHOD(PDOStatement, execute)
423421
if (!Z_ISUNDEF(param.parameter)) {
424422
zval_ptr_dtor(&param.parameter);
425423
}
424+
if (stmt->bound_params) {
425+
zend_hash_destroy(stmt->bound_params);
426+
FREE_HASHTABLE(stmt->bound_params);
427+
stmt->bound_params = NULL;
428+
}
426429
RETURN_FALSE;
427430
}
428431
} ZEND_HASH_FOREACH_END();

‎ext/pdo/tests/pdo_bind_reentrant_execute.phpt‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
--TEST--
2+
PDO: re-executing the statement while a bound value is converted to string
3+
--EXTENSIONS--
4+
pdo
5+
--SKIPIF--
6+
<?php
7+
$dir = getenv('REDIR_TEST_DIR');
8+
if (false == $dir) die('skip no driver');
9+
require_once $dir . 'pdo_test.inc';
10+
PDOTest::skip();
11+
?>
12+
--FILE--
13+
<?php
14+
if (getenv('REDIR_TEST_DIR') === false) putenv('REDIR_TEST_DIR='.__DIR__ . '/../../pdo/tests/');
15+
require_once getenv('REDIR_TEST_DIR') . 'pdo_test.inc';
16+
17+
class ReExecute
18+
{
19+
public function __toString(): string
20+
{
21+
global $stmt;
22+
$stmt->execute(['x', 'y']);
23+
return 'r';
24+
}
25+
}
26+
27+
$db = PDOTest::factory();
28+
$db->exec('CREATE TABLE test_bind_reentrant (name varchar(10))');
29+
$stmt = $db->prepare('SELECT name FROM test_bind_reentrant WHERE name = ? OR name = ?');
30+
31+
$stmt->execute(['a', new ReExecute()]);
32+
$stmt->bindValue(2, new ReExecute());
33+
echo "Done\n";
34+
?>
35+
--CLEAN--
36+
<?php
37+
require_once getenv('REDIR_TEST_DIR') . 'pdo_test.inc';
38+
$db = PDOTest::factory();
39+
PDOTest::dropTableIfExists($db, 'test_bind_reentrant');
40+
?>
41+
--EXPECT--
42+
Done

‎ext/pdo/tests/pdo_execute_array_binding_failure.phpt‎

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,67 @@
1+
--TEST--
2+
PDO: execute() leaves no bindings when binding its array argument fails
3+
--EXTENSIONS--
4+
pdo
5+
--SKIPIF--
6+
<?php
7+
$dir = getenv('REDIR_TEST_DIR');
8+
if (false == $dir) die('skip no driver');
9+
require_once $dir . 'pdo_test.inc';
10+
PDOTest::skip();
11+
?>
12+
--FILE--
13+
<?php
14+
if (getenv('REDIR_TEST_DIR') === false) putenv('REDIR_TEST_DIR='.__DIR__ . '/../../pdo/tests/');
15+
require_once getenv('REDIR_TEST_DIR') . 'pdo_test.inc';
16+
17+
class ThrowingString
18+
{
19+
public function __toString(): string
20+
{
21+
throw new RuntimeException('conversion failed');
22+
}
23+
}
24+
25+
function bound_params_count(PDOStatement $stmt): string
26+
{
27+
ob_start();
28+
$stmt->debugDumpParams();
29+
preg_match('/^Params:\s+(\d+)$/m', ob_get_clean(), $m);
30+
return $m[1];
31+
}
32+
33+
$db = PDOTest::factory();
34+
$db->exec('CREATE TABLE test_execute_bind_fail (id int, name varchar(10))');
35+
$db->exec("INSERT INTO test_execute_bind_fail (id, name) VALUES (1, 'a')");
36+
$db->exec("INSERT INTO test_execute_bind_fail (id, name) VALUES (2, 'b')");
37+
38+
$stmt = $db->prepare('SELECT name FROM test_execute_bind_fail WHERE id = :id AND name = :name');
39+
$id = 1;
40+
$name = 'a';
41+
$stmt->bindParam(':id', $id);
42+
$stmt->bindParam(':name', $name);
43+
44+
try {
45+
$stmt->execute([':id' => 2, ':name' => new ThrowingString()]);
46+
} catch (RuntimeException $e) {
47+
echo $e::class, ": ", $e->getMessage(), PHP_EOL;
48+
}
49+
echo "bound params after failure: ", bound_params_count($stmt), PHP_EOL;
50+
51+
var_dump($stmt->execute([':id' => 2, ':name' => 'b']));
52+
var_dump($stmt->fetchAll(PDO::FETCH_COLUMN));
53+
?>
54+
--CLEAN--
55+
<?php
56+
require_once getenv('REDIR_TEST_DIR') . 'pdo_test.inc';
57+
$db = PDOTest::factory();
58+
PDOTest::dropTableIfExists($db, 'test_execute_bind_fail');
59+
?>
60+
--EXPECT--
61+
RuntimeException: conversion failed
62+
bound params after failure: 0
63+
bool(true)
64+
array(1) {
65+
[0]=>
66+
string(1) "b"
67+
}

0 commit comments

Comments
 (0)