Opened 8 years ago

Closed 8 years ago

#5534 defect closed invalid (invalid)

calling loseConnection on a TLSMemoryBIOProtocol instance might never succeed

Reported by: Adam Goodman Owned by:
Priority: high Milestone:
Component: core Keywords:
Cc: Branch:
Author:

Description

When you call .loseConnection() on a TLSMemoryBIOProtocol instance, the protocol instance won't actually close its underlying transport until it has gotten a chance to send a TLS-layer close alert. In particular, if a protocol instance has the '_writeBlockedOnRead' flag set, it will wait until this state has cleared, then shut down the connection cleanly.

Normally, this is probably a good thing, but if you're communicating with an unresponsive peer, this could make the connection hang open indefinitely.

One simple example here would be a TCP server that accepts incoming connections and then does absolutely nothing. The TLS client will attempt to initiate a handshake and quickly get stuck in a '_writeBlockedOnRead' state, and then be unable to shut itself down.

I came across this while trying to enforce a timeout with an HTTPClientFactory. Notably, (in Twisted 11.1.0 and newer) the HTTP client factory does not propagate its timeout error back up the deferred chain until its connection has successfully closed.

I've attached a simple proof-of-concept. It starts a local TCP server which accepts incoming connections, does nothing for 60 seconds, then closes them. If we attempt to perform a plain HTTP request against it, the request times out correctly. If we attempt to perform an HTTPS request, then it doesn't time out until the server closes the connection; if the server never did so, then the connection would presumably remain open forever.

Perhaps the loseConnection method on TLSMemoryBIOProtocol needs to set some sort of timeout, after which is simply shuts down the underlying TCP connection?

Attachments (1)

poc.py (1.5 KB) - added by Adam Goodman 8 years ago.
proof-of-concept

Download all attachments as: .zip

Change History (2)

Changed 8 years ago by Adam Goodman

Attachment: poc.py added

proof-of-concept

comment:1 Changed 8 years ago by Itamar Turner-Trauring

Resolution: invalid
Status: newclosed

Thanks for filing a ticket!

However, this is probably not something we ought to be fixing in this layer. To begin with, this is not TLS-specific. For example, if you have data in your write buffer and you can loseConnection, the connection won't be lost until the data is written out... which will never happen if the other side is reading.

The general solution in these cases is to use callLater to call transport.abortConnection() if the connection hasn't been closed in a sufficient amount of time, which will then close the connection unconditionally. However, the specifics of how that should work (how long a timeout, are we ok as long as bytes are trickling or do we want hard cutoff etc.) are application specific so it seems like we oughtn't do it by default and leave it up to higher-level application code.

A utility function of some sort that makes it easier to support the common case would be useful, you could open a ticket for that if you wanted to work on that.

Note: See TracTickets for help on using tickets.