From 86f08e88af7bcd2d5c5a86c4274b9dad76f0dd45 Mon Sep 17 00:00:00 2001 From: jrd Date: Sat, 8 Aug 2026 03:28:32 +0000 Subject: [PATCH 1/9] buffer.cpp: name the real bound behind the block-count divide The comment attributes the exactness of the block-count divide ( iInSize / iBlockSize ) to the sequence-number byte being "much smaller" than the coded audio. That ratio holds (worst reachable case 1/12), but it is not the condition. Measured with a harness linking the unmodified buffer.cpp: the divide is exact across the whole protocol-reachable domain (59,973 blocksize x factor pairs, blocksize 9..19999, factor {1,2,4}, zero miscounts) and first fails exactly at factor == blocksize, where AddressSanitizer reports a heap-buffer-overflow READ in the window-move invalidate path and one block of bytes from behind the packet reaches the jitter buffer. What keeps that corner unreachable is the properties validator in protocol.cpp (base network packet size >= 10, hence blocksize >= 9; factor restricted to {1, 2, 4}), two files away. The rewritten comment names the bound and the cross-file coupling that enforces it. Audit row 1; comments only, no code change. Co-Authored-By: Claude Fable 5 --- src/buffer.cpp | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/src/buffer.cpp b/src/buffer.cpp index 441b4121e4..a707137b91 100644 --- a/src/buffer.cpp +++ b/src/buffer.cpp @@ -164,8 +164,16 @@ bool CNetBuf::Put ( const CVector& vecbyData, int iInSize ) return false; } - // to get the number of input blocks we assume that the number of bytes for - // the sequence number is much smaller than the number of coded audio bytes + // This divide is exact if and only if iNumBlocks * iNumBytesSeqNum < iBlockSize, + // since the actual input is iNumBlocks * ( iBlockSize + iNumBytesSeqNum ) bytes. The + // sequence number merely being "much smaller" than the coded audio is not the + // condition: at iNumBlocks == iBlockSize the count comes out one too high whatever + // the ratio is. The bound is not enforced here but by the properties validator in + // protocol.cpp, EvaluateNetwTranspPropsMes, which rejects a base network packet size + // below CELT_MINIMUM_NUM_BYTES (10) and a block size factor outside + // { FRAME_SIZE_FACTOR_PREFERRED, _DEFAULT, _SAFE }. With iNumBytesSeqNum == 1 and + // iBlockSize == iBaseNetworkPacketSize - 1 (channel.cpp), the worst reachable case is + // a factor of 4 against a block size of 9. const int iNumBlocks = /* floor */ ( iInSize / iBlockSize ); // copy new data in internal buffer From 2b63f9a2f34a457ffb2ac3f4d42265507bf274c9 Mon Sep 17 00:00:00 2001 From: jrd Date: Sat, 8 Aug 2026 03:28:32 +0000 Subject: [PATCH 2/9] buffer.cpp: correct the wrap-detection horizon: 128 counts, not 256 Two numbers in this comment are off, both in the safe direction. The detection horizon is 128 counts, not 256: iSeqNumDiff is folded into the signed range [-128, 127] a few lines above, so a packet more than 128 counts late is mistaken for an early one. And ">100 ms" understates the horizon: the sequence advance rate was measured on 8 real client/server configurations with a CLOCK_MONOTONIC probe at 375.0 counts/s for the 128-sample default and 750.0 counts/s for 64-sample OPUS64 frames (48000/frame to four significant figures; 64 samples is the frame floor, so 750/s is the fastest possible advance). 128 counts is therefore 171 ms at the fastest frame rate and 341 ms at the default. The conclusion -- such a packet indicates a bad network situation and is useless anyway -- survives with the corrected numbers. Audit row 2; comments only, no code change. Co-Authored-By: Claude Fable 5 --- src/buffer.cpp | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/buffer.cpp b/src/buffer.cpp index a707137b91..72982ec190 100644 --- a/src/buffer.cpp +++ b/src/buffer.cpp @@ -198,9 +198,12 @@ bool CNetBuf::Put ( const CVector& vecbyData, int iInSize ) iSeqNumDiff -= 256; } - // The 1-byte sequence number wraps around at a count of 256. So, if a packet is delayed - // further than this we cannot detect it. But it does not matter since such a packet is - // more than 100 ms delayed so we have a bad network situation anyway. Therefore we + // The 1-byte sequence number is folded into a signed difference above, so a + // delayed packet is mistaken for an early one once it is more than 128 counts + // late, not 256. At the fastest possible frame rate that is still 171 ms + // (64-sample frames, 750 counts/s) and at the default frame size 341 ms + // (128-sample frames, 375 counts/s), so such a packet is long useless either + // way and we have a bad network situation anyway. Therefore we // assume that the sequence number difference between the received and local counter is // correct. The idea of the following code is that we always move our "buffer window" so // that the received packet fits into the buffer. By doing this we are robust against From 3a4a7159c151a671acc4ac21c1469cb29aa9c7ac Mon Sep 17 00:00:00 2001 From: jrd Date: Sat, 8 Aug 2026 03:28:32 +0000 Subject: [PATCH 3/9] buffer.cpp: document the second loss channel beside the window move The tradeoff the comment admits is real and is now measured: across a 3.2M-call Get() sweep, zero late packets were discarded (the "never throw away" half holds) and 115,448 valid blocks were invalidated by window moves, every one of them never played. But the window move is not the only way a valid block dies: 157,758 blocks were overwritten in their slot before playout in the same corpus. At a buffer length of 1 the overwrite channel is the only one (a window move cannot invalidate anything there); from length 3 upward the window-move channel dominates. An ablation arm that refuses late packets shows the admitted downside is the price of the buffer working at all: without the window move, occupancy pins at 1.366 blocks at every size and dropouts rise from 0.012% to 6.704% (sigma = 1 frame of jitter, buffer length >= 4) -- the late packet is the only thing that rewinds the playout clock, so the window move is the sole mechanism by which the buffer acquires depth. The added text documents the second loss channel. Audit row 4; comments only, no code change. Co-Authored-By: Claude Fable 5 --- src/buffer.cpp | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/buffer.cpp b/src/buffer.cpp index 72982ec190..93be5d52fa 100644 --- a/src/buffer.cpp +++ b/src/buffer.cpp @@ -210,7 +210,11 @@ bool CNetBuf::Put ( const CVector& vecbyData, int iInSize ) // sample rate offsets between client/server or buffer glitches in the audio driver since // we adjust the window. The downside is that we never throw away single packets which arrive // too late so we throw away valid packets when we move the "buffer window" to the delayed - // packet and then back to the correct place when the next normal packet is received. But + // packet and then back to the correct place when the next normal packet is received. + // Note that this is not the only way a valid block is lost: a block can also be + // overwritten in its slot before it is played out. That second channel is the only + // one that exists at a buffer length of 1, while the window move dominates from a + // buffer length of 3 upwards. But // tests showed that the new buffer strategy does not perform worse than the old jitter // buffer which did not use any sequence number at all. if ( iSeqNumDiff < 0 ) From 64af334b02c6d6f2141feb4b62924dac412eb208 Mon Sep 17 00:00:00 2001 From: jrd Date: Sat, 8 Aug 2026 03:28:33 +0000 Subject: [PATCH 4/9] client.cpp: non-zero iActiveChannels does not imply a server restart Measured against one server process that never restarted (same pid verified alive after the run): a connected client that goes silent for 40 s -- longer than CON_TIME_OUT_SEC_MAX = 30 s -- is dropped by the receive-timeout path in CChannel::GetData(), and its next packet is treated as a new connection, producing a second CLIENT_ID with iActiveChannels != 0. The log of the single server process reads connected, then idling, then connected again. So "the server must have been restarted on the fly" names a cause that is not the only reachable one: a traffic gap longer than the receive timeout is enough. The remedy ( ClearClientChannels ) is correct in both cases; only the stated cause was wrong. Audit row 26; comments only, no code change. Co-Authored-By: Claude Fable 5 --- src/client.cpp | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/client.cpp b/src/client.cpp index e1532c96b2..532b6597bc 100644 --- a/src/client.cpp +++ b/src/client.cpp @@ -999,9 +999,12 @@ void CClient::OnControllerInMuteMyself ( bool bMute ) void CClient::OnClientIDReceived ( int iServerChanID ) { - // if we have just connected to a running server, iActiveChannels will be 0 - // if iActiveChannels is not 0, the server must have been restarted on the fly - // in that case, channels might have changed, so clear our list to get it afresh. + // If we have just connected to a running server, iActiveChannels will be 0. + // If it is not 0, the server has begun a NEW connection for us while this client kept + // running. A restart is only one way that happens: the server also drops a channel whose + // receive timeout expires ( CON_TIME_OUT_SEC_MAX, channel.h ) and treats the next packet + // from the same peer as a new connection, so a traffic gap of longer than that is enough. + // Either way the channel list we hold may be stale, so clear it and get it afresh. if ( iActiveChannels != 0 ) { qInfo() << "> Server restarted?"; From 3166c5979465d6b5c69bb99a7122bd5755299ab0 Mon Sep 17 00:00:00 2001 From: jrd Date: Sat, 8 Aug 2026 03:28:33 +0000 Subject: [PATCH 5/9] server.cpp: drop the outdated QtConcurrent 5-parameter rationale This path no longer uses QtConcurrent::run at all: the decode workers are dispatched through CThreadPool::enqueue ( threadpool.h ), which is variadic. QtConcurrent survives only in connectdlg.cpp. The cited Qt5 limit is real and is exactly 5 -- measured against Qt 5.15.13: a free function compiles with 5 arguments and fails with 6; a member call compiles with 4 arguments after the object and fails with 5 -- but the call site passes 4 arguments after the callable, so the limit would not bind even on the old path. Premise and consequence are both obsolete; the replacement note records that nothing in the threading forces the flag to be a member. Audit row 44; comments only, no code change. Co-Authored-By: Claude Fable 5 --- src/server.cpp | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/server.cpp b/src/server.cpp index a49eab776c..a1e67b6704 100644 --- a/src/server.cpp +++ b/src/server.cpp @@ -662,7 +662,11 @@ void CServer::OnTimer() bool bUseMT = false; int iNumBlocks = 0; // init number of blocks for multithreading int iMTBlockSize = 0; // init block size for multithreading - bChannelIsNowDisconnected = false; // note that the flag must be a member function since QtConcurrent::run can only take 5 params + // The flag is a member variable, not a local. Nothing in the threading forces that: the + // decode workers below are dispatched through CThreadPool::enqueue ( threadpool.h ), which is + // variadic and takes any number of arguments. The five-argument cap this note used to cite is + // Qt5 QtConcurrent::run's, and that path is no longer used here. + bChannelIsNowDisconnected = false; { // Make put and get calls thread safe. From d6e66585c153cfc68b9e813324b5e1ff220d6931 Mon Sep 17 00:00:00 2001 From: jrd Date: Sat, 8 Aug 2026 03:28:33 +0000 Subject: [PATCH 6/9] server.cpp: state the real reason the disconnect flag needs no mutex "each thread can only set it to true and never to false" is contradicted by the clear further up in the same function: OnTimer sets bChannelIsNowDisconnected = false once per tick. In the default configuration multithreading is off ( bUseMultithreading defaults to false in main.cpp, and the server also disables it itself when only one core is found ), the decode then runs inline, and the same thread writes both values. The access is safe, but by ordering, not by one-way writes: the clear is sequenced before any decode work is enqueued on the thread pool, and the flag is read back only after every future has been waited on -- the join supplies the ordering. The store itself has been std::atomic since e6eed12f. The rewritten comment states that mechanism, which is also the constraint a future change must keep: moving the clear into the concurrent region would break it. Audit row 62; comments only, no code change. Co-Authored-By: Claude Fable 5 --- src/server.cpp | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/server.cpp b/src/server.cpp index a1e67b6704..d6f575be02 100644 --- a/src/server.cpp +++ b/src/server.cpp @@ -948,9 +948,14 @@ void CServer::DecodeReceiveData ( const int iChanCnt, const int iNumClients ) FreeChannel ( iCurChanID ); // note that the channel is now not in use - // note that no mutex is needed for this shared resource since it is a - // std::atomic write (not a read-modify-write operation) and also each - // thread can only set it to true and never to false + // Note that no mutex is needed for this shared resource: the store is a + // std::atomic write, not a read-modify-write operation. It is NOT true that + // a thread can only ever set this to true: OnTimer clears it to false once + // per tick, and in the default configuration ( multithreading off ) the + // decode runs inline, so the same thread writes both values. What makes the + // access safe is the ordering -- the clear is sequenced before any decode + // work is handed to the thread pool, and the flag is read back only after + // every future has been waited on. bChannelIsNowDisconnected = true; // since the channel is no longer in use, we should return From 78ed25f3cc1f73fd93504455881e7f64057f7c44 Mon Sep 17 00:00:00 2001 From: jrd Date: Sat, 8 Aug 2026 03:41:43 +0000 Subject: [PATCH 7/9] channel.cpp: comment the true per-packet overhead (measured 46/66 B, not 77) The 28 + 26 + 23 = 77-byte overhead is a PPPoE-over-ATM DSL model. A packet capture on a real internet path (~90k packets) measures 20-byte IP headers and a 14-byte Ethernet header with no PPPoE, ATM, or VLAN present: 46 bytes over IPv4 and 66 over IPv6, the latter a header the constant does not account for at all. So GetUploadRateKbps overstates true IPv4/Ethernet cost by roughly 15% to 50% across the settings, worst at the lowest bit-rate where the bandwidth-constrained user it is meant to help is most affected. The code is unchanged here; the comment now records the real overhead and marks the constant FIXME, because what the figure should be is a design decision (which layer to bill) for the maintainers. Audit row 8; comment describes the current DEFECT, code unchanged (fix is separate work). Co-Authored-By: Claude Fable 5 --- src/channel.cpp | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/src/channel.cpp b/src/channel.cpp index 7755b7ec92..2465e2022c 100644 --- a/src/channel.cpp +++ b/src/channel.cpp @@ -716,12 +716,20 @@ int CChannel::GetUploadRateKbps() { const int iAudioSizeOut = iNetwFrameSizeFact * iAudioFrameSizeSamples; - // we assume that the UDP packet which is transported via IP has an - // additional header size of ("Network Music Performance (NMP) in narrow - // band networks; Carot, Kraemer, Schuller; 2006") + // The 77-byte per-packet overhead below (28 + 26 + 23) models a PPPoE-over-ATM DSL + // access path, following ("Network Music Performance (NMP) in narrow band networks; + // Carot, Kraemer, Schuller; 2006"): // 8 (UDP) + 20 (IP without optional fields) = 28 bytes // 2 (PPP) + 6 (PPPoE) + 18 (MAC) = 26 bytes // 5 (RFC1483B) + 8 (AAL) + 10 (ATM) = 23 bytes + // A packet capture on a real internet path measures 46 bytes over IPv4/Ethernet + // (20 IP + 8 UDP + 14 Ethernet, no PPPoE, no ATM, no VLAN) and 66 over IPv6, so this + // constant is 31 bytes too large for IPv4 and 20 bytes too small for the IPv6 header + // it never accounts for. The figure a client can actually justify is 28 (IPv4) or + // 48 (IPv6) at L3/L4, since the access encapsulation is invisible to the endpoint. + // As it stands the returned rate overstates real IPv4/Ethernet cost by roughly 15% to + // 50%, worst at the lowest bit-rate settings. FIXME: the constant should reflect a + // measurable path, but the "right" figure is a design decision (which layer to bill). return ( iNetwFrameSize * iNetwFrameSizeFact + 28 + 26 + 23 /* header */ ) * 8 /* bits per byte */ * SYSTEM_SAMPLE_RATE_HZ / iAudioSizeOut / 1000; } From cca203b0f264e35bdb42fe49f0c46de8c504fcba Mon Sep 17 00:00:00 2001 From: jrd Date: Sat, 8 Aug 2026 03:41:43 +0000 Subject: [PATCH 8/9] server.cpp: comment that the realtime path still allocates (send-path copy) "no memory must be allocated" in the realtime path is the intent, not the current behaviour. With a malloc/free interposer keyed by calling stack, a running server with 3 clients allocated 73,308 times under OnTimer over 96 s; 91% are CSocket::SendPacket, where taking the argument as a const CVector and casting the const away deep-copies every outgoing datagram -- one malloc, one memmove, one free per audio packet (packet count and malloc count match exactly). CNetBufWithStats::Init also allocates on the path. Attribution is by stack, not thread name: OnTimer runs on the Qt event-loop thread through a queued connection, not the TimeCritical timer thread, and matching the thread name undercounts. Code unchanged; the comment now states that the guarantee is not currently met and marks the copies FIXME. Audit row 20; comment describes the current DEFECT, code unchanged (fix is separate work). Co-Authored-By: Claude Fable 5 --- src/server.cpp | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/src/server.cpp b/src/server.cpp index d6f575be02..60c0d58a83 100644 --- a/src/server.cpp +++ b/src/server.cpp @@ -165,10 +165,17 @@ CServer::CServer ( const int iNewMaxNumChan, iServerFrameSizeSamples = SYSTEM_FRAME_SIZE_SAMPLES; } - // To avoid audio clitches, in the entire realtime timer audio processing - // routine including the ProcessData no memory must be allocated. Since we - // do not know the required sizes for the vectors, we allocate memory for - // the worst case here: + // To avoid audio glitches, the realtime timer audio routine ( OnTimer -> + // ProcessData ) should allocate no memory. The worst-case vectors below are + // pre-sized here for that reason. Note that the goal is not currently met: the + // send path still allocates once per outgoing audio packet. CSocket::SendPacket + // takes its argument as a const CVector, and the ( CVector ) cast that + // strips the const deep-copies the whole datagram -- one malloc, one memmove, one + // free for every packet sent, on this same timer thread ( attribute by stack, not + // by thread name: OnTimer runs on the Qt event-loop thread via a queued connection ). + // CNetBufWithStats::Init also allocates on this path. FIXME: remove these before + // relying on the no-allocation guarantee. Since we do not know the required sizes + // for the vectors, we allocate memory for the worst case here: // allocate worst case memory for the temporary vectors vecChanIDsCurConChan.Init ( iMaxNumChannels ); From 731b309c0555d1a9132eac91e7acd719f4902852 Mon Sep 17 00:00:00 2001 From: jrd Date: Sat, 8 Aug 2026 03:41:43 +0000 Subject: [PATCH 9/9] client.cpp: comment that FindClientChannel here is not guaranteed to return 0 "should always return channel 0" is false for a server-controlled input. FindClientChannel returns INVALID_INDEX (-1) for any server channel id >= MAX_NUM_CHANNELS (150); EvaluateClientIDMes validates only the 1-byte length, never the range, so a server can send 150..255. Confirmed on the wire with a fake server and a real headless client: a UBSan build reports "index -1 out of bounds for type 'CClientChannel [150]'" in SetRemoteChanGain (reached via this line when the mute-me flag is set) and again in the gain/pan timer, and the out-of-bounds read is forwarded back onto the network as a CHANNEL_GAIN message. AddressSanitizer stays silent because index -1 lands on a preceding member inside the same allocation, with no redzone -- so a clean ASan run here is not exoneration. Code unchanged; the comment now states the hazard and marks the missing range check FIXME. Audit row 27; comment describes the current DEFECT, code unchanged (fix is separate work). Co-Authored-By: Claude Fable 5 --- src/client.cpp | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/client.cpp b/src/client.cpp index 532b6597bc..c35d7807e4 100644 --- a/src/client.cpp +++ b/src/client.cpp @@ -1012,7 +1012,14 @@ void CClient::OnClientIDReceived ( int iServerChanID ) } // allocate and map client-side channel 0 - int iChanID = FindClientChannel ( iServerChanID, true ); // should always return channel 0 + // In normal operation this returns channel 0. It is NOT guaranteed: FindClientChannel + // returns INVALID_INDEX ( -1 ) for any iServerChanID >= MAX_NUM_CHANNELS ( 150 ), and a + // server-sent CLIENT_ID is only length-checked ( EvaluateClientIDMes, protocol.cpp ), never + // range-checked, so a malicious or buggy server can deliver an id of 150..255. The result is + // used below without a guard; with the headless mute-me-in-personal-mix flag set it reaches + // SetRemoteChanGain, which dereferences &clientChannels[-1]. FIXME: reject iServerChanID + // outside [0, MAX_NUM_CHANNELS) here or in EvaluateClientIDMes before this line. + int iChanID = FindClientChannel ( iServerChanID, true ); // for headless mode we support to mute our own signal in the personal mix // (note that the check for headless is done in the main.cpp and must not