fix(protocol): name protocol faults on the wire, and make MULTI atomic at queue time #472
There are no files selected for viewing
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,7 +2,7 @@ use memchr::{memchr, memchr2}; | |
|
|
||
| use bytes::{Buf, Bytes, BytesMut}; | ||
|
|
||
| use super::frame::{Frame, FrameVec, ParseError, ProtoFault}; | ||
|
|
||
| /// Parse an inline command from the buffer. | ||
| /// | ||
|
|
@@ -32,6 +32,7 @@ pub fn parse_inline( | |
| // forever. Redis caps this at PROTO_INLINE_MAX_SIZE. | ||
| if buf.len() > max_inline_size { | ||
| return Err(ParseError::Invalid { | ||
| kind: ProtoFault::InlineTooBig, | ||
| message: "Protocol error: too big inline request".into(), | ||
| offset: 0, | ||
| }); | ||
|
|
@@ -44,6 +45,7 @@ pub fn parse_inline( | |
| // any inline request past the cap regardless of termination). | ||
| if crlf_pos > max_inline_size { | ||
| return Err(ParseError::Invalid { | ||
| kind: ProtoFault::InlineTooBig, | ||
| message: "Protocol error: too big inline request".into(), | ||
| offset: 0, | ||
| }); | ||
|
|
@@ -52,6 +54,24 @@ pub fn parse_inline( | |
| // Extract line content before CRLF | ||
| let line = &buf[..crlf_pos]; | ||
|
|
||
| // Quoted inline arguments take the careful path. Redis parses inline | ||
| // commands with `sdssplitargs`, which understands quoting and escapes; | ||
| // Moon used to split on whitespace unconditionally, so `SET k "a b"` | ||
| // became three arguments with literal quote bytes in them, and an | ||
| // unterminated quote was silently accepted as part of a key. | ||
| // | ||
| // memchr2 over the line is one SIMD pass and is only paid once per inline | ||
| // command — the unquoted case (every benchmark, every redis-cli one-liner | ||
| // without spaces in values) keeps the original loop untouched below. | ||
| if memchr2(b'"', b'\'', line).is_some() { | ||
| let args = split_args_quoted(line)?; | ||
| buf.advance(crlf_pos + 2); | ||
| if args.is_empty() { | ||
| return Ok(None); | ||
| } | ||
| return Ok(Some(Frame::Array(args))); | ||
| } | ||
|
|
||
| // Split by whitespace (spaces and tabs) using SIMD, filtering empty slices | ||
| let mut args = FrameVec::new(); | ||
| let mut start = 0; | ||
|
|
@@ -89,6 +109,145 @@ pub fn parse_inline( | |
| Ok(Some(Frame::Array(args))) | ||
| } | ||
|
|
||
| /// Split an inline command line that contains at least one quote character. | ||
| /// | ||
| /// A port of Redis's `sdssplitargs` (`sds.c`), which is what defines inline | ||
| /// argument syntax for every client that speaks it — telnet users, `redis-cli` | ||
| /// pasting a quoted value, and the health-check scripts that send `PING\r\n` | ||
| /// down a bare socket. | ||
| /// | ||
| /// Rules, all of them Redis's: | ||
| /// * double quotes honour `\xHH` hex and the `\n \r \t \b \a` escapes; | ||
| /// any other `\<c>` is that literal character. | ||
| /// * single quotes honour only `\'`; everything else is literal. | ||
| /// * a closing quote must be followed by whitespace or end-of-line — | ||
| /// `"foo"bar` is an error, not two tokens. | ||
| /// * an unterminated quote is an error for the whole request. | ||
| /// | ||
| /// Returns `ParseError::Invalid` with [`ProtoFault::UnbalancedQuotes`] on any | ||
| /// of the error cases, which the caller turns into Redis's | ||
| /// `-ERR Protocol error: unbalanced quotes in request` and then closes. | ||
| fn split_args_quoted(line: &[u8]) -> Result<FrameVec, ParseError> { | ||
| #[inline] | ||
| fn hex_val(c: u8) -> Option<u8> { | ||
| match c { | ||
| b'0'..=b'9' => Some(c - b'0'), | ||
| b'a'..=b'f' => Some(c - b'a' + 10), | ||
| b'A'..=b'F' => Some(c - b'A' + 10), | ||
| _ => None, | ||
| } | ||
| } | ||
| let unbalanced = || ParseError::Invalid { | ||
| kind: ProtoFault::UnbalancedQuotes, | ||
| message: "Protocol error: unbalanced quotes in request".into(), | ||
| offset: 0, | ||
| }; | ||
|
|
||
| let mut args = FrameVec::new(); | ||
| let mut i = 0; | ||
| loop { | ||
| while i < line.len() && (line[i] == b' ' || line[i] == b'\t') { | ||
| i += 1; | ||
| } | ||
| if i >= line.len() { | ||
| return Ok(args); | ||
| } | ||
|
|
||
| // One token. `current` is only allocated for tokens that actually | ||
| // need unescaping; a plain token is copied once, same as the fast path. | ||
| let mut current: Vec<u8> = Vec::new(); | ||
| let quote = match line[i] { | ||
| q @ (b'"' | b'\'') => { | ||
| i += 1; | ||
| Some(q) | ||
| } | ||
| _ => None, | ||
| }; | ||
|
|
||
| match quote { | ||
| Some(b'"') => loop { | ||
| if i >= line.len() { | ||
| return Err(unbalanced()); | ||
| } | ||
| match line[i] { | ||
| b'\\' if i + 3 < line.len() && line[i + 1] == b'x' => { | ||
| match (hex_val(line[i + 2]), hex_val(line[i + 3])) { | ||
| (Some(hi), Some(lo)) => { | ||
| current.push(hi * 16 + lo); | ||
| i += 4; | ||
| } | ||
| // Not a valid hex escape: `\x` is literal, exactly | ||
| // as Redis falls through here. | ||
| _ => { | ||
| current.push(b'x'); | ||
| i += 2; | ||
| } | ||
| } | ||
| } | ||
| b'\\' if i + 1 < line.len() => { | ||
| current.push(match line[i + 1] { | ||
| b'n' => b'\n', | ||
| b'r' => b'\r', | ||
| b't' => b'\t', | ||
| b'b' => 0x08, | ||
| b'a' => 0x07, | ||
| other => other, | ||
| }); | ||
| i += 2; | ||
| } | ||
| b'"' => { | ||
| // A closing quote must end the token. | ||
| if i + 1 < line.len() && line[i + 1] != b' ' && line[i + 1] != b'\t' { | ||
| return Err(unbalanced()); | ||
| } | ||
| i += 1; | ||
| break; | ||
| } | ||
| c => { | ||
| current.push(c); | ||
| i += 1; | ||
| } | ||
| } | ||
| }, | ||
| Some(_) => loop { | ||
| // Single quotes: only \' is an escape. | ||
| if i >= line.len() { | ||
| return Err(unbalanced()); | ||
| } | ||
| match line[i] { | ||
| b'\\' if i + 1 < line.len() && line[i + 1] == b'\'' => { | ||
| current.push(b'\''); | ||
| i += 2; | ||
| } | ||
| b'\'' => { | ||
| if i + 1 < line.len() && line[i + 1] != b' ' && line[i + 1] != b'\t' { | ||
| return Err(unbalanced()); | ||
| } | ||
| i += 1; | ||
| break; | ||
| } | ||
| c => { | ||
| current.push(c); | ||
| i += 1; | ||
| } | ||
| } | ||
| }, | ||
| None => { | ||
| while i < line.len() { | ||
| match line[i] { | ||
| b' ' | b'\t' | b'\n' | b'\r' | 0x0b | 0x0c => break, | ||
| c => { | ||
| current.push(c); | ||
| i += 1; | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
Comment on lines
+235
to
+245
There was a problem hiding this comment. 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win 🧩 Analysis chain🌐 Web query:
💡 Result: In Redis, the sdssplitargs function is used to parse inline command arguments. Its behavior regarding tokens containing double quotes is strict to prevent malformed input [1][2][3]. If a token is unquoted (i.e., not wrapped in double or single quotes), it must not contain double quotes [4][2]. Specifically, sdssplitargs enforces a rule where a closing quote (either double or single) must be immediately followed by a whitespace character or be at the very end of the input string [1][2][3]. If a double quote appears within what is otherwise an unquoted token—such as "foo"bar—the parser will interpret the first double quote as a closing quote and then encounter the subsequent characters (like 'b' or 'a') as a violation of this rule [1][2][3]. This violation triggers an error, and the function returns NULL [1][2][3]. The documentation explicitly cites "foo"bar as an example of an invalid format that causes the parser to fail [1][2][3]. This design ensures that quoted and unquoted segments are not ambiguously mixed, maintaining consistent parsing logic [4]. Citations:
🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- inline.rs relevant sections ---'
sed -n '50,90p;110,245p' src/protocol/inline.rs
printf '%s\n' '--- parser symbols and tests ---'
rg -n -C 3 'split_args_quoted|parse_inline|unbalanced|0x0b|0x0c|sdssplitargs' src/protocol tests 2>/dev/null || true
printf '%s\n' '--- tracked files containing inline parser tests ---'
git ls-files | rg '(^|/)(inline|protocol).*(_test|test|tests)?|inline\.rs$' || trueRepository: pilotspace/moon Length of output: 27423 🏁 Script executed: #!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
p = Path("src/protocol/inline.rs").read_text()
for needle in ("fn split_args_quoted", "fn split_args", "parse_frame_zerocopy"):
pos = p.find(needle)
print(f"{needle}: {pos}")
PY
printf '%s\n' '--- Redis reference implementation excerpt ---'
if command -v curl >/dev/null 2>&1; then
curl -L --fail --silent --show-error \
https://raw.githubusercontent.com/redis/redis/unstable/deps/hiredis/sds.c |
rg -n -C 18 'sdssplitargs|inq|insq' | head -n 180
else
printf '%s\n' 'curl unavailable'
fiRepository: pilotspace/moon Length of output: 6688 🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- parse_inline and token finalization ---'
sed -n '1,115p;230,280p' src/protocol/inline.rs
printf '%s\n' '--- inline parser tests after line 440 ---'
sed -n '280,520p' src/protocol/inline.rsRepository: pilotspace/moon Length of output: 12010 🏁 Script executed: #!/bin/bash
set -eu
python3 - <<'PY'
from enum import Enum
def redis_split(line: bytes):
out, i = [], 0
whitespace = b" \n\r\t\v\f"
while True:
while i < len(line) and line[i] in whitespace:
i += 1
if i >= len(line):
return out
cur = bytearray()
inq = insq = False
while True:
c = line[i] if i < len(line) else 0
if inq:
if c == 92 and i + 1 < len(line) and line[i+1] == ord("x") and i + 3 < len(line):
# Representative inputs below do not use hex escapes.
cur.extend(line[i+1:i+2]); i += 2
elif c == 92 and i + 1 < len(line):
cur.append(line[i+1]); i += 2
elif c == ord('"'):
if i + 1 < len(line) and line[i+1] not in whitespace:
return None
inq = False; i += 1; break
elif i >= len(line):
return None
else:
cur.append(c); i += 1
elif insq:
if c == 92 and i + 1 < len(line) and line[i+1] == ord("'"):
cur.append(ord("'")); i += 2
elif c == ord("'"):
if i + 1 < len(line) and line[i+1] not in whitespace:
return None
insq = False; i += 1; break
elif i >= len(line):
return None
else:
cur.append(c); i += 1
else:
if i >= len(line) or line[i] in whitespace:
break
if c == ord('"'):
inq = True; i += 1
elif c == ord("'"):
insq = True; i += 1
else:
cur.append(c); i += 1
out.append(bytes(cur))
def moon_split(line: bytes):
# This models split_args_quoted's relevant branches, including its
# space/tab-only leading and closing checks.
out, i = [], 0
while True:
while i < len(line) and line[i] in b" \t":
i += 1
if i >= len(line):
return out
cur = bytearray()
quote = line[i] if line[i] in (ord('"'), ord("'")) else None
if quote is not None:
i += 1
if quote == ord('"') or quote == ord("'"):
while True:
if i >= len(line): return None
c = line[i]
if c == quote:
if i + 1 < len(line) and line[i+1] not in b" \t":
return None
i += 1; break
cur.append(c); i += 1
else:
while i < len(line) and line[i] not in b" \t\n\r\v\f":
cur.append(line[i]); i += 1
out.append(bytes(cur))
cases = [b'SET k v"x', b'SET k v"x foo"', b'SET k "v"x', b'SET k "v" x',
b'SET\vkey value', b'SET key\v"value"', b'SET key "value"\vnext']
for line in cases:
print(line, "redis=", redis_split(line), "moon=", moon_split(line))
PYRepository: pilotspace/moon Length of output: 145 🏁 Script executed: #!/bin/bash
set -eu
python3 - <<'PY'
WS_REDIS = set(b" \n\r\t\v\f")
WS_MOON_LEADING = set(b" \t")
WS_MOON_UNQUOTED = set(b" \t\n\r\v\f")
def redis_split(line, limit=1000):
out, i, steps = [], 0, 0
while True:
while i < len(line) and line[i] in WS_REDIS:
i += 1
if i >= len(line):
return ("ok", out)
cur = bytearray()
inq = insq = False
while True:
steps += 1
if steps > limit:
return ("stuck", i)
c = line[i] if i < len(line) else 0
if inq:
if c == ord('"'):
if i + 1 < len(line) and line[i + 1] not in WS_REDIS:
return ("error", "closing quote")
inq = False; i += 1; break
cur.append(c); i += 1
elif insq:
if c == ord("'"):
if i + 1 < len(line) and line[i + 1] not in WS_REDIS:
return ("error", "closing quote")
insq = False; i += 1; break
cur.append(c); i += 1
else:
if i >= len(line) or c in WS_REDIS:
break
if c == ord('"'):
inq = True; i += 1
elif c == ord("'"):
insq = True; i += 1
else:
cur.append(c); i += 1
if inq or insq:
return ("error", "unbalanced quote")
out.append(bytes(cur))
def moon_split(line, limit=1000):
out, i, steps = [], 0, 0
while True:
while i < len(line) and line[i] in WS_MOON_LEADING:
i += 1
if i >= len(line):
return ("ok", out)
cur = bytearray()
quote = line[i] if line[i] in (ord('"'), ord("'")) else None
if quote is not None:
i += 1
while True:
steps += 1
if steps > limit:
return ("stuck", i)
if quote is not None:
if i >= len(line):
return ("error", "unbalanced quote")
c = line[i]
if c == quote:
if i + 1 < len(line) and line[i + 1] not in WS_MOON_LEADING:
return ("error", "closing quote")
i += 1; break
cur.append(c); i += 1
else:
if i >= len(line) or line[i] in WS_MOON_UNQUOTED:
break
cur.append(line[i]); i += 1
out.append(bytes(cur))
cases = [
b'SET k v"x',
b'SET k v"x foo"',
b'SET k "v"x',
b'SET k "v" x',
b'SET key\v"value"',
b'SET key\n"value"',
b'SET key "value"\vnext',
]
for case in cases:
print(repr(case))
print(" redis:", redis_split(case))
print(" moon :", moon_split(case))
PYRepository: pilotspace/moon Length of output: 820 🏁 Script executed: #!/bin/bash
set -eu
python3 - <<'PY'
WS_REDIS = set(b" \n\r\t\v\f")
WS_MOON_LEADING = set(b" \t")
WS_MOON_UNQUOTED = set(b" \t\n\r\v\f")
def redis_split(line, limit=1000):
out, i, steps = [], 0, 0
while True:
while i < len(line) and line[i] in WS_REDIS:
i += 1
if i >= len(line):
return ("ok", out)
cur = bytearray()
inq = insq = False
while True:
steps += 1
if steps > limit:
return ("stuck", i)
if i >= len(line):
if inq or insq:
return ("error", "unbalanced quote")
break
c = line[i]
if inq:
if c == ord('"'):
if i + 1 < len(line) and line[i + 1] not in WS_REDIS:
return ("error", "closing quote")
inq = False; i += 1; break
cur.append(c); i += 1
elif insq:
if c == ord("'"):
if i + 1 < len(line) and line[i + 1] not in WS_REDIS:
return ("error", "closing quote")
insq = False; i += 1; break
cur.append(c); i += 1
else:
if c in WS_REDIS:
break
if c == ord('"'):
inq = True; i += 1
elif c == ord("'"):
insq = True; i += 1
else:
cur.append(c); i += 1
out.append(bytes(cur))
def moon_split(line, limit=1000):
out, i, steps = [], 0, 0
while True:
while i < len(line) and line[i] in WS_MOON_LEADING:
i += 1
if i >= len(line):
return ("ok", out)
cur = bytearray()
quote = line[i] if line[i] in (ord('"'), ord("'")) else None
if quote is not None:
i += 1
while True:
steps += 1
if steps > limit:
return ("stuck", i)
if quote is not None:
if i >= len(line):
return ("error", "unbalanced quote")
c = line[i]
if c == quote:
if i + 1 < len(line) and line[i + 1] not in WS_MOON_LEADING:
return ("error", "closing quote")
i += 1; break
cur.append(c); i += 1
else:
if i >= len(line) or line[i] in WS_MOON_UNQUOTED:
break
cur.append(line[i]); i += 1
out.append(bytes(cur))
for case in (
b'SET k v"x',
b'SET k v"x foo"',
b'SET k "v"x',
b'SET k "v" x',
b'SET key\v"value"',
b'SET key\n"value"',
b'SET key "value"\vnext',
):
print(repr(case))
print(" redis:", redis_split(case))
print(" moon :", moon_split(case))
PYRepository: pilotspace/moon Length of output: 834 Match Redis quote and whitespace handling in Treat 🤖 Prompt for AI Agents |
||
| } | ||
| args.push(Frame::BulkString(Bytes::from(current))); | ||
| } | ||
| } | ||
|
|
||
| /// SIMD-accelerated CRLF position finder. Returns position of \r. | ||
| #[inline] | ||
| fn find_crlf_position(buf: &[u8]) -> Option<usize> { | ||
|
|
||
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
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the
wire_textdoc comment.The doc says the method "Returns
Nonefor the two byte-carrying variants". The signature is-> &'static str, so it never returnsNone; the byte-carrying variants return the placeholder"Protocol error: expected '$', got '?'". Reword the doc to describe the placeholder.📝 Proposed doc fix
📝 Committable suggestion
🤖 Prompt for AI Agents