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

net/http: MaxBytesReader's Read panics when n < -1 #45101

Closed
amwolff opened this issue Mar 18, 2021 · 5 comments
Closed

net/http: MaxBytesReader's Read panics when n < -1 #45101

amwolff opened this issue Mar 18, 2021 · 5 comments
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@amwolff
Copy link
Contributor

amwolff commented Mar 18, 2021

What version of Go are you using (go version)?

$ go version
go version go1.16.2 linux/amd64

Does this issue reproduce with the latest release?

Affirmative.

What operating system and processor architecture are you using (go env)?

go env Output
$ go env

What did you do?

Passed n < -1 to the MaxBytesReader constructor.

https://play.golang.org/p/aFTkl-eFmOZ

It won't compile in the playground, not sure why (timeout running go build).

What did you expect to see?

An error; behaviour similar to how io.LimitReader behaves (https://play.golang.org/p/ZcUjYXNlGgR).

What did you see instead?

panic: runtime error: slice bounds out of range [:-1]

goroutine 1 [running]:
net/http.(*maxBytesReader).Read(0xc000121f40, 0xc000022350, 0x4, 0x4, 0xc000048710, 0x44164a, 0xc000026000)
        /usr/local/go/src/net/http/request.go:1147 +0x206
...

The fix would be pretty simple, although far from an elegant one:

func MaxBytesReader(w http.ResponseWriter, r io.ReadCloser, n int64) io.ReadCloser {
+	if n < -1 {
+		return &maxBytesReader{w: w, r: r, n: -1}
+	}
	return &maxBytesReader{w: w, r: r, n: n}
}

It was a bit unexpected for me, and if it's indeed an unexpected behaviour, I can work on this and provide some fix.

@cherrymui cherrymui added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Mar 18, 2021
@cherrymui cherrymui added this to the Backlog milestone Mar 18, 2021
@cherrymui
Copy link
Member

cc @bradfitz

@networkimprov
Copy link

cc @empijei @neild @fraenkel

@neild
Copy link
Contributor

neild commented Mar 19, 2021

The documentation states that "MaxByteReader is similar to io.LimitReader", and io.LimitedReader's documentation states that "Read returns EOF when N <= 0". Either the documentation or behavior of http.MaxByteReader is wrong.

I think that for consistency with io.LimitReader, MaxBytesReader should also treat negative limits as equivalent to 0. I'm happy to review a CL if you'd like to send me one.

@neild neild added help wanted and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Mar 19, 2021
@cherrymui cherrymui added the NeedsFix The path to resolution is known, but the work has not been done. label Mar 19, 2021
@amwolff
Copy link
Contributor Author

amwolff commented Mar 19, 2021

@neild Yes! Absolutely. I will work on this and send you a patch as soon as I have one.

@gopherbot
Copy link

Change https://golang.org/cl/303171 mentions this issue: net/http: treat MaxBytesReader's negative limits equivalently to zero

@dmitshur dmitshur modified the milestones: Backlog, Go1.17 Mar 23, 2021
@golang golang locked and limited conversation to collaborators Mar 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

6 participants