Read any pending data before shutting down stream #63
Conversation
levkk
changed the title
levkk
changed the title
| @@ -339,6 +339,10 @@ where | |||
| ready!(self.write_io(cx))?; | |||
| } | |||
|
|
|||
| while self.session.wants_read() { | |||
| ready!(self.read_io(cx))?; | |||
There was a problem hiding this comment.
should consider the case where return value is equal to 0?
According to rfc https://www.rfc-editor.org/rfc/rfc8446#page-87 I don't think this is necessary. And waiting for packet before shutdown is rather a DoS risk, as the other party may send TLS packets slowly enough to prevent the TCP connection from being closed. |
|
I'm considering having the TlsStream shutdown not actually do a TCP shutdown, which has two benefits
|
|
I think I agree. The bug seems to be more on the client side here. I've submitted a fix to SQLx which was my specific use case and that worked without this patch. Even if this patch is merged, I think there still will be a race condition between clients sending CloseNotify and us polling the socket for read data. |
|
I will close this PR and I will consider remove TCP shutdown in next major version. |
I ran into the issue where another Rustls client (
launchbadge/sqlx) is sending CloseNotify (correctly I believe) before shutting down the stream. If the server doesn't wait for it when shutting down the socket, the client throws:This PR ensures if there are any data still in the stream, it's read in and only then the stream is closed. All incoming data at this point is discarded, so this shouldn't be security issue I think, but I'm not an expert.