fix(mysqli,pdo): preserve binary SQL literals when sqlcommenter is installed

PuvaanRaaj · claude · PuvaanRaaj · commit b48a443f0dc8 · 2026-07-08T14:59:38.000+08:00
The mysqli and PDO query pre-hooks reassigned the local $query to mb_convert_encoding($query, 'UTF-8') for the db.query.text span attribute. When opentelemetry-sqlcommenter is also installed, that same (now UTF-8-mangled) variable was injected and returned as the substituted statement argument, so any non-UTF-8 byte in the executed SQL was replaced with 0x3F ('?'). This silently corrupted BINARY/VARBINARY/BLOB writes that embed raw bytes in a quoted literal (e.g. sha1() into a BINARY(20) column). Convert to UTF-8 only for the span attribute (a separate $displayQuery), and inject sqlcommenter into the raw $query so the wire statement keeps its exact bytes. This mirrors the already-correct PDO::prepare hook. When the sqlcommenter attribute is enabled, the attribute is set from a UTF-8-safe copy of the injected query so span export stays valid. Adds integration regression tests (real MySQL) proving a binary literal round-trips unmodified through mysqli::query, PDO::exec and PDO::query with sqlcommenter installed; both fail before this change and pass after. Fixes open-telemetry/opentelemetry-php#1960 - Rollback: revert commit; restores prior (corrupting) behavior. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Risk-Level: medium AI-Agent: claude-opus-4-8
diff --git a/src/Instrumentation/MySqli/composer.json b/src/Instrumentation/MySqli/composer.json
@@ -34,6 +34,7 @@
     "phpstan/phpstan": "^1.1",
     "phpstan/phpstan-phpunit": "^1.0",
     "psalm/plugin-phpunit": "^0.19.2",
+    "open-telemetry/opentelemetry-sqlcommenter": "^0.2",
     "open-telemetry/sdk": "^1.0",
     "phpunit/phpunit": "^9.5",
     "vimeo/psalm": "6.4.0"
diff --git a/src/Instrumentation/MySqli/src/MySqliInstrumentation.php b/src/Instrumentation/MySqli/src/MySqliInstrumentation.php
@@ -447,26 +447,32 @@ private static function queryPreHook(string $spanName, CachedInstrumentation $in
         $span = self::startSpan($spanName, $instrumentation, $class, $function, $filename, $lineno, []);
         $mysqli = $obj ? $obj : $params[0];
         $query = $obj ? $params[0] : $params[1];
-        $query = mb_convert_encoding($query ?? self::UNDEFINED, 'UTF-8');
-        if (!is_string($query)) {
-            $query = self::UNDEFINED;
+
+        // The UTF-8 conversion is only for the (protobuf-safe) span attribute. It must not
+        // be assigned back to $query, which may be returned as the executed statement when
+        // sqlcommenter is present: converting would replace non-UTF-8 bytes with `?` and
+        // corrupt binary literals (BINARY/VARBINARY/BLOB) on the wire. See #1960.
+        $displayQuery = is_string($query) ? mb_convert_encoding($query, 'UTF-8') : self::UNDEFINED;
+        if (!is_string($displayQuery)) {
+            $displayQuery = self::UNDEFINED;
         }
         $span->setAttributes([
-            TraceAttributes::DB_QUERY_TEXT => $query,
-            TraceAttributes::DB_OPERATION_NAME => self::extractQueryCommand($query),
+            TraceAttributes::DB_QUERY_TEXT => $displayQuery,
+            TraceAttributes::DB_OPERATION_NAME => self::extractQueryCommand($displayQuery),
         ]);
 
         self::addTransactionLink($tracker, $span, $mysqli);
 
-        if (class_exists('OpenTelemetry\Contrib\SqlCommenter\SqlCommenter') && $query !== self::UNDEFINED) {
+        if (class_exists('OpenTelemetry\Contrib\SqlCommenter\SqlCommenter') && is_string($query)) {
             /**
              * @psalm-suppress UndefinedClass
              */
             $commenter = \OpenTelemetry\Contrib\SqlCommenter\SqlCommenter::getInstance();
+            // Inject into the raw query so binary literals are preserved on the wire.
             $query = $commenter->inject($query);
             if ($commenter->isAttributeEnabled()) {
                 $span->setAttributes([
-                    TraceAttributes::DB_QUERY_TEXT => (string) $query,
+                    TraceAttributes::DB_QUERY_TEXT => mb_convert_encoding($query, 'UTF-8'),
                 ]);
             }
             if ($obj) {
diff --git a/src/Instrumentation/MySqli/tests/Integration/MySqliSqlCommenterBinaryTest.php b/src/Instrumentation/MySqli/tests/Integration/MySqliSqlCommenterBinaryTest.php
@@ -0,0 +1,89 @@
+<?php
+
+declare(strict_types=1);
+
+namespace OpenTelemetry\Tests\Instrumentation\MySqli\Integration;
+
+use ArrayObject;
+use mysqli;
+use mysqli_result;
+use OpenTelemetry\API\Instrumentation\Configurator;
+use OpenTelemetry\API\Trace\Propagation\TraceContextPropagator;
+use OpenTelemetry\Context\ScopeInterface;
+use OpenTelemetry\Contrib\SqlCommenter\SqlCommenter;
+use OpenTelemetry\SDK\Trace\ImmutableSpan;
+use OpenTelemetry\SDK\Trace\SpanExporter\InMemoryExporter;
+use OpenTelemetry\SDK\Trace\SpanProcessor\SimpleSpanProcessor;
+use OpenTelemetry\SDK\Trace\TracerProvider;
+use PHPUnit\Framework\TestCase;
+
+/**
+ * Regression test for #1960.
+ *
+ * When sqlcommenter is installed, the mysqli query pre-hook substitutes its (possibly
+ * modified) query back as the executed statement. It must inject into the raw query so
+ * binary literals survive on the wire, rather than UTF-8-converting the executed
+ * statement and replacing non-UTF-8 bytes with `?`.
+ */
+class MySqliSqlCommenterBinaryTest extends TestCase
+{
+    private ScopeInterface $scope;
+    /** @var ArrayObject<int, ImmutableSpan> */
+    private ArrayObject $storage;
+
+    public function setUp(): void
+    {
+        $this->storage = new ArrayObject();
+        $tracerProvider = new TracerProvider(
+            new SimpleSpanProcessor(
+                new InMemoryExporter($this->storage)
+            )
+        );
+
+        $this->scope = Configurator::create()
+            ->withTracerProvider($tracerProvider)
+            ->withPropagator(TraceContextPropagator::getInstance())
+            ->activate();
+    }
+
+    public function tearDown(): void
+    {
+        $this->scope->detach();
+    }
+
+    public function test_binary_literal_survives_sqlcommenter_injection(): void
+    {
+        // The corrupting code path is the sqlcommenter branch of the pre-hook, so it must
+        // actually be reachable for this test to be meaningful.
+        $this->assertTrue(class_exists(SqlCommenter::class), 'sqlcommenter must be installed to exercise this path');
+
+        $host = getenv('MYSQL_HOST') ?: '127.0.0.1';
+        $mysqli = new mysqli($host, 'otel_user', 'otel_passwd', 'otel_db');
+        $mysqli->set_charset('utf8mb4');
+
+        $mysqli->query('DROP TABLE IF EXISTS otel_binary_1960');
+        $mysqli->query('CREATE TABLE otel_binary_1960 (h BINARY(20) NOT NULL) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4');
+
+        // sha1("hello world") — 20 bytes containing several high (non-UTF-8) bytes.
+        $bin = hex2bin('2aae6c35c94fcfb415dbe95f408b9ce91ee846ed');
+        $this->assertIsString($bin);
+
+        $escaped = "'" . $mysqli->real_escape_string($bin) . "'";
+        $mysqli->query("INSERT INTO otel_binary_1960 (h) VALUES ($escaped)");
+
+        $result = $mysqli->query('SELECT h FROM otel_binary_1960');
+        $this->assertInstanceOf(mysqli_result::class, $result);
+        $row = $result->fetch_row();
+        $this->assertIsArray($row);
+        $this->assertIsString($row[0]);
+
+        $mysqli->query('DROP TABLE IF EXISTS otel_binary_1960');
+        $mysqli->close();
+
+        $this->assertSame(
+            bin2hex($bin),
+            bin2hex($row[0]),
+            'binary column bytes must be stored unmodified when sqlcommenter is installed',
+        );
+    }
+}
diff --git a/src/Instrumentation/PDO/composer.json b/src/Instrumentation/PDO/composer.json
@@ -26,6 +26,7 @@
     "phpstan/phpstan": "^1.1",
     "phpstan/phpstan-phpunit": "^1.0",
     "psalm/plugin-phpunit": "^0.19.2",
+    "open-telemetry/opentelemetry-sqlcommenter": "^0.2",
     "open-telemetry/sdk": "^1.0",
     "open-telemetry/test-utils": "^0.2.0",
     "phpunit/phpunit": "^9.5",
diff --git a/src/Instrumentation/PDO/src/PDOInstrumentation.php b/src/Instrumentation/PDO/src/PDOInstrumentation.php
@@ -114,12 +114,17 @@ public static function register(): void
                 /** @psalm-suppress ArgumentTypeCoercion */
                 $builder = self::makeBuilder($instrumentation, 'PDO::query', $function, $class, $filename, $lineno)
                     ->setSpanKind(SpanKind::KIND_CLIENT);
-                $query = mb_convert_encoding($params[0] ?? self::UNDEFINED, 'UTF-8');
-                if (!is_string($query)) {
-                    $query = self::UNDEFINED;
+                $query = $params[0] ?? null;
+                // The UTF-8 conversion is only for the (protobuf-safe) span attribute. It must
+                // not be assigned back to $query, which may be returned as the executed statement
+                // when sqlcommenter is present: converting would replace non-UTF-8 bytes with `?`
+                // and corrupt binary literals (BINARY/VARBINARY/BLOB) on the wire. See #1960.
+                $displayQuery = is_string($query) ? mb_convert_encoding($query, 'UTF-8') : self::UNDEFINED;
+                if (!is_string($displayQuery)) {
+                    $displayQuery = self::UNDEFINED;
                 }
                 if ($class === PDO::class) {
-                    $builder->setAttribute(DbAttributes::DB_QUERY_TEXT, $query);
+                    $builder->setAttribute(DbAttributes::DB_QUERY_TEXT, $displayQuery);
                 }
                 $parent = Context::getCurrent();
                 $span = $builder->startSpan();
@@ -129,7 +134,7 @@ public static function register(): void
 
                 Context::storage()->attach($span->storeInContext($parent));
 
-                if (class_exists('OpenTelemetry\Contrib\SqlCommenter\SqlCommenter') && $query !== self::UNDEFINED) {
+                if (class_exists('OpenTelemetry\Contrib\SqlCommenter\SqlCommenter') && is_string($query)) {
                     if (array_key_exists(DbAttributes::DB_SYSTEM_NAME, $attributes)) {
                         /** @psalm-suppress PossiblyInvalidCast */
                         switch ((string) $attributes[DbAttributes::DB_SYSTEM_NAME]) {
@@ -142,7 +147,7 @@ public static function register(): void
                                 $query = $commenter->inject($query);
                                 if ($commenter->isAttributeEnabled()) {
                                     $span->setAttributes([
-                                        DbAttributes::DB_QUERY_TEXT => (string) $query,
+                                        DbAttributes::DB_QUERY_TEXT => mb_convert_encoding($query, 'UTF-8'),
                                     ]);
                                 }
 
@@ -170,12 +175,17 @@ public static function register(): void
                 /** @psalm-suppress ArgumentTypeCoercion */
                 $builder = self::makeBuilder($instrumentation, 'PDO::exec', $function, $class, $filename, $lineno)
                     ->setSpanKind(SpanKind::KIND_CLIENT);
-                $query = mb_convert_encoding($params[0] ?? self::UNDEFINED, 'UTF-8');
-                if (!is_string($query)) {
-                    $query = self::UNDEFINED;
+                $query = $params[0] ?? null;
+                // The UTF-8 conversion is only for the (protobuf-safe) span attribute. It must
+                // not be assigned back to $query, which may be returned as the executed statement
+                // when sqlcommenter is present: converting would replace non-UTF-8 bytes with `?`
+                // and corrupt binary literals (BINARY/VARBINARY/BLOB) on the wire. See #1960.
+                $displayQuery = is_string($query) ? mb_convert_encoding($query, 'UTF-8') : self::UNDEFINED;
+                if (!is_string($displayQuery)) {
+                    $displayQuery = self::UNDEFINED;
                 }
                 if ($class === PDO::class) {
-                    $builder->setAttribute(DbAttributes::DB_QUERY_TEXT, $query);
+                    $builder->setAttribute(DbAttributes::DB_QUERY_TEXT, $displayQuery);
                 }
                 $parent = Context::getCurrent();
                 $span = $builder->startSpan();
@@ -185,7 +195,7 @@ public static function register(): void
 
                 Context::storage()->attach($span->storeInContext($parent));
 
-                if (class_exists('OpenTelemetry\Contrib\SqlCommenter\SqlCommenter') && $query !== self::UNDEFINED) {
+                if (class_exists('OpenTelemetry\Contrib\SqlCommenter\SqlCommenter') && is_string($query)) {
                     if (array_key_exists(DbAttributes::DB_SYSTEM_NAME, $attributes)) {
                         /** @psalm-suppress PossiblyInvalidCast */
                         switch ((string) $attributes[DbAttributes::DB_SYSTEM_NAME]) {
@@ -198,7 +208,7 @@ public static function register(): void
                                 $query = $commenter->inject($query);
                                 if ($commenter->isAttributeEnabled()) {
                                     $span->setAttributes([
-                                        DbAttributes::DB_QUERY_TEXT => (string) $query,
+                                        DbAttributes::DB_QUERY_TEXT => mb_convert_encoding($query, 'UTF-8'),
                                     ]);
                                 }
 
diff --git a/src/Instrumentation/PDO/tests/Integration/PdoSqlCommenterBinaryTest.php b/src/Instrumentation/PDO/tests/Integration/PdoSqlCommenterBinaryTest.php
@@ -0,0 +1,113 @@
+<?php
+
+declare(strict_types=1);
+
+namespace OpenTelemetry\Tests\Instrumentation\PDO\Integration;
+
+use ArrayObject;
+use OpenTelemetry\API\Instrumentation\Configurator;
+use OpenTelemetry\API\Trace\Propagation\TraceContextPropagator;
+use OpenTelemetry\Context\ScopeInterface;
+use OpenTelemetry\Contrib\SqlCommenter\SqlCommenter;
+use OpenTelemetry\SDK\Trace\ImmutableSpan;
+use OpenTelemetry\SDK\Trace\SpanExporter\InMemoryExporter;
+use OpenTelemetry\SDK\Trace\SpanProcessor\SimpleSpanProcessor;
+use OpenTelemetry\SDK\Trace\TracerProvider;
+use PDO;
+use PHPUnit\Framework\TestCase;
+
+/**
+ * Regression test for #1960.
+ *
+ * When sqlcommenter is installed and the driver is mysql/postgresql, the PDO::query and
+ * PDO::exec pre-hooks substitute their query back as the executed statement. They must
+ * inject into the raw query so binary literals survive on the wire, rather than
+ * UTF-8-converting the executed statement and replacing non-UTF-8 bytes with `?`.
+ */
+class PdoSqlCommenterBinaryTest extends TestCase
+{
+    private ScopeInterface $scope;
+    /** @var ArrayObject<int, ImmutableSpan> */
+    private ArrayObject $storage;
+
+    public function setUp(): void
+    {
+        $this->storage = new ArrayObject();
+        $tracerProvider = new TracerProvider(
+            new SimpleSpanProcessor(
+                new InMemoryExporter($this->storage)
+            )
+        );
+
+        $this->scope = Configurator::create()
+            ->withTracerProvider($tracerProvider)
+            ->withPropagator(TraceContextPropagator::getInstance())
+            ->activate();
+    }
+
+    public function tearDown(): void
+    {
+        $this->scope->detach();
+    }
+
+    public function test_binary_literal_survives_sqlcommenter_injection_via_exec(): void
+    {
+        $pdo = $this->connect();
+
+        // sha1("hello world") — 20 bytes containing several high (non-UTF-8) bytes.
+        $bin = hex2bin('2aae6c35c94fcfb415dbe95f408b9ce91ee846ed');
+        $this->assertIsString($bin);
+
+        $pdo->exec('INSERT INTO otel_binary_1960 (h) VALUES (' . $pdo->quote($bin) . ')');
+        $stored = $this->fetchStored($pdo);
+
+        $pdo->exec('DROP TABLE IF EXISTS otel_binary_1960');
+
+        $this->assertSame(bin2hex($bin), bin2hex($stored), 'PDO::exec must not corrupt binary literals when sqlcommenter is installed');
+    }
+
+    public function test_binary_literal_survives_sqlcommenter_injection_via_query(): void
+    {
+        $pdo = $this->connect();
+
+        // sha1("") — 20 bytes.
+        $bin = hex2bin('da39a3ee5e6b4b0d3255bfef95601890afd80709');
+        $this->assertIsString($bin);
+
+        $pdo->query('INSERT INTO otel_binary_1960 (h) VALUES (' . $pdo->quote($bin) . ')');
+        $stored = $this->fetchStored($pdo);
+
+        $pdo->exec('DROP TABLE IF EXISTS otel_binary_1960');
+
+        $this->assertSame(bin2hex($bin), bin2hex($stored), 'PDO::query must not corrupt binary literals when sqlcommenter is installed');
+    }
+
+    private function connect(): PDO
+    {
+        if (!in_array('mysql', PDO::getAvailableDrivers(), true)) {
+            $this->markTestSkipped('pdo_mysql driver not available');
+        }
+        // The corrupting code path is the sqlcommenter branch of the pre-hook, so it must
+        // actually be reachable for this test to be meaningful.
+        $this->assertTrue(class_exists(SqlCommenter::class), 'sqlcommenter must be installed to exercise this path');
+
+        $host = getenv('MYSQL_HOST') ?: '127.0.0.1';
+        $pdo = new PDO("mysql:host={$host};dbname=otel_db;charset=utf8mb4", 'otel_user', 'otel_passwd', [
+            PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION,
+        ]);
+        $pdo->exec('DROP TABLE IF EXISTS otel_binary_1960');
+        $pdo->exec('CREATE TABLE otel_binary_1960 (h BINARY(20) NOT NULL) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4');
+
+        return $pdo;
+    }
+
+    private function fetchStored(PDO $pdo): string
+    {
+        $result = $pdo->query('SELECT h FROM otel_binary_1960');
+        $this->assertNotFalse($result);
+        $stored = $result->fetchColumn();
+        $this->assertIsString($stored);
+
+        return $stored;
+    }
+}
