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

Comments

Projects
None yet
3 participants
@chanxuehong
Copy link
Contributor

commented Mar 27, 2016

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

This comment has been minimized.

Copy link
Contributor Author

commented Mar 27, 2016

updated, see above

@bradfitz

This comment has been minimized.

Copy link
Member

commented Mar 27, 2016

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

This comment has been minimized.

Copy link
Contributor Author

commented Mar 27, 2016

@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

This comment has been minimized.

Copy link
Contributor Author

commented Mar 27, 2016

@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

This comment has been minimized.

Copy link
Member

commented May 10, 2016

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

This comment has been minimized.

Copy link

commented May 10, 2016

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

@gopherbot gopherbot closed this in 4d8031c May 11, 2016

@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.
You can’t perform that action at this time.