Harden checks for iChID

[pljones]

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've verified that this Pull Request follows the general code principles
• I tested my code and it does what I want See above
• 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]

As a taste of another LLM's review style, here's what Cursor Composer 2.5 Fast wrote in response to

Take a look at the diff to main and let me know if

• the possibility of nullptr causing a crash is resolved by the guard in CJamSession::DisconnectClient
• moving the Mutex in CJamRecorder::OnDisconnected has any negative side effects
• if any other use of iChId needs guarding

---

Here is a review of commit 0fc8ee50 against the three questions.

1. Does the nullptr guard in DisconnectClient fix the crash?

Yes, for the scenarios that would actually dereference a null pointer.

Before the change, DisconnectClient unconditionally did:

vecptrJamClients[iChID]->Disconnect();

That crashes whenever the slot is already nullptr, which can happen in several legitimate orderings on the recorder thread:

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 CJamRecorder slots run on the dedicated recorder thread via queued connections, so they are serialized by the event loop.
• The unlocked reads of isRecording and currentSession are not a real TOCTOU race across threads; another slot cannot interleave on that thread between the check and the lock.
• Early return when currentSession == nullptr avoids taking the mutex unnecessarily (e.g. after OnEnd() has already torn down the session).
• ReaperProjectFromCurrentSession / AudacityLofFromCurrentSession are only called under the mutex from OnEnd(), 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: iChID is not validated against vecptrJamClients.size() / MAX_NUM_CHANNELS. That is pre-existing; the server is expected to only emit valid channel IDs.
• Server-side iChID use (server.cpp, etc.) is unrelated to this recorder race.
• chIdDisconnected is 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.

[Copilot]

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 ChIdMutex locking in CJamRecorder::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.

[ann0see]

If @mcfnord had a local reproducer, he should rerun it to test this.

[mcfnord]

(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 jamrecorder.o alone: control is this PR's base c862872e, arm is c4f0b45c.

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 + .rpp + .lof written by both binaries, byte-identical totals in two of three run pairs.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

[pljones]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

That really doesn't say anything. "No new comments". Is that "Everything is now wonderful" or "You didn't change anything as far as I can see"?

[mcfnord]

@dtinth suggested replacing CoPilot reviews with CodeRabbit. I've been clicking the down thumb.

[pljones]

This is a server crasher - can we get it reviewed and merged?

[softins]

Looks good to me. I haven't built or run it, but the changes look sensible and localised.

[ann0see]

One review nit, from tracing the disconnect-before-first-frame path (OnDisconnected -> DisconnectClient when a client connects and disconnects between two recorder ticks):

In the new nullptr branch of CJamSession::DisconnectClient, setting chIdDisconnected = iChID means CJamSession::Frame() will drop the first frame of whichever client next gets this channel ID (the "too late" guard at jamrecorder.cpp:257). In this branch no frame was ever recorded for the channel in this session, so nothing can be in flight - a bare return; would avoid dropping that frame.

Impact either way is a single block at session start; fine to merge as-is if you prefer keeping it simple.

Used AI: ox-alpha (model), opencode (harness)

[pljones]

I wish the AI tools would comment on the code with inline suggestions...

[dtinth]

CodeRabbit does comment as inline suggestions. Here's an example.

I think OpenCode with Ox Alpha will be able to comment using inline comments too, just need to explicitly prompt it.

[ann0see]

Ox Alpha is dead now anyway.

[mcfnord]

I wish the AI tools would comment on the code with inline suggestions...

Mine does when I ask it to. Yesterday I invited it to "hint" in just a single sentence primarily because the PR had Draft status. My AI also tracks preferences of individuals, so you'll probably get code suggestions going forward.

[pljones]

In this case, I can't put the AI comment from ox-alpha (model), opencode (harness) into context -- when I look at the code, it looks like it's already doing what it says to do...

[ann0see]

Let me dig out the full document...

[ann0see]

AI: Remote crash (null-pointer dereference) in jam recorder on disconnect-before-first-frame

• Severity: High (availability; conditional on recording being enabled)
• Type: Memory safety / unchecked null dereference reachable from network state machine (OWASP A06-ish coding defect)
• Status: Validated by code-path trace of the signal ordering. Not dynamically reproduced. Upstream fix pending: open PR #3909 ("Harden checks for iChID", pljones) adds exactly this guard in CJamSession::DisconnectClient; author requests verification before merge, mcfnord reports re-running a reproducer against the branch in-thread (checked 2026-08-25). No standalone issue exists on the tracker.
• Confidence: High for the defect; Medium-High for remote reachability (depends on timing across threads, but the window is a full timer period and the trigger is repeatable).

Location

• src/recorder/jamrecorder.cpp:590-604 (CJamRecorder::OnDisconnected) — checks currentSession != nullptr but not vecptrJamClients[iChID] != nullptr.
• src/recorder/jamrecorder.cpp:222-235 (CJamSession::DisconnectClient) — first statement dereferences: vecptrJamClients[iChID]->Disconnect();
• src/recorder/jamcontroller.cpp:169 — ClientDisconnected → OnDisconnected wiring.
• Emission point: src/server.cpp:941-953 — ClientDisconnected(iCurChanID) is emitted in the decode phase of OnTimer, before the audio-frame loop that would have produced the client's first AudioFrame.

Description

A session is created lazily on the first recorded frame (jamrecorder.cpp:616-632). Once a session exists, every connected channel is expected to have a non-null entry in vecptrJamClients. That invariant does not hold for a channel that connects and disconnects between two recorder frames:

1. Server has recording enabled (-R) and an active session (any streaming client, or attacker-created via the late-frame path below).
2. Attacker opens a new source port and sends one correctly-sized audio packet → channel allocated/connected.
3. Immediately sends CLM_DISCONNECTION from the same port → server.cpp:558-568 sets iConTimeOut = 1.
4. Next timer tick: decode phase runs before the AudioFrame loop; GetData() returns GS_CHAN_NOW_DISCONNECTED (channel.cpp:644-652) → emit ClientDisconnected(ch) and FreeChannel(ch) — no AudioFrame was ever emitted for this channel.
5. Recorder thread processes OnDisconnected(ch) while session is active → DisconnectClient dereferences vecptrJamClients[ch] == nullptr → SIGSEGV; server process dies.

Note OnFrame's own late-frame guard (chIdDisconnected, jamrecorder.cpp:257-262) only protects the opposite ordering (frame after disconnect), confirming this path was considered but the reverse order was not.

Impact

Remote, unauthenticated crash (DoS) of any recording-enabled server that currently hosts a session. Repeated trivially.

Fix direction

In CJamRecorder::OnDisconnected (or CJamSession::DisconnectClient), treat "no such client in current session" as a no-op:

if ( vecptrJamClients[iChID] == nullptr ) { return; }  // never seen this session

One-line change, off the real-time thread; no protocol impact.

Note on PR #3909's variant: it sets chIdDisconnected = iChID before returning in the null branch. That makes CJamSession::Frame()'s late-frame guard (jamrecorder.cpp:257-262) drop the first frame of the next client that gets this channel ID. In the null case no frame was ever recorded for this channel this session, so nothing can be in flight — a bare return; would be marginally more correct. Impact limited to one block (~few ms); harmless either way.

[ann0see]

A session is created lazily on the first recorded frame (jamrecorder.cpp:616-632). Once a session exists, every connected channel is expected to have a non-null entry in vecptrJamClients. That invariant does not hold for a channel that connects and disconnects between two recorder frames:

I assume that it thinks that the first frame (audio frame?) is skipped with this fix due to lazy loading setup?

I don't think it's really severe... Especially if we disconnect.