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 <d.csapak@proxmox.com>
 [ TL: reword commit message and change error to ConnectionAborted ]
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>

diff --git a/proxmox-rest-server/src/connection.rs b/proxmox-rest-server/src/connection.rs
index 3815a8f40c082410c8e0091f4a57541f616003d2..d9fa0a735d0819d8e6ced43ab8db350ae6c59da4 100644 (file)
--- 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)),