twitter / twemproxy

A fast, light-weight proxy for memcached and redis
Apache License 2.0
12.16k stars 2.06k forks source link

command with no keys, the return value is inconsistent with redis #661

Open 631086083 opened 2 years ago

631086083 commented 2 years ago

Describe the bug hello,when I uses a mget with no keys ,the proxy close the conn from client to server. And the client (go-redis v8) reply a EOF is not we expected. such as get and other commands

To Reproduce I uses a mget with no keys.

req decode is : *1\r\n#4\r\nMget\r\n

Expected behavior return: ERR wrong number of arguments for 'mget' command

TysonAndre commented 2 years ago

Checking for an invalid number of arguments (e.g. 0 for mget, odd for mset, etc.) is probably doable.

For redis, mget is well-formed (an array *1 with 1 element that's the command name and 0 args), for memcached, get with 0 arguments is not allowed by the protocol but also sending ERROR there may work.

Retrieval command:
------------------

The retrieval commands "get" and "gets" operate like this:

get <key>*\r\n
gets <key>*\r\n

- <key>* means one or more key strings separated by whitespace.
» telnet 127.0.0.1 11211
Trying 127.0.0.1...
Connected to 127.0.0.1.
Escape character is '^]'.
get   
ERROR
get k
END
631086083 commented 2 years ago

hello, I made a brief modification. When the number of parameters does not match, an error is returned. When a tcp connection is working normally, it will not be actively closed by the proxy, even if a command that does not conform to the rules is sent.

Do you think this can be merged into the master branch?

https://github.com/631086083/twemproxy/commit/3736ff568c91ae0588672fbc87230445c5827203

TysonAndre commented 2 years ago

That seems right and I'd probably merge that if I don't find anything else during the review (and the CLA is signed) - it also needs to handle auth in the reply code.

if (r->type == MSG_REQ_REDIS_AUTH && array_len(r->keys) > 0) { is what it should be, I think, see redis_handle_auth_req

127.0.0.1:6379> auth
(error) ERR wrong number of arguments for 'auth' command
631086083 commented 2 years ago

https://github.com/631086083/twemproxy/commit/a7dbaee01373186add1124909ea42a222400d83b

This seems to be a very bad implementation. 囧