ext/pdo: Clear bindings when execute() fails to bind its array

[iliaal]

PDOStatement::execute() dropped the existing bindings before binding its array argument, so when an entry failed part-way through, for example a value whose string conversion throws, the entries bound so far stayed installed and the next execute() without arguments ran with them. The partial table is now dropped on failure. really_register_bound_param() 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 (verified under valgrind).

[kamil-tekiela]

Why do you say this is a bug? IMHO PDO should not keep its previous bindings even if the new one fails. What makes you think that it should? Is it documented as such somewhere? Is there something I am misunderstanding here?

[kamil-tekiela]

I think easier fix is this:

in PDOStatement::execute()

 			if (!really_register_bound_param(&param, stmt, 1)) {
 				if (!Z_ISUNDEF(param.parameter)) {
 					zval_ptr_dtor(&param.parameter);
 				}
+				if (stmt->bound_params) {
+					zend_hash_destroy(stmt->bound_params);
+					FREE_HASHTABLE(stmt->bound_params);
+					stmt->bound_params = NULL;
+				}
 				RETURN_FALSE;
 			}

And to fix UAF you can do:

static bool really_register_bound_param(struct pdo_bound_param_data *param, pdo_stmt_t *stmt, bool is_param)
 {
 	HashTable *hash;
 	zval *parameter;
 	struct pdo_bound_param_data *pparam = NULL;
 
-	hash = is_param ? stmt->bound_params : stmt->bound_columns;
-
-	if (!hash) {
-		ALLOC_HASHTABLE(hash);
-		zend_hash_init(hash, 13, NULL, param_dtor, 0);
-
-		if (is_param) {
-			stmt->bound_params = hash;
- 		} else {
-			stmt->bound_columns = hash;
-		}
-	}
-
 	if (!Z_ISREF(param->parameter)) {
 	...
 	/* delete any other parameter registered with this number.
 	 * If the parameter is named, it will be removed and correctly
 	 * disposed of by the hash_update call that follows */
+	hash = is_param ? stmt->bound_params : stmt->bound_columns;
+	if (!hash) {
+		ALLOC_HASHTABLE(hash);
+		zend_hash_init(hash, 13, NULL, param_dtor, 0);
+		if (is_param) {
+			stmt->bound_params = hash;
+		} else {
+			stmt->bound_columns = hash;
+		}
+	}
+
 	if (param->paramno >= 0) {
 		zend_hash_index_del(hash, param->paramno);
 	}

and in pdo_stmt.c:367-378

 		if (!stmt->methods->param_hook(stmt, pparam, PDO_PARAM_EVT_ALLOC)) {
-			PDO_HANDLE_STMT_ERR();
 			if (pparam->name) {
 				zend_hash_del(hash, pparam->name);
 			} else {
 				zend_hash_index_del(hash, pparam->paramno);
 			}
 			ZVAL_UNDEF(&param->parameter);
+			PDO_HANDLE_STMT_ERR();
 			return 0;
 		}

This is a very strange problem to encounter and very low priority to fix, but since you are trying to fix this already, I think this might be better. I have not tested this code above.

[iliaal]

The bug is the partial state: after the throw, the next execute() runs with the entries bound before it (:id = 2 from the failed array, :name unbound). Clearing is a better outcome than restoring the old set, so I switched to your version in 027fb9d.

[kamil-tekiela]

You can add also a test like this:

--TEST--
PDO Common: re-executing the statement while a bound value is converted to string
--EXTENSIONS--
pdo
--SKIPIF--
<?php
$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='.__DIR__ . '/../../pdo/tests/');
require_once getenv('REDIR_TEST_DIR') . 'pdo_test.inc';

class ReExecute {
    public function __toString(): string {
        global $stmt;
        $stmt->execute(['x', 'y']);
        return 'r';
    }
}

$db = PDOTest::factory();
$db->exec('CREATE TABLE test_bind_reentrant (name varchar(10))');
$stmt = $db->prepare('SELECT name FROM test_bind_reentrant WHERE name = ? OR name = ?');

$stmt->execute(['a', new ReExecute()]);
$stmt->bindValue(2, new ReExecute());
echo "Done\n";
?>
--CLEAN--
<?php
require_once getenv('REDIR_TEST_DIR') . 'pdo_test.inc';
$db = PDOTest::factory();
PDOTest::dropTableIfExists($db, 'test_bind_reentrant');
?>
--EXPECT--
Done

which should throw under valgrind without this PR.

And I agree that the bug isn't very impactful, so we don't have to backport it.

[iliaal]

Added as a separate test in 03c997a.

[kamil-tekiela]

I would wait with merging until the Windows test is fixed.