Missing error handling in Eio.Net.run_server if "accept" fails

[vog]

The current implementation of Eio.Net.run_server provides no error handling at all if the call to "accept" fails. This means that the whole server is terminated and all running response fibers are canceled.

Is this on purpose?

The typical cause for such a failure is "too many open files", which in that case means "too many open/accepted sockets". A typical soft limit is 1024 and a typical hard limit is 2048. However, the default value for max_connections is max_int, which means every naive usage of Eio.Net.run_server is vulnerable to a simple denial of service attack if more than 1024 or 2048 connections are opened in parallel.

Naively I would have expected (but maybe there are reasons against doing so?):

• that either max_connections has a low enough default value so this won't happen so easily (unless request handlers are opening tons of files or connections on their own),
• or that the call to accept resp. accept_fork is guarded such that on exception, only that single connection is lost, then a call to on_error resp. a separate on_accept_error, followed by some waiting time (something in the range of 5-500 milliseconds, perhaps also a parameter to run_server), and finally retry the next accept/_fork.

Moreover, I noticed that:

• The semaphore is not properly released in that case. Since it is acquired outside of accept_fork, it should be released in a protection around of accept_fork, not a protection of the request handler called by accept_fork.

My questions are:

• Would you accept a PR that fixes this?
• If so, which solution would you prefer?
• For the waiting time, should we adjust the interface of Eio.Net.run_server to require a clock? Or be tricky and use Eio_unix.sleep? Or just Eio.Fiber.yield without any actual waiting time (could still be user provided within on_accept_error)?

[talex5]

(I didn't write run_server so I don't know the original intention, but from looking at the code...)

every naive usage of Eio.Net.run_server is vulnerable to a simple denial of service attack if more than 1024 or 2048 connections are opened in parallel.

I think that's always going to be true. e.g. an attacker can just open 1024 connections and wait, preventing anyone else from using the server. Probably the Eio docs should link to guidance on best practices for protecting a server that's open to the public (e.g. tracking source IP addresses, raising the FD limit, etc).

It's also possible that accept fails because the service is leaking FDs elsewhere, in which case crashing and letting the service manager restart it is probably better than waiting forever.

Providing a lower default for max_connections seems tricky, as we don't know how many extra FDs will be needed per connection, but it would be good to note that it's worth setting it for public servers.

An optional on_accept_error seems reasonable too (and the user can be responsible for adding the delay).

The semaphore is not properly released in that case.

Yes. But the semaphore isn't public, and run_server is shutting down anyway by that point, so I don't think it makes any difference.

[vog]

While I agree that protecting a public facing webservice is out of scope for this function, I'd still opt for a safe default in a non-attack scenario.

Each connection uses at least one fd, the open socket, so even if the handler opens no other fd, setting max_connections to a value above 1024 is simply a programming error on any typical Linux installation (where ulimit -n shows 1024): It is like a time bomb waiting for the 1025's (entirely non-malicious) connection to reliably crash the webservice.

I'd propose a conservative default limit of max_connections=256, which means that every handler has 3 more fds to work with, e.g. to open some file and/or doing some database request. If the user then decides for a higher limit, or uses a large amount of fds, then they have at least an obvious connection between the "too many open files" exception and their own code.

Regarding my hint to the semaphore, my point was not that it is a programming error right now, but that it is written in a way that simple future changes (such as adding on_accept_error) will make it behave wrongly - which would not be the case if it had been released in a clean way in the first place, that is, released outside of accept_fork.