Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
631086083 opened this issue Dec 30, 2021 · 4 comments
Open

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

631086083 opened this issue Dec 30, 2021 · 4 comments

Comments

@631086083
Copy link

631086083 commented Dec 30, 2021

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 631086083 changed the title mget no keys, the return value is inconsistent with redis command with no keys, the return value is inconsistent with redis Dec 30, 2021
@TysonAndre
Copy link
Collaborator

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
Copy link
Author

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?

631086083@3736ff5

@TysonAndre
Copy link
Collaborator

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
Copy link
Author

631086083@a7dbaee

This seems to be a very bad implementation. 囧

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants