File Descriptor Leak Reaching Soft Limit with DDoS Requests

[aonq]

We were testing Crow. We noticed when sending DDoS requests, it hits the fd soft limit, causing new fd open()s fail with errno 24 "Too many open files".

We discovered that during each handling in do_accept() in http_server.h, the shared pointer of Connection "p" is copied to the lambda in asio::io_service's post(), causing it to be enqueued and handled later. If we hammer with for example 2000 requests, each request takes several hundred milliseconds, Crow keeps creating and holding new Connections until hitting the 1024 fd limit.

        void do_accept()
        {
            if (!shutting_down_)
            {
                uint16_t service_idx = pick_io_service_idx();
                asio::io_service& is = *io_service_pool_[service_idx];
                task_queue_length_pool_[service_idx]++;
                CROW_LOG_DEBUG << &is << " {" << service_idx << "} queue length: " << task_queue_length_pool_[service_idx];

                auto p = std::make_shared<Connection<Adaptor, Handler, Middlewares...>>(
                  is, handler_, server_name_, middlewares_,
                  get_cached_date_str_pool_[service_idx], *task_timer_pool_[service_idx], adaptor_ctx_, task_queue_length_pool_[service_idx]);

                acceptor_.async_accept(
                  p->socket(),
                  [this, p, &is, service_idx](error_code ec) {
                      if (!ec)
                      {
                          is.post(
                            [p] {
                                p->start();
                            });
                      }
                      else
                      {
                          task_queue_length_pool_[service_idx]--;
                          CROW_LOG_DEBUG << &is << " {" << service_idx << "} queue length: " << task_queue_length_pool_[service_idx];
                      }
                      do_accept();
                  });
            }
        }

We did a temporary workaround:
We added a blocking check before is.post(...) -- If the task queue is larger than max_task_queue_length_, the new Connection won't be forwarded to p->start().

                acceptor_.async_accept(
                  p->socket(),
                  [this, p, &is, service_idx](error_code ec) {
                      // New change
                      if(max_task_queue_length_.has_value() && task_queue_length_pool_[service_idx] > max_task_queue_length_.value())
                      {
                        CROW_LOG_DEBUG << "Too many queued tasks for io_service " << &is << " {" << service_idx << "}, rejecting connection. Queue length: " << task_queue_length_pool_[service_idx];
                      }
                      else if (!ec)
                      {
                          is.post(
                            [p] {
                                p->start();
                            });
                      }

This works for now, but we would like to know is there a better solution on the fd level in Crow? Thanks!

[gittiver]

Well, how would you set max_task_queue_length to a useful value, as you don't know where each tasks state is?
I would think, it is Ok if we hit the file descriptor limit then to give an error log.

[aonq]

For the useful value:
We added max_connections() in app.h to let the user set the limit, which equals the max_task_queue_length value. But the point is not the exact value -- Any value below the FD soft limit prevents exhaustion.

For task states:
The state doesn't matter here -- When we construct a Connection, it opens an FD. So the FD count equals the number of alive Connections, regardless of which state each task is in.

For logging an error:
Logging alone doesn't protect the Crow server from DDoS attacks. After it hits the FD limit, new requests will still try to construct new Connections and open new FDs.

I think Crow should stop accepting new connections when it hits the FD limit, rather than accepting them and failing.

[gittiver]

I think the construction of the Connection should be done inside of the async accept block, fd needs to be created later.

[gittiver]

For task states:
The state doesn't matter here -- When we construct a Connection, it opens an FD. So the FD count equals the number of alive Connections, regardless of which state each task is in.

I agree.

[aonq]

I think the construction of the Connection should be done inside of the async accept block, fd needs to be created later.

I agree with your idea but couldn't implement it.
acceptor_.async_accept( needs a valid socket from p, so its construction can't happen after the async_accept call.

[gittiver]

I would propose that you put your max connection workaround in a PR and we solve the other problem in a separate PR maybe it make sense to create an issue/ feature request too for it.

[aonq]

Is my understanding on the two problems same as yours?
Issue 1: No limitation on max number of connections.
Issue 2: Construction of Connection and creation of fd starts too early.

I will prepare the first PR. Shall I link the fix branch to this ticket 1188?
I will create another ticket for issue 2, which you can use for the second PR.

[gittiver]

I will prepare the first PR. Shall I link the fix branch to this ticket 1188?
Yes.

I will create another ticket for issue 2, which you can use for the second PR.
Yes.

[aonq]

Opened the PR for fixing issue 1 here: #1217
Created a new ticket for issue 2 here: #1218