fasthttp icon indicating copy to clipboard operation
fasthttp copied to clipboard

It is not safe to read all stream body to memory without a max size limit.

Open Quons opened this issue 1 year ago • 8 comments

https://github.com/valyala/fasthttp/blob/57b9352ad1cc93a0aaaa72b2130e03ace8a5b118/http.go#L427 I think it would be safe to stop reading the request body into memory and return an error when it exceeds the maximum request body size. Otherwise, it may lead to an out-of-memory (OOM) error when the request body is too large.

Quons avatar Apr 23 '24 12:04 Quons

Users using body streaming on the server side should use Request.BodyStream() to get an io.Reader and read as much as they need. It's not recommended to to call Request.Body() to then get the full body. I agree that we should probably maybe do something here to prevent this. I'm open to a pull request or suggestion on how to fix this.

erikdubbelboer avatar Apr 29 '24 12:04 erikdubbelboer

If you want to add a read data size limit to the Body function, can you abandon the Body function and only support BodyStream? BodyStream can satisfy the caller's control of the read data size.

byte0o avatar Jun 24 '24 09:06 byte0o

@byte0o That would be a backward incompatible change, right?

gaby avatar Jul 13 '24 19:07 gaby

@gab Yes, you can first mark the Body function as deprecated

byte0o avatar Jul 14 '24 01:07 byte0o

@erikdubbelboer I would like to start contributing to this. From the above discussion, it seems that we can mark the Body() function as deprecated but it is getting used at many places in the same file. We should probably replace those also, right?

dojutsu-user avatar Sep 05 '24 13:09 dojutsu-user

@dojutsu-user maybe you can have a look at changing it so that Request.Body() prints a warning using Server.Logger (so only when used in a server context).

erikdubbelboer avatar Sep 07 '24 13:09 erikdubbelboer

@erikdubbelboer We'll have to inject the Server.Logger instance in the request, right? Currently, there's no logger that I can fine in the req.

dojutsu-user avatar Sep 08 '24 07:09 dojutsu-user

Yeah but keep it private, we don't want to expose that.

erikdubbelboer avatar Sep 08 '24 17:09 erikdubbelboer