net/http: MaxBytesReader's behavior does not match the comments #10884

chanxuehong · 2015-05-17T05:39:19Z

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 · 2015-05-17T11:22:34Z

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 · 2015-05-17T12:11:38Z

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

chanxuehong · 2015-05-17T13:50:45Z

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 · 2015-05-17T14:13:04Z

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 · 2015-07-07T19:00:17Z

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

chanxuehong · 2015-07-08T11:16:42Z

I think there are still a little problem:

if maxBytesReader initialized with n<0, it will have a problem
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
}

ianlancetaylor added this to the Go1.5Maybe milestone Jun 3, 2015

ianlancetaylor assigned bradfitz Jun 3, 2015

bradfitz closed this as completed in d6e6baa Jul 7, 2015

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

chanxuehong mentioned this issue Mar 27, 2016

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

Closed

golang locked and limited conversation to collaborators Jul 11, 2016

gopherbot added the FrozenDueToAge label Jul 11, 2016

rsc unassigned bradfitz Jun 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

net/http: MaxBytesReader's behavior does not match the comments #10884

net/http: MaxBytesReader's behavior does not match the comments #10884

chanxuehong commented May 17, 2015

adg commented May 17, 2015

chanxuehong commented May 17, 2015

chanxuehong commented May 17, 2015

chanxuehong commented May 17, 2015

gopherbot commented Jul 7, 2015

chanxuehong commented Jul 8, 2015

net/http: MaxBytesReader's behavior does not match the comments #10884

net/http: MaxBytesReader's behavior does not match the comments #10884

Comments

chanxuehong commented May 17, 2015

adg commented May 17, 2015

chanxuehong commented May 17, 2015

chanxuehong commented May 17, 2015

chanxuehong commented May 17, 2015

gopherbot commented Jul 7, 2015

chanxuehong commented Jul 8, 2015