Fix void post hook overwriting retval in OTel hook polyfill#3920
Conversation
|
✨ Fix all issues with BitsAI or with Cursor
|
Leiyks
force-pushed
the
leiyks/fix-otel-void-posthook-retval
branch
from
d4a4192 to
b93b323
Compare
![chatgpt-codex-connector[bot]](https://avatars.github.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b93b323195
ℹ️ 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".
| \OpenTelemetry\Instrumentation\hook( | ||
| Target::class, | ||
| 'handle', | ||
| pre: null, |
There was a problem hiding this comment.
In PHP 7.x runs of tests/ext/sandbox/install_hook, this new test is parsed as part of the PHPT and pre:/post: named arguments are a syntax error before the regression can run. The repository still carries PHP 7 test coverage and adjacent install-hook tests add explicit skips for PHP-version-specific syntax, so this will fail those jobs rather than testing the C fix; use positional arguments here (and skip only versions that cannot parse : void, if needed).
Useful? React with 👍 / 👎.
dd_uhook_end in ext/hook/uhook_otel.c masked the post closure's return
type with IS_VOID (= 14, the raw type code) instead of MAY_BE_VOID
(= 1 << 14, the bitmask flag used inside PURE_MASK). The bitwise AND
between a bitmask and a raw type code is always zero, so the "skip
retval replacement for :void post hooks" branch never fired and every
:void post hook silently overwrote the function's actual return value
with the closure's implicit-null return.
The visible symptom is a TypeError on Symfony\Component\HttpKernel\Kernel::handle
when opentelemetry-auto-symfony is present in vendor/. dd-trace polyfills
\OpenTelemetry\Instrumentation\hook and forces extension_loaded("opentelemetry")
to true, so the package's :void post hook on HttpKernel::handle is installed
against the polyfill — and then nuked the Response the outer Kernel::handle
was about to return. The bug isn't Symfony-version-specific; it triggers
for any :void post hook on any function with a non-void return type.
Add a focused .phpt regression covering both the :void preserves-retval
path and the typed-non-void overrides-retval path.
The OpenTelemetry hook polyfill (dd_register_opentelemetry_wrapper) is gated on PHP_VERSION_ID >= 80000 in ext/hook/uhook.c:1183, and the test itself uses named-argument syntax (pre:, post:) introduced in PHP 8.0. Without the SKIPIF, CI fails on every PHP 7.x leg with a parse error before SKIPIF would even be evaluated.
|
Fix is right, the test needs fixing :-) |
Benchmarks [ tracer ]Benchmark execution time: 2026-05-27 16:27:15 Comparing candidate commit 6211e0a in PR branch Found 4 performance improvements and 4 performance regressions! Performance is the same for 186 metrics, 0 unstable metrics. scenario:MessagePackSerializationBench/benchMessagePackSerialization-opcache
scenario:PDOBench/benchPDOOverhead-opcache
scenario:PDOBench/benchPDOOverheadWithDBM-opcache
scenario:PHPRedisBench/benchRedisOverhead-opcache
scenario:SamplingRuleMatchingBench/benchRegexMatching1
scenario:SamplingRuleMatchingBench/benchRegexMatching2
scenario:SamplingRuleMatchingBench/benchRegexMatching3
scenario:SamplingRuleMatchingBench/benchRegexMatching4-opcache
|
The MAY_BE_VOID regression test is meaningful only when DD_TRACE_ENABLED=1 at request shutdown. When tracing is disabled, ddtrace_post_deactivate fires before PHP releases the extra reference it holds on :void closure literals declared in the main script's op_array, producing a stray leak warning unrelated to the IS_VOID/MAY_BE_VOID fix this test guards. Gate the test on datadog.trace.cli_enabled=1 (with an explicit comment) so make test_c_disabled cleanly skips it, mirroring the precedent set by tests/ext/bailout_double_hook_clear.phpt.
|
Nice find! |
2 participants
Summary
dd_uhook_endinext/hook/uhook_otel.cmasked the post closure's return type withIS_VOID(the raw type code,14) instead ofMAY_BE_VOID(the bitmask flag1 << 14used insidePURE_MASK). The AND between a bitmask and a raw type code is always zero, so the "skip retval replacement for:voidpost hooks" branch never fired and every:voidpost hook silently overwrote the function's actual return value with the closure's implicit-null return..phptregression covering both branches (:voidpreserves retval, typed-non-void overrides retval).How the bug surfaced
dd-trace polyfills
\OpenTelemetry\Instrumentation\hookand forcesextension_loaded("opentelemetry")to true (ext/hook/uhook_otel.c:269). Packages likeopentelemetry-auto-symfonyregister hooks against this polyfill even when the real OTel C extension is absent.SymfonyInstrumentation::register()installs a:voidpost hook onSymfony\Component\HttpKernel\HttpKernel::handle. With the buggy mask, that:voidclosure's implicit null return was copied into the real Response retval. The outerSymfony\Component\HttpKernel\Kernel::handle's:Responseruntime check then fired:Not Symfony-version-specific. Triggers for any
:voidpost hook on any function with a non-void return type.