Several unbounded ioutil.ReadAll calls with HTTP request/responses

[mdlayher]

Did a quick grep and filtered out vendored code and tests, and found a few instances of unbounded ioutil.ReadAll calls. Since some of these accept HTTP request and response bodies, it may be a good idea to set some sane upper bound to avoid limitless memory consumption.

I suspect a few MiB of data might be okay in the worst case, but I'm not super familiar with e.g. the remote read/write traffic.

$ grep -r "ioutil.ReadAll" | grep -v vendor | grep -v "_test.go"
documentation/examples/remote_storage/remote_storage_adapter/main.go:           compressed, err := ioutil.ReadAll(r.Body)
documentation/examples/remote_storage/remote_storage_adapter/main.go:           compressed, err := ioutil.ReadAll(r.Body)
documentation/examples/remote_storage/remote_storage_adapter/opentsdb/client.go:        buf, err = ioutil.ReadAll(resp.Body)
documentation/examples/remote_storage/example_write_adapter/server.go:          compressed, err := ioutil.ReadAll(r.Body)
web/web.go:     text, err := ioutil.ReadAll(file)
discovery/triton/triton.go:     data, err := ioutil.ReadAll(resp.Body)
discovery/marathon/marathon.go: body, err := ioutil.ReadAll(resp.Body)
storage/remote/codec.go:        compressed, err := ioutil.ReadAll(r.Body)
storage/remote/client.go:       compressed, err = ioutil.ReadAll(httpResp.Body)

If this isn't a concern, feel free to close the issue. Just wanted to make sure it was noted.

[gouthamve]

This is especially interesting for remote-read as it has the highest potential to nuke Prometheus.

[mdlayher]

Indeed. Ping @tomwilkie for thoughts since I noticed a couple of these in remote storage.

[tomwilkie]

Yeah we should bound them with a LimitedReader. Prometheus doesn't do a lot to protect itself, but thats not an excuse not to improve things!

I want to get 2.0 merged in before fixing this kind of bug though, as I don't want it to get lost in the merge.

[mdlayher]

Makes sense for me. Just so it's tracked here, do you know what a sane upper bound could be?

[tomwilkie]

I'll gather some stats from our Cortex but I don't image more than 10Mb.

[mattbostock]

@tomwilkie: See the MaxBytesReader function from the http package.

[adamdecaf]

I created #4376 (sorry about commit spam above).

@tomwilkie did you ever find a better limit? I just picked 10mb.

Also, I wasn't sure what to pick as a limit for storage/remote/client.go. Can those files be a lot bigger than 10mb?