Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion src/runtime/valkey_jsc/protocol_jsc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,6 @@ pub fn valkey_error_to_js(
RedisError::ConnectionTimeout => JscError::REDIS_CONNECTION_TIMEOUT,
RedisError::IdleTimeout => JscError::REDIS_IDLE_TIMEOUT,
RedisError::NestingDepthExceeded => JscError::REDIS_INVALID_RESPONSE,
RedisError::LineTooLong => JscError::REDIS_INVALID_RESPONSE,
RedisError::JSError => return global.take_exception(JsError::Thrown),
RedisError::OutOfMemory => {
let _ = global.throw_out_of_memory();
Expand Down
12 changes: 2 additions & 10 deletions src/valkey/valkey_protocol.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@
ConnectionTimeout,
IdleTimeout,
NestingDepthExceeded,
LineTooLong,
}

bun_core::impl_tag_error!(RedisError);
Expand Down Expand Up @@ -247,23 +246,18 @@
Ok(byte)
}

pub fn read_until_crlf(&mut self) -> Result<&'a [u8], RedisError> {
let buffer = &self.buffer[self.pos..];
let limit = buffer.len().min(Self::MAX_LINE_LEN + 1);
let start = self.crlf_skip.min(limit);
let start = self.crlf_skip.min(buffer.len());
self.crlf_skip = 0;
for (i, &byte) in buffer.iter().enumerate().take(limit).skip(start) {
for (i, &byte) in buffer.iter().enumerate().skip(start) {
if byte == b'\r' && buffer.len() > i + 1 && buffer[i + 1] == b'\n' {
let result = &buffer[0..i];
self.pos += i + 2;
return Ok(result);
}
}
if buffer.len() > Self::MAX_LINE_LEN + 1 {
return Err(RedisError::LineTooLong);
}

Err(RedisError::InvalidResponse)

Check warning on line 260 in src/valkey/valkey_protocol.rs

View check run for this annotation

Claude / Claude Code Review

Removing MAX_LINE_LEN leaves line-terminated replies with no buffer-growth bound

Removing `MAX_LINE_LEN` entirely leaves line-terminated replies (`+ - : , # ( _`) with no buffer-growth bound at all — a server that streams an unterminated `+` line now causes `read_buffer` to grow indefinitely, whereas length-prefixed types are still capped at `MAX_BULK_LEN` (whose retained doc comment says it exists precisely so "the connection state machine stops buffering instead of growing the read buffer toward an attacker-chosen size"). Consider raising the line cap to `MAX_BULK_LEN` (51
Comment thread
robobun marked this conversation as resolved.
}

pub fn read_integer(&mut self) -> Result<i64, RedisError> {
Expand Down Expand Up @@ -344,8 +338,6 @@
/// attacker-chosen size.
const MAX_BULK_LEN: i64 = 512 * 1024 * 1024;

const MAX_LINE_LEN: usize = 512 * 1024;

/// Caps an aggregate's `Vec::with_capacity` so the total bytes reserved
/// across the whole parse — every nesting level combined — never exceed
/// the input buffer's size. Re-applying a per-level "remaining buffer"
Expand Down
73 changes: 73 additions & 0 deletions test/js/valkey/reliability/resp-nesting-depth.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -239,6 +239,79 @@ describe("Valkey: RESP Nesting Depth Handling", () => {
});
});

describe("Valkey: RESP line-terminated replies (>512KB)", () => {
// The Zig parser scanned for CRLF over the whole buffer. A later hardening
// pass capped the scan window at 512KB, which rejects valid RESP simple
// strings and error replies that a real server emits (Lua
// redis.status_reply(...) / redis.error_reply(...) with a long payload).
// https://redis.io/docs/latest/develop/reference/protocol-spec/ places no
// length limit on `+` / `-` lines.

async function roundtrip(payload: Buffer): Promise<any> {
const { server, port } = await createMockRedisServer([payload, Buffer.from("+PONG\r\n")]);
try {
const client = new Bun.RedisClient(`redis://127.0.0.1:${port}`, {
autoReconnect: false,
connectionTimeout: 5000,
});
try {
const reply = await client.send("PING", []);
// The client must still be usable for the next command: a parse
// failure here used to latch `failed` and reject forever.
const pong = await client.send("PING", []);
return { reply, pong };
} finally {
client.close();
}
} finally {
server.close();
}
}

test("accepts a simple string (`+`) reply longer than 512KB", async () => {
const body = Buffer.alloc(600_000, "x").toString();
const { reply, pong } = await roundtrip(Buffer.from(`+${body}\r\n`));
expect(typeof reply).toBe("string");
expect(reply.length).toBe(600_000);
expect(pong).toBe("PONG");
});

test("accepts a simple string at exactly 512KB + 1 bytes", async () => {
// 524288 worked, 524289 was rejected by an off-by-one in the scan window.
const body = Buffer.alloc(512 * 1024 + 1, "y").toString();
const { reply, pong } = await roundtrip(Buffer.from(`+${body}\r\n`));
expect(reply.length).toBe(512 * 1024 + 1);
expect(pong).toBe("PONG");
});

test("surfaces an error (`-`) reply longer than 512KB with its original text", async () => {
const body = "ERR user_script:1: " + Buffer.alloc(600_000, "e").toString();
const { server, port } = await createMockRedisServer([
Buffer.from(`-${body}\r\n`),
Buffer.from("+PONG\r\n"),
]);
try {
const client = new Bun.RedisClient(`redis://127.0.0.1:${port}`, {
autoReconnect: false,
connectionTimeout: 5000,
});
try {
const rejection = await client.send("EVAL", ["script", "0"]).then(
() => null,
(e: any) => e,
);
expect(rejection?.message).toBe(body);
// Client must survive and serve the next command.
expect(await client.send("PING", [])).toBe("PONG");
} finally {
client.close();
}
} finally {
server.close();
}
});
});

describe("Valkey: RESP push frame routing", () => {
test("resolves a pending command with its own reply when an out-of-band push frame precedes it", async () => {
const payload = Buffer.from(
Expand Down
22 changes: 9 additions & 13 deletions test/js/valkey/valkey-gc.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -132,10 +132,10 @@ test.concurrent("RedisClient survives GC across many short-lived instances", asy
});

// A RESP scalar line (simple string, error, integer, ...) must end with CRLF.
// The reader bounds how many bytes it will accumulate while waiting for that
// terminator (MAX_LINE_LEN = 512 KiB), so a server that streams an endless
// unterminated line gets a protocol error promptly instead of the client
// buffering and rescanning the whole line on every socket read.
// The reader imposes no length cap on the line (the RESP spec has none and
// Lua `redis.status_reply`/`redis.error_reply` emit arbitrary payloads), so a
// server that closes mid-line rejects the pending command with a
// connection-closed error rather than a protocol error.
Comment thread
robobun marked this conversation as resolved.
Outdated
test.concurrent("rejects a RESP simple-string reply whose line terminator never arrives", async () => {
// Minimal mock Redis server: replies +OK to the HELLO handshake, then
// answers the next command with `payload`.
Expand All @@ -161,12 +161,9 @@ test.concurrent("rejects a RESP simple-string reply whose line terminator never
});
}

// 1) A simple-string reply whose CRLF terminator never arrives. Once more
// than 512 KiB of the line has accumulated, the client must fail the
// reply with a protocol error rather than keep waiting for a terminator
// that never comes. (The server closes the socket after the payload so
// that a client which keeps waiting still settles the promise -- with a
// connection-closed error instead of the expected protocol error.)
// 1) A simple-string reply whose CRLF terminator never arrives. The reader
// treats the unterminated bytes as a partial reply and keeps waiting; when
// the server closes, the pending command is rejected as connection-closed.
{
const unterminated = Buffer.from("+" + Buffer.alloc(600_000, "A").toString());
const { server, port } = await listen(unterminated, true);
Expand All @@ -179,7 +176,7 @@ test.concurrent("rejects a RESP simple-string reply whose line terminator never
await client.send("PING", []);
expect.unreachable();
} catch (error: any) {
expect(error.code).toBe("ERR_REDIS_INVALID_RESPONSE");
expect(error.code).toBe("ERR_REDIS_CONNECTION_CLOSED");
} finally {
client.close();
}
Expand All @@ -188,8 +185,7 @@ test.concurrent("rejects a RESP simple-string reply whose line terminator never
}
}

// 2) A large but properly terminated simple string under the bound still
// parses.
// 2) A large, properly terminated simple string still parses.
{
const value = Buffer.alloc(100_000, "B").toString();
const { server, port } = await listen(Buffer.from("+" + value + "\r\n"), false);
Expand Down
Loading