Skip to content

Commit

Permalink
Prevent server from exiting on ECONNABORTED (#1874)
Browse files Browse the repository at this point in the history
* 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
- giampaolo/pyftpdlib#105
  giampaolo/pyftpdlib@0f82232
  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
  • Loading branch information
grembo authored Aug 20, 2024
1 parent c8754f3 commit b4c3518
Showing 1 changed file with 16 additions and 2 deletions.
18 changes: 16 additions & 2 deletions tonic/src/transport/server/incoming.rs
Original file line number Diff line number Diff line change
@@ -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},
Expand All @@ -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)?
}
}
}
}
}
Expand Down Expand Up @@ -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 => {
Expand Down

0 comments on commit b4c3518

Please sign in to comment.