Repository navigation
fix(voice): detect RTP padding with the correct bit and strip it after decryption - #594
Merged
Merged
Conversation
…r decryption ReadPacket tested the RTP padding flag as 0x04. RFC 3550 section 5.1 lays the first octet out as V=0xC0, P=0x20, X=0x10, CC=0x0F, so 0x04 falls inside the CSRC count nibble. Discord audio carries a CSRC count of zero, so the flag was never true and padding was never removed. The neighbouring extension and CSRC masks are correct and match the bit indices the comment above them documents, which is what made the slip hard to see. Padding was also removed before decryption, using the last octet of the packet to size it. That octet is ciphertext at that point, so the length read was meaningless; had the flag ever been true, the payload would have been truncated by an arbitrary amount. Padding sits at the end of the encrypted payload and can only be measured once the payload is plaintext, so it is now stripped after decryption. Finally, a packet may consist entirely of padding. Such a packet carries no media, advancing the sequence number without advancing the timestamp, and passing it on leaves the decryptor with an empty frame. Those packets are now skipped. The effect on a DAVE session is that padding left attached moves the frame's terminating marker away from the end of the buffer, so libdave does not recognise the frame as encrypted and returns a generic decryption failure. Measured against a live Discord voice channel, receive-side frame loss fell from between three and eleven percent to zero, the higher figures coming from mobile clients where bandwidth probing produces more padded packets. Adds tests covering padding removal across several padding lengths, the padding-only case, the helper's handling of absent and implausible counts, and a guard that the three first-octet masks match the documented bit indices and do not overlap.
topi314
approved these changes
Aug 25, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #593
What this fixes
ReadPackettested the RTP padding flag as0x04. Per RFC 3550 §5.1 the first octet isV=0xC0,P=0x20,X=0x10,CC=0x0F, so0x04lands inside the CSRC count nibble. Discord audio carriesCC=0, so padding was never detected and never removed.Under DAVE this causes hard decrypt failures. Padding left attached moves the frame's
0xFAFAterminating marker away from the end of the buffer, libdave stops recognising the frame as encrypted, and returns a generic decryption failure. Measured receive-side loss was 3.2% to 11.1% depending on client, and 0.00% with this change.This is a regression from
1dc5f5bf(feat: DAVE (#507)), which changed the mask from a correct0x20. The extension and CSRC masks alongside it are correct and match the bit indices documented in the comment above them.Changes
ReadPacketpreviously read the length fromu.receiveBuffer[n-1]before decryption, where that octet is ciphertext. This was latent only because the mask never matched. Had it matched, payloads would have been truncated arbitrarily.stripRTPPaddingleaves a payload unchanged when the count is zero or exceeds the payload, so a malformed packet degrades to a decrypt failure rather than silent corruption.Tests
voice/udp_conn_test.gocovers:ReadPacketat 1, 4 and 32 bytes, plus the unpadded casestripRTPPaddingwith absent, zero and implausible countsAll four
ReadPacketand padding-only assertions fail onmasterand pass with this change. The unpadded case passes in both, so the tests discriminate rather than failing indiscriminately.The tests use
godave.NewNoopSessionand a passthrough encrypter, so they need no network, no CGO and no libdave.Verification
Verified against a live Discord voice channel. Receive-side frame loss fell from 3.2% to 11.1% down to 0.00% across 1979 frames with zero sequence gaps. Full
voicepackage tests,go build ./...andgo vetall pass.I have not tested this against video, or with several speakers at once. Both go through
ReadPacket, so a second opinion on those paths would be worth having before merging.