fix(mysqli,pdo): preserve binary SQL literals when sqlcommenter is installed by PuvaanRaaj · Pull Request #651 · open-telemetry/opentelemetry-php-contrib

PuvaanRaaj · 2026-07-08T06:59:58Z

Problem

Fixes open-telemetry/opentelemetry-php#1960.

MySqliInstrumentation::queryPreHook and the PDO::query / PDO::exec pre-hooks reassign the local $query to mb_convert_encoding($query, 'UTF-8') in order to populate the db.query.text span attribute. When open-telemetry/opentelemetry-sqlcommenter is also installed, that same (now UTF-8-mangled) variable is injected by sqlcommenter and returned from the pre-hook as the substituted statement argument. Any byte in the executed SQL that is not part of a valid UTF-8 sequence is therefore replaced with 0x3F (?) on the wire.

This silently corrupts any SQL containing a binary literal — BINARY / VARBINARY / BLOB writes that embed raw bytes inside a quoted string literal via mysqli, PDO::query or PDO::exec. The reporter hit it in production with sha1(...) writes into a BINARY(20) column: every inserted row stored a mangled hash.

Without sqlcommenter the pre-hook returns [] and the original argument is preserved, so the corruption is limited to the (harmless) span attribute — which is why this went unnoticed.

Fix

Convert to UTF-8 only for the span attribute, via a separate $displayQuery; never assign it back to $query.
Inject sqlcommenter into the raw $query so the executed statement keeps its exact bytes.
When the sqlcommenter attribute is enabled, set db.query.text from a UTF-8-safe copy of the injected query, so span export stays valid.

This mirrors the already-correct PDO::prepare hook in the same file, which only uses the converted value inline as the attribute value.

Tests

Added integration regression tests (against real MySQL) proving a binary literal round-trips unmodified through mysqli::query, PDO::exec and PDO::query when sqlcommenter is installed:

MySqliSqlCommenterBinaryTest
PdoSqlCommenterBinaryTest

Both fail before this change and pass after (verified locally via the package Docker test env). open-telemetry/opentelemetry-sqlcommenter is added as a require-dev of the two packages so the corrupting code path is actually exercised.

Verified locally per package: test-integration, style, phan, psalm, composer validate all green.

🤖 Generated with Claude Code

welcome · 2026-07-08T07:00:02Z

Thanks for opening your first pull request! If you haven't yet signed our Contributor License Agreement (CLA), then please do so that we can accept your contribution. A link should appear shortly in this PR if you have not already signed one.

linux-foundation-easycla · 2026-07-08T07:00:10Z

The committers listed above are authorized under a signed CLA.

✅ login: PuvaanRaaj / name: PuvaanRaaj (39e2287)

PuvaanRaaj · 2026-07-08T07:00:39Z

@open-telemetry/php-approvers @brettmc this fixes a silent data-corruption bug (#1960): binary SQL literals get mangled on the wire when sqlcommenter is installed. Includes real-MySQL regression tests that fail before / pass after. Review welcome when convenient. 🙏

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b48a443f0d

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-07-08T07:05:06Z

+        $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', [


Start MySQL for the PDO binary sqlcommenter test

This new integration test opens a mysql: DSN whenever pdo_mysql is available, but the PDO CI jobs do not start the MySQL service. I checked .github/workflows/php.yml: Instrumentation/PDO is in the matrix and both PHPUnit steps run for it, while the only Start Mysql step is gated on matrix.project == 'Instrumentation/MySqli'. On those PDO jobs there is no otel_db/otel_user server at 127.0.0.1, so the test will error before exercising the regression; either start MySQL for the PDO project or skip when the service is not configured.

Useful? React with 👍 / 👎.

Good catch — fixed in dc6b523. The `Start Mysql` CI step was gated on `Instrumentation/MySqli` only, so the PDO job had no server and the test would error on connect (the driver is present, so it never hit the skip). Extended the step condition to also start MySQL for `Instrumentation/PDO`; credentials (`otel_db`/`otel_user`/`otel_passwd` @ 127.0.0.1) match the mysql service in docker-compose. If `pdo_mysql` is absent from the image the test still skips gracefully.

…stalled 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. Risk-Level: medium AI-Agent: claude-opus-4-8

PuvaanRaaj · 2026-07-08T07:25:34Z

/easycla

The #1960 PDO regression test opens a real mysql DSN. The PDO CI matrix job had no MySQL service (Start Mysql was gated on MySqli only), so the test errored instead of exercising the regression. Gate the step on PDO as well; credentials match the mysql service in docker-compose.

ChrisLightfootWild

Thanks for looking into this.

I noted this on the PR:

Are the changes here entirely agent generated?

ChrisLightfootWild · 2026-07-08T19:31:32Z

                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)) {


This change is not needed, as you are already checking this on line 123.

PuvaanRaaj requested a review from a team as a code owner July 8, 2026 06:59

chatgpt-codex-connector Bot reviewed Jul 8, 2026

View reviewed changes

PuvaanRaaj force-pushed the fix/1960-binary-sql-corruption branch from b48a443 to 39e2287 Compare July 8, 2026 07:25

ChrisLightfootWild reviewed Jul 8, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

fix(mysqli,pdo): preserve binary SQL literals when sqlcommenter is installed#651
PuvaanRaaj wants to merge 2 commits into
open-telemetry:mainfrom
PuvaanRaaj:fix/1960-binary-sql-corruption

PuvaanRaaj commented Jul 8, 2026

Uh oh!

welcome Bot commented Jul 8, 2026

Uh oh!

linux-foundation-easycla Bot commented Jul 8, 2026 •

edited

Loading

Uh oh!

PuvaanRaaj commented Jul 8, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Uh oh!

chatgpt-codex-connector Bot Jul 8, 2026

Uh oh!

PuvaanRaaj Jul 8, 2026

Uh oh!

PuvaanRaaj commented Jul 8, 2026

Uh oh!

ChrisLightfootWild left a comment

Uh oh!

ChrisLightfootWild Jul 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

PuvaanRaaj commented Jul 8, 2026

Problem

Fix

Tests

Uh oh!

welcome Bot commented Jul 8, 2026

Uh oh!

linux-foundation-easycla Bot commented Jul 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PuvaanRaaj commented Jul 8, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Jul 8, 2026

Choose a reason for hiding this comment

Uh oh!

PuvaanRaaj Jul 8, 2026

Choose a reason for hiding this comment

Uh oh!

PuvaanRaaj commented Jul 8, 2026

Uh oh!

ChrisLightfootWild left a comment

Choose a reason for hiding this comment

Uh oh!

ChrisLightfootWild Jul 8, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

linux-foundation-easycla Bot commented Jul 8, 2026 •

edited

Loading