Skip to content

fix(tracer): use atomic_store for writer->headers assignment#3945

Merged
realFlowControl merged 1 commit into
masterfrom
florian/fix-3917-curl-headers-race
Jun 3, 2026
Merged

fix(tracer): use atomic_store for writer->headers assignment#3945
realFlowControl merged 1 commit into
masterfrom
florian/fix-3917-curl-headers-race

Conversation

@realFlowControl

@realFlowControl realFlowControl commented Jun 3, 2026

Copy link
Copy Markdown
Member

Description

writer->headers is declared _Atomic but was being stored via a plain non-atomic assignment in dd_curl_set_headers, while dd_curl_reset_headers reads it with atomic_exchange. This replaces the plain write with atomic_store to match.

Note: the root cause of the crash in #3917 is not fully identified/fixed with this PR

Reviewer checklist

  • Test coverage seems ok.
  • Appropriate labels assigned.

@datadog-official

datadog-official Bot commented Jun 3, 2026

Copy link
Copy Markdown

Pipelines  Tests

Fix all issues with BitsAI

⚠️ Warnings

🚦 14 Pipeline jobs failed

DataDog/apm-reliability/dd-trace-php | test_extension_ci: [7.0]   View in Datadog   GitLab

See error 2 failed tests: Enabling dynamic instrumentation and Installing a live debugger span decoration probe.

DataDog/apm-reliability/dd-trace-php | test_extension_ci: [7.2]   View in Datadog   GitLab

See error Assertion failed in zend_hash.c at line 712: Assertion `((ht)->gc.refcount == 1) || ((ht)->u.flags & (1<<6))' failed due to unexpected state of HashTable.

DataDog/apm-reliability/dd-trace-php | test_extension_ci: [7.3]   View in Datadog   GitLab

See error Assertion failed in zend_hash.c:903 during dynamic instrumentation. Error: `(zend_gc_refcount(&(ht)->gc) == 1) || ((ht)->u.flags & (1<<6))`

View all 14 failed jobs.

ℹ️ Info

No other issues found (see more)

🧪 All tests passed
❄️ No new flaky tests detected

🔄 Datadog auto-retried 3 jobs - 3 passed on retry View in Datadog

🎯 Code Coverage (details)
Patch Coverage: 100.00%
Overall Coverage: 54.12% (-0.04%)

Useful? React with 👍 / 👎

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: 4a370a1 | Docs | Datadog PR Page | Give us feedback!

writer->headers is declared _Atomic but was stored via a plain
non-atomic assignment in dd_curl_set_headers, while dd_curl_reset_headers
reads it with atomic_exchange. Use atomic_store to match.
@realFlowControl realFlowControl force-pushed the florian/fix-3917-curl-headers-race branch from b2e6e2b to 4a370a1 Compare June 3, 2026 08:53
@realFlowControl realFlowControl changed the title fix: move static curl headers into writer thread to fix heap corruption race fix: use atomic_store for writer->headers assignment Jun 3, 2026
@realFlowControl realFlowControl changed the title fix: use atomic_store for writer->headers assignment fix(tracer): use atomic_store for writer->headers assignment Jun 3, 2026
@realFlowControl realFlowControl marked this pull request as ready for review June 3, 2026 08:54
@realFlowControl realFlowControl requested a review from a team as a code owner June 3, 2026 08:54

@bwoebi bwoebi left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I surely can approve this, but atomic_store only matters when it's sequenced relative to other operations.

This shouldn't fix anything at all.

@realFlowControl realFlowControl merged commit 654679c into master Jun 3, 2026
2131 of 2146 checks passed
@realFlowControl realFlowControl deleted the florian/fix-3917-curl-headers-race branch June 3, 2026 10:39
@github-actions github-actions Bot added this to the 1.21.0 milestone Jun 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants