net/http/server.go:Serve creates unlimited goroutines

[gopherbot]

by sidnei.da.silva:

The http server implementation creates a new goroutine for each incoming request. 

Now, requests are complex beasts and could be held active for a long time, which means
it is trivial to OOM a Go http server that uses the std library naively by causing too
many goroutines to be active at the same time.

A solution similar to the one suggested in the Effective Go documentation
(http://golang.org/doc/effective_go.html#channels) could be implemented, by allowing
users of the http server to restrict the maximum number of outstanding requests could be
active at once, with a sane default.

[gopherbot]

Comment 1 by disposaboy@dby.me:

That's not a solution. It's just adding another problem. i.e. now it's easier to DOS
your server

[rsc]

Comment 2:

If this is a problem for you, a solution is to write your own implementation of
net.Listener that caps the number of connections it accepts at one time, and then pass
it to the http server, so that the server never needs to talk to more than N connections
at once.
Of course, as comment #1 says, now you have a new problem.

Status changed to WorkingAsIntended.

[gopherbot]

Comment 3 by sidnei.da.silva:

One can just easily add a proxy in front, say haproxy, to limit the number of
connections, that's not the point. 
The problem is that the default implementation is open to a resource exaustion attack
(the C++ example in https://www.owasp.org/index.php/Resource_exhaustion loosely maps to
this).
While limiting the number of active connections can make it simpler to DOS by exausting
available connections, not limiting the available connections could cause resource
starvation at the OS level, which could have bigger consequences.
I actually filed this bug because people are looking at the implementation and making
assumptions that it is fine to do so in general 'because the Go http libraries do this
as well':
mozilla-services/heka#359 (comment)
So at minimum it might make sense to put a bold warning around the documentation that
there is a potential issue with exposing the default http server naked to the internet
without writing a custom net.Listener implementation that caps the number of connections.

[rsc]

Comment 4:

You are trading one DoS attack for another.
I don't understand why you are so sure that being open to one kind of DoS
is preferable to being open to a different kind of DoS.

[gopherbot]

Comment 5 by sidnei.da.silva:

I'm not the one who decides what is preferable or not. 
Being open to resource exaustion can grant you an entry in CVE (see some examples here
http://cwe.mitre.org/data/definitions/400.html), while having limits to resource
consumption was never to my knowledge a CVE-worthy issue.

[niemeyer]

Comment 6:

> You are trading one DoS attack for another.
This CL seems to present a different perspective on such issues:
[golang-dev] code review 12541052: runtime: impose stack size limit (issue #12541052)
"""
The goal is to stop only those programs that would keep
going and run the machine out of memory, but before they do that.
1 GB seems implausibly large, and it can be adjusted.
"""
https://golang.org/cl/12541052/

[ianlancetaylor]

Comment 7:

It doesn't seem like the same sort of thing to me.  Normally an external user can not
force a server to use a large stack.  Can you describe the relationship you see?

[niemeyer]

Comment 8:

Perhaps I'm just not seeing the light, but apparently both of them hardcode an
artificial limit to block the exhaustion of resources before depletion.
The fact a user can impose the depletion seems to make the argument even stronger.

[ianlancetaylor]

Comment 9:

The difference is that a runaway stack normally indicates a program error, so crashing
the program is a reasonable response.  In fact, if you really do have a runaway stack,
the program will already crash.  CL 12541052 just changes the point at which it will
crash: changes it to a point where it is likely to tie up all your system resources.
Creating more and more goroutines does not normally indicate a program error.  It
indicates that your server is carrying a heavier load.  There is no clear point where
the runtime should say "this program is in error and should crash."  Adding more
goroutines is unlikely to exhaust the program's resources.  It's other things that will
do that.  And simply limiting the number of goroutines won't let your server handle a
heavy load any better.  Or so it seems to me.

[niemeyer]

Comment 10:

Sidnei's suggestion wasn't to limit the number of goroutines in general, but to offer an
easy way to impose a limit on the number of http requests handled concurrently, which
seems similar in essence to what you describe.
It's also very easy to trigger stack growth externally (pretty much all marshallers are
recursive).

[ianlancetaylor]

Comment 11:

This will probably sound like a dumb question, but how does it help to simply limit the
number of concurrent requests that the HTTP server will handle?  What case will work in
that scenario that would fail without it?

[adg]

Comment 12:

Here's a CL that adds a LimitListener to the go.net repository. The test demonstrates
how to start an HTTP server that serves a bounded number of concurrent requests.
https://golang.org/cl/12727043

[adg]

Comment 13:

This issue was updated by revision golang/net@beab8eb.

R=golang-dev, dsymonds, rsc
CC=golang-dev
https://golang.org/cl/12727043