fix(resp,limits): reject nested arrays, store-down latch, clamp dial to call budget

Reject a nested array reply instead of recursing into it. decodeReply
now takes a depth parameter; a '*' type byte is accepted only at
depth 0, so an element that is itself an array (decodeArray calls
decodeReply at depth+1) is a protocol error rather than a recursive
call. A crafted "*1\r\n*1\r\n..." stream repeated could otherwise
recurse to Go's stack limit — a fatal, unrecoverable error, not a
panic recoverPanic can catch — from only a few megabytes of wire
input. Also lower respMaxArrayLen from 1<<20 to 1024: none of this
client's commands (AUTH, SELECT, INCRBY, EXPIRE, GET) ever return an
array reply at all, so even 1024 is generous headroom, not a limit
tuned to real traffic.

Add a store-down latch to the limiter (limits.go): after any store
operation fails, record the failure time; for storeDownLatchFor (5s)
afterward, every storeIncrBy/storeGet call short-circuits straight to
the fail-open/fail-closed policy without attempting the store at all.
One request can touch several counters (its own and its group's,
request/token/cost, day/month) — each respClient call is already
bounded to ~respCallTimeout on its own, but without the latch every
one of those still paid a fresh bounded wait against a store that had
just proven unreachable. After the window, the next operation probes
the real store again: a natural half-open retry, no separate recovery
state. Guarded by the same logMu mutex lastLogAt already uses for this
kind of small timestamp state.

Clamp respClient's dial timeout to the remaining call budget:
dialTimeoutFor returns min(respDialTimeout, time.Until(deadline)),
with a respDeadlineExceededErr (implements net.Error, Timeout() true)
returned outright if the deadline has already elapsed — e.g. this
call queued behind the client's mutex for its whole budget while an
earlier call was stuck. Without this, a future increase to
respDialTimeout alone, leaving respCallTimeout untouched, could
silently let one dial attempt consume more of a call's total budget
than the call-level deadline is supposed to allow.

New tests: a nested-array reply and an array length one over the new
1024 cap, both asserted as protocol errors; a countingErrorStore stub
proving a second checkAndCount within the latch window makes zero
additional store calls, and that the store is probed again once the
window elapses. Tightened TestLimiter_FailOpenTrue_
HungRedisServer_FallsBackQuickly's bound from <6s to <3s now that the
latch collapses its two store ops into one real network attempt.
This commit is contained in:
2026-08-20 02:29:41 +01:00
parent 7ee635cd27
commit 2acc0a2a84
5 changed files with 264 additions and 68 deletions
+43 -17
View File
@@ -36,10 +36,13 @@ func TestEncodeCommand(t *testing.T) {
// TestDecodeReply covers every RESP2 type respClient must decode: simple
// string, error, integer, bulk string (including the null bulk "$-1"), and
// array (including the null array "*-1" and nesting).
// a single top-level array (including the null array "*-1"). Every call
// passes depth 0 — a top-level reply, matching every real call site in
// resp.go (attemptPipelineLocked, handshakeLocked, and decodeArray's own
// element loop, which passes depth+1).
func TestDecodeReply(t *testing.T) {
t.Run("simple string", func(t *testing.T) {
v, err := decodeReply(newReader("+OK\r\n"))
v, err := decodeReply(newReader("+OK\r\n"), 0)
if err != nil {
t.Fatalf("decodeReply: %v", err)
}
@@ -49,7 +52,7 @@ func TestDecodeReply(t *testing.T) {
})
t.Run("error reply decodes to respErr, not a decode error", func(t *testing.T) {
v, err := decodeReply(newReader("-ERR wrong number of arguments\r\n"))
v, err := decodeReply(newReader("-ERR wrong number of arguments\r\n"), 0)
if err != nil {
t.Fatalf("decodeReply returned an error for a well-formed RESP error reply: %v", err)
}
@@ -63,7 +66,7 @@ func TestDecodeReply(t *testing.T) {
})
t.Run("integer", func(t *testing.T) {
v, err := decodeReply(newReader(":42\r\n"))
v, err := decodeReply(newReader(":42\r\n"), 0)
if err != nil {
t.Fatalf("decodeReply: %v", err)
}
@@ -73,7 +76,7 @@ func TestDecodeReply(t *testing.T) {
})
t.Run("negative integer", func(t *testing.T) {
v, err := decodeReply(newReader(":-7\r\n"))
v, err := decodeReply(newReader(":-7\r\n"), 0)
if err != nil {
t.Fatalf("decodeReply: %v", err)
}
@@ -83,7 +86,7 @@ func TestDecodeReply(t *testing.T) {
})
t.Run("bulk string", func(t *testing.T) {
v, err := decodeReply(newReader("$5\r\nhello\r\n"))
v, err := decodeReply(newReader("$5\r\nhello\r\n"), 0)
if err != nil {
t.Fatalf("decodeReply: %v", err)
}
@@ -94,7 +97,7 @@ func TestDecodeReply(t *testing.T) {
})
t.Run("null bulk (missing key)", func(t *testing.T) {
v, err := decodeReply(newReader("$-1\r\n"))
v, err := decodeReply(newReader("$-1\r\n"), 0)
if err != nil {
t.Fatalf("decodeReply: %v", err)
}
@@ -103,8 +106,8 @@ func TestDecodeReply(t *testing.T) {
}
})
t.Run("array", func(t *testing.T) {
v, err := decodeReply(newReader("*2\r\n:5\r\n:1\r\n"))
t.Run("array (flat, not nested)", func(t *testing.T) {
v, err := decodeReply(newReader("*2\r\n:5\r\n:1\r\n"), 0)
if err != nil {
t.Fatalf("decodeReply: %v", err)
}
@@ -115,7 +118,7 @@ func TestDecodeReply(t *testing.T) {
})
t.Run("null array", func(t *testing.T) {
v, err := decodeReply(newReader("*-1\r\n"))
v, err := decodeReply(newReader("*-1\r\n"), 0)
if err != nil {
t.Fatalf("decodeReply: %v", err)
}
@@ -125,12 +128,12 @@ func TestDecodeReply(t *testing.T) {
})
t.Run("malformed type byte is a decode error", func(t *testing.T) {
if _, err := decodeReply(newReader("?nope\r\n")); err == nil {
if _, err := decodeReply(newReader("?nope\r\n"), 0); err == nil {
t.Fatal("want error for an unknown reply type byte")
}
})
// The following four subtests are review item 3: decodeBulk/
// The following subtests are review item 3 (round 1): decodeBulk/
// decodeArray must reject an out-of-range wire length before
// allocating, and decodeBulk must validate its CRLF trailer.
@@ -139,25 +142,25 @@ func TestDecodeReply(t *testing.T) {
// slice length; make([]byte, n+2) with that unguarded would
// panic. Reaching this line without panicking already proves the
// fix; the error check confirms it fails cleanly too.
if _, err := decodeReply(newReader("$9223372036854775807\r\n")); err == nil {
if _, err := decodeReply(newReader("$9223372036854775807\r\n"), 0); err == nil {
t.Fatal("want an error for a bulk length exceeding respMaxBulkLen")
}
})
t.Run("bulk length just over the cap is a protocol error", func(t *testing.T) {
if _, err := decodeReply(newReader(fmt.Sprintf("$%d\r\n", respMaxBulkLen+1))); err == nil {
if _, err := decodeReply(newReader(fmt.Sprintf("$%d\r\n", respMaxBulkLen+1)), 0); err == nil {
t.Fatal("want an error for a bulk length one over respMaxBulkLen")
}
})
t.Run("huge array length is a protocol error", func(t *testing.T) {
if _, err := decodeReply(newReader("*1000000000\r\n")); err == nil {
if _, err := decodeReply(newReader("*1000000000\r\n"), 0); err == nil {
t.Fatal("want an error for an array length exceeding respMaxArrayLen")
}
})
t.Run("bulk with wrong trailer is a protocol error", func(t *testing.T) {
if _, err := decodeReply(newReader("$5\r\nhelloXX")); err == nil {
if _, err := decodeReply(newReader("$5\r\nhelloXX"), 0); err == nil {
t.Fatal("want an error when a bulk reply's trailing bytes are not CRLF")
}
})
@@ -167,10 +170,33 @@ func TestDecodeReply(t *testing.T) {
// readLine would keep buffering every byte offered until the
// reader errors (e.g. EOF) rather than bailing out early.
huge := strings.Repeat("x", respMaxLineLen+10)
if _, err := decodeReply(newReader("+" + huge)); err == nil {
if _, err := decodeReply(newReader("+"+huge), 0); err == nil {
t.Fatal("want an error for a reply line exceeding respMaxLineLen")
}
})
// The following subtests are review item 1 (round 3): decodeReply
// must reject an array nested inside another array's elements — the
// only way a reply could otherwise recurse — and the array cap must
// be the new, much lower respMaxArrayLen (1024), since no command
// this client sends ever gets an array reply back at all.
t.Run("nested array reply is a protocol error, not unbounded recursion", func(t *testing.T) {
// A one-element array whose element is itself a one-element array.
// Repeated, this pattern is what would otherwise recurse to Go's
// stack limit (a fatal, unrecoverable error) from only a few
// megabytes of wire input; one level already exercises the guard,
// since decodeReply rejects any '*' at depth > 0 outright.
if _, err := decodeReply(newReader("*1\r\n*1\r\n:1\r\n"), 0); err == nil {
t.Fatal("want an error for an array nested inside another array")
}
})
t.Run("array length just over the new (1024) cap is a protocol error", func(t *testing.T) {
if _, err := decodeReply(newReader(fmt.Sprintf("*%d\r\n", respMaxArrayLen+1)), 0); err == nil {
t.Fatal("want an error for an array length one over respMaxArrayLen")
}
})
}
// newReader wraps s in a *bufio.Reader, matching what respClient reads