Skip to content

Commit 065c296

Browse files
JustinMeansMean-E
andcommitted
Fix WebSocket continuation frame data loss in processFrames
processFrames matched only .binary and .text opcodes; .continuation frames fell through to default: break. For any fragmented WebSocket message the intermediate and final continuation frames were silently discarded, leaving a partial buffer that then corrupted the next unrelated delivery. Introduce FrameAccumulator (internal, testable) that accepts all data-frame opcodes and flushes once fin==true. Use it in both ServerConnectionHandler and ClientConnectionHandler processFrames. Add FrameAccumulatorTests covering two- and three-fragment assembly, empty payload, non-fin continuation, and buffer-clear-after-flush to prevent cross-message contamination. Co-Authored-By: Mean-E <mean-e@users.noreply.github.com>
1 parent 6c1cc70 commit 065c296

3 files changed

Lines changed: 173 additions & 15 deletions

File tree

‎Sources/Transmission/ClientManager.swift‎

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -191,18 +191,17 @@ private enum ClientConnectionHandler {
191191
system: TransmissionSystem,
192192
node: RemoteNode
193193
) async throws {
194-
var buffer = Data()
194+
var accumulator = FrameAccumulator()
195195

196196
for try await frame in inbound {
197197
switch frame.opcode {
198198
case .text, .binary:
199-
var data = frame.data
200-
if let bytes = data.readBytes(length: data.readableBytes) {
201-
buffer.append(contentsOf: bytes)
199+
if let message = accumulator.feed(frame) {
200+
await system.decodeAndDeliver(data: message, from: node)
202201
}
203-
if frame.fin {
204-
await system.decodeAndDeliver(data: buffer, from: node)
205-
buffer = Data()
202+
case .continuation:
203+
if let message = accumulator.feed(frame) {
204+
await system.decodeAndDeliver(data: message, from: node)
206205
}
207206
case .ping:
208207
let pong = WebSocketFrame(fin: true, opcode: .pong, data: frame.data)

‎Sources/Transmission/ServerManager.swift‎

Lines changed: 40 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -140,19 +140,19 @@ private enum ServerConnectionHandler {
140140
system: TransmissionSystem,
141141
node: RemoteNode
142142
) async throws {
143-
var buffer = Data()
143+
var accumulator = FrameAccumulator()
144144

145145
for try await frame in inbound {
146146
switch frame.opcode {
147147
case .text, .binary:
148-
var data = frame.data
149-
if let bytes = data.readBytes(length: data.readableBytes) {
150-
buffer.append(contentsOf: bytes)
151-
system.metrics.recordMessageReceived(bytes: bytes.count)
148+
if let message = accumulator.feed(frame) {
149+
system.metrics.recordMessageReceived(bytes: message.count)
150+
await system.decodeAndDeliver(data: message, from: node)
152151
}
153-
if frame.fin {
154-
await system.decodeAndDeliver(data: buffer, from: node)
155-
buffer = Data()
152+
case .continuation:
153+
if let message = accumulator.feed(frame) {
154+
system.metrics.recordMessageReceived(bytes: message.count)
155+
await system.decodeAndDeliver(data: message, from: node)
156156
}
157157
case .ping:
158158
let pong = WebSocketFrame(fin: true, opcode: .pong, data: frame.data)
@@ -168,6 +168,38 @@ private enum ServerConnectionHandler {
168168
}
169169
}
170170

171+
/// Reassembles a WebSocket message from one or more frames.
172+
///
173+
/// WebSocket messages may be split across multiple frames: the first frame carries
174+
/// opcode `.binary` or `.text` with `fin == false`, intermediate frames carry
175+
/// opcode `.continuation` with `fin == false`, and the last fragment carries
176+
/// opcode `.continuation` with `fin == true`. Callers must feed every data frame
177+
/// (including continuation frames) into `feed(_:)` and act on the returned `Data`
178+
/// when a complete message has been assembled.
179+
///
180+
/// Previously `processFrames` only matched `.binary` and `.text` opcodes, silently
181+
/// discarding any `.continuation` frames via `default: break`. This caused:
182+
/// - Fragmented message data to be silently dropped.
183+
/// - The partial buffer from the first fragment to be flushed with the NEXT unrelated
184+
/// message, corrupting every subsequent delivery.
185+
struct FrameAccumulator {
186+
private var buffer = Data()
187+
188+
/// Feed one inbound frame. Returns the assembled message data when the final
189+
/// fragment (or a standalone non-fragmented frame) has been received;
190+
/// returns nil when more frames are still needed to complete the message.
191+
mutating func feed(_ frame: WebSocketFrame) -> Data? {
192+
var frameData = frame.data
193+
if let bytes = frameData.readBytes(length: frameData.readableBytes) {
194+
buffer.append(contentsOf: bytes)
195+
}
196+
guard frame.fin else { return nil }
197+
let message = buffer
198+
buffer = Data()
199+
return message
200+
}
201+
}
202+
171203
private enum UpgradeResult: Sendable {
172204
case websocket(NIOAsyncChannel<WebSocketFrame, WebSocketFrame>, clientNodeID: String?)
173205
case notUpgraded
Lines changed: 127 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,127 @@
1+
import Testing
2+
import Foundation
3+
import NIO
4+
import NIOWebSocket
5+
@testable import Transmission
6+
7+
/// Tests for FrameAccumulator, which reassembles WebSocket message fragments.
8+
///
9+
/// Before the fix, processFrames matched only .binary and .text opcodes. Continuation
10+
/// frames fell through to `default: break`, silently discarding their data and leaving
11+
/// the partial buffer unflushed. The next unrelated message would then have the stale
12+
/// bytes prepended, corrupting delivery.
13+
///
14+
/// The fix introduces FrameAccumulator which handles .continuation frames identically
15+
/// to the initial .binary/.text fragment: accumulate bytes, flush when fin == true.
16+
@Suite("FrameAccumulator Tests")
17+
struct FrameAccumulatorTests {
18+
19+
// MARK: - Helpers
20+
21+
private func makeFrame(bytes: [UInt8], opcode: WebSocketOpcode, fin: Bool) -> WebSocketFrame {
22+
var buffer = ByteBuffer()
23+
buffer.writeBytes(bytes)
24+
return WebSocketFrame(fin: fin, opcode: opcode, data: buffer)
25+
}
26+
27+
// MARK: - Non-fragmented (single frame, fin == true)
28+
29+
@Test("Single binary frame with fin=true returns assembled data immediately")
30+
func singleBinaryFrame() {
31+
var acc = FrameAccumulator()
32+
let frame = makeFrame(bytes: [0x01, 0x02, 0x03], opcode: .binary, fin: true)
33+
let result = acc.feed(frame)
34+
#expect(result == Data([0x01, 0x02, 0x03]))
35+
}
36+
37+
@Test("Single text frame with fin=true returns assembled data immediately")
38+
func singleTextFrame() {
39+
var acc = FrameAccumulator()
40+
let frame = makeFrame(bytes: Array("hello".utf8), opcode: .text, fin: true)
41+
let result = acc.feed(frame)
42+
#expect(result == Data("hello".utf8))
43+
}
44+
45+
@Test("Empty single frame returns empty Data (not nil)")
46+
func emptyPayloadFrame() {
47+
var acc = FrameAccumulator()
48+
let frame = makeFrame(bytes: [], opcode: .binary, fin: true)
49+
let result = acc.feed(frame)
50+
#expect(result == Data())
51+
}
52+
53+
// MARK: - Fragmented messages (the bug site)
54+
55+
@Test("Fragmented message: binary(fin=false) + continuation(fin=true) assembles correctly")
56+
func twoFragmentMessage() {
57+
var acc = FrameAccumulator()
58+
59+
// First fragment: opcode .binary, fin == false — should return nil (not complete)
60+
let first = makeFrame(bytes: [0xAA, 0xBB], opcode: .binary, fin: false)
61+
let midResult = acc.feed(first)
62+
#expect(midResult == nil, "Intermediate frame must not flush the buffer")
63+
64+
// Final fragment: opcode .continuation, fin == true — must return assembled data
65+
let last = makeFrame(bytes: [0xCC, 0xDD], opcode: .continuation, fin: true)
66+
let finalResult = acc.feed(last)
67+
#expect(finalResult == Data([0xAA, 0xBB, 0xCC, 0xDD]),
68+
"Continuation frame data must be appended and flushed when fin == true")
69+
}
70+
71+
@Test("Three-fragment message assembles in order")
72+
func threeFragmentMessage() {
73+
var acc = FrameAccumulator()
74+
75+
let f1 = makeFrame(bytes: [0x01], opcode: .binary, fin: false)
76+
let f2 = makeFrame(bytes: [0x02], opcode: .continuation, fin: false)
77+
let f3 = makeFrame(bytes: [0x03], opcode: .continuation, fin: true)
78+
79+
#expect(acc.feed(f1) == nil)
80+
#expect(acc.feed(f2) == nil)
81+
let result = acc.feed(f3)
82+
#expect(result == Data([0x01, 0x02, 0x03]))
83+
}
84+
85+
@Test("Continuation with fin=false does not flush")
86+
func continuationWithoutFin() {
87+
var acc = FrameAccumulator()
88+
let f1 = makeFrame(bytes: [0xDE], opcode: .binary, fin: false)
89+
let f2 = makeFrame(bytes: [0xAD], opcode: .continuation, fin: false)
90+
91+
#expect(acc.feed(f1) == nil)
92+
#expect(acc.feed(f2) == nil, "Continuation without fin must not flush")
93+
}
94+
95+
// MARK: - Buffer is cleared after flush (no stale data leaks into next message)
96+
97+
@Test("Buffer resets after a complete message; next message starts clean")
98+
func bufferClearsAfterFlush() {
99+
var acc = FrameAccumulator()
100+
101+
// Message 1: single frame
102+
let msg1 = makeFrame(bytes: [0x11], opcode: .binary, fin: true)
103+
let result1 = acc.feed(msg1)
104+
#expect(result1 == Data([0x11]))
105+
106+
// Message 2: must NOT contain bytes from message 1
107+
let msg2 = makeFrame(bytes: [0x22], opcode: .binary, fin: true)
108+
let result2 = acc.feed(msg2)
109+
#expect(result2 == Data([0x22]),
110+
"Stale bytes from the previous message must not contaminate the next")
111+
}
112+
113+
@Test("Buffer resets after fragmented message; next message starts clean")
114+
func bufferClearsAfterFragmentedFlush() {
115+
var acc = FrameAccumulator()
116+
117+
// Fragmented message 1
118+
#expect(acc.feed(makeFrame(bytes: [0xAA], opcode: .binary, fin: false)) == nil)
119+
let result1 = acc.feed(makeFrame(bytes: [0xBB], opcode: .continuation, fin: true))
120+
#expect(result1 == Data([0xAA, 0xBB]))
121+
122+
// Independent message 2: must not include 0xAA or 0xBB
123+
let result2 = acc.feed(makeFrame(bytes: [0xFF], opcode: .binary, fin: true))
124+
#expect(result2 == Data([0xFF]),
125+
"Previously accumulated bytes must be cleared after fin frame flushes")
126+
}
127+
}

0 commit comments

Comments
 (0)