Skip to content

proxy-types: prevent race condition when reading client.m_context.connection#293

Closed
ViniciusCestarii wants to merge 1 commit into
bitcoin-core:masterfrom
ViniciusCestarii:fix-clientdestroy-race
Closed

proxy-types: prevent race condition when reading client.m_context.connection#293
ViniciusCestarii wants to merge 1 commit into
bitcoin-core:masterfrom
ViniciusCestarii:fix-clientdestroy-race

Conversation

@ViniciusCestarii

Copy link
Copy Markdown
Contributor

Fixes data race on reading m_context.connection in clientDestroy() in proxy-types.h.

The read was unsynchronized while the disconnect cleanup callback (registered with addSyncCleanup in the ProxyClientBase constructor) clears the same field under m_context.loop->m_mutex. When a ProxyClient is destroyed on the async cleanup thread while the connection is torn down on the event loop thread, the two accesses race.

In practice the race is not harmfull because the value only selects a log message but it can fail the TSan build, which makes the CI fail.

The regression test added in 90be835 let the disconnect-during-cleanup path run, exposing the race under TSan.

Fix: lock m_context.loop->m_mutex around the read, matching the lock the disconnect callback already uses for the write.

@DrahtBot

DrahtBot commented Jun 9, 2026

Copy link
Copy Markdown

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.
A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #286 (proxy-client: fix TSan data race in clientDestroy by ryanofsky)
  • #269 (proxy: add local connection limit to ListenConnections by enirox001)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@ViniciusCestarii

Copy link
Copy Markdown
Contributor Author

Just looked #286. Closing this pr since this is a duplicate

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