diff --git a/adapter/redis_compat_commands.go b/adapter/redis_compat_commands.go index e44360be..245f7cdc 100644 --- a/adapter/redis_compat_commands.go +++ b/adapter/redis_compat_commands.go @@ -206,32 +206,87 @@ func (r *RedisServer) setnx(conn redcon.Conn, cmd redcon.Command) { conn.WriteInt(1) } +// clientSubcommandArgCount is the total cmd.Args length (including +// CLIENT + subcommand) required by no-operand CLIENT subcommands +// like GETNAME / ID / INFO. +const clientSubcommandArgCount = 2 + +// checkClientArity verifies cmd.Args has exactly want elements and +// writes the standard Redis wrong-arity error otherwise. Returns +// true when the caller should stop handling (bad arity). +func checkClientArity(conn redcon.Conn, cmd redcon.Command, sub string, want int) bool { + if len(cmd.Args) == want { + return false + } + conn.WriteError("ERR wrong number of arguments for 'client|" + strings.ToLower(sub) + "' command") + return true +} + +// clientSetName handles CLIENT SETNAME. SETNAME is shared with +// HELLO's SETNAME clause; both write into the same connState.clientName +// slot so a client that uses HELLO SETNAME once and then queries +// CLIENT GETNAME gets the right answer without having to re-issue +// CLIENT SETNAME. +func clientSetName(conn redcon.Conn, cmd redcon.Command, state *connState) { + if checkClientArity(conn, cmd, "SETNAME", clientSetNameArgCount) { + return + } + state.clientName = string(cmd.Args[2]) + conn.WriteString("OK") +} + +func clientGetName(conn redcon.Conn, cmd redcon.Command, state *connState) { + if checkClientArity(conn, cmd, "GETNAME", clientSubcommandArgCount) { + return + } + if state.clientName == "" { + conn.WriteNull() + return + } + conn.WriteBulkString(state.clientName) +} + +func (r *RedisServer) clientID(conn redcon.Conn, cmd redcon.Command, state *connState) { + if checkClientArity(conn, cmd, "ID", clientSubcommandArgCount) { + return + } + conn.WriteInt64(int64(r.ensureConnID(state))) //nolint:gosec // connID monotonic counter, guaranteed <= math.MaxInt64 in practice +} + +func (r *RedisServer) clientInfo(conn redcon.Conn, cmd redcon.Command, state *connState) { + if checkClientArity(conn, cmd, "INFO", clientSubcommandArgCount) { + return + } + id := r.ensureConnID(state) + conn.WriteBulkString(fmt.Sprintf("id=%d addr=%s name=%s", id, conn.RemoteAddr(), state.clientName)) +} + +// clientSetInfo handles CLIENT SETINFO . elastickv does +// not persist the advertised attributes (lib-name / lib-ver, etc.), but +// it MUST still enforce exact arity — otherwise `CLIENT SETINFO` with +// no operands returns OK and masks a client bug that real Redis would +// have surfaced as a wrong-arity error. +func clientSetInfo(conn redcon.Conn, cmd redcon.Command) { + if checkClientArity(conn, cmd, "SETINFO", clientSetInfoArgCount) { + return + } + conn.WriteString("OK") +} + func (r *RedisServer) client(conn redcon.Conn, cmd redcon.Command) { sub := strings.ToUpper(string(cmd.Args[1])) state := getConnState(conn) switch sub { case "SETINFO": - conn.WriteString("OK") + clientSetInfo(conn, cmd) case "SETNAME": - // SETNAME is shared with HELLO's SETNAME clause; both write into - // the same connState.clientName slot so a client that uses - // HELLO SETNAME once and then queries CLIENT GETNAME gets the - // right answer without having to re-issue CLIENT SETNAME. - if len(cmd.Args) >= clientSetNameMinArgs { - state.clientName = string(cmd.Args[2]) - } - conn.WriteString("OK") + clientSetName(conn, cmd, state) case "GETNAME": - if state.clientName == "" { - conn.WriteNull() - return - } - conn.WriteBulkString(state.clientName) + clientGetName(conn, cmd, state) case "ID": - conn.WriteInt64(int64(r.ensureConnID(state))) //nolint:gosec // connID monotonic counter, guaranteed <= math.MaxInt64 in practice + r.clientID(conn, cmd, state) case "INFO": - id := r.ensureConnID(state) - conn.WriteBulkString(fmt.Sprintf("id=%d addr=%s name=%s", id, conn.RemoteAddr(), state.clientName)) + r.clientInfo(conn, cmd, state) default: conn.WriteError("ERR unsupported CLIENT subcommand '" + sub + "'") } @@ -255,9 +310,20 @@ const ( // helloReplyArrayLen is the number of elements in the flat // alternating key/value reply: 7 pairs = 14 elements. helloReplyArrayLen = 14 - // clientSetNameMinArgs is the arg count at which CLIENT SETNAME - // has its operand (CLIENT + SETNAME + name = 3). - clientSetNameMinArgs = 3 + // clientSetNameArgCount is the EXACT cmd.Args length for CLIENT + // SETNAME (CLIENT + SETNAME + name = 3). Kept as an exact-arity + // constant — not a minimum — because checkClientArity compares + // `len(cmd.Args) == want`; renaming it to *MinArgs would invite a + // future refactor that "just" swaps the helper for a >= check and + // silently re-introduces the wrong-arity silent-OK bug. + clientSetNameArgCount = 3 + // clientSetInfoArgCount is the EXACT cmd.Args length for CLIENT + // SETINFO (CLIENT + SETINFO + attr + value = 4). Real Redis + // rejects any other arity for `client|setinfo`; without this + // check we would keep returning OK for `CLIENT SETINFO` with no + // operands, matching exactly the silent-success behaviour this + // PR is supposed to eliminate for every CLIENT subcommand. + clientSetInfoArgCount = 4 ) // helloParseError is the internal signal used by parseHelloArgs to diff --git a/adapter/redis_hello_test.go b/adapter/redis_hello_test.go index 4d3341a9..5c6fd15d 100644 --- a/adapter/redis_hello_test.go +++ b/adapter/redis_hello_test.go @@ -22,7 +22,7 @@ import ( // array-header) is the only way to distinguish `proto` (integer 2) // from `version` (bulk "7.0.0") or `modules` (empty array header). type helloReplyElement struct { - kind string // "bulk" | "int" | "arrayHeader" + kind string // "bulk" | "string" | "int" | "arrayHeader" | "null" str string num int64 } @@ -46,7 +46,10 @@ func (c *helloRecordingConn) Close() error { return nil } func (c *helloRecordingConn) WriteError(msg string) { c.err = msg } func (c *helloRecordingConn) WriteString(s string) { c.str = s - c.writes = append(c.writes, helloReplyElement{kind: "bulk", str: s}) + // Recorded as a distinct "string" kind so a future regression that + // swaps WriteBulkString for WriteString (or vice versa) fails the + // wire-shape assertions instead of passing silently. + c.writes = append(c.writes, helloReplyElement{kind: "string", str: s}) } func (c *helloRecordingConn) WriteBulk(bulk []byte) { c.writes = append(c.writes, helloReplyElement{kind: "bulk", str: string(bulk)}) @@ -419,3 +422,110 @@ elastickv_redis_requests_total{command="HELLO",node_address="10.0.0.1:50051",nod ) require.NoError(t, err) } + +func TestClient_SetName_RejectsWrongArity(t *testing.T) { + t.Parallel() + + r := newHelloTestServer(t, true) + conn := &helloRecordingConn{} + state := getConnState(conn) + state.clientName = "prev" + + // CLIENT SETNAME with no value (2 args total). + r.client(conn, redcon.Command{Args: [][]byte{ + []byte(cmdClient), []byte("SETNAME"), + }}) + require.Contains(t, conn.err, "wrong number of arguments") + require.Equal(t, "prev", state.clientName, "malformed SETNAME must not clobber clientName") + + // CLIENT SETNAME foo bar (4 args total) — also rejected. + conn2 := &helloRecordingConn{} + state2 := getConnState(conn2) + state2.clientName = "prev" + r.client(conn2, redcon.Command{Args: [][]byte{ + []byte(cmdClient), []byte("SETNAME"), []byte("foo"), []byte("bar"), + }}) + require.Contains(t, conn2.err, "wrong number of arguments") + require.Equal(t, "prev", state2.clientName) +} + +func TestClient_GetName_RejectsWrongArity(t *testing.T) { + t.Parallel() + + r := newHelloTestServer(t, true) + conn := &helloRecordingConn{} + + // CLIENT GETNAME extra (3 args total) — GETNAME takes no operand. + r.client(conn, redcon.Command{Args: [][]byte{ + []byte(cmdClient), []byte("GETNAME"), []byte("extra"), + }}) + require.Contains(t, conn.err, "wrong number of arguments") +} + +func TestClient_ID_RejectsWrongArity(t *testing.T) { + t.Parallel() + + r := newHelloTestServer(t, true) + conn := &helloRecordingConn{} + + // CLIENT ID junk (3 args total) — ID takes no operand. + r.client(conn, redcon.Command{Args: [][]byte{ + []byte(cmdClient), []byte("ID"), []byte("junk"), + }}) + require.Contains(t, conn.err, "wrong number of arguments") +} + +func TestClient_Info_RejectsWrongArity(t *testing.T) { + t.Parallel() + + r := newHelloTestServer(t, true) + conn := &helloRecordingConn{} + + // CLIENT INFO extra (3 args total) — INFO takes no operand. + r.client(conn, redcon.Command{Args: [][]byte{ + []byte(cmdClient), []byte("INFO"), []byte("extra"), + }}) + require.Contains(t, conn.err, "wrong number of arguments") +} + +// CLIENT SETINFO must enforce exact arity too; the prior implementation +// returned OK for any arity including zero operands, which silently +// hid client bugs that real Redis would reject as wrong-arity. Pin the +// "missing operands" and "extra operand" cases so a regression cannot +// re-introduce the silent-success behaviour. +func TestClient_SetInfo_RejectsWrongArity(t *testing.T) { + t.Parallel() + + r := newHelloTestServer(t, true) + + // CLIENT SETINFO with no operands (2 args total). + conn := &helloRecordingConn{} + r.client(conn, redcon.Command{Args: [][]byte{ + []byte(cmdClient), []byte("SETINFO"), + }}) + require.Contains(t, conn.err, "wrong number of arguments") + require.NotEqual(t, "OK", conn.str, + "missing-operand SETINFO must not silently return OK") + + // CLIENT SETINFO attr (3 args total) — missing value. + conn2 := &helloRecordingConn{} + r.client(conn2, redcon.Command{Args: [][]byte{ + []byte(cmdClient), []byte("SETINFO"), []byte("lib-name"), + }}) + require.Contains(t, conn2.err, "wrong number of arguments") + + // CLIENT SETINFO attr value extra (5 args total) — extra operand. + conn3 := &helloRecordingConn{} + r.client(conn3, redcon.Command{Args: [][]byte{ + []byte(cmdClient), []byte("SETINFO"), []byte("lib-name"), []byte("redis-go"), []byte("extra"), + }}) + require.Contains(t, conn3.err, "wrong number of arguments") + + // CLIENT SETINFO attr value (4 args total) — accepted shape. + conn4 := &helloRecordingConn{} + r.client(conn4, redcon.Command{Args: [][]byte{ + []byte(cmdClient), []byte("SETINFO"), []byte("lib-name"), []byte("redis-go"), + }}) + require.Empty(t, conn4.err, "well-formed SETINFO must not produce an error") + require.Equal(t, "OK", conn4.str, "well-formed SETINFO replies OK") +}