http: avoid fd exhaustion #12274

theuni · 2018-01-26T04:25:11Z

Addresses #11368. This adds a file descriptor limiter for the http server. It's possible for the open fd count to potentially exhaust the user's defined limit if not checked.

I'm having a hard time reproducing the issue now, I'm not sure what's changed in my environment. I was finally able to hit it using some pretty nasty tricks, but I'm less worried about this now for 0.16.

I do think it's worth considering taking de6cfce, 5f5099a, and (part of) 3b0ebb3 for 0.16, though, as they help significantly and are easy to review. I'm happy to break that out into a new PR if necessary.

Responses are usually added to the response queue by worker threads. As an optimization, and in order to close connections as quickly as possible, allow for immediate replies (bypassing the queue) when there is no need for a worker to service the request.

No functional change, just preparation for the next commits.

Rather than potentially accepting an unbounded number of connections, give them a ceiling of twice the size of the processing queue. Once the ceiling is reached, new connections will not be accepted until the count has fallen back under the limit. Note that file descriptors are removed from the limiter in the request's close callback.

If we're unable to service a request, close the connection rather than potentially keeping it alive longer. This helps to be slightly more fair to new connections.

This avoids assigning file descriptors faster than we can close them. It is the same as using an evconnlistener with the LEV_OPT_DEFERRED_ACCEPT flag, which is only available with libevent 2.1+.

Libevent 2.2 adds a callback allowing us to drop connections as they come in. Use it when possible.

tjps · 2018-01-26T09:14:38Z

src/httpserver.cpp

@@ -288,14 +376,19 @@ static void http_request_cb(struct evhttp_request* req, void* arg)
    if (i != iend) {
        std::unique_ptr<HTTPWorkItem> item(new HTTPWorkItem(std::move(hreq), path, i->handler));
        assert(workQueue);
-        if (workQueue->Enqueue(item.get()))
+        if (workQueue->Enqueue(item.get())) {
+            // Disable reading to work around a libevent bug, fixed in 2.2.0.


Should this block be wrapped in a LIBEVENT_VERSION_NUMBER macro guard so it only occurs in the affected version range?

Seems like it would be easier for future cleanups to see where the version macro is checked, say in the instance that a minimum version of libevent is defined and code like this could be safely removed.

This is just move-only from above. There's no need to guard it with an ifdef because there's no api difference, it's purely a runtime check.

tjps · 2018-01-26T09:21:01Z

src/httpserver.cpp

+
+    const unsigned int m_limit;
+    std::vector<evconnlistener*> m_listeners;
+    std::set<evutil_socket_t> m_sockets;


Not that it probably matters in most configurations where the fd limit is ~1K, but making this an unordered_set would be slightly beneficial in the high fd limit cases.

laanwj · 2018-01-28T09:03:12Z

This needs to be tested by those experiencing the issue: @KanoczTomas @vii

promag · 2018-01-29T21:32:08Z

Concept ACK.

Why don't understand why you need dc53bec, seems like an unnecessary refactor.

5f5099a and 3b0ebb3 could be in a separate PR, are simple and make sense even without the fd exhaustion problem.

theuni · 2018-01-29T22:30:18Z

@promag dc53bec is just a small chunk of c56cf1a that seemed reasonable on its own.

vii · 2018-01-30T04:36:15Z

src/httpserver.cpp

@@ -300,6 +300,22 @@ static void connection_close_cb(evhttp_connection* conn, void *arg)
    }
 }

+#if LIBEVENT_VERSION_NUMBER >= 0x02020001


awesome that you're using the callback - one way to unambiguously detect the presence of the new feature would be to add a new autoconf test maybe using AC_LINK_IFELSE for evhttp_set_newreqcb

vii · 2018-01-30T04:42:48Z

Awesome! The defer accept tweak will defend against typical inadvertent denial of services

Note that the original issue can still be activated depending on the file descriptor limit that is set, as the budgeting in the daemon does not account for this usage or the usage in the db code - see
#11785

To test it, something like this slowhttptest program

slowhttptest -c 40000 -r 1000 -u 'http://127.0.0.1:8332/rest/block/000000000019d6689c085ae165831e934ff763ae46a2a6c172b3f1b60a8ce26f.json'

or another tool which can simulate a slowloris (HTTP connection header trickling) attack

laanwj · 2018-01-30T11:27:45Z

Sorry, I'm removing the 0.16.0 milestone here. As it introduces many new concepts and work-arounds it is hard to review last-minute, the problem is hard to trigger in the first place, and it is not a regression since 0.15. Also according to @vii it doesn't solve the issue completely. I think getting the fd issue fixed properly should be a focus for 0.16.1.

eklitzke

This was just a superficial check, I can do another pass later in more detail. I'm curious if you can reproduce the fd exhaustion issue.

eklitzke · 2018-03-11T07:46:36Z

src/httpserver.cpp

+    g_limiter = MakeUnique<ConnectionLimiter>(std::move(listeners), workQueueDepth * 2);
+    evhttp_set_gencb(http, http_request_cb, g_limiter.get());
+
+#if LIBEVENT_VERSION_NUMBER >= 0x02020001


Would like to see this defined separately as:

#if LIBEVENT_VERSION_NUMBER >= 0x02020001 #define HAVE_EVHTTP_SET_NEWREQCB 1 #endif

That way the check here is more obvious, particularly since you do the check elsewhere. Of course, even better than this would be to use AC_CHECK_FUNC()...

eklitzke · 2018-03-11T07:49:13Z

src/netbase.cpp

+{
+    bool ret = false;
+#ifdef TCP_DEFER_ACCEPT
+    static constexpr int set = 1;


This is the first time I've seen static constexpr used correctly in the Bitcoin code, congratulations.

fanquake · 2018-03-21T16:08:22Z

@eklitzke I can still produce a crash with these changes on top of 2405ce1:

date='2015-09-16T16:18:02Z' progress=0.269372 cache=313.8MiB(2336744txo)
2018-03-21T16:04:44Z UpdateTip: new best=00000000000000000c7112c52408963acb80e1fedc71005859d6ef9a22f84d79 height=374820 version=0x00000003 log2_work=83.354829 tx=84036580 date='2015-09-16T16:20:12Z' progress=0.269373 cache=313.8MiB(2337189txo)
2018-03-21T16:04:44Z LevelDB read failure: IO error: /Users/fanquake/Library/Application Support/Bitcoin/chainstate/019338.ldb: Too many open files
2018-03-21T16:04:44Z Fatal LevelDB error: IO error: /Users/fanquake/Library/Application Support/Bitcoin/chainstate/019338.ldb: Too many open files
2018-03-21T16:04:44Z You can use -debug=leveldb to get more complete diagnostic messages
2018-03-21T16:04:44Z libevent: Error from accept() call: Too many open files
2018-03-21T16:04:44Z libevent: Error from accept() call: Too many open files
2018-03-21T16:04:44Z libevent: Error from accept() call: Too many open files
2018-03-21T16:04:44Z libevent: Error from accept() call: Too many open files

When last discussed with @theuni he was going to revamp these changes.

eklitzke · 2018-03-21T22:59:54Z

@fanquake Interesting -- do you have a script to stress test it to induce that condition? Or does that happen during normal operation?

DrahtBot · 2018-07-20T20:30:00Z

The last travis run for this pull request was 175 days ago and is thus outdated. To trigger a fresh travis build, this pull request should be closed and re-opened.

maflcko · 2018-07-20T20:36:00Z

src/httpserver.cpp

+    {
+        // Disable socket accepting if adding this connection puts us equal to the limit
+        if (!Interrupted() && m_sockets.insert(fd).second && m_sockets.size() == m_limit) {
+            LogPrint(BCLog::HTTP, "Suspending new connections");


All calls to LogPrintf() and LogPrint() should be terminated with \n

DrahtBot · 2018-08-21T16:44:29Z

Needs rebase

DrahtBot · 2018-12-03T16:52:35Z

There hasn't been much activity lately and the patch still needs rebase, so I am closing this for now. Please let me know when you want to continue working on this, so the pull request can be re-opened.

BlockMechanic · 2019-11-07T18:16:03Z

So is this the recommended fix for #11368 ? I just bumped into it and need to resolve it

theuni added 6 commits January 25, 2018 17:49

http: return initialized listeners

dc53bec

No functional change, just preparation for the next commits.

http: don't re-use connections when slots are full

3b0ebb3

If we're unable to service a request, close the connection rather than potentially keeping it alive longer. This helps to be slightly more fair to new connections.

http: defer accepting until there's data to read

de6cfce

This avoids assigning file descriptors faster than we can close them. It is the same as using an evconnlistener with the LEV_OPT_DEFERRED_ACCEPT flag, which is only available with libevent 2.1+.

http: use new libevent callback to drop connections quickly

cdcc0ed

Libevent 2.2 adds a callback allowing us to drop connections as they come in. Use it when possible.

fanquake added the RPC/REST/ZMQ label Jan 26, 2018

fanquake added this to the 0.16.0 milestone Jan 26, 2018

tjps reviewed Jan 26, 2018

View reviewed changes

vii reviewed Jan 30, 2018

View reviewed changes

laanwj removed this from the 0.16.0 milestone Jan 30, 2018

laanwj added this to the 0.16.1 milestone Jan 30, 2018

laanwj mentioned this pull request Feb 1, 2018

File descriptor problem, causing leveldb crash #12323

Closed

khelle mentioned this pull request Feb 20, 2018

Bitcoin core 0.15.1 started to crash #12483

Closed

eklitzke reviewed Mar 11, 2018

View reviewed changes

laanwj removed this from the 0.16.1 milestone May 17, 2018

Nico205 added a commit to machinecoin-project/machinecoin-core that referenced this pull request May 17, 2018

rpc: applied PR bitcoin/bitcoin#12274, in attempt to fix crashes

52d38c9

DrahtBot closed this Jul 20, 2018

DrahtBot reopened this Jul 20, 2018

maflcko reviewed Jul 20, 2018

View reviewed changes

This was referenced Aug 17, 2018

~~Correctly~~ terminate HTTP server #13501

Closed

Dump HTTP request headers #13992

Closed

DrahtBot mentioned this pull request Aug 20, 2018

Add const modifier to HTTPRequest methods #14006

Merged

DrahtBot added the Needs rebase label Aug 21, 2018

DrahtBot added the Up for grabs label Dec 3, 2018

DrahtBot closed this Dec 3, 2018

laanwj removed the Needs rebase label Oct 24, 2019

This was referenced Nov 7, 2019

Fix http server *IGNORE* #17406

Closed

Fix http server bitcointalkcoin/bitcointalkcoin#1

Merged

fjahr mentioned this pull request May 22, 2020

bitcoin core crashes when too many rpc calls are made #11368

Closed

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http: avoid fd exhaustion #12274

http: avoid fd exhaustion #12274

theuni commented Jan 26, 2018 •

edited by DrahtBot

tjps Jan 26, 2018

theuni Jan 29, 2018

tjps Jan 26, 2018

theuni Jan 29, 2018

laanwj commented Jan 28, 2018

promag commented Jan 29, 2018

theuni commented Jan 29, 2018

vii Jan 30, 2018

vii commented Jan 30, 2018

laanwj commented Jan 30, 2018

eklitzke left a comment

eklitzke Mar 11, 2018

eklitzke Mar 11, 2018

fanquake commented Mar 21, 2018 •

edited

eklitzke commented Mar 21, 2018

DrahtBot commented Jul 20, 2018

maflcko Jul 20, 2018

DrahtBot commented Aug 21, 2018

DrahtBot commented Dec 3, 2018

BlockMechanic commented Nov 7, 2019

http: avoid fd exhaustion #12274

http: avoid fd exhaustion #12274

Conversation

theuni commented Jan 26, 2018 • edited by DrahtBot

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laanwj commented Jan 28, 2018

promag commented Jan 29, 2018

theuni commented Jan 29, 2018

Choose a reason for hiding this comment

vii commented Jan 30, 2018

laanwj commented Jan 30, 2018

eklitzke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fanquake commented Mar 21, 2018 • edited

eklitzke commented Mar 21, 2018

DrahtBot commented Jul 20, 2018

Choose a reason for hiding this comment

DrahtBot commented Aug 21, 2018

DrahtBot commented Dec 3, 2018

BlockMechanic commented Nov 7, 2019

theuni commented Jan 26, 2018 •

edited by DrahtBot

fanquake commented Mar 21, 2018 •

edited