Skip to content

Commit 8b5bf9e

Browse files
committed
fix(ipc): enforce the frame ceiling on delimited frames
What: check the assembled frame's UTF-8 byte size inside the delimiter loop, before the buffer reset, and return after onOversizedFrame instead of parsing. Why: the ceiling only ran on the trailing partial segment, so an oversized frame whose delimiter arrived in the same chunk was parsed and dispatched instead of dropping the connection. Impact: a peer can no longer bypass MAX_FRAME_BYTES by sending the delimiter with the overrun; the connection drops with a single bounded log line. The O(chunk) byte accounting is unchanged, and the exact-ceiling boundary still passes.
1 parent bbbef70 commit 8b5bf9e

2 files changed

Lines changed: 196 additions & 1 deletion

File tree

‎scripts/agent-evals/ipc-client.test.ts‎

Lines changed: 183 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -85,6 +85,56 @@ const startPeer = async (socketPath: string) => {
8585
}
8686
}
8787

88+
const ACK_FRAME =
89+
JSON.stringify({
90+
type: "message",
91+
data: {
92+
type: IpcMessageType.Ack,
93+
origin: IpcOrigin.Server,
94+
data: { clientId: "client-1", pid: process.pid, ppid: process.ppid },
95+
},
96+
}) + DELIMITER
97+
98+
// A schema-valid TaskEvent frame padded to an exact UTF-8 byte length by growing
99+
// the payload string, so a test can sit precisely on the ceiling boundary. The
100+
// padding is ASCII, so byte length and code-unit length coincide.
101+
const taskEventFrameOfBytes = (bytes: number): string => {
102+
const base = JSON.stringify({
103+
type: "message",
104+
data: {
105+
type: IpcMessageType.TaskEvent,
106+
origin: IpcOrigin.Server,
107+
data: { eventName: RooCodeEventName.TaskStarted, payload: [""], taskId: 1 } satisfies TaskEvent,
108+
},
109+
})
110+
const paddingBytes = bytes - Buffer.byteLength(base, "utf8")
111+
112+
if (paddingBytes < 0)
113+
throw new Error(`frame base is ${Buffer.byteLength(base, "utf8")} bytes, cannot shrink to ${bytes}`)
114+
115+
return base.replace('[""]', () => JSON.stringify(["A".repeat(paddingBytes)]))
116+
}
117+
118+
// Completes the handshake and then writes `payload` in a single call, so the test
119+
// controls exactly which bytes share a chunk.
120+
const startOneShotPeer = async (socketPath: string, payload: string): Promise<() => Promise<void>> => {
121+
const server = net.createServer((socket) => {
122+
socket.on("error", () => {})
123+
socket.write(ACK_FRAME)
124+
socket.write(payload)
125+
})
126+
127+
await new Promise<void>((resolve, reject) => {
128+
server.once("error", reject)
129+
server.listen(resolveSocketPath(socketPath), resolve)
130+
})
131+
132+
return () => new Promise<void>((resolve) => server.close(() => resolve()))
133+
}
134+
135+
const ceilingLogs = (logs: unknown[][]): unknown[][] =>
136+
logs.filter((entry) => String(entry[0]).includes("MAX_FRAME_BYTES"))
137+
88138
describe("eval ipc client", () => {
89139
it("completes the ack handshake and reports readiness", async () => {
90140
const dir = await socketDir()
@@ -550,6 +600,139 @@ describe("eval ipc client", () => {
550600
}
551601
})
552602

603+
it("drops an oversized frame whose delimiter arrives in the same chunk", async () => {
604+
const dir = await socketDir()
605+
const socketPath = socketPathIn(dir, "oversized-inline")
606+
const logs: unknown[][] = []
607+
const events: TaskEvent[] = []
608+
const ceiling = 1024
609+
// A well-formed frame that is one byte past the ceiling. Without a check
610+
// inside the delimiter loop it parses and dispatches instead of dropping.
611+
const oversized = taskEventFrameOfBytes(ceiling + 1) + DELIMITER
612+
const closePeer = await startOneShotPeer(socketPath, oversized)
613+
614+
const client = new EvalIpcClient(socketPath, (...args) => logs.push(args), { maxFrameBytes: ceiling })
615+
client.on(IpcMessageType.TaskEvent, (event) => events.push(event as TaskEvent))
616+
617+
try {
618+
await waitFor(() => !client.isConnected && ceilingLogs(logs).length > 0, 10_000)
619+
620+
assert.equal(ceilingLogs(logs).length, 1, "exactly one bounded log line")
621+
assert.equal(events.length, 0, "a frame past the ceiling must never be dispatched")
622+
assert.equal(client.isConnected, false, "the connection is dropped")
623+
} finally {
624+
client.disconnect()
625+
await closePeer()
626+
await fs.rm(dir, { recursive: true, force: true })
627+
}
628+
})
629+
630+
it("stops scanning the chunk after an oversized frame instead of parsing what follows", async () => {
631+
const dir = await socketDir()
632+
const socketPath = socketPathIn(dir, "oversized-then-valid")
633+
const logs: unknown[][] = []
634+
const events: TaskEvent[] = []
635+
const ceiling = 1024
636+
// The oversized frame and a valid frame share one chunk. The handler must
637+
// return, not break: a break leaves the offset unmoved, so the rest of the
638+
// chunk becomes the tail, re-populates the buffer and logs a second time.
639+
const chunk = taskEventFrameOfBytes(ceiling + 1) + DELIMITER + taskEventFrameOfBytes(256) + DELIMITER
640+
const closePeer = await startOneShotPeer(socketPath, chunk)
641+
642+
const client = new EvalIpcClient(socketPath, (...args) => logs.push(args), { maxFrameBytes: ceiling })
643+
client.on(IpcMessageType.TaskEvent, (event) => events.push(event as TaskEvent))
644+
645+
try {
646+
await waitFor(() => !client.isConnected && ceilingLogs(logs).length > 0, 10_000)
647+
// Give any stray second frame a chance to surface before asserting.
648+
await new Promise((resolve) => setTimeout(resolve, 60))
649+
650+
assert.equal(ceilingLogs(logs).length, 1, "the tail must not re-trip the ceiling")
651+
assert.equal(events.length, 0, "the frame after the oversized one must not be processed")
652+
assert.equal(client.isConnected, false, "the connection is dropped")
653+
} finally {
654+
client.disconnect()
655+
await closePeer()
656+
await fs.rm(dir, { recursive: true, force: true })
657+
}
658+
})
659+
660+
it("passes a frame at exactly the ceiling and drops one byte past it", async () => {
661+
const ceiling = 512
662+
const run = async (name: string, bytes: number) => {
663+
const dir = await socketDir()
664+
const socketPath = socketPathIn(dir, name)
665+
const logs: unknown[][] = []
666+
const events: TaskEvent[] = []
667+
const closePeer = await startOneShotPeer(socketPath, taskEventFrameOfBytes(bytes) + DELIMITER)
668+
const client = new EvalIpcClient(socketPath, (...args) => logs.push(args), { maxFrameBytes: ceiling })
669+
670+
client.on(IpcMessageType.TaskEvent, (event) => events.push(event as TaskEvent))
671+
672+
try {
673+
await waitFor(() => events.length > 0 || ceilingLogs(logs).length > 0, 10_000)
674+
return { events: events.length, drops: ceilingLogs(logs).length, connected: client.isConnected }
675+
} finally {
676+
client.disconnect()
677+
await closePeer()
678+
await fs.rm(dir, { recursive: true, force: true })
679+
}
680+
}
681+
682+
const atCeiling = await run("ceiling-exact", ceiling)
683+
assert.equal(atCeiling.drops, 0, "a frame at exactly the ceiling is legal")
684+
assert.equal(atCeiling.events, 1, "the frame at the ceiling is dispatched")
685+
assert.equal(atCeiling.connected, true, "the connection survives a frame at the ceiling")
686+
687+
const pastCeiling = await run("ceiling-plus-one", ceiling + 1)
688+
assert.equal(pastCeiling.drops, 1, "one byte past the ceiling drops the connection")
689+
assert.equal(pastCeiling.events, 0, "the frame past the ceiling is never dispatched")
690+
assert.equal(pastCeiling.connected, false, "the connection is dropped")
691+
})
692+
693+
it("counts a delimited frame's multibyte content in bytes, not characters", async () => {
694+
const dir = await socketDir()
695+
const socketPath = socketPathIn(dir, "multibyte-inline")
696+
const logs: unknown[][] = []
697+
const events: TaskEvent[] = []
698+
const ceiling = 2048
699+
// 1000 three-byte characters: 1000 code units (under the ceiling) but 3000
700+
// wire bytes (over it), and the delimiter shares the chunk with them.
701+
const frame =
702+
JSON.stringify({
703+
type: "message",
704+
data: {
705+
type: IpcMessageType.TaskEvent,
706+
origin: IpcOrigin.Server,
707+
data: {
708+
eventName: RooCodeEventName.TaskStarted,
709+
payload: ["가".repeat(1000)],
710+
taskId: 1,
711+
} satisfies TaskEvent,
712+
},
713+
}) + DELIMITER
714+
const closePeer = await startOneShotPeer(socketPath, frame)
715+
716+
const client = new EvalIpcClient(socketPath, (...args) => logs.push(args), { maxFrameBytes: ceiling })
717+
client.on(IpcMessageType.TaskEvent, (event) => events.push(event as TaskEvent))
718+
719+
try {
720+
await waitFor(() => !client.isConnected && ceilingLogs(logs).length > 0, 10_000)
721+
722+
assert.equal(
723+
ceilingLogs(logs).length,
724+
1,
725+
"the byte ceiling must drop a frame whose code-unit count is under it",
726+
)
727+
assert.equal(events.length, 0, "the multibyte frame past the byte ceiling is never dispatched")
728+
assert.equal(client.isConnected, false, "the connection is dropped")
729+
} finally {
730+
client.disconnect()
731+
await closePeer()
732+
await fs.rm(dir, { recursive: true, force: true })
733+
}
734+
})
735+
553736
it("keeps the production frame ceiling far above any legitimate task event", () => {
554737
assert.equal(MAX_FRAME_BYTES, 64 * 1024 * 1024)
555738
})

‎scripts/agent-evals/ipc-client.ts‎

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -124,7 +124,19 @@ export class EvalIpcClient extends EventEmitter<EvalIpcClientEvents> {
124124
let index: number
125125

126126
while ((index = chunk.indexOf(DELIMITER, offset)) !== -1) {
127-
const frame = this._buffer + chunk.slice(offset, index)
127+
const segment = chunk.slice(offset, index)
128+
const frameBytes = this._bufferBytes + Buffer.byteLength(segment, "utf8")
129+
130+
// A delimiter in the same chunk as the overrun must not buy the frame a
131+
// pass: the ceiling applies to the assembled frame, not just the tail.
132+
// Returning (not breaking) stops the scan, so the closed connection
133+
// cannot re-populate the buffer and log a second time.
134+
if (frameBytes > this._maxFrameBytes) {
135+
this.onOversizedFrame()
136+
return
137+
}
138+
139+
const frame = this._buffer + segment
128140
this._buffer = ""
129141
this._bufferBytes = 0
130142
if (frame.length > 0) this.onFrame(frame)

0 commit comments

Comments
 (0)