twemproxy icon indicating copy to clipboard operation
twemproxy copied to clipboard

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

Open 631086083 opened this issue 3 years ago • 4 comments

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

631086083 avatar Dec 30 '21 11:12 631086083

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

TysonAndre avatar Dec 30 '21 14:12 TysonAndre

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

631086083 avatar Jan 04 '22 10:01 631086083

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

TysonAndre avatar Jan 04 '22 15:01 TysonAndre

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

This seems to be a very bad implementation. 囧

631086083 avatar Jan 10 '22 03:01 631086083