From 8cbc828674c696b346b67293b0887d4916058a51 Mon Sep 17 00:00:00 2001 From: Matthew Curland Date: Thu, 7 Dec 2023 18:19:18 -0700 Subject: [PATCH 1/7] Restore binary offsets of PDOStatement parameters Entity identifiers that are PHP resource streams need to work for all references, not just the first one. PDOStatement->execute is reading the binary resources but not restoring the original offsets. No other code is restoring these streams to their original position so that they can be reused. Examples of failures include empty collections on read (the first lazy load collection on an entity populates correctly, the second is empty) and foreign-key violations while persisting changes (the first entity join produces the correct SQL, the second has no data to read and the FK is violated with the missing binary data). Making this change as close as possible to the external code that moves the stream pointer eliminates the need to do this in calling code. Resource offsets are retrieved immediately before execute in case they change between the bindValue and execute calls. The request was originally for the PDO driver but IBMDB2, Mysql, and PgSQL drivers are also covered. Other drivers will likely also need work. No attempt has been made to fix the deprecated bindParam code path. I do not believe this is called by the current Doctrine code, and is regardless much harder to patch because the reference variables can be replaced during execute, so the original resources may no longer be available to restore after the call. A functional test was added for bindValue and a resource with a seekable position. #5895 --- src/Driver/IBMDB2/Statement.php | 26 +++++++++- src/Driver/Mysqli/Statement.php | 30 +++++++++--- src/Driver/PDO/Statement.php | 87 +++++++++++++++++++++++++++++++++ src/Driver/PgSQL/Statement.php | 21 +++++++- tests/Functional/BlobTest.php | 25 ++++++++++ 5 files changed, 180 insertions(+), 9 deletions(-) diff --git a/src/Driver/IBMDB2/Statement.php b/src/Driver/IBMDB2/Statement.php index 699e236d715..bb9d970f0d3 100644 --- a/src/Driver/IBMDB2/Statement.php +++ b/src/Driver/IBMDB2/Statement.php @@ -10,12 +10,15 @@ use Doctrine\DBAL\Driver\Statement as StatementInterface; use Doctrine\DBAL\ParameterType; use Doctrine\Deprecations\Deprecation; +use Throwable; use function assert; use function db2_bind_param; use function db2_execute; use function error_get_last; use function fclose; +use function fseek; +use function ftell; use function func_num_args; use function is_int; use function is_resource; @@ -28,6 +31,7 @@ use const DB2_LONG; use const DB2_PARAM_FILE; use const DB2_PARAM_IN; +use const SEEK_SET; final class Statement implements StatementInterface { @@ -213,8 +217,28 @@ private function createTemporaryFile() */ private function copyStreamToStream($source, $target): void { + $resetTo = false; + if (stream_get_meta_data($source)['seekable']) { + $resetTo = ftell($source); + } + if (@stream_copy_to_stream($source, $target) === false) { - throw CannotCopyStreamToStream::new(error_get_last()); + $copyToStreamError = error_get_last(); + if ($resetTo !== false) { + try { + fseek($source, $resetTo, SEEK_SET); + } catch (Throwable $e) { + // Swallow, we want the original exception from stream_copy_to_stream + } + } + + throw CannotCopyStreamToStream::new($copyToStreamError); } + + if ($resetTo === false) { + return; + } + + fseek($source, $resetTo, SEEK_SET); } } diff --git a/src/Driver/Mysqli/Statement.php b/src/Driver/Mysqli/Statement.php index be555e3dd09..64e1cd2affd 100644 --- a/src/Driver/Mysqli/Statement.php +++ b/src/Driver/Mysqli/Statement.php @@ -19,11 +19,16 @@ use function count; use function feof; use function fread; +use function fseek; +use function ftell; use function func_num_args; use function get_resource_type; use function is_int; use function is_resource; use function str_repeat; +use function stream_get_meta_data; + +use const SEEK_SET; final class Statement implements StatementInterface { @@ -218,15 +223,26 @@ private function bindTypedParameters(): void private function sendLongData(array $streams): void { foreach ($streams as $paramNr => $stream) { - while (! feof($stream)) { - $chunk = fread($stream, 8192); + $resetTo = false; + if (stream_get_meta_data($stream)['seekable']) { + $resetTo = ftell($stream); + } - if ($chunk === false) { - throw FailedReadingStreamOffset::new($paramNr); - } + try { + while (! feof($stream)) { + $chunk = fread($stream, 8192); - if (! $this->stmt->send_long_data($paramNr - 1, $chunk)) { - throw StatementError::new($this->stmt); + if ($chunk === false) { + throw FailedReadingStreamOffset::new($paramNr); + } + + if (! $this->stmt->send_long_data($paramNr - 1, $chunk)) { + throw StatementError::new($this->stmt); + } + } + } finally { + if ($resetTo !== false) { + fseek($stream, $resetTo, SEEK_SET); } } } diff --git a/src/Driver/PDO/Statement.php b/src/Driver/PDO/Statement.php index e631fad3ddb..c21043e28d9 100644 --- a/src/Driver/PDO/Statement.php +++ b/src/Driver/PDO/Statement.php @@ -7,17 +7,27 @@ use Doctrine\DBAL\Driver\Statement as StatementInterface; use Doctrine\DBAL\ParameterType; use Doctrine\Deprecations\Deprecation; +use PDO; use PDOException; use PDOStatement; use function array_slice; +use function fseek; +use function ftell; use function func_get_args; use function func_num_args; +use function is_resource; +use function stream_get_meta_data; + +use const SEEK_SET; final class Statement implements StatementInterface { private PDOStatement $stmt; + /** @var mixed[]|null */ + private ?array $paramResources = null; + /** @internal The statement can be only instantiated by its driver connection. */ public function __construct(PDOStatement $stmt) { @@ -43,6 +53,9 @@ public function bindValue($param, $value, $type = ParameterType::STRING) } $pdoType = ParameterTypeMap::convertParamType($type); + if ($pdoType === PDO::PARAM_LOB) { + $this->trackParamResource($value); + } try { return $this->stmt->bindValue($param, $value, $pdoType); @@ -126,12 +139,86 @@ public function execute($params = null): ResultInterface ); } + $resourceOffsets = $this->getResourceOffsets(); try { $this->stmt->execute($params); } catch (PDOException $exception) { throw Exception::new($exception); + } finally { + if ($resourceOffsets !== null) { + $this->restoreResourceOffsets($resourceOffsets); + } } return new Result($this->stmt); } + + /** + * Track a binary parameter reference at binding time. These + * are cached for later analysis by the getResourceOffsets. + * + * @param mixed $resource + */ + private function trackParamResource($resource): void + { + if (! is_resource($resource)) { + return; + } + + $this->paramResources ??= []; + $this->paramResources[] = $resource; + } + + /** + * Determine the offset that any resource parameters needs to be + * restored to after the statement is executed. Call immediately + * before execute (not during bindValue) to get the most accurate offset. + * + * @return int[]|null Return offsets to restore if needed. The array may be sparse. + */ + private function getResourceOffsets(): ?array + { + if ($this->paramResources === null) { + return null; + } + + $resourceOffsets = null; + foreach ($this->paramResources as $index => $resource) { + $position = false; + if (stream_get_meta_data($resource)['seekable']) { + $position = ftell($resource); + } + + if ($position === false) { + continue; + } + + $resourceOffsets ??= []; + $resourceOffsets[$index] = $position; + } + + if ($resourceOffsets === null) { + $this->paramResources = null; + } + + return $resourceOffsets; + } + + /** + * Restore resource offsets moved by PDOStatement->execute + * + * @param int[]|null $resourceOffsets The offsets returned by getResourceOffsets. + */ + private function restoreResourceOffsets(?array $resourceOffsets): void + { + if ($resourceOffsets === null || $this->paramResources === null) { + return; + } + + foreach ($resourceOffsets as $index => $offset) { + fseek($this->paramResources[$index], $offset, SEEK_SET); + } + + $this->paramResources = null; + } } diff --git a/src/Driver/PgSQL/Statement.php b/src/Driver/PgSQL/Statement.php index ca6aa3b1023..26a61b3146c 100644 --- a/src/Driver/PgSQL/Statement.php +++ b/src/Driver/PgSQL/Statement.php @@ -10,6 +10,8 @@ use TypeError; use function assert; +use function fseek; +use function ftell; use function func_num_args; use function get_class; use function gettype; @@ -26,6 +28,9 @@ use function pg_send_execute; use function sprintf; use function stream_get_contents; +use function stream_get_meta_data; + +use const SEEK_SET; final class Statement implements StatementInterface { @@ -151,10 +156,24 @@ public function execute($params = null): Result switch ($this->parameterTypes[$parameter]) { case ParameterType::BINARY: case ParameterType::LARGE_OBJECT: + $isResource = is_resource($value); + $resource = $value; + $resetTo = false; + if ($isResource) { + if (stream_get_meta_data($resource)['seekable']) { + $resetTo = ftell($resource); + } + } + $escapedParameters[] = $value === null ? null : pg_escape_bytea( $this->connection, - is_resource($value) ? stream_get_contents($value) : $value, + $isResource ? stream_get_contents($value) : $value, ); + + if ($resetTo !== false) { + fseek($resource, $resetTo, SEEK_SET); + } + break; default: $escapedParameters[] = $value; diff --git a/tests/Functional/BlobTest.php b/tests/Functional/BlobTest.php index 212f4a4480e..f00fd8bb3e6 100644 --- a/tests/Functional/BlobTest.php +++ b/tests/Functional/BlobTest.php @@ -10,6 +10,9 @@ use Doctrine\DBAL\Types\Types; use function fopen; +use function fseek; +use function ftell; +use function fwrite; use function str_repeat; use function stream_get_contents; @@ -198,6 +201,28 @@ public function testBlobBindingDoesNotOverwritePrevious(): void self::assertEquals(['test1', 'test2'], $actual); } + public function testBindValueResetsStream(): void + { + if (TestUtil::isDriverOneOf('oci8')) { + self::markTestIncomplete('The oci8 driver does not support stream resources as parameters'); + } + + $stmt = $this->connection->prepare( + "INSERT INTO blob_table(id, clobcolumn, blobcolumn) VALUES (1, 'ignored', ?)", + ); + + $stream = fopen('php://temp', 'rb+'); + fwrite($stream, 'a test'); + fseek($stream, 2); + $stmt->bindValue(1, $stream, ParameterType::LARGE_OBJECT); + + $stmt->execute(); + + self::assertEquals(2, ftell($stream), 'Resource parameter should be reset to position before execute.'); + + $this->assertBlobContains('test'); + } + private function assertBlobContains(string $text): void { [, $blobValue] = $this->fetchRow(); From 8a284ac726b1c640492959a94afd6b37469a0ed4 Mon Sep 17 00:00:00 2001 From: Matthew Curland Date: Fri, 8 Dec 2023 23:44:02 -0700 Subject: [PATCH 2/7] SQLite3 and OCI8, restore driver resource offsets Adding drivers to completed set. --- src/Driver/OCI8/Statement.php | 94 +++++++++++++++++++++++++++++++- src/Driver/SQLite3/Statement.php | 89 +++++++++++++++++++++++++++++- 2 files changed, 179 insertions(+), 4 deletions(-) diff --git a/src/Driver/OCI8/Statement.php b/src/Driver/OCI8/Statement.php index 015a14b7be9..a48f361ac37 100644 --- a/src/Driver/OCI8/Statement.php +++ b/src/Driver/OCI8/Statement.php @@ -9,11 +9,15 @@ use Doctrine\DBAL\ParameterType; use Doctrine\Deprecations\Deprecation; +use function fseek; +use function ftell; use function func_num_args; use function is_int; +use function is_resource; use function oci_bind_by_name; use function oci_execute; use function oci_new_descriptor; +use function stream_get_meta_data; use const OCI_B_BIN; use const OCI_B_BLOB; @@ -21,6 +25,7 @@ use const OCI_D_LOB; use const OCI_NO_AUTO_COMMIT; use const OCI_TEMP_BLOB; +use const SEEK_SET; use const SQLT_CHR; final class Statement implements StatementInterface @@ -34,6 +39,9 @@ final class Statement implements StatementInterface /** @var array */ private array $parameterMap; + /** @var mixed[]|null */ + private ?array $paramResources = null; + private ExecutionMode $executionMode; /** @@ -65,6 +73,10 @@ public function bindValue($param, $value, $type = ParameterType::STRING): bool ); } + if ($type === ParameterType::BINARY || $type === ParameterType::LARGE_OBJECT) { + $this->trackParamResource($value); + } + return $this->bindParam($param, $value, $type); } @@ -164,11 +176,87 @@ public function execute($params = null): ResultInterface $mode = OCI_NO_AUTO_COMMIT; } - $ret = @oci_execute($this->statement, $mode); - if (! $ret) { - throw Error::new($this->statement); + $resourceOffsets = $this->getResourceOffsets(); + try { + $ret = @oci_execute($this->statement, $mode); + if (! $ret) { + throw Error::new($this->statement); + } + } finally { + if ($resourceOffsets !== null) { + $this->restoreResourceOffsets($resourceOffsets); + } } return new Result($this->statement); } + + /** + * Track a binary parameter reference at binding time. These + * are cached for later analysis by the getResourceOffsets. + * + * @param mixed $resource + */ + private function trackParamResource($resource): void + { + if (! is_resource($resource)) { + return; + } + + $this->paramResources ??= []; + $this->paramResources[] = $resource; + } + + /** + * Determine the offset that any resource parameters needs to be + * restored to after the statement is executed. Call immediately + * before execute (not during bindValue) to get the most accurate offset. + * + * @return int[]|null Return offsets to restore if needed. The array may be sparse. + */ + private function getResourceOffsets(): ?array + { + if ($this->paramResources === null) { + return null; + } + + $resourceOffsets = null; + foreach ($this->paramResources as $index => $resource) { + $position = false; + if (stream_get_meta_data($resource)['seekable']) { + $position = ftell($resource); + } + + if ($position === false) { + continue; + } + + $resourceOffsets ??= []; + $resourceOffsets[$index] = $position; + } + + if ($resourceOffsets === null) { + $this->paramResources = null; + } + + return $resourceOffsets; + } + + /** + * Restore resource offsets moved by PDOStatement->execute + * + * @param int[]|null $resourceOffsets The offsets returned by getResourceOffsets. + */ + private function restoreResourceOffsets(?array $resourceOffsets): void + { + if ($resourceOffsets === null || $this->paramResources === null) { + return; + } + + foreach ($resourceOffsets as $index => $offset) { + fseek($this->paramResources[$index], $offset, SEEK_SET); + } + + $this->paramResources = null; + } } diff --git a/src/Driver/SQLite3/Statement.php b/src/Driver/SQLite3/Statement.php index 01c3b8bbb91..5bde99ea898 100644 --- a/src/Driver/SQLite3/Statement.php +++ b/src/Driver/SQLite3/Statement.php @@ -10,9 +10,14 @@ use SQLite3Stmt; use function assert; +use function fseek; +use function ftell; use function func_num_args; use function is_int; +use function is_resource; +use function stream_get_meta_data; +use const SEEK_SET; use const SQLITE3_BLOB; use const SQLITE3_INTEGER; use const SQLITE3_NULL; @@ -33,6 +38,9 @@ final class Statement implements StatementInterface private SQLite3 $connection; private SQLite3Stmt $statement; + /** @var mixed[]|null */ + private ?array $paramResources = null; + /** @internal The statement can be only instantiated by its driver connection. */ public function __construct(SQLite3 $connection, SQLite3Stmt $statement) { @@ -58,7 +66,12 @@ public function bindValue($param, $value, $type = ParameterType::STRING): bool ); } - return $this->statement->bindValue($param, $value, $this->convertParamType($type)); + $sqliteType = $this->convertParamType($type); + if ($sqliteType === SQLITE3_BLOB) { + $this->trackParamResource($value); + } + + return $this->statement->bindValue($param, $value, $sqliteType); } /** @@ -109,10 +122,15 @@ public function execute($params = null): Result } } + $resourceOffsets = $this->getResourceOffsets(); try { $result = $this->statement->execute(); } catch (\Exception $e) { throw Exception::new($e); + } finally { + if ($resourceOffsets !== null) { + $this->restoreResourceOffsets($resourceOffsets); + } } assert($result !== false); @@ -133,4 +151,73 @@ private function convertParamType(int $type): int return self::PARAM_TYPE_MAP[$type]; } + + /** + * Track a binary parameter reference at binding time. These + * are cached for later analysis by the getResourceOffsets. + * + * @param mixed $resource + */ + private function trackParamResource($resource): void + { + if (! is_resource($resource)) { + return; + } + + $this->paramResources ??= []; + $this->paramResources[] = $resource; + } + + /** + * Determine the offset that any resource parameters needs to be + * restored to after the statement is executed. Call immediately + * before execute (not during bindValue) to get the most accurate offset. + * + * @return int[]|null Return offsets to restore if needed. The array may be sparse. + */ + private function getResourceOffsets(): ?array + { + if ($this->paramResources === null) { + return null; + } + + $resourceOffsets = null; + foreach ($this->paramResources as $index => $resource) { + $position = false; + if (stream_get_meta_data($resource)['seekable']) { + $position = ftell($resource); + } + + if ($position === false) { + continue; + } + + $resourceOffsets ??= []; + $resourceOffsets[$index] = $position; + } + + if ($resourceOffsets === null) { + $this->paramResources = null; + } + + return $resourceOffsets; + } + + /** + * Restore resource offsets moved by PDOStatement->execute + * + * @param int[]|null $resourceOffsets The offsets returned by getResourceOffsets. + */ + private function restoreResourceOffsets(?array $resourceOffsets): void + { + if ($resourceOffsets === null || $this->paramResources === null) { + return; + } + + foreach ($resourceOffsets as $index => $offset) { + fseek($this->paramResources[$index], $offset, SEEK_SET); + } + + $this->paramResources = null; + } } From e5d859564ea384128c236b88eed30330cf2d1b02 Mon Sep 17 00:00:00 2001 From: Matthew Curland Date: Sat, 9 Dec 2023 21:58:27 -0700 Subject: [PATCH 3/7] SQLSrv, restore driver resource offsets Adding drivers to completed set. --- src/Driver/PDO/SQLSrv/Statement.php | 98 +++++++++++++++++++++++++++++ src/Driver/SQLSrv/Statement.php | 98 ++++++++++++++++++++++++++++- 2 files changed, 193 insertions(+), 3 deletions(-) diff --git a/src/Driver/PDO/SQLSrv/Statement.php b/src/Driver/PDO/SQLSrv/Statement.php index a63ff79d7ea..ab77747fb20 100644 --- a/src/Driver/PDO/SQLSrv/Statement.php +++ b/src/Driver/PDO/SQLSrv/Statement.php @@ -5,16 +5,26 @@ use Doctrine\DBAL\Driver\Exception\UnknownParameterType; use Doctrine\DBAL\Driver\Middleware\AbstractStatementMiddleware; use Doctrine\DBAL\Driver\PDO\Statement as PDOStatement; +use Doctrine\DBAL\Driver\Result; use Doctrine\DBAL\ParameterType; use Doctrine\Deprecations\Deprecation; use PDO; +use function fseek; +use function ftell; use function func_num_args; +use function is_resource; +use function stream_get_meta_data; + +use const SEEK_SET; final class Statement extends AbstractStatementMiddleware { private PDOStatement $statement; + /** @var mixed[]|null */ + private ?array $paramResources = null; + /** @internal The statement can be only instantiated by its driver connection. */ public function __construct(PDOStatement $statement) { @@ -104,6 +114,94 @@ public function bindValue($param, $value, $type = ParameterType::STRING): bool ); } + if ($type === ParameterType::LARGE_OBJECT || $type === ParameterType::BINARY) { + $this->trackParamResource($value); + } + return $this->bindParam($param, $value, $type); } + + /** + * {@inheritDoc} + */ + public function execute($params = null): Result + { + $resourceOffsets = $this->getResourceOffsets(); + try { + return parent::execute($params); + } finally { + if ($resourceOffsets !== null) { + $this->restoreResourceOffsets($resourceOffsets); + } + } + } + + /** + * Track a binary parameter reference at binding time. These + * are cached for later analysis by the getResourceOffsets. + * + * @param mixed $resource + */ + private function trackParamResource($resource): void + { + if (! is_resource($resource)) { + return; + } + + $this->paramResources ??= []; + $this->paramResources[] = $resource; + } + + /** + * Determine the offset that any resource parameters needs to be + * restored to after the statement is executed. Call immediately + * before execute (not during bindValue) to get the most accurate offset. + * + * @return int[]|null Return offsets to restore if needed. The array may be sparse. + */ + private function getResourceOffsets(): ?array + { + if ($this->paramResources === null) { + return null; + } + + $resourceOffsets = null; + foreach ($this->paramResources as $index => $resource) { + $position = false; + if (stream_get_meta_data($resource)['seekable']) { + $position = ftell($resource); + } + + if ($position === false) { + continue; + } + + $resourceOffsets ??= []; + $resourceOffsets[$index] = $position; + } + + if ($resourceOffsets === null) { + $this->paramResources = null; + } + + return $resourceOffsets; + } + + /** + * Restore resource offsets moved by PDOStatement->execute + * + * @param int[]|null $resourceOffsets The offsets returned by getResourceOffsets. + */ + private function restoreResourceOffsets(?array $resourceOffsets): void + { + if ($resourceOffsets === null || $this->paramResources === null) { + return; + } + + foreach ($resourceOffsets as $index => $offset) { + fseek($this->paramResources[$index], $offset, SEEK_SET); + } + + $this->paramResources = null; + } } diff --git a/src/Driver/SQLSrv/Statement.php b/src/Driver/SQLSrv/Statement.php index 227c33456c6..242ff1d870e 100644 --- a/src/Driver/SQLSrv/Statement.php +++ b/src/Driver/SQLSrv/Statement.php @@ -10,15 +10,20 @@ use Doctrine\Deprecations\Deprecation; use function assert; +use function fseek; +use function ftell; use function func_num_args; use function is_int; +use function is_resource; use function sqlsrv_execute; use function SQLSRV_PHPTYPE_STREAM; use function SQLSRV_PHPTYPE_STRING; use function sqlsrv_prepare; use function SQLSRV_SQLTYPE_VARBINARY; +use function stream_get_meta_data; use function stripos; +use const SEEK_SET; use const SQLSRV_ENC_BINARY; use const SQLSRV_ENC_CHAR; use const SQLSRV_PARAM_IN; @@ -58,6 +63,13 @@ final class Statement implements StatementInterface */ private array $types = []; + /** + * Resources used as bound values. + * + * @var mixed[]|null + */ + private ?array $paramResources = null; + /** * Append to any INSERT query to retrieve the last insert id. */ @@ -97,6 +109,10 @@ public function bindValue($param, $value, $type = ParameterType::STRING): bool ); } + if ($type === ParameterType::LARGE_OBJECT || $type === ParameterType::BINARY) { + $this->trackParamResource($value); + } + $this->variables[$param] = $value; $this->types[$param] = $type; @@ -159,10 +175,17 @@ public function execute($params = null): ResultInterface } } - $this->stmt ??= $this->prepare(); + $resourceOffsets = $this->getResourceOffsets(); + try { + $this->stmt ??= $this->prepare(); - if (! sqlsrv_execute($this->stmt)) { - throw Error::new(); + if (! sqlsrv_execute($this->stmt)) { + throw Error::new(); + } + } finally { + if ($resourceOffsets !== null) { + $this->restoreResourceOffsets($resourceOffsets); + } } return new Result($this->stmt); @@ -220,4 +243,73 @@ private function prepare() return $stmt; } + + /** + * Track a binary parameter reference at binding time. These + * are cached for later analysis by the getResourceOffsets. + * + * @param mixed $resource + */ + private function trackParamResource($resource): void + { + if (! is_resource($resource)) { + return; + } + + $this->paramResources ??= []; + $this->paramResources[] = $resource; + } + + /** + * Determine the offset that any resource parameters needs to be + * restored to after the statement is executed. Call immediately + * before execute (not during bindValue) to get the most accurate offset. + * + * @return int[]|null Return offsets to restore if needed. The array may be sparse. + */ + private function getResourceOffsets(): ?array + { + if ($this->paramResources === null) { + return null; + } + + $resourceOffsets = null; + foreach ($this->paramResources as $index => $resource) { + $position = false; + if (stream_get_meta_data($resource)['seekable']) { + $position = ftell($resource); + } + + if ($position === false) { + continue; + } + + $resourceOffsets ??= []; + $resourceOffsets[$index] = $position; + } + + if ($resourceOffsets === null) { + $this->paramResources = null; + } + + return $resourceOffsets; + } + + /** + * Restore resource offsets moved by PDOStatement->execute + * + * @param int[]|null $resourceOffsets The offsets returned by getResourceOffsets. + */ + private function restoreResourceOffsets(?array $resourceOffsets): void + { + if ($resourceOffsets === null || $this->paramResources === null) { + return; + } + + foreach ($resourceOffsets as $index => $offset) { + fseek($this->paramResources[$index], $offset, SEEK_SET); + } + + $this->paramResources = null; + } } From fa5acba3eea65fc3acc029fb533b03675b1add11 Mon Sep 17 00:00:00 2001 From: "Alexander M. Turek" Date: Sun, 24 Nov 2024 12:41:43 +0100 Subject: [PATCH 4/7] Update test --- tests/Functional/BlobTest.php | 30 ++++++++++++++++++++++++------ 1 file changed, 24 insertions(+), 6 deletions(-) diff --git a/tests/Functional/BlobTest.php b/tests/Functional/BlobTest.php index f00fd8bb3e6..d02fe424b28 100644 --- a/tests/Functional/BlobTest.php +++ b/tests/Functional/BlobTest.php @@ -9,9 +9,10 @@ use Doctrine\DBAL\Types\Type; use Doctrine\DBAL\Types\Types; +use function array_map; +use function assert; use function fopen; use function fseek; -use function ftell; use function fwrite; use function str_repeat; use function stream_get_contents; @@ -208,19 +209,36 @@ public function testBindValueResetsStream(): void } $stmt = $this->connection->prepare( - "INSERT INTO blob_table(id, clobcolumn, blobcolumn) VALUES (1, 'ignored', ?)", + "INSERT INTO blob_table(id, clobcolumn, blobcolumn) VALUES (?, 'ignored', ?)", ); $stream = fopen('php://temp', 'rb+'); + assert($stream !== false); + fwrite($stream, 'a test'); fseek($stream, 2); - $stmt->bindValue(1, $stream, ParameterType::LARGE_OBJECT); + $stmt->bindValue(1, 1, ParameterType::INTEGER); + $stmt->bindValue(2, $stream, ParameterType::LARGE_OBJECT); - $stmt->execute(); + $stmt->executeStatement(); - self::assertEquals(2, ftell($stream), 'Resource parameter should be reset to position before execute.'); + $stmt->bindValue(1, 2, ParameterType::INTEGER); + $stmt->executeStatement(); - $this->assertBlobContains('test'); + $stmt->bindValue(1, 3, ParameterType::INTEGER); + $stmt->executeStatement(); + + $rows = array_map( + function (array $row): array { + $row[0] = $this->connection->convertToPHPValue($row[0], Types::INTEGER); + $row[1] = $this->connection->convertToPHPValue($row[1], Types::STRING); + + return $row; + }, + $this->connection->fetchAllNumeric('SELECT id, blobcolumn FROM blob_table'), + ); + + self::assertSame([[1, 'test'], [2, 'test'], [3, 'test']], $rows); } private function assertBlobContains(string $text): void From 950dc7fe09e06341cdeb768bcaea7430f6a9bd50 Mon Sep 17 00:00:00 2001 From: Matthew Curland Date: Tue, 25 Feb 2025 12:59:06 -0700 Subject: [PATCH 5/7] Modify test The test as previously modified succeeded without the stream restore action (comment out the fseek call in src/Driver/PDO/Statement.php). The expectation with the fix is that the resource sent to bindValue is read during the first call to execute. This is the point the stream pointer is moved and needs to be removed. For PDO SQLite, additional calls execute (without rebinding the stream value) do not read the stream a second time. If this needs to be applied for subsequent calls to execute, then bindValue needs to forward the provided $param to trackParamResource for a paramResources index, and paramResources should not be cleared at the end of restoreResourceOffsets. I need a ci pass to see if this needed. If the test needs to run without the ftell check, then the statement needs to be recreated and/or bindValue called a second time on the stream before executing the statement. This will force a second read of the stream, whereas currently it is only read one time. --- tests/Functional/BlobTest.php | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/tests/Functional/BlobTest.php b/tests/Functional/BlobTest.php index d02fe424b28..217010f11ab 100644 --- a/tests/Functional/BlobTest.php +++ b/tests/Functional/BlobTest.php @@ -13,6 +13,7 @@ use function assert; use function fopen; use function fseek; +use function ftell; use function fwrite; use function str_repeat; use function stream_get_contents; @@ -239,6 +240,15 @@ function (array $row): array { ); self::assertSame([[1, 'test'], [2, 'test'], [3, 'test']], $rows); + + // Executing this multiple times assures that the same bound stream is written + // multiple times. However, that is not the primary issue being tested. The impetus + // for this test is that reading the stream to extract the value moves the stream + // pointer, so that using the stream with a new statement (and hence a new call + // to bindValue and a new initial call to execute*) causes a different value to + // be read from the stream results. All that we have learned so far is that bindValue + // reuses the value obtained during the first execution of the statement. + self::assertEquals(2, ftell($stream), 'Resource parameter should be reset to position before execute.'); } private function assertBlobContains(string $text): void From 11eefe126611eb525e283fb51099a99dc7d9eec4 Mon Sep 17 00:00:00 2001 From: Matthew Curland Date: Tue, 25 Feb 2025 13:58:47 -0700 Subject: [PATCH 6/7] Restore stream offsets with subsequent execute Driver statement instances needs to maintain resources offsets for streams passed to bindValue after the first call to execute. Modified array compare in testBindValueResetsStream to explicitly read the stream and read remaining stream content instead of checking current pointer to conclude the test. Fixed for PDO, SQLite3, SQLSrv, OCI8 --- src/Driver/OCI8/Statement.php | 13 ++++++------- src/Driver/PDO/SQLSrv/Statement.php | 13 ++++++------- src/Driver/PDO/Statement.php | 13 ++++++------- src/Driver/SQLSrv/Statement.php | 13 ++++++------- src/Driver/SQLite3/Statement.php | 13 ++++++------- tests/Functional/BlobTest.php | 18 +++++++++++------- 6 files changed, 41 insertions(+), 42 deletions(-) diff --git a/src/Driver/OCI8/Statement.php b/src/Driver/OCI8/Statement.php index a48f361ac37..5ed9c654514 100644 --- a/src/Driver/OCI8/Statement.php +++ b/src/Driver/OCI8/Statement.php @@ -74,7 +74,7 @@ public function bindValue($param, $value, $type = ParameterType::STRING): bool } if ($type === ParameterType::BINARY || $type === ParameterType::LARGE_OBJECT) { - $this->trackParamResource($value); + $this->trackParamResource($param, $value); } return $this->bindParam($param, $value, $type); @@ -195,16 +195,17 @@ public function execute($params = null): ResultInterface * Track a binary parameter reference at binding time. These * are cached for later analysis by the getResourceOffsets. * - * @param mixed $resource + * @param int|string $param + * @param mixed $resource */ - private function trackParamResource($resource): void + private function trackParamResource($param, $resource): void { if (! is_resource($resource)) { return; } - $this->paramResources ??= []; - $this->paramResources[] = $resource; + $this->paramResources ??= []; + $this->paramResources[$param] = $resource; } /** @@ -256,7 +257,5 @@ private function restoreResourceOffsets(?array $resourceOffsets): void foreach ($resourceOffsets as $index => $offset) { fseek($this->paramResources[$index], $offset, SEEK_SET); } - - $this->paramResources = null; } } diff --git a/src/Driver/PDO/SQLSrv/Statement.php b/src/Driver/PDO/SQLSrv/Statement.php index ab77747fb20..cae86ea11a0 100644 --- a/src/Driver/PDO/SQLSrv/Statement.php +++ b/src/Driver/PDO/SQLSrv/Statement.php @@ -115,7 +115,7 @@ public function bindValue($param, $value, $type = ParameterType::STRING): bool } if ($type === ParameterType::LARGE_OBJECT || $type === ParameterType::BINARY) { - $this->trackParamResource($value); + $this->trackParamResource($param, $value); } return $this->bindParam($param, $value, $type); @@ -140,16 +140,17 @@ public function execute($params = null): Result * Track a binary parameter reference at binding time. These * are cached for later analysis by the getResourceOffsets. * - * @param mixed $resource + * @param int|string $param + * @param mixed $resource */ - private function trackParamResource($resource): void + private function trackParamResource($param, $resource): void { if (! is_resource($resource)) { return; } - $this->paramResources ??= []; - $this->paramResources[] = $resource; + $this->paramResources ??= []; + $this->paramResources[$param] = $resource; } /** @@ -201,7 +202,5 @@ private function restoreResourceOffsets(?array $resourceOffsets): void foreach ($resourceOffsets as $index => $offset) { fseek($this->paramResources[$index], $offset, SEEK_SET); } - - $this->paramResources = null; } } diff --git a/src/Driver/PDO/Statement.php b/src/Driver/PDO/Statement.php index c21043e28d9..d57d4b12eb0 100644 --- a/src/Driver/PDO/Statement.php +++ b/src/Driver/PDO/Statement.php @@ -54,7 +54,7 @@ public function bindValue($param, $value, $type = ParameterType::STRING) $pdoType = ParameterTypeMap::convertParamType($type); if ($pdoType === PDO::PARAM_LOB) { - $this->trackParamResource($value); + $this->trackParamResource($param, $value); } try { @@ -157,16 +157,17 @@ public function execute($params = null): ResultInterface * Track a binary parameter reference at binding time. These * are cached for later analysis by the getResourceOffsets. * - * @param mixed $resource + * @param int|string $param + * @param mixed $resource */ - private function trackParamResource($resource): void + private function trackParamResource($param, $resource): void { if (! is_resource($resource)) { return; } - $this->paramResources ??= []; - $this->paramResources[] = $resource; + $this->paramResources ??= []; + $this->paramResources[$param] = $resource; } /** @@ -218,7 +219,5 @@ private function restoreResourceOffsets(?array $resourceOffsets): void foreach ($resourceOffsets as $index => $offset) { fseek($this->paramResources[$index], $offset, SEEK_SET); } - - $this->paramResources = null; } } diff --git a/src/Driver/SQLSrv/Statement.php b/src/Driver/SQLSrv/Statement.php index 242ff1d870e..4fab501ae8c 100644 --- a/src/Driver/SQLSrv/Statement.php +++ b/src/Driver/SQLSrv/Statement.php @@ -110,7 +110,7 @@ public function bindValue($param, $value, $type = ParameterType::STRING): bool } if ($type === ParameterType::LARGE_OBJECT || $type === ParameterType::BINARY) { - $this->trackParamResource($value); + $this->trackParamResource($param, $value); } $this->variables[$param] = $value; @@ -248,16 +248,17 @@ private function prepare() * Track a binary parameter reference at binding time. These * are cached for later analysis by the getResourceOffsets. * - * @param mixed $resource + * @param int|string $param + * @param mixed $resource */ - private function trackParamResource($resource): void + private function trackParamResource($param, $resource): void { if (! is_resource($resource)) { return; } - $this->paramResources ??= []; - $this->paramResources[] = $resource; + $this->paramResources ??= []; + $this->paramResources[$param] = $resource; } /** @@ -309,7 +310,5 @@ private function restoreResourceOffsets(?array $resourceOffsets): void foreach ($resourceOffsets as $index => $offset) { fseek($this->paramResources[$index], $offset, SEEK_SET); } - - $this->paramResources = null; } } diff --git a/src/Driver/SQLite3/Statement.php b/src/Driver/SQLite3/Statement.php index 5bde99ea898..4db65acc5b1 100644 --- a/src/Driver/SQLite3/Statement.php +++ b/src/Driver/SQLite3/Statement.php @@ -68,7 +68,7 @@ public function bindValue($param, $value, $type = ParameterType::STRING): bool $sqliteType = $this->convertParamType($type); if ($sqliteType === SQLITE3_BLOB) { - $this->trackParamResource($value); + $this->trackParamResource($param, $value); } return $this->statement->bindValue($param, $value, $sqliteType); @@ -156,16 +156,17 @@ private function convertParamType(int $type): int * Track a binary parameter reference at binding time. These * are cached for later analysis by the getResourceOffsets. * - * @param mixed $resource + * @param int|string $param + * @param mixed $resource */ - private function trackParamResource($resource): void + private function trackParamResource($param, $resource): void { if (! is_resource($resource)) { return; } - $this->paramResources ??= []; - $this->paramResources[] = $resource; + $this->paramResources ??= []; + $this->paramResources[$param] = $resource; } /** @@ -217,7 +218,5 @@ private function restoreResourceOffsets(?array $resourceOffsets): void foreach ($resourceOffsets as $index => $offset) { fseek($this->paramResources[$index], $offset, SEEK_SET); } - - $this->paramResources = null; } } diff --git a/tests/Functional/BlobTest.php b/tests/Functional/BlobTest.php index 217010f11ab..0f852533ccb 100644 --- a/tests/Functional/BlobTest.php +++ b/tests/Functional/BlobTest.php @@ -13,7 +13,6 @@ use function assert; use function fopen; use function fseek; -use function ftell; use function fwrite; use function str_repeat; use function stream_get_contents; @@ -229,10 +228,12 @@ public function testBindValueResetsStream(): void $stmt->bindValue(1, 3, ParameterType::INTEGER); $stmt->executeStatement(); - $rows = array_map( - function (array $row): array { + $blobType = Type::getType(Types::BLOB); + $platform = $this->connection->getDatabasePlatform(); + $rows = array_map( + function (array $row) use ($blobType, $platform): array { $row[0] = $this->connection->convertToPHPValue($row[0], Types::INTEGER); - $row[1] = $this->connection->convertToPHPValue($row[1], Types::STRING); + $row[1] = stream_get_contents($blobType->convertToPHPValue($row[1], $platform)); return $row; }, @@ -246,9 +247,12 @@ function (array $row): array { // for this test is that reading the stream to extract the value moves the stream // pointer, so that using the stream with a new statement (and hence a new call // to bindValue and a new initial call to execute*) causes a different value to - // be read from the stream results. All that we have learned so far is that bindValue - // reuses the value obtained during the first execution of the statement. - self::assertEquals(2, ftell($stream), 'Resource parameter should be reset to position before execute.'); + // be read from the stream results. Currently, we have verified one of two things: + // either the stream is reset with each execute, or that bindValue reuses the value + // obtained during the first execution of the statement. Which of these holds is not + // consistent across implementations. For example, the default pdo_sqlite test target + // does not reread the stream on subsequent executes, but pdo_pgsql does. + self::assertEquals('test', stream_get_contents($stream)); } private function assertBlobContains(string $text): void From 14ed8b566c6410d7f7e9ce9475a9a71c4edeabb9 Mon Sep 17 00:00:00 2001 From: Matthew Curland Date: Wed, 26 Feb 2025 11:24:58 -0700 Subject: [PATCH 7/7] DB2 execute with large object fails second call An execute* function with a bound large object parameter can be called once, but not twice. Execute is clearing the lob collection, but is not removing the handles this data is written to from the bound parameters. Clear the parameters instead and leave the lob collection intact so that it can be used in subsequent calls to repopulate new handles. Note that the resources in the lob collection have already been restored to their original resource positions. --- src/Driver/IBMDB2/Statement.php | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/Driver/IBMDB2/Statement.php b/src/Driver/IBMDB2/Statement.php index bb9d970f0d3..4d1821e967c 100644 --- a/src/Driver/IBMDB2/Statement.php +++ b/src/Driver/IBMDB2/Statement.php @@ -157,7 +157,13 @@ public function execute($params = null): ResultInterface fclose($handle); } - $this->lobs = []; + // Clear parameters bound to closed handles. + // $this->lobs itself is not cleared, allowing a + // second call to execute without rebinding, which + // is supported for other parameter types. + foreach ($this->lobs as $param => $value) { + unset($this->parameters[$param]); + } if ($result === false) { throw StatementError::new($this->stmt);