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

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.

NEWS

@@ -87,6 +87,8 @@ PHP                                                                        NEWS
     that is not in the result set. (Ilia Alshanetsky)
   . Fixed bug GH-23962 (Destroying a persistent PDO instance rolls back a
     transaction still in use by another instance). (Lazizbek Ergashev)
+  . Fixed PDOStatement::execute() leaving a partial set of bindings in place
+    when binding its array argument fails. (Ilia Alshanetsky, Kamil Tekiela)
 
 - PDO_PGSQL:
   . Improved performance of the SQL parser that handles placeholders.

ext/pdo/pdo_stmt.c

@@ -260,19 +260,6 @@ static bool really_register_bound_param(struct pdo_bound_param_data *param, pdo_
 	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)) {
 		parameter = &param->parameter;
 	} else {
@@ -348,6 +335,17 @@ static bool really_register_bound_param(struct pdo_bound_param_data *param, pdo_
 	/* 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);
 	}
@@ -362,7 +360,6 @@ static bool really_register_bound_param(struct pdo_bound_param_data *param, pdo_
 	/* tell the driver we just created a parameter */
 	if (stmt->methods->param_hook) {
 		if (!stmt->methods->param_hook(stmt, pparam, PDO_PARAM_EVT_ALLOC)) {
-			PDO_HANDLE_STMT_ERR();
 			/* undo storage allocation; the hash will free the parameter
 			 * name if required */
 			if (pparam->name) {
@@ -373,6 +370,7 @@ static bool really_register_bound_param(struct pdo_bound_param_data *param, pdo_
 			ZVAL_UNDEF(&param->driver_params);
 			/* param->parameter is freed by hash dtor */
 			ZVAL_UNDEF(&param->parameter);
+			PDO_HANDLE_STMT_ERR();
 			return false;
 		}
 	}
@@ -434,6 +432,11 @@ PHP_METHOD(PDOStatement, execute)
 				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;
 			}
 		} ZEND_HASH_FOREACH_END();

ext/pdo/tests/pdo_bind_reentrant_execute.phpt

@@ -0,0 +1,42 @@
+--TEST--
+PDO: 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

ext/pdo/tests/pdo_execute_array_binding_failure.phpt

@@ -0,0 +1,67 @@
+--TEST--
+PDO: execute() leaves no bindings when binding its array argument fails
+--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 ThrowingString
+{
+    public function __toString(): string
+    {
+        throw new RuntimeException('conversion failed');
+    }
+}
+
+function bound_params_count(PDOStatement $stmt): string
+{
+    ob_start();
+    $stmt->debugDumpParams();
+    preg_match('/^Params:\s+(\d+)$/m', ob_get_clean(), $m);
+    return $m[1];
+}
+
+$db = PDOTest::factory();
+$db->exec('CREATE TABLE test_execute_bind_fail (id int, name varchar(10))');
+$db->exec("INSERT INTO test_execute_bind_fail (id, name) VALUES (1, 'a')");
+$db->exec("INSERT INTO test_execute_bind_fail (id, name) VALUES (2, 'b')");
+
+$stmt = $db->prepare('SELECT name FROM test_execute_bind_fail WHERE id = :id AND name = :name');
+$id = 1;
+$name = 'a';
+$stmt->bindParam(':id', $id);
+$stmt->bindParam(':name', $name);
+
+try {
+    $stmt->execute([':id' => 2, ':name' => new ThrowingString()]);
+} catch (RuntimeException $e) {
+    echo $e::class, ": ", $e->getMessage(), PHP_EOL;
+}
+echo "bound params after failure: ", bound_params_count($stmt), PHP_EOL;
+
+var_dump($stmt->execute([':id' => 2, ':name' => 'b']));
+var_dump($stmt->fetchAll(PDO::FETCH_COLUMN));
+?>
+--CLEAN--
+<?php
+require_once getenv('REDIR_TEST_DIR') . 'pdo_test.inc';
+$db = PDOTest::factory();
+PDOTest::dropTableIfExists($db, 'test_execute_bind_fail');
+?>
+--EXPECT--
+RuntimeException: conversion failed
+bound params after failure: 0
+bool(true)
+array(1) {
+  [0]=>
+  string(1) "b"
+}