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: http.MaxBytesReader has a bug #14981

Closed
chanxuehong opened this issue Mar 27, 2016 · 6 comments
Closed

net/http: http.MaxBytesReader has a bug #14981

chanxuehong opened this issue Mar 27, 2016 · 6 comments
Milestone

Comments

@chanxuehong
Copy link
Contributor

Please answer these questions before submitting your issue. Thanks!

  1. What version of Go are you using (go version)?
    go version go1.6 windows/amd64
  2. What operating system and processor architecture are you using (go env)?
    set GOARCH=amd64
    set GOBIN=
    set GOEXE=.exe
    set GOHOSTARCH=amd64
    set GOHOSTOS=windows
    set GOOS=windows
    set GOPATH=d:\gopath
    set GORACE=
    set GOROOT=d:\go
    set GOTOOLDIR=d:\go\pkg\tool\windows_amd64
    set GO15VENDOREXPERIMENT=1
    set CC=gcc
    set GOGCCFLAGS=-m64 -mthreads -fmessage-length=0
    set CXX=g++
    set CGO_ENABLED=1
  3. What did you do?
    If possible, provide a recipe for reproducing the error.
    A complete runnable program is good.
    A link on play.golang.org is best.

http://play.golang.org/p/bfo9_XiI49

  1. What did you expect to see?
----------------------------------------
99 <nil>
0 EOF
0 EOF
0 EOF
----------------------------------------
100 <nil>
0 EOF
0 EOF
0 EOF
----------------------------------------
100 <nil>
0 http: request body too large
0 http: request body too large
0 http: request body too large
  1. What did you see instead?
----------------------------------------
99 <nil>
0 EOF
0 EOF
0 EOF
----------------------------------------
100 <nil>
0 EOF
0 http: request body too large
0 http: request body too large
----------------------------------------
100 <nil>
0 http: request body too large
0 EOF
0 http: request body too large

here is my code to fix it.

type maxBytesReader struct {
    w         ResponseWriter
    r         io.ReadCloser // underlying reader
    n         int64         // max bytes remaining
    stopped   bool
    sawEOF    bool
    overLimit bool
}

func (l *maxBytesReader) Read(p []byte) (n int, err error) {
    toRead := l.n
    if l.n <= 0 {
        if l.overLimit {
            return l.tooLarge()
        }
        if l.sawEOF {
            return 0, io.EOF
        }
        // The underlying io.Reader may not return (0, io.EOF)
        // at EOF if the requested size is 0, so read 1 byte
        // instead. The io.Reader docs are a bit ambiguous
        // about the return value of Read when 0 bytes are
        // requested, and {bytes,strings}.Reader gets it wrong
        // too (it returns (0, nil) even at EOF).
        toRead = 1
    }
    if int64(len(p)) > toRead {
        p = p[:toRead]
    }
    n, err = l.r.Read(p)
    if err == io.EOF {
        l.sawEOF = true
    }
    if l.n <= 0 {
        // If we had zero bytes to read remaining (but hadn't seen EOF)
        // and we get a byte here, that means we went over our limit.
        if n > 0 {
            l.overLimit = true
            return l.tooLarge()
        }
        return 0, err
    }
    l.n -= int64(n)
    return
}
@chanxuehong
Copy link
Contributor Author

updated, see above

@bradfitz
Copy link
Contributor

Can you briefly describe the bug? It's not immediately obvious from the description. Is it confusion about whether the boundary value n is inclusive or exclusive?

We don't take patches in the bug tracker. To submit a fix, see https://golang.org/doc/contribute.html. Although I suspect we'd just want to update documentation if this is a boundary issue.

@chanxuehong
Copy link
Contributor Author

@bradfitz
I'm sorry my English is not good.
it is not a patch or CL, it is just a issue.
how to fix it, I do not care.

JUST for a io.Reader, I think the result is not correct

----------------------------------------
99 <nil>
0 EOF
0 EOF
0 EOF
----------------------------------------
100 <nil>
0 EOF
0 http: request body too large
0 http: request body too large
----------------------------------------
100 <nil>
0 http: request body too large
0 EOF
0 http: request body too large

@chanxuehong
Copy link
Contributor Author

@bradfitz , see
#10884

Either the docs should be amended to say "returns a non-EOF error for a Read up to or beyond the limit", or the behaviour should be changed. IMO, the more useful functionality is the documented behaviour, but the implementation would be a little more complex.

What say you @bradfitz?

I suggest update documentation

If changes the document to "returns a non-EOF error for a Read up to or beyond the limit",
Read method would be (like io.LimitedReader.Read)

 func (l *maxBytesReader) Read(p []byte) (n int, err error) { 
    if l.n <= 0 { 
        return l.tooLarge()
    } 
    if int64(len(p)) > l.n { 
        p = p[:l.n] 
    } 
    n, err = l.r.Read(p) 
    l.n -= int64(n) 
    return 
 } 

@bradfitz bradfitz self-assigned this Apr 9, 2016
@bradfitz bradfitz added this to the Go1.7 milestone Apr 9, 2016
@bradfitz
Copy link
Contributor

If I understand this bug correctly, it's that @chanxuehong expects error values returned by Read to be the same on subsequent Read calls after a non-nil error was returned by Read. Generally Go doesn't make that promise on its io.Reader types, but we could here.

I sent https://go-review.googlesource.com/23009

@gopherbot
Copy link

CL https://golang.org/cl/23009 mentions this issue.

@golang golang locked and limited conversation to collaborators May 11, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants