Harden checks for iChID - #3909
Conversation
|
As a taste of another LLM's review style, here's what Cursor Composer 2.5 Fast wrote in response to
Here is a review of commit 1. Does the
|
| Scenario | How nullptr arises |
|---|---|
| Disconnect before first recorded frame | Client connects and leaves before any AudioFrame creates a CJamClient |
Frame() already disconnected |
Address/channel change path calls DisconnectClient, then sets nullptr |
End() already cleaned up |
Last client triggers Stopped → OnEnd() → End() disconnects everyone; a queued ClientDisconnected arrives later |
| Duplicate disconnect | First call deletes the client; second call sees nullptr |
The guard prevents the crash in all of those cases.
2. Does moving the mutex in OnDisconnected have negative side effects?
No meaningful ones. The move looks intentional and aligns with OnFrame.
Old pattern:
QMutexLocker ...; // held for entire function
if (!isRecording) warn;
if (currentSession == nullptr) return;
DisconnectClient(...);New pattern:
if (!isRecording) warn;
if (currentSession == nullptr) return;
QMutexLocker ...; // held only for mutation
DisconnectClient(...);This mirrors OnFrame, which deliberately calls Start() (and thus OnEnd()) before taking ChIdMutex:
if ( !isRecording )
{
Start();
}
// ...
{
QMutexLocker mutexLocker ( &ChIdMutex );
currentSession->Frame ( iChID, name, address, numAudioChannels, data, iServerFrameSizeSamples );
}Why that is safe:
- All
CJamRecorderslots run on the dedicated recorder thread via queued connections, so they are serialized by the event loop. - The unlocked reads of
isRecordingandcurrentSessionare not a real TOCTOU race across threads; another slot cannot interleave on that thread between the check and the lock. - Early return when
currentSession == nullptravoids taking the mutex unnecessarily (e.g. afterOnEnd()has already torn down the session). ReaperProjectFromCurrentSession/AudacityLofFromCurrentSessionare only called under the mutex fromOnEnd(), so the new comments documenting that are accurate.
Pre-existing quirk (unchanged by this diff): the !isRecording branch logs a warning but does not return, so if currentSession were ever non-null while not recording, it would still call DisconnectClient. In practice OnEnd() clears both under the same lock, so that path should not occur.
3. Do any other iChID uses need guarding?
Within the recorder path, the mutex coverage is now complete. Every entry point that touches per-channel session state goes through ChIdMutex:
| Entry point | Mutex | Per-channel null safety |
|---|---|---|
OnFrame |
Yes, before Frame() |
Frame() checks vecptrJamClients[iChID] |
OnDisconnected |
Yes, before DisconnectClient() |
New guard in DisconnectClient() |
OnEnd / Start |
Yes, around session lifecycle | End() checks != nullptr before disconnect |
Internal Frame() → DisconnectClient() |
Caller (OnFrame) holds mutex |
Now guarded at callee |
CJamSession::DisconnectClient, Frame, and End rely on the caller holding ChIdMutex; all current callers do.
Nothing else in this diff scope obviously needs the same treatment. Worth knowing but outside this change:
- Bounds:
iChIDis not validated againstvecptrJamClients.size()/MAX_NUM_CHANNELS. That is pre-existing; the server is expected to only emit valid channel IDs. - Server-side
iChIDuse (server.cpp, etc.) is unrelated to this recorder race. chIdDisconnectedis a single int, not per-channel, which is a pre-existing design constraint for concurrent multi-client disconnects, not introduced here.
Bottom line: The branch looks sound. The nullptr guard fixes the crash; the mutex move is a safe refinement, not a regression; and no additional iChID guarding is required in the recorder beyond what you have, aside from the optional chIdDisconnected tweak mentioned above if you want to harden against duplicate disconnect + stale frame ordering.
There was a problem hiding this comment.
Pull request overview
This PR hardens the recorder’s handling of client disconnects to prevent a potential server crash during recording, by avoiding dereferencing a null client pointer and adjusting mutex usage around disconnect handling.
Changes:
- Add a null-pointer guard in
CJamSession::DisconnectClient()to prevent dereferencing an absent client instance. - Document mutex expectations for project/LOF generation helpers.
- Move
ChIdMutexlocking inCJamRecorder::OnDisconnected()to avoid locking when returning early.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if ( vecptrJamClients[iChID] == nullptr ) | ||
| { | ||
| return; | ||
| } |
There was a problem hiding this comment.
🤖 AI: Confirmed on this branch: 390 guarded early returns, 389 of them followed by a frame that allocated a fresh CJamClient; a second run, 220 and 219. Once the guard fires, the late frame is the normal continuation, not an edge case.
|
If @mcfnord had a local reproducer, he should rerun it to test this. |
| QThread::currentThread()->exit(); | ||
| } | ||
|
|
||
| // ChIdMutex held |
There was a problem hiding this comment.
Maybe rather @requires or something else that basically says that the mutex must be held here. Isn't there a way to check if the mutex is held code wise and else just return with error?
There was a problem hiding this comment.
Are you noticing that the reader locks, but the writers do not? I plan a PR for it, after this one is merged.
|
(I don't understand where it says "2 321 293"... missing commas? And what's unordinary about this recording? I can think of what it might mean.) 🤖 AI: The reproducer was re-run against this branch. One worktree, one object set, so the two binaries differ in Control: dies 4 of 4 runs, 1–2 s in, exit 139, the fault address from the original report. Arm: 2 321 293 cycles over 300 s, plus four further 20 s runs, without it. Ordinary recording is unaffected — two clients, 40 s, WAV + |
Short description of changes
Fixes a potential server crash. (Raised by @mcfnord)
CHANGELOG: Bugfix: Remove potential server crash when recording
Context: Fixes an issue?
N/A
Does this change need documentation? What needs to be documented and how?
No.
Status of this Pull Request
No functional change. I've not run the tests that confirmed the issue.
What is missing until this pull request can be merged?
Verification the fix on the branch works as intended would be good.
Checklist
I tested my code and it does what I wantSee above