Nice find. Thanks for reporting!

Thanks!

It's kind of disappointing that evhttp has no way to limit file descriptors. This is the so-manieth hack we need to accommodate it. It's also not very actively maintained so trying to push any fix upstream will take a long time, if ever. I sometimes wonder if moving to libevhtp, which seems to be the quasi-standard for http servers on top of libevent, would solve these issues.

Should we add a stress test to travis?

@laanwj yes it is disappointing that there isn't a better API to manage this systemic issue in evhttp.

Even more unfortunately, some configurations in Travis use an old version of libevent that doesn't have the bevcb callback or the ability to count added file descriptors. I've updated the pull request correspondingly. Protection is only available on newer libevents :(

What programs are available in Travis? Various configurations of https://github.com/shekyan/slowhttptest would be good tests, for example

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

I've updated the pull request correspondingly. Protection is only available on newer libevents :(

I think it would make sense to add a "known issue" to the release notes, recommending people that stumble on file descriptor problems while doing a lot of separate RPC requests to upgrade their libevent.

FWIW minimum seems to be 2.1.4:

$ git tag --contains 0fa107d8cbc652dacb722fcf650bb6b3ffbe8dac
release-2.1.4-alpha
release-2.1.5-beta
release-2.1.6-beta
release-2.1.7-rc
release-2.1.8-stable

This was discussed on IRC and according to @theuni this doesn't fully solve the problem, unfortunately. He could still fairly easy crash the server with many connections. So it will have to be solved from inside libevent.

Yes, I've spent the last 2 days trying to work out our best option here. The provided patch is a big help, but I think there are a few more things we can do, namely a libevent patch. I'll push up some more changes tomorrow and we can discuss what it makes sense to pull in.

@theuni here is a libevent patch that provides sufficient functionality to constrain the number of file-descriptors to a tight bound: libevent/libevent#578

In the meantime, as we work through the process to change libevent, the pull request here mitigates the original issue and defends against similar ones

@vii Nice work on the upstream patch. It'll be great to have a real solution for this.

I have a few concerns about the approach here:

• event_base_get_num_events is not a great representative of the connection count, as our own timers and triggers will throw off the count
• I think keep-alive connections need to be closed if they're re-used once the connection limit has been reached, otherwise it becomes trivial to monopolize the available slots.
• It unfortunately doesn't help with libevent 2.0.x, which is by far the majority in the wild.

I've adapted your approach and added in a few extras which manage to work-around the issue for libevent 2.0.x as well. With this, I'm unable to raise my fd count above the desired ceiling, no matter how hard I hammer.

Please have a look here and see what you think: https://github.com/theuni/bitcoin/commits/http-fd-limit. Note that there's an unlikely potential race at shutdown that I haven't yet addressed.

Great!

It unfortunately doesn't help with libevent 2.0.x, which is by far the majority in the wild.

Yes - it would be nice to have a (potentially messy) workaround for older libevent, and a clean solution for future upstream versions.

@theuni - your patch looks great! I was cautious about modifying so much.

Given the discussion, I've reduced my pull request to the simple improvement of making sure to request as many file descriptors as possible.

With @theuni's improvement to only accept connections that have data on, honest HTTP connections will go into the connection limiter. However, I suspect adversaries using things like the slowloris attack (which send incomplete requests and then pause) will not be caught and will use resources. These can be controlled if the libevent request is included, but otherwise can be detected but not perfectly controlled using the approach in the first request, which binds to the rather toothless bevcb callback.

One way to look at this is that the root issue is that there is a process wide file-descriptor allocation defined in https://github.com/bitcoin/bitcoin/blob/master/src/init.cpp#L907 but the RPC interface isn't accounted for. Would be great to integrate @theuni's ConnectionLimiter into that!

Is it worth defending against slowloris style attacks given the RPC interface is generally not accessible to adversaries?

In this pull request, his small change to request as many file-descriptors as possible should help mitigate the original issue on many default configurations (e.g. Ubuntu).

this small change to request more file-descriptors does not depend on the libevent patch (which is making progress towards acceptance) but alleviates the issue in many configurations - except for deliberate attacks

Please review!

the libevent change has been merged into that repo so we can have a complete fix libevent/libevent#578 - but the limited PR here does not depend on it

Happy to make a more complete fix if it would be reviewed!

@vii Great work on the upstream fix!

I'll clean up my changes and PR them separately. I'll go ahead and work in the bevcb for supported versions, so that we can take advantage of that when possible.

As for raising the fd limit, I have mixed feelings. I worry that this will simply mask problems. I believe @gmaxwell was in favor, though.

@gmaxwell: assuming we manage to contain this issue, do you still think maxing out the fd limit is beneficial?

the issue that it masks is that several usages of file-descriptors are not budgeted for in the process wide file-descriptor allocation defined in https://github.com/bitcoin/bitcoin/blob/master/src/init.cpp#L907

It would be quite difficult to carefully audit for all usages of file-descriptors as library functions may use them for various purposes. Even once the budgeting is done, this will be helpful!

Please consider and merge

What I'd like to know is how does other software handle this? Is it frowned upon for daemons to increase the file descriptor count without being asked? I thought the usual case for UNIX is to make changing resource limits up to the user/sysadmin, and changing them un-asked is deemed rude, but this might be different for file descriptors.

@laanwj we are not attempting to change the maximum, just adjusting to use the maximum we are allowed

Note here is some similar code in the wine project - rather more battlehardened but with the same effect on Linux

https://source.winehq.org/source/libs/wine/loader.c#L661

@theuni brought this PR to my attention in a conversation we had earlier today. I was actually planning to implement the same behavior, for a completely different reason: I would like to increase the number of file descriptors that leveldb is allowed to use, because performance testing I have done makes me think that the low limit leveldb currently has is causing issues with which pages Linux decides to keep in the page cache. I think increasing the soft limit is safe and reasonable.

@eklitzke Is that an ACK on this?

Closing this, reviewing and development bled out and a rebase has been necessary for months. Let me know if I should reopen it.