From b4c351840ab2eaeb2734f194a40a755c4ca02424 Mon Sep 17 00:00:00 2001
From: grembo <freebsd@grem.de>
Date: Tue, 20 Aug 2024 11:32:29 +0200
Subject: [PATCH] Prevent server from exiting on ECONNABORTED (#1874)

* Prevent server from exiting on ECONNABORTED

On FreeBSD, accept can fail with ECONNABORTED, which means that
"A connection arrived, but it was on the listen queue"
(see `man 2 accept`).

Without this patch, a server without the tls feature  exits returing 0
in this case, which makes it vulnerable not only to intentional denial
of service, but also to unintentional crashes, e.g., by haproxy TCP
health checks.

The problem can be reproduced on any FreeBSD system by running the
tonic "helloworld" example (without feature TLS) and then sending a
packet using nmap:

    cd examples
    cargo run --bin helloworld-server --no-default-features &
    nmap -sT -p 50051 -6 ::1
    # server exits

When running the example with the feature tls enabled, it won't exit
(as the tls event loop in tonic/src/transport/server/incoming.rs
handles errors gracefully):

    cd examples
    cargo run --bin helloworld-server --no-default-features \
      features=tls &
    nmap -sT -p 50051 -6 ::1
    # server keeps running

This patch is not optimal - it removes some generic error parameters
to gain access to `std::io::Error::kind()`. The logic itself should be
sound.

See also:

- https://man.freebsd.org/cgi/man.cgi?accept(2)
  Accept man page
- https://github.com/giampaolo/pyftpdlib/issues/105
  https://github.com/giampaolo/pyftpdlib/commit/0f822329e3431ec1bc6250c03e938c65a61b2eb4
  Basically the same issue (and its fix) in another project

* Handle ECONNABORTED without breaking APIs

This simplifies the previous patch a lot. The string search
is ugly (also not 100% if it's needed or if we could handle
errors like in the TLS enabled accept loop).

* Next iteration based on feedback

* More review feedback

* Only use std::io if needed
---
 tonic/src/transport/server/incoming.rs | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/tonic/src/transport/server/incoming.rs b/tonic/src/transport/server/incoming.rs
index fe578b2a5..b7d2f8d6c 100644
--- a/tonic/src/transport/server/incoming.rs
+++ b/tonic/src/transport/server/incoming.rs
@@ -1,6 +1,8 @@
 use super::service::ServerIo;
 #[cfg(feature = "tls")]
 use super::service::TlsAcceptor;
+#[cfg(not(feature = "tls"))]
+use std::io;
 use std::{
     net::{SocketAddr, TcpListener as StdTcpListener},
     pin::{pin, Pin},
@@ -27,7 +29,19 @@ where
         let mut incoming = pin!(incoming);
 
         while let Some(item) = incoming.next().await {
-            yield item.map(ServerIo::new_io)?
+            yield match item {
+                Ok(_) => item.map(ServerIo::new_io)?,
+                Err(e) => {
+                    let e = e.into();
+                    tracing::debug!(error = %e, "accept loop error");
+                    if let Some(e) = e.downcast_ref::<io::Error>() {
+                        if e.kind() == io::ErrorKind::ConnectionAborted {
+                            continue;
+                        }
+                    }
+                    Err(e)?
+                }
+            }
         }
     }
 }
@@ -65,7 +79,7 @@ where
                 }
 
                 SelectOutput::Err(e) => {
-                    tracing::debug!(message = "Accept loop error.", error = %e);
+                    tracing::debug!(error = %e, "accept loop error");
                 }
 
                 SelectOutput::Done => {