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 behavior does not match the comments #10884

Closed
chanxuehong opened this issue May 17, 2015 · 6 comments

Comments

Projects
None yet
6 participants
@chanxuehong
Copy link
Contributor

commented May 17, 2015

package main

import (
    "fmt"
    "io"
    "io/ioutil"
    "net/http"
    "strings"
)

func test1(w http.ResponseWriter, r *http.Request) {
    rd := strings.NewReader("12345")
    rc := ioutil.NopCloser(rd)

    mr := http.MaxBytesReader(w, rc, 5)
    fmt.Println(io.Copy(ioutil.Discard, mr))
}

func test2(w http.ResponseWriter, r *http.Request) {
    r.Body = http.MaxBytesReader(w, r.Body, 5)
    fmt.Println(io.Copy(ioutil.Discard, r.Body))
}

func main() {
    http.HandleFunc("/test1", test1)
    http.HandleFunc("/test2", test2)
    http.ListenAndServe(":80", nil)
}

for test1:
the reader rd 's content does not beyond the http.MaxBytesReader mr 's limit, but we got

5 http: request body too large

for test2(use postman, post 12345, use raw, not form-data, x-www-form-urlencoded):

we got

5 <nil>

see
https://github.com/golang/go/blob/master/src/net/http/request.go#L730

if n==l.n and err==nil and next call return n==0 and err==io.EOF, The program does not consider this

@adg

This comment has been minimized.

Copy link
Contributor

commented May 17, 2015

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?

@chanxuehong

This comment has been minimized.

Copy link
Contributor Author

commented May 17, 2015

@adg for test2, even it read up to the limit, the maxBytesReader still returned io.EOF

@chanxuehong

This comment has been minimized.

Copy link
Contributor Author

commented May 17, 2015

Here is my change, for MaxBytesReader and maxBytesReader

func MaxBytesReader(w ResponseWriter, r io.ReadCloser, n int64) io.ReadCloser {
    if n < 0 {
        panic("n should be greater than or equal to 0")
    }
    return &maxBytesReader{w: w, r: r, n: n + 1}
}

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

func (l *maxBytesReader) Read(p []byte) (n int, err error) {
    // l.n > 0
    if int64(len(p)) > l.n {
        p = p[:l.n]
    }
    n, err = l.r.Read(p)
    l.n -= int64(n)
    if l.n <= 0 { // beyond, and n >= l.n > 0
        if !l.stopped {
            l.stopped = true
            if res, ok := l.w.(*response); ok {
                res.requestTooLarge()
            }
        }
        n--
        err = errors.New("http: request body too large")
        return
    }
    return
}
@chanxuehong

This comment has been minimized.

Copy link
Contributor Author

commented May 17, 2015

if the docs be amended to say "returns a non-EOF error for a Read up to the limit", we can change
maxBytesReader.Read(p []byte) (n int, err error) to this:

and I think this is better

func (l *maxBytesReader) Read(p []byte) (n int, err error) {
    if l.n <= 0 {
        if !l.stopped {
            l.stopped = true
            if res, ok := l.w.(*response); ok {
                res.requestTooLarge()
            }
        }
        return 0, errors.New("http: request body too large")
    }
    if int64(len(p)) > l.n {
        p = p[:l.n]
    }
    n, err = l.r.Read(p)
    l.n -= int64(n)
    if l.n <= 0 {
        if !l.stopped {
            l.stopped = true
            if res, ok := l.w.(*response); ok {
                res.requestTooLarge()
            }
        }
        err = errors.New("http: request body too large")
        return
    }
    return
}
@gopherbot

This comment has been minimized.

Copy link

commented Jul 7, 2015

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

@bradfitz bradfitz closed this in d6e6baa Jul 7, 2015

@mikioh mikioh modified the milestones: Go1.5, Go1.5Maybe Jul 7, 2015

@chanxuehong

This comment has been minimized.

Copy link
Contributor Author

commented Jul 8, 2015

I think there are still a little problem:

  1. if maxBytesReader initialized with n<0, it will have a problem
  2. I think that this will never trigger
    see https://github.com/golang/go/blob/master/src/net/http/request.go#L740
func (l *maxBytesReader) Read(p []byte) (n int, err error) {
    toRead := l.n
    if l.n == 0 {
        if l.sawEOF {
            return l.tooLarge()  // -------> I think that this will never trigger
        }
        // 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 {
            return l.tooLarge()
        }
        return 0, err
    }
    l.n -= int64(n)
    if l.n < 0 {
        l.n = 0
    }
    return
}

Here is my code:

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

func (l *maxBytesReader) tooLarge() (n int, err error) {
    if !l.stopped {
        l.stopped = true
        if res, ok := l.w.(*response); ok {
            res.requestTooLarge()
        }
    }
    return 0, errors.New("http: request body too large")
}

func (l *maxBytesReader) Read(p []byte) (n int, err error) {
    if l.n < 0 {
        return l.tooLarge() // Reached if and only if maxBytesReader initialized with n<0
    }

    toRead := l.n
    if l.n == 0 {
        // 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 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 {
            return l.tooLarge()
        }
        return 0, err // Usually 0, io.EOF
    }
    l.n -= int64(n)
    return
}

But I think the following code is better,
Many times a read operation will be less

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

func (l *maxBytesReader) Read(p []byte) (n int, err error) {
    // The underlying io.Reader may not return (0, io.EOF)
    // at EOF if the requested size is 0, so read 1 more 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 := l.n + 1
    if toRead <= 0 {
        // Reached if and only if maxBytesReader initialized with n<0
        if !l.stopped {
            l.stopped = true
            if res, ok := l.w.(*response); ok {
                res.requestTooLarge()
            }
        }
        return 0, errors.New("http: request body too large")
    }
    if int64(len(p)) > toRead {
        p = p[:toRead]
    }
    n, err = l.r.Read(p)
    if l.n < int64(n) {
        // means we went over our limit.
        if !l.stopped {
            l.stopped = true
            if res, ok := l.w.(*response); ok {
                res.requestTooLarge()
            }
        }
        return int(l.n), errors.New("http: request body too large")
    }
    l.n -= int64(n)
    return
}

@golang golang locked and limited conversation to collaborators Jul 11, 2016

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.