Skip to content

Loading…

Redis-cli --pipe blocks forever when the piped data contains syntax errors #681

Closed
Corni opened this Issue · 2 comments

2 participants

@Corni

Hey,
when I feed redis-cli --pipe with the following example data (or any other broken protocol format)
blablub
it gives me the following errors:
root@alfred:~# cat test.redis |redis-cli --pipe
All data transferred. Waiting for the last reply...
ERR unknown command 'blablub*2'
ERR unknown command '$4'
ERR wrong number of arguments for 'echo' command
ERR unknown command '$20'
ERR unknown command 'W뇽'

and then hangs forever. We came across this when we generated broken commands and redis-cli hang on our production servers for some hours. In this case, redis-cli should just throw an error and exit.
All this was obvserved with redis 2.4.15, but as the release notes don't contain a notice about this bug, I expect this bug to still remain in the latest stable release

@daniel-levin daniel-levin added a commit to daniel-levin/redis that referenced this issue
@daniel-levin daniel-levin Stops redis-cli --pipe from blocking with malformed protocol data (#681)
This fixes the issue #681. Now, redis-cli exits when the piped data
contains syntax errors.
49d7388
@antirez
Owner

Hello, I'm not sure the right fix is the one proposed by PR #951, where once an error is detected the process aborts. This somewhat limits the applicability of --pipe, or at least that could be a different option, like, stop as soon as an error is detected in pipe mode. But the root cause of the issue is that that the final ECHO command is concatenated in a way that Redis can't parse it if the previous command is not correct, I would fix that instead... Writing a patch.

@antirez antirez added a commit that referenced this issue
@antirez redis-cli --pipe: send final ECHO in a safer way.
If the protocol read from stdin happened to contain grabage (invalid
random chars), in the previous implementation it was possible to end
with something like:

dksfjdksjflskfjl*2\r\n$4\r\nECHO....

That is invalid as the *2 should start into a new line. Now we prefix
the ECHO with a CRLF that has no effects on the server but prevents this
issues most of the times.

Of course if the offending wrong sequence is something like:

$3248772349\r\n

No one is going to save us as Redis will wait for data in the context of
a big argument, so this fix does not cover all the cases.

This partially fixes issue #681.
fbb97c6
@antirez antirez added a commit that referenced this issue
@antirez redis-cli --pipe: send final ECHO in a safer way.
If the protocol read from stdin happened to contain grabage (invalid
random chars), in the previous implementation it was possible to end
with something like:

dksfjdksjflskfjl*2\r\n$4\r\nECHO....

That is invalid as the *2 should start into a new line. Now we prefix
the ECHO with a CRLF that has no effects on the server but prevents this
issues most of the times.

Of course if the offending wrong sequence is something like:

$3248772349\r\n

No one is going to save us as Redis will wait for data in the context of
a big argument, so this fix does not cover all the cases.

This partially fixes issue #681.
24b3799
@antirez antirez added a commit that referenced this issue
@antirez redis-cli: introduced --pipe-timeout.
When in --pipe mode, after all the data transfer to the server is
complete, now redis-cli waits at max the specified amount of
seconds (30 by default, use 0 to wait forever) without receiving any
reply at all from the server. After this time limit the operation is
aborted with an error.

That's related to issue #681.
3e35012
@antirez antirez added a commit that referenced this issue
@antirez redis-cli: introduced --pipe-timeout.
When in --pipe mode, after all the data transfer to the server is
complete, now redis-cli waits at max the specified amount of
seconds (30 by default, use 0 to wait forever) without receiving any
reply at all from the server. After this time limit the operation is
aborted with an error.

That's related to issue #681.
1135e9f
@antirez
Owner

Fixed, closing.

@antirez antirez closed this
@JackieXie168 JackieXie168 pushed a commit that referenced this issue
@jdoliner jdoliner Adds a coro pool for backfill chunks.
Backfill chunks go in a queue and are then popped off by a coro pool
rather than just spawning a new coroutine. Notice this has a huge bug in
that backfills don't block on the things in the queue. That's coming up
next.

FixeZ (in part) #681.
fc43745
@snario snario referenced this issue in ericrasmussen/pyramid_redis_sessions
Closed

synchronize to current redis-py #56

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.