Skip to content

fix(tracer): add null guard clause in sidecar reconnect callback#3499

Merged
Leiyks merged 2 commits intomasterfrom
leiyks/fix-sidecar-shutdown
Nov 28, 2025
Merged

fix(tracer): add null guard clause in sidecar reconnect callback#3499
Leiyks merged 2 commits intomasterfrom
leiyks/fix-sidecar-shutdown

Conversation

@Leiyks
Copy link
Copy Markdown
Contributor

@Leiyks Leiyks commented Nov 24, 2025 •
edited
Loading

Description

Fixes a use-after-free crash in sidecar shutdown that occurs when the reconnect callback is invoked after endpoints have been freed.

image

Related Jiras: APMS-17794

@pr-commenter
Copy link
Copy Markdown

pr-commenter Bot commented Nov 24, 2025 •
edited
Loading

Benchmarks [ tracer ]

Benchmark execution time: 2025-11-28 15:42:04

Comparing candidate commit 35e6a4e in PR branch leiyks/fix-sidecar-shutdown with baseline commit 9468244 in branch master.

Found 1 performance improvements and 4 performance regressions! Performance is the same for 186 metrics, 3 unstable metrics.

scenario:SamplingRuleMatchingBench/benchRegexMatching1

  • 🟥 execution_time [+24.675ns; +103.725ns] or [+2.039%; +8.569%]

scenario:SamplingRuleMatchingBench/benchRegexMatching3

  • 🟥 execution_time [+73.514ns; +144.286ns] or [+6.199%; +12.167%]

scenario:SamplingRuleMatchingBench/benchRegexMatching4

  • 🟥 execution_time [+57.006ns; +131.794ns] or [+4.809%; +11.118%]

scenario:SpanBench/benchOpenTelemetryAPI

  • 🟥 mem_peak [+1.204MB; +1.244MB] or [+2.881%; +2.975%]

scenario:TraceSerializationBench/benchSerializeTrace

  • 🟩 execution_time [-45.537µs; -29.763µs] or [-10.254%; -6.702%]

@Leiyks Leiyks changed the title fix: add null guard check in callback fix(tracer): add null guard clause in sidecar reconnect callback Nov 24, 2025
@Leiyks Leiyks marked this pull request as ready for review November 24, 2025 19:21
@Leiyks Leiyks requested a review from a team as a code owner November 24, 2025 19:21
@Leiyks
fix: add null guard check in callback
6122feb 
Signed-off-by: Alexandre Rulleau <[email protected]>
@Leiyks Leiyks force-pushed the leiyks/fix-sidecar-shutdown branch from b90299d to 6122feb Compare November 24, 2025 19:22
realFlowControl
realFlowControl approved these changes Nov 28, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@realFlowControl realFlowControl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch!
I have a comment which is just lack of knowledge about this part of the code, otherwise it looks fine to me.

Comment thread ext/sidecar.c
Comment on lines +119 to +122
if (!ddtrace_endpoint || !dogstatsd_endpoint) {
return;
}

Copy link
Copy Markdown
Member

@realFlowControl realFlowControl Nov 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The dd_sidecar_connection_factory_ex function uses another pattern:

dd-trace-php/ext/sidecar.c

Lines 161 to 165 in 6122feb

// Should not happen, unless the agent url is malformed
if (!ddtrace_endpoint) {
return NULL;
}
ZEND_ASSERT(dogstatsd_endpoint != NULL);

I am not 100% certain why a ZEND_ASSERT is sufficient below, but most likely because ddtrace_endpoint and dogstatsd_endpoint should be kept in sync anyway.

So maybe this is just fine ..

Copy link
Copy Markdown
Contributor Author

@Leiyks Leiyks Nov 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this is the original reason this was put there i believe

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Nov 28, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 61.71%. Comparing base (9468244) to head (35e6a4e).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3499      +/-   ##
==========================================
+ Coverage   61.69%   61.71%   +0.01%     
==========================================
  Files         142      142              
  Lines       12923    12923              
  Branches     1695     1695              
==========================================
+ Hits         7973     7975       +2     
+ Misses       4196     4193       -3     
- Partials      754      755       +1

see 1 file with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9468244...35e6a4e. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Leiyks Leiyks merged commit 17189c6 into master Nov 28, 2025
1994 of 2007 checks passed
@Leiyks Leiyks deleted the leiyks/fix-sidecar-shutdown branch November 28, 2025 15:50
@github-actions github-actions Bot added this to the 1.15.0 milestone Nov 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@realFlowControl realFlowControl realFlowControl approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

1.15.0

Development

Successfully merging this pull request may close these issues.

3 participants

@Leiyks @codecov-commenter @realFlowControl