From dc9531d3023e8975789fc36ab17f8aa9c2907ac5 Mon Sep 17 00:00:00 2001 From: Dominik Csapak Date: Wed, 13 Nov 2024 11:39:37 +0100 Subject: [PATCH] fix #5868: rest-server: handshake detection: avoid infinite loop on connections abort When a connection is closed by the client before we have enough data to determine if it contains a TLS Handshake or not, the socket stays in a readable state. While we setup a tokio backed timeout of 10s for the connection build-up here, this timeout does not trigger on said early connection abort from the client side, causing then the async_io loop to endlessly loop around peeking into the client, which always returns the last available bytes before the connection was closed. This in turn causes 100% CPU usage for one of the PBS threads. The timeout not triggering is rather odd, and does indicate some potential for further improvement in tokio itself, but our questionable use of the WouldBlock error does violate the API contract, so this is not a clear cut. Such an early connection abort is often triggered by monitoring solutions, which use it to relatively cheaply check if TCP on a port still works as "is service up" heuristic. To fix this, save the amount of bytes peek returned and if they did not change between invocations of the callback, we can assume that the connection was closed and thus exit the connection attempt with an error. Signed-off-by: Dominik Csapak [ TL: reword commit message and change error to ConnectionAborted ] Signed-off-by: Thomas Lamprecht --- proxmox-rest-server/src/connection.rs | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/proxmox-rest-server/src/connection.rs b/proxmox-rest-server/src/connection.rs index 3815a8f4..d9fa0a73 100644 --- a/proxmox-rest-server/src/connection.rs +++ b/proxmox-rest-server/src/connection.rs @@ -477,6 +477,7 @@ impl AcceptBuilder { const HANDSHAKE_BYTES_LEN: usize = 5; let future = async { + let mut previous_peek_len = 0; incoming_stream .async_io(tokio::io::Interest::READABLE, || { let mut buf = [0; HANDSHAKE_BYTES_LEN]; @@ -500,7 +501,15 @@ impl AcceptBuilder { // This means we will peek into the stream's queue until we got // HANDSHAKE_BYTE_LEN bytes or an error. Ok(peek_len) if peek_len < HANDSHAKE_BYTES_LEN => { - Err(io::ErrorKind::WouldBlock.into()) + log::info!("peek_len = {peek_len}"); + // if we detect the same peek len again but still got a readable stream, + // the connection was probably closed, so abort here + if peek_len == previous_peek_len { + Err(io::ErrorKind::ConnectionAborted.into()) + } else { + previous_peek_len = peek_len; + Err(io::ErrorKind::WouldBlock.into()) + } } // Either we got Ok(HANDSHAKE_BYTES_LEN) or some error. res => res.map(|_| contains_tls_handshake_fragment(&buf)),