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

http/request: maxBytesReader.Read int64 overflow #54408

Closed
zhixun-cxy opened this issue Aug 12, 2022 · 3 comments
Closed

http/request: maxBytesReader.Read int64 overflow #54408

zhixun-cxy opened this issue Aug 12, 2022 · 3 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@zhixun-cxy
Copy link

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

$ go version
go version go1.18.4 darwin/amd64

Does this issue reproduce with the latest release?

Yes

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

go env Output
$ go env
GO111MODULE="auto"
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/chenxiangyu/Library/Caches/go-build"
GOENV="/Users/chenxiangyu/Library/Application Support/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/chenxiangyu/go/pkg/mod"
GONOPROXY="gitlab.alibaba-inc.com"
GONOSUMDB="gitlab.alibaba-inc.com"
GOOS="darwin"
GOPATH="/Users/chenxiangyu/go"
GOPRIVATE="gitlab.alibaba-inc.com"
GOPROXY="http://gomodule-repository.aone.alibaba-inc.com"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
GOVCS=""
GOVERSION="go1.18.4"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD=""
GOWORK=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -arch x86_64 -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/mv/q0m312c13h71lt7km7xxfk740000gn/T/go-build2968071884=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

I wrote the following code in main.go:

package main

import (
	"bytes"
	"io"
	"net/http"
)

func main() {
	buffer := bytes.NewBufferString("foo")
	reader := http.MaxBytesReader(nil, io.NopCloser(buffer), int64(1<<63-1))
	res := make([]byte, 3)
	if _, err := reader.Read(res); err != nil {
		println(err.Error())
	}
	println(string(res))
}

Then run the command go run main.go

What did you expect to see?

Starting: /Users/chenxiangyu/go/bin/dlv dap --check-go-version=false --listen=127.0.0.1:56670 --log-dest=3 from /Users/chenxiangyu/arrow-cli/example/
DAP server listening at: 127.0.0.1:56670
Type 'dlv help' for list of commands.
foo
Process 35830 has exited with status 0
Detaching
dlv dap (35802) exited with code: 0

What did you see instead?

Exception has occurred: panic
"runtime error: slice bounds out of range [:-9223372036854775808]"
Stack:
3 0x00000000012ebb2a in net/http.(*maxBytesReader).Read
at /usr/local/go/src/net/http/request.go:1152
4 0x00000000013e2d0a in main.main
at /Users/chenxiangyu/arrow-cli/example/main.go:13

@zhixun-cxy
Copy link
Author

before

func (l *maxBytesReader) Read(p []byte) (n int, err error) {
	if l.err != nil {
		return 0, l.err
	}
	if len(p) == 0 {
		return 0, nil
	}
	// If they asked for a 32KB byte read but only 5 bytes are
	// remaining, no need to read 32KB. 6 bytes will answer the
	// question of the whether we hit the limit or go past it.
	if int64(len(p)) > l.n+1 {
		p = p[:l.n+1]
	}
	n, err = l.r.Read(p)

	if int64(n) <= l.n {
		l.n -= int64(n)
		l.err = err
		return n, err
	}

	n = int(l.n)
	l.n = 0

	// The server code and client code both use
	// maxBytesReader. This "requestTooLarge" check is
	// only used by the server code. To prevent binaries
	// which only using the HTTP Client code (such as
	// cmd/go) from also linking in the HTTP server, don't
	// use a static type assertion to the server
	// "*response" type. Check this interface instead:
	type requestTooLarger interface {
		requestTooLarge()
	}
	if res, ok := l.w.(requestTooLarger); ok {
		res.requestTooLarge()
	}
	l.err = errors.New("http: request body too large")
	return n, l.err
}

after

func (l *maxBytesReader) Read(p []byte) (n int, err error) {
	if l.err != nil {
		return 0, l.err
	}
	if len(p) == 0 {
		return 0, nil
	}
        
        // 0 < len(p) < 2^63
	if int64(len(p)) - 1 > l.n {
		p = p[:l.n+1]
	}
	n, err = l.r.Read(p)

	if int64(n) <= l.n {
		l.n -= int64(n)
		l.err = err
		return n, err
	}

	n = int(l.n)
	l.n = 0

	type requestTooLarger interface {
		requestTooLarge()
	}
	if res, ok := l.w.(requestTooLarger); ok {
		res.requestTooLarge()
	}
	l.err = errors.New("http: request body too large")
	return n, l.err
}

@seankhliao seankhliao added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Aug 12, 2022
@seankhliao
Copy link
Member

cc @neild

@gopherbot
Copy link

Change https://go.dev/cl/423314 mentions this issue: net/http: fix overflow

cuiweixie added a commit to cuiweixie/go that referenced this issue Aug 12, 2022
cuiweixie added a commit to cuiweixie/go that referenced this issue Aug 12, 2022
cuiweixie added a commit to cuiweixie/go that referenced this issue Aug 12, 2022
cuiweixie added a commit to cuiweixie/go that referenced this issue Aug 12, 2022
@seankhliao seankhliao added this to the Unplanned milestone Aug 20, 2022
@dmitshur dmitshur modified the milestones: Unplanned, Go1.20 Aug 26, 2022
@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Aug 26, 2022
@golang golang locked and limited conversation to collaborators Aug 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants