Skip to content

Commit

Permalink
Restore binary offsets of PDOStatement parameters
Browse files Browse the repository at this point in the history
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 covers. 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
  • Loading branch information
mcurland committed Jul 22, 2023
1 parent 96d5a70 commit 68007b2
Show file tree
Hide file tree
Showing 5 changed files with 210 additions and 8 deletions.
19 changes: 19 additions & 0 deletions src/Driver/IBMDB2/Statement.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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
{
Expand Down Expand Up @@ -213,8 +217,23 @@ private function createTemporaryFile()
*/
private function copyStreamToStream($source, $target): void
{
$resetTo = false;
try {
if (stream_get_meta_data($source)['seekable']) {
$resetTo = ftell($source);
}
} catch (Throwable $e) {
// Swallow
}

if (@stream_copy_to_stream($source, $target) === false) {
throw CannotCopyStreamToStream::new(error_get_last());
}

if ($resetTo === false) {
return;
}

fseek($source, $resetTo, SEEK_SET);
}
}
35 changes: 28 additions & 7 deletions src/Driver/Mysqli/Statement.php
Original file line number Diff line number Diff line change
Expand Up @@ -13,17 +13,23 @@
use Doctrine\Deprecations\Deprecation;
use mysqli_sql_exception;
use mysqli_stmt;
use Throwable;

use function array_fill;
use function assert;
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
{
Expand Down Expand Up @@ -210,15 +216,30 @@ private function bindTypedParameters(): void
private function sendLongData(array $streams): void
{
foreach ($streams as $paramNr => $stream) {
while (! feof($stream)) {
$chunk = fread($stream, 8192);

if ($chunk === false) {
throw FailedReadingStreamOffset::new($paramNr);
$resetTo = false;
try {
if (stream_get_meta_data($stream)['seekable']) {
$resetTo = ftell($stream);
}
} catch (Throwable $e) {
// Swallow
}

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);
}
}
}
Expand Down
113 changes: 113 additions & 0 deletions src/Driver/PDO/Statement.php
Original file line number Diff line number Diff line change
Expand Up @@ -6,17 +6,30 @@
use Doctrine\DBAL\Driver\Statement as StatementInterface;
use Doctrine\DBAL\ParameterType;
use Doctrine\Deprecations\Deprecation;
use PDO;
use PDOException;
use PDOStatement;
use Throwable;

use function array_fill;
use function array_slice;
use function count;
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)
{
Expand All @@ -38,6 +51,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);
Expand Down Expand Up @@ -117,12 +133,109 @@ 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;
}

$resources = &$this->paramResources;
$resources ??= [];
$resources[] = $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.
*/
private function getResourceOffsets()
{
$resources = &$this->paramResources;
if ($resources === null) {
return null;
}

$count = count($resources);
$resourceOffsets = null;
$deref = [];
for ($i = 0; $i < $count; ++$i) {
$resource = $resources[$i];
$position = false;
try {
if (stream_get_meta_data($resource)['seekable']) {
$position = ftell($resource);
}
} catch (Throwable $e) {
// Swallow
} finally {
if ($position === false) {
if ($resourceOffsets !== null) {
$resourceOffsets[] = -1;
}
} else {
$resourceOffsets ??= $i !== 0 ? array_fill(0, $i, -1) : [];
$resourceOffsets[] = $position;
}
}
}

if ($resourceOffsets === null) {
$resources = null;
}

return $resourceOffsets;
}

/**
* Restore resource offsets moved by PDOStatement->execute
*
* @param int[]|null $resourceOffsets The offsets returned by getResourceOffsets.
*/
private function restoreResourceOffsets($resourceOffsets): void
{
if ($resourceOffsets === null) {
return;
}

$resources = &$this->paramResources;
if ($resources === null) {
return;
}

$count = count($resources);
for ($i = 0; $i < $count; ++$i) {
$offset = $resourceOffsets[$i];
// Check the special value indicating that we can't restore the offset
if ($offset === -1) {
continue;
}

fseek($resources[$i], $offset, SEEK_SET);
}

$resources = null;
}
}
26 changes: 25 additions & 1 deletion src/Driver/PgSQL/Statement.php
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,12 @@
use Doctrine\DBAL\ParameterType;
use Doctrine\Deprecations\Deprecation;
use PgSql\Connection as PgSqlConnection;
use Throwable;
use TypeError;

use function assert;
use function fseek;
use function ftell;
use function func_num_args;
use function get_class;
use function gettype;
Expand All @@ -26,6 +29,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
{
Expand Down Expand Up @@ -151,10 +157,28 @@ public function execute($params = null): Result
switch ($this->parameterTypes[$parameter]) {
case ParameterType::BINARY:
case ParameterType::LARGE_OBJECT:
$isResource = $value !== null && is_resource($value);
$resource = $value;
$resetTo = false;
if ($isResource) {
try {
if (stream_get_meta_data($resource)['seekable']) {
$resetTo = ftell($resource);
}
} catch (Throwable $e) {
// Swallow
}
}

$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;
Expand Down
25 changes: 25 additions & 0 deletions tests/Functional/BlobTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,9 @@
use Doctrine\DBAL\Types\Type;

use function fopen;
use function fseek;
use function ftell;
use function fwrite;
use function str_repeat;
use function stream_get_contents;

Expand Down Expand Up @@ -197,6 +200,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();
Expand Down

0 comments on commit 68007b2

Please sign in to comment.