fix: prevent reuse of the stream after an error#7014
Conversation
|
I take it this PR is not ready for review? (I'm wondering because you mentioned it from the other PR) |
link2xt
force-pushed
the
link2xt/error-capturing-stream
branch
2 times, most recently
from
204c889 to
d0ec495
Compare
|
It's ready now. |
link2xt
force-pushed
the
link2xt/error-capturing-stream
branch
from
d0ec495 to
d7bd4d0
Compare
Hocuri
left a comment
Hocuri
left a comment
There was a problem hiding this comment.
Thanks, fingers crossed 🤞 that this will help!
When a stream timeouts, `tokio_io_timeout::TimeoutStream` returns an error once, but then allows to keep using the stream, e.g. calling `poll_read()` again. This can be dangerous if the error is ignored. For example in case of IMAP stream, if IMAP command is sent, but then reading the response times out and the error is ignored, it is possible to send another IMAP command. In this case leftover response from a previous command may be read and interpreted as the response to the new IMAP command. ErrorCapturingStream wraps the stream to prevent its reuse after an error.
link2xt
force-pushed
the
link2xt/error-capturing-stream
branch
from
d7bd4d0 to
3010d28
Compare
|
While this prevents reading more data after getting a low-level error, this unfortunately may not protect from misuse in case of additional buffering on top. After looking at the implementation of tokio BufReader it does not seem possible on a first glance to get a read error and then get access to the buffer by doing a read of different size, but we also have buffering inside the deflate decompressor used by IMAP stream and |
|
|
||
| use crate::net::SessionStream; | ||
|
|
||
| /// Stream that remembers the first error |
There was a problem hiding this comment.
I only remembers whether an error took place. The comment should be fixed
| /// If true, the stream has already returned an error once. | ||
| /// | ||
| /// All read and write operations return error in this case. | ||
| is_broken: bool, |
There was a problem hiding this comment.
Btw, separate flags is_{read,write}_broken may be introduced, though this is probably not necessary. Still, this way the reader may finish reading useful responses from the server even if writing breaks
There was a problem hiding this comment.
If we write some command, and writing breaks recoverably (e.g. times out) we don't want whatever response arrives to be read and misinterpreted as a response to the next command.
There was a problem hiding this comment.
It can't be interpreted as a response to the next command because the next command can't even be written to the socket (because is_write_broken persists). But responses to the previous commands can be read, what is wrong with them? Usually protocols allow commands to be sent in batches and responses can be processed asynchronously
I have now implemented similar protection at the higher level where it works better: chatmail/async-imap#122 |
3 participants
When a stream timeouts,
tokio_io_timeout::TimeoutStreamreturns an error once, but then allows to keep using the stream, e.g. callingpoll_read()again.This can be dangerous if the error is ignored.
For example in case of IMAP stream,
if IMAP command is sent,
but then reading the response
times out and the error is ignored,
it is possible to send another IMAP command.
In this case leftover response
from a previous command may be read
and interpreted as the response
to the new IMAP command.
ErrorCapturingStream wraps the stream
to prevent its reuse after an error.