Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

StreamRequestBody shouldn't read more data than actual need. #1819

Merged

Conversation

newacorn
Copy link
Contributor

@newacorn newacorn commented Aug 5, 2024

The StreamRequestBody feature on the server side should not read content that does not belong to the current request body.This is more logical and consistent with the result of not using the StreamRequestBody feature.
Fixes: #1816.
The reason for addressing this issue is that the Read method of requestStream assumes it should only read up to the Content-Length number of bytes.

@newacorn newacorn force-pushed the fix_request_body_stream_read_bug branch from 8363ef6 to 066cf08 Compare August 5, 2024 22:03
@newacorn
Copy link
Contributor Author

newacorn commented Aug 5, 2024

fasthttp/http.go

Line 2286 in 1fb3453

if contentLength >= 0 && maxBodySize >= contentLength {

Here, contentLength is guaranteed to be greater than or equal to 0 because the ContinueReadBodyStream function has already filtered out cases where contentLength is less than 0. It is assumed that a negative Content-Length value other than -1 should not be processed:

fasthttp/http.go

Line 1372 in 1fb3453

if contentLength == -2 {

Below code will also handle cases where maxBodySize is less than contentLength.

fasthttp/http.go

Line 2295 in 1fb3453

if contentLength > maxBodySize {

Copy link
Collaborator

@erikdubbelboer erikdubbelboer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, the fix looks good. But the new file needs to be formatted. And http.go needs some more newlines:

  http.go:2287: line is 147 characters (lll)
  		A fixed-length pre-read function should be used here; otherwise, it may read content beyond the request body into areas outside of the br buffer.
  http.go:2288: line is 160 characters (lll)
  	        This could affect the handling of the next request in the br buffer, if there is one. The original two branches can be handled with this single branch.

The StreamRequestBody feature on the server side should not read content that does not belong to the current request body.This is more logical and consistent with the result of not using the StreamRequestBody feature.Fixes: valyala#1816.
@newacorn newacorn force-pushed the fix_request_body_stream_read_bug branch from 066cf08 to 4ccdb7c Compare August 7, 2024 21:24
http.go Outdated Show resolved Hide resolved
server_test.go Outdated Show resolved Hide resolved
newacorn and others added 2 commits August 11, 2024 01:47
Co-authored-by: Erik Dubbelboer <[email protected]>
Co-authored-by: Erik Dubbelboer <[email protected]>
@newacorn
Copy link
Contributor Author

@erikdubbelboer May I ask what tools you use to lint the fasthttp codebase?

@erikdubbelboer
Copy link
Collaborator

@newacorn you can see that here:

uses: golangci/golangci-lint-action@v6
with:
version: v1.56.2

@erikdubbelboer erikdubbelboer merged commit a1db411 into valyala:master Aug 11, 2024
19 checks passed
@erikdubbelboer
Copy link
Collaborator

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG: content-length smaller than prefetched body size will lead requestStream Read() PANIC
2 participants