Skip to content

Ensure ChannelInfo mutex taken - #3930

Open
pljones wants to merge 1 commit into
jamulussoftware:mainfrom
pljones:ChannelInfo-locks
Open

Ensure ChannelInfo mutex taken#3930
pljones wants to merge 1 commit into
jamulussoftware:mainfrom
pljones:ChannelInfo-locks

Conversation

@pljones

@pljones pljones commented Aug 29, 2026

Copy link
Copy Markdown
Collaborator

Short description of changes

Addresses a missing Mutex issue.

CHANGELOG: SKIP

Context: Fixes an issue?

Reported server crash bug.

Does this change need documentation? What needs to be documented and how?

No

Status of this Pull Request

Needs fix verified by reporter.

What is missing until this pull request can be merged?

Review and testing.

Checklist

  • I've verified that this Pull Request follows the general code principles
  • I tested my code and it does what I want
  • My code follows the style guide
  • I waited some time after this Pull Request was opened and all GitHub checks completed without errors.
  • I've filled all the content above

@pljones pljones added this to the Release 4.0.0 milestone Aug 29, 2026
@pljones pljones self-assigned this Aug 29, 2026
@pljones pljones added the bug Something isn't working label Aug 29, 2026
@pljones pljones added this to Tracking Aug 29, 2026
@pljones pljones added AI AI generated or potentially AI generated backport_required A change to main that needs fix on an existing release. labels Aug 29, 2026
@github-project-automation github-project-automation Bot moved this to Triage in Tracking Aug 29, 2026
@pljones
pljones force-pushed the ChannelInfo-locks branch from 1fed382 to 75379ef Compare August 29, 2026 16:49
@mcfnord

mcfnord commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

🤖 AI: Confirmed against the reported crash. Under AddressSanitizer the reproducer — repeated short-lived connections to a server started with -R — reports a heap-use-after-free on unpatched main: the mix timer copies the channel name (CServer::AudioFrame) while the socket thread frees it through PutAudioDataFindChannelInitChannelResetInfo(). On this branch the same run stayed clean across 128056 recorded connections over three reps, zero reports; the unpatched control reported at 69736.

One residual on the same field: bIsIdentified is now written under Mutex in ResetInfo() and SetChanInfo(), but the mixer reads it off that lock in PrepAndSendPacket (channel.cpp:719). ThreadSanitizer reports that read racing the ResetInfo() write at channel.cpp:364 — socket thread against the OnTimer mix thread — when a channel slot is recycled. Making it atomic, as bIsEnabled directly above it already is, closes that without putting Mutex on the per-frame path:

-    bool              bIsIdentified;
+    std::atomic<bool> bIsIdentified;

@pljones
pljones force-pushed the ChannelInfo-locks branch from 75379ef to 5ed0004 Compare August 30, 2026 09:49
@pljones pljones moved this from Triage to Waiting on Team in Tracking Aug 30, 2026
@mcfnord mcfnord mentioned this pull request Aug 30, 2026
5 tasks
Comment thread src/channel.h
void ResetInfo(); // reset does not emit a message
QString GetName();
void SetChanInfo ( const CChannelCoreInfo& NChanInf );
CChannelCoreInfo GetChanInfo();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Just double checking here. This was changed from a CChannelCoreInfo& to CChannelCoreInfo so that instead of returning a reference to the original, it returns a copy that was obtained under mutex protection?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI AI generated or potentially AI generated backport_required A change to main that needs fix on an existing release. bug Something isn't working

Projects

Status: Waiting on Team

Development

Successfully merging this pull request may close these issues.

3 participants