[opentelemetry-php-contrib] auto-mysqli & auto-pdo: $query = mb_convert_encoding(..., 'UTF-8') reassignment corrupts binary literals in SQL when sqlcommenter is installed

[ttys3]

Summary

MySqliInstrumentation::queryPreHook and PDOInstrumentation's PDO::query / PDO::exec hooks reassign the local $query variable to mb_convert_encoding($query, 'UTF-8') for the purpose of populating the db.query.text span attribute. When open-telemetry/opentelemetry-sqlcommenter is also installed, the same (now corrupted) variable is then injected and returned from the pre-hook as the substituted argument for mysqli_query() / PDO::query() / PDO::exec(). The wire-level SQL therefore loses any byte that is not part of a valid UTF-8 sequence (replaced with 0x3F, ?).

In practice this destroys any SQL that contains a binary literal — BINARY / VARBINARY / BLOB writes via mysqli (or PDO::query/PDO::exec) that embed raw bytes inside a quoted string literal end up with high bytes silently replaced by ? in the column.

We hit this in production with sha1(info_dict) writes to a BINARY(20) column — every newly inserted row ended up storing a mangled hash, breaking downstream lookups.

Affected packages

• open-telemetry/opentelemetry-auto-mysqli (any version that includes #442)
• open-telemetry/opentelemetry-auto-pdo (same PR, same pattern; only PDO::query and PDO::exec hooks — PDO::prepare is unaffected and is in fact the example of the correct usage in the same file)
• Triggered by the additional presence of open-telemetry/opentelemetry-sqlcommenter in the autoloader. Without sqlcommenter, the pre-hook falls through to return [] and the original argument is preserved (the mb_convert_encoding corruption stays local to the span attribute, which is harmless).

For PDO, the additional gate is attributes[DB_SYSTEM_NAME] in {mysql, postgresql}, so SQLite / SQL Server users escape unscathed.

Offending code

src/Instrumentation/MySqli/src/MySqliInstrumentation.php (main branch as of writing):

private static function queryPreHook(string $spanName, ...): array
{
    $span = self::startSpan(...);
    $mysqli = $obj ? $obj : $params[0];
    $query = $obj ? $params[0] : $params[1];
    $query = mb_convert_encoding($query ?? self::UNDEFINED, 'UTF-8'); // <-- here
    if (!is_string($query)) {
        $query = self::UNDEFINED;
    }
    $span->setAttributes([
        TraceAttributes::DB_QUERY_TEXT => $query,
        TraceAttributes::DB_OPERATION_NAME => self::extractQueryCommand($query),
    ]);
    ...
    if (class_exists('OpenTelemetry\Contrib\SqlCommenter\SqlCommenter') && $query !== self::UNDEFINED) {
        $commenter = \OpenTelemetry\Contrib\SqlCommenter\SqlCommenter::getInstance();
        $query = $commenter->inject($query);
        ...
        return [1 => $query]; // <-- substitutes corrupted SQL back into mysqli_query args
    }
    return [];
}

src/Instrumentation/PDO/src/PDOInstrumentation.php repeats the same pattern around lines 117 and 173 (for PDO::query and PDO::exec respectively).

For comparison, the PDO::prepare hook in the same file (around line 230) does the right thing:

$builder->setAttribute(DbAttributes::DB_QUERY_TEXT, mb_convert_encoding($params[0] ?? 'undefined', 'UTF-8'));

The conversion result is used inline as the attribute value only, not assigned back to $query, so it cannot leak into the substituted return value.

Minimal reproduction

<?php
// Requires: opentelemetry PECL extension loaded, plus composer:
//   open-telemetry/opentelemetry-auto-mysqli
//   open-telemetry/opentelemetry-sqlcommenter
require __DIR__ . '/vendor/autoload.php';

$mysqli = new mysqli('127.0.0.1', 'root', '', 'test');
$mysqli->set_charset('utf8mb4');

$mysqli->query("DROP TABLE IF EXISTS t");
$mysqli->query("CREATE TABLE t (h BINARY(20) NOT NULL) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4");

// Well-known test vector: sha1("hello world"). Any 20-byte payload
// containing high-bit bytes will reproduce the same byte-replacement
// pattern.
$hex = '2aae6c35c94fcfb415dbe95f408b9ce91ee846ed';
$bin = hex2bin($hex);
$escaped = "'" . $mysqli->real_escape_string($bin) . "'";
$mysqli->query("INSERT INTO t (h) VALUES ($escaped)");

$r = $mysqli->query("SELECT HEX(h) AS h FROM t LIMIT 1");
$row = $r->fetch_assoc();

echo "input:  $hex\n";
echo "stored: " . strtolower($row['h']) . "\n";
echo "match?  " . (strtolower($row['h']) === $hex ? "YES" : "NO ✗") . "\n";

Expected output:

input:  2aae6c35c94fcfb415dbe95f408b9ce91ee846ed
stored: 2aae6c35c94fcfb415dbe95f408b9ce91ee846ed
match?  YES

Actual output:

input:  2aae6c35c94fcfb415dbe95f408b9ce91ee846ed
stored: 2a3f6c353f4fcfb4153f3f5f403f3f3f1e3f463f
match?  NO ✗

The output byte pattern matches exactly what mb_convert_encoding($sql_with_binary, 'UTF-8') produces in pure PHP — verifiable independently with no MySQL involved:

$bin = hex2bin('2aae6c35c94fcfb415dbe95f408b9ce91ee846ed');
$sql = "VALUES ('$bin')";
echo bin2hex(mb_convert_encoding($sql, 'UTF-8'));
// the embedded 20-byte payload comes out as
// 2a3f6c353f4fcfb4153f3f5f403f3f3f1e3f463f

ASCII bytes (2a, 6c, 35, 4f, 15, 5f, 40, 1e, 46) survive; lone high bytes are each replaced with 0x3F (?); and the coincidentally-valid 2-byte UTF-8 sequence cf b4 (U+03F4) survives intact. This is the documented behavior of mb_convert_encoding(..., 'UTF-8') on input that isn't valid UTF-8 — the surprise is just that an instrumentation hook is applying it to a SQL string that the application intended to be byte-exact.

Verification that this is the cause

Bypassing the OTel autoloader removes the corruption:

require vendor/autoload.php?	set_charset('utf8mb4')?	Result
no	yes	INTACT
yes	yes	CORRUPTED
no	no	INTACT
yes	no	CORRUPTED

Removing opentelemetry-sqlcommenter from the autoloader (keeping auto-mysqli) also removes the corruption, because the pre-hook then falls through to return [] and the substituted argument is never returned.

Why this slipped through

I believe the intent of mb_convert_encoding($query, 'UTF-8') is purely to satisfy the OTel attribute-value-must-be-UTF-8 invariant — db.query.text cannot legally contain non-UTF-8 bytes per the semconv. That coercion is correct as a display value. The mistake is using the same $query local variable for two different purposes:

1. As the value displayed in tracing UI (must be valid UTF-8).
2. As the value substituted back into mysqli_query() / PDO::query() (must preserve every byte exactly, including binary literals).

Suggested fix

Separate the two roles by introducing a dedicated variable for the span attribute, leaving $query untouched for the substitution path. SqlCommenter::inject itself is pure string concatenation ($query . Utils::formatComments(...)), so when fed the unmangled original it preserves binary bytes correctly.

src/Instrumentation/MySqli/src/MySqliInstrumentation.php:

 private static function queryPreHook(string $spanName, CachedInstrumentation $instrumentation, MySqliTracker $tracker, $obj, array $params, ?string $class, string $function, ?string $filename, ?int $lineno): array
 {
     $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;
+    // Coerce to valid UTF-8 only for the span attribute (semconv requires it).
+    // Do NOT overwrite $query, because $query may flow into the substitution
+    // branch below and end up replacing mysqli_query()'s argument — any binary
+    // literal embedded in the SQL would lose its high bytes to 0x3F.
+    $query_for_span = mb_convert_encoding($query ?? self::UNDEFINED, 'UTF-8');
+    if (!is_string($query_for_span)) {
+        $query_for_span = self::UNDEFINED;
     }
     $span->setAttributes([
-        TraceAttributes::DB_QUERY_TEXT => $query,
-        TraceAttributes::DB_OPERATION_NAME => self::extractQueryCommand($query),
+        TraceAttributes::DB_QUERY_TEXT => $query_for_span,
+        TraceAttributes::DB_OPERATION_NAME => self::extractQueryCommand($query_for_span),
     ]);

     self::addTransactionLink($tracker, $span, $mysqli);

-    if (class_exists('OpenTelemetry\Contrib\SqlCommenter\SqlCommenter') && $query !== self::UNDEFINED) {
+    if (class_exists('OpenTelemetry\Contrib\SqlCommenter\SqlCommenter') && $query_for_span !== self::UNDEFINED) {
         /**
          * @psalm-suppress UndefinedClass
          */
         $commenter = \OpenTelemetry\Contrib\SqlCommenter\SqlCommenter::getInstance();
         $query = $commenter->inject($query);
         if ($commenter->isAttributeEnabled()) {
             $span->setAttributes([
                 TraceAttributes::DB_QUERY_TEXT => (string) $query,
             ]);
         }
         if ($obj) {
             return [
                 0 => $query,
             ];
         }

         return [
             1 => $query,
         ];
     }

     return [];
 }

src/Instrumentation/PDO/src/PDOInstrumentation.php — same shape, applied to both the PDO::query and PDO::exec hooks (the PDO::prepare hook in the same file already follows the correct pattern: it uses mb_convert_encoding(...) inline as the attribute value and never assigns it back to $query).

 pre: static function (PDO $pdo, array $params, string $class, string $function, ?string $filename, ?int $lineno) use ($pdoTracker, $instrumentation) {
     /** @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] ?? self::UNDEFINED;
+    $query_for_span = mb_convert_encoding($query, 'UTF-8');
+    if (!is_string($query_for_span)) {
+        $query_for_span = self::UNDEFINED;
     }
     if ($class === PDO::class) {
-        $builder->setAttribute(DbAttributes::DB_QUERY_TEXT, $query);
+        $builder->setAttribute(DbAttributes::DB_QUERY_TEXT, $query_for_span);
     }
     $parent = Context::getCurrent();
     $span = $builder->startSpan();

     $attributes = $pdoTracker->trackedAttributesForPdo($pdo);
     $span->setAttributes($attributes);

     Context::storage()->attach($span->storeInContext($parent));

-    if (class_exists('OpenTelemetry\Contrib\SqlCommenter\SqlCommenter') && $query !== self::UNDEFINED) {
+    if (class_exists('OpenTelemetry\Contrib\SqlCommenter\SqlCommenter') && $query_for_span !== self::UNDEFINED) {
         if (array_key_exists(DbAttributes::DB_SYSTEM_NAME, $attributes)) {
             ...
         }
     }

     return [];
 },

(PDO::exec is the same delta with 'PDO::exec' as the span name.)

Workaround for users hitting this in production today

Removing open-telemetry/opentelemetry-sqlcommenter from composer.json breaks the substitution branch and restores correct behavior at the cost of losing trace-context-as-SQL-comment correlation. DB-call spans continue to work.

For application code that cannot avoid binary-in-SQL-string patterns, a defensive workaround is to emit binary values as MySQL hex literals X'<hex>' (pure ASCII, survives mb_convert_encoding unchanged) or to use prepared statements with bind_param (the parameter value never enters the SQL string in the first place).

Environment

• PHP 8.2.x on Linux (also confirmed reproducible on PHP 8.3)
• opentelemetry PECL extension 1.2.x
• open-telemetry/opentelemetry-auto-mysqli ^0.1
• open-telemetry/opentelemetry-sqlcommenter ^0.1
• MySQL 8.x (column type BINARY(20), but any BINARY/VARBINARY/BLOB column populated by SQL string interpolation is affected)

[jerrytfleung]

Thanks @ttys3 for pointing out this bug and providing suggestion to fix. Would you move forward to create a PR to fix?