RakVoice: add relay mode for client-server voice - #42
Conversation
RakVoice sends voice peer-to-peer, which a dedicated-server game cannot
use: clients are connected only to the server, never to each other. This
adds a relay path where clients send frames to a host that forwards them
without decoding, so the server stays authoritative over who hears whom
without paying for a codec.
Relay frames carry the talker's GUID in the header, since the sender is
now the relay rather than the speaker:
[id][format version][origin guid][channel id][sequence][opus payload]
The format version is an escape hatch: a future layout change is rejected
by today's build instead of being misparsed. Every offset derives from the
one before it, so the writer and both readers cannot drift apart.
Also in relay mode:
- channels are keyed by the origin GUID rather than packet->guid, so
each speaker gets its own decoder instead of collapsing into one
- SetPerSpeakerOutput lets a caller pull per-speaker PCM instead of the
pre-mixed buffer, which is what makes 3D positioning possible
- idle relay channels are reaped; OnClosedConnection never fires for
them because relay speakers are peers of the host, not of us
Two latent bugs surfaced while wiring this up and are fixed here, both
reachable without any of the above:
- the constructor never initialised zeroBufferedOutput or
bufferedOutputCount, so Update() read indeterminate values and could
write through a null bufferedOutput on any attached-but-uninitialised
instance
- CloseVoiceChannel sent ID_RAKVOICE_CLOSE_CHANNEL unconditionally, so
a peer that never opened a channel still received one on disconnect
Peer-to-peer behaviour is unchanged when relay mode is off; the decode
path was extracted into DecodeIntoChannel and is statement-identical.
Adds 15 unit tests covering the wire layout and ReadRelayOrigin's handling
of hostile input, including the origin/sender mismatch a host must reject
to prevent one client speaking as another.
Note: ID_RAKVOICE_RELAY_DATA is inserted after ID_RAKVOICE_DATA and shifts
every subsequent id. Peers must be rebuilt together.
|
Warning Review limit reached
Next review available in: 52 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
WalkthroughRakVoice adds a versioned relay-frame protocol, relay-host forwarding, relay-client decoding by origin GUID, per-speaker output APIs, channel cleanup, encoder bitrate control, and unit tests for frame parsing and sizing. ChangesRakVoice relay voice
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant VoiceClient
participant RelayHost
participant SpeakerClient
VoiceClient->>RelayHost: send versioned Opus relay frame
RelayHost->>RelayHost: read stamped origin GUID
RelayHost->>SpeakerClient: forward relay frame unchanged
SpeakerClient->>SpeakerClient: create origin channel and decode payload
SpeakerClient-->>VoiceClient: expose decoded per-speaker frame
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Source/src/RakVoice.cpp (1)
880-895: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
OnVoiceDatadecodes without validatingpacket->length.An
ID_RAKVOICE_DATApacket of 1 or 2 bytes makes thememcpyat Line 890 read past the buffer, andpacket->length - headerSizeunderflows to a huge unsigned value that is then passed toopus_decodeas the payload length — a remote-triggerable out-of-bounds read. The relay path (OnRelayVoiceData) already guards this; the direct path should too.🐛 Proposed guard
index = voiceChannels.GetIndexFromKey(packet->guid, &objectExists); - if (objectExists) + if (objectExists && packet->length > (unsigned)headerSize) { memcpy(&packetMessageNumber, packet->data + 1, sizeof(unsigned short));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Source/src/RakVoice.cpp` around lines 880 - 895, Add a packet-length guard at the start of RakVoice::OnVoiceData before reading the message number or decoding audio, requiring packet->length to be at least headerSize. Return immediately for shorter packets, while preserving the existing voiceChannels lookup and DecodeIntoChannel flow for valid packets.
🧹 Nitpick comments (1)
Source/include/mafianet/MessageIdentifiers.h (1)
199-201: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider claiming a reserved slot instead of inserting mid-enum.
Inserting
ID_RAKVOICE_RELAY_DATAhere renumbers every ID after it (ID_AUTOPATCHER_*throughID_RESERVED_9), which breaks the wire protocol for any peer or persisted capture built against the old header.ID_RESERVED_3exists precisely for additive IDs and would keep numbering stable.If the renumbering is intentional (the PR notes peers must be rebuilt together), this is fine as-is — just confirm no external tooling or non-rebuilt component decodes these IDs.
♻️ Alternative that preserves existing IDs
- /// RakVoice relay frame: origin GUID + format version + channel + sequence - /// + Opus payload. Routed through a relay host rather than peer-to-peer. - ID_RAKVOICE_RELAY_DATA,and instead:
- ID_RESERVED_3, + /// RakVoice relay frame: origin GUID + format version + channel + sequence + /// + Opus payload. Routed through a relay host rather than peer-to-peer. + ID_RAKVOICE_RELAY_DATA,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Source/include/mafianet/MessageIdentifiers.h` around lines 199 - 201, The new ID_RAKVOICE_RELAY_DATA enum entry renumbers all subsequent message identifiers and can break compatibility with existing peers and captures. Move it into the existing ID_RESERVED_3 slot, preserving the numeric values of ID_AUTOPATCHER_* through ID_RESERVED_9, and update the surrounding comments to describe the relay frame.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Source/include/mafianet/RakVoice.h`:
- Around line 136-141: Update the SetEncoderBitrate documentation to state that
the bitrate is applied to both currently open channels and channels opened
later. Clarify that bitsPerSecond 0 leaves Opus at its default only for
subsequently opened channels, while existing channels retain their current
encoder setting.
In `@Source/src/RakVoice.cpp`:
- Around line 944-971: Limit relay-originated channel creation in
RakVoice::GetOrCreateChannel by enforcing a maximum number of concurrent relay
speakers before calling OpenChannel for a new origin. If the cap is reached,
return nullptr so OnRelayVoiceData drops the frame; preserve existing channels
and normal channel creation while below the limit, and ensure the cap is applied
only to relay speaker allocation.
- Around line 182-199: Harden RakVoice::RelayFrame before forwarding: reject
null recipients, packets lacking data, packets whose first byte is not
ID_RAKVOICE_RELAY_DATA, packets exceeding RAKVOICE_RELAY_HEADER_SIZE +
RAKVOICE_MAX_OPUS_PACKET_SIZE, and packets whose ReadRelayOrigin result differs
from packet->guid. Only enter the recipient loop after all checks pass,
preserving the existing forwarding behavior for valid frames.
---
Outside diff comments:
In `@Source/src/RakVoice.cpp`:
- Around line 880-895: Add a packet-length guard at the start of
RakVoice::OnVoiceData before reading the message number or decoding audio,
requiring packet->length to be at least headerSize. Return immediately for
shorter packets, while preserving the existing voiceChannels lookup and
DecodeIntoChannel flow for valid packets.
---
Nitpick comments:
In `@Source/include/mafianet/MessageIdentifiers.h`:
- Around line 199-201: The new ID_RAKVOICE_RELAY_DATA enum entry renumbers all
subsequent message identifiers and can break compatibility with existing peers
and captures. Move it into the existing ID_RESERVED_3 slot, preserving the
numeric values of ID_AUTOPATCHER_* through ID_RESERVED_9, and update the
surrounding comments to describe the relay frame.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3c655e2e-b38e-40f3-8a26-2362f511e1cc
📒 Files selected for processing (4)
Source/include/mafianet/MessageIdentifiers.hSource/include/mafianet/RakVoice.hSource/src/RakVoice.cppTests/Unit/RakVoiceRelayTests.cpp
Addresses review findings on the relay PR. All four were reproduced against the code before fixing. RelayFrame validated centrally. The origin GUID is written by the client, so a modified client could stamp another player's GUID and be relayed as them. The Framework host checks this before calling, but nothing in the public API said it had to, and a host that forgets is silently exploitable. RelayFrame now rejects an origin that is not the transport-authenticated sender, frames outside the valid size range, a wrong packet id, and a null or empty recipient list. Callers may still re-check; they can no longer forget. Concurrent relay speakers are capped. Each new origin allocates a decoder plus two ring buffers and is held for the reap timeout, so fabricated origins drove unbounded allocation. RAKVOICE_MAX_RELAY_SPEAKERS is a memory backstop, not an application policy -- a caller wanting fewer simultaneous speakers should cap above this layer, where it can choose which ones to keep. Speakers already established are not starved by a flood of fabricated ones. Fixes a pre-existing remote out-of-bounds read in OnVoiceData, which is reachable on the plain peer-to-peer path with no relay involved. A one or two byte ID_RAKVOICE_DATA packet made the header memcpy read past the buffer, and packet->length - headerSize underflowed to a huge unsigned value passed to opus_decode as the payload length. Present before this branch; fixed here because this is the repository that owns it. Corrects the SetEncoderBitrate docs, which claimed the bitrate applied only to channels opened later. It is also applied to every open encoding channel, and passing 0 does not restore the Opus default on existing encoders. Adds 3 tests for the speaker cap, driven through a test-only subclass since GetOrCreateChannel needs Init() but not a live peer. Mutation checked: removing the cap fails StopsAllocatingPastTheSpeakerCeiling. 144/144 pass.
|
All four findings reproduced against the code and fixed in 8138508. None were false positives.
Unbounded channel allocation (Major) — fixed. Added
144/144 tests pass (126 before this PR). The cap fix is mutation-checked — removing the bound makes |
Audit of the remaining packet entry points after the OnVoiceData out-of-bounds read, looking for the same class of bug. All four below are pre-existing on master, reachable by any connected peer, and unrelated to relay mode. OpenChannel asserted on a remotely supplied sample rate. RakAssert is a real assert() in debug builds, so any peer could abort a debug server by sending a channel-open packet with a bogus rate. Rejects silently now, as the surrounding error paths already did. OpenChannel used the sample rate without checking the read succeeded. A packet too short to carry it left the value indeterminate and assigned it to remoteSampleRate before validating. The value is now initialised and the read checked. OnReceive dispatched on data[0] without a length check, so a zero-length packet read past the buffer before any handler could validate. OnOpenChannelReply lacked the initialisation guard OnOpenChannelRequest has. An unsolicited reply on an uninitialised instance opened a channel with bufferSizeBytes of 0 and allocated empty rings. Off the relay path the encoder create failed first and bailed, which is why this stayed latent -- a decode-only relay channel skips that create entirely. Adds 5 tests covering rejected and accepted sample rates, truncated packets, the pre-init reply, and the empty-packet dispatch. Note the truncated-rate test pins the outcome rather than the mechanism: with the value initialised, removing the read check alone does not change behaviour, so that check is defensive rather than load-bearing. 149/149 pass.
|
Since the
Added 5 tests: rejected and accepted sample rates, truncated packet, pre-init reply, empty-packet dispatch. 149/149 pass. One honest caveat on coverage. The truncated-rate test pins the outcome (no channel opened), not the mechanism. I initialised the value to 0 as well as checking the read, so removing the read check alone doesn't change observable behaviour and the test still passes — I verified that by mutation rather than assuming otherwise. The initialiser is the load-bearing fix; the read check is defensive clarity. Worth knowing before anyone treats that test as protecting the check. The debug-abort one is probably the most worth a release note: it needs no relay mode, no voice traffic, just a connected peer and a single malformed packet. |
1 participant
Adds a relay mode to RakVoice so it can be used by a dedicated-server game. Today
SendFrame(guid)transmits peer-to-peer, which such a game cannot use — clients connect only to the server, never to each other. In relay mode clients send frames to a host that forwards them without decoding, so the server stays authoritative over who hears whom without running a codec.Needed by MafiaHub/Framework's voice chat, which is blocked on this landing here.
Wire format
The talker's GUID travels in the header because the sender is now the relay, not the speaker. The format-version byte is a deliberate escape hatch — a future layout change gets rejected by today's build rather than misparsed. Every offset derives from the one before it (
RAKVOICE_RELAY_OFFSET_*inRakVoice.h), so the writer and both readers cannot drift apart.What relay mode changes
packet->guid; otherwise every speaker arrives under the relay's GUID and collapses into a single decoder.SetPerSpeakerOutput. Lets a caller pull per-speaker PCM (ReceiveFrameFrom) instead of the pre-mixed buffer — the pre-mix leaves no way to position speakers individually.OnClosedConnectionnever fires for relay speakers, since they're peers of the host rather than of us, so nothing else would ever free their channels.SetRelayHostmakes the host returnRR_CONTINUE_PROCESSINGso frames reach the application loop to be handed back toRelayFrame(). Without it a plugin-attached host swallows every frame insideRakPeer::Receive.Peer-to-peer behaviour is unchanged when relay mode is off. The decode path was extracted into
DecodeIntoChanneland is statement-identical to whatOnVoiceDatadid before.Two latent bugs fixed
Both are reachable without relay mode, on any attached-but-uninitialised
RakVoice:zeroBufferedOutputorbufferedOutputCount— onlyInit()did.Update()therefore read indeterminate values every network cycle and, if the bool read non-zero, wrote through a nullbufferedOutputfor an indeterminate count.CloseVoiceChannelsentID_RAKVOICE_CLOSE_CHANNELunconditionally, so a peer that never opened a channel still got one on graceful disconnect.Testing
141/141 pass (126 before; the 15 new ones are additive, no existing test changed).
Tests/Unit/RakVoiceRelayTests.cppcovers the wire layout andReadRelayOrigin's handling of hostile input: truncation inside the origin field, unknown format versions, the all-ones sentinel, and the origin/sender mismatch a host must reject to stop one client speaking as another.ReadRelayOriginisstaticspecifically so this is testable without a liveRakPeerInterface.Mutation-checked rather than assumed: disabling the version check in
ReadRelayOriginmakesRejectsUnknownFormatVersionfail, and the layout assertions are deliberately hard-coded numbers so a silent offset change trips them.Breaking change
ID_RAKVOICE_RELAY_DATAis inserted afterID_RAKVOICE_DATAand shifts every subsequent id, includingID_READY_EVENT_SET, the RPC4 and two-way-auth ids, andID_USER_PACKET_ENUM. Peers must be rebuilt together — a peer built against the old header misparses everything past that point.Summary by CodeRabbit