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: NewRequest panics if body is typed nil #32897

Closed
ix-sean-benoit opened this issue Jul 2, 2019 · 5 comments
Closed

net/http: NewRequest panics if body is typed nil #32897

ix-sean-benoit opened this issue Jul 2, 2019 · 5 comments

Comments

@ix-sean-benoit
Copy link

@ix-sean-benoit ix-sean-benoit commented Jul 2, 2019

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

$ go version
go version go1.11.11 linux/amd64

Does this issue reproduce with the latest release?

Yep. Go Playground link: https://play.golang.org/p/UuBFg4Z31NY

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

go env Output
$ go env
GOARCH="amd64"
GOBIN=""
GOCACHE="$HOME/.cache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="$HOME/go"
GOPROXY=""
GORACE=""
GOROOT="$HOME/sdk/go1.11.11"
GOTMPDIR=""
GOTOOLDIR="$HOME/sdk/go1.11.11/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build796944957=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Passing a nil as a function argument and then passing that argument into http.NewRequest as the body parameter (3rd parameter), causes http.NewRequest to panic.

https://play.golang.org/p/UuBFg4Z31NY

What did you expect to see?

A new *http.Request with an empty body and no error are returned OR a nil pointer and a non-nil error are returned. There should not be a panic.

What did you see instead?

http.NewRequest panics due to a nil pointer dereference.

@slrz

This comment has been minimized.

Copy link

@slrz slrz commented Jul 2, 2019

This is not a bug. You're passing a non-nil io.Reader to NewRequest. The latter thus expects to be able to call its methods.

It is perfectly possible to implement io.Reader using a nil pointer. NewRequest cannot return an error for this case.

See the following as an example (adjusted from your code):
https://play.golang.org/p/9byc9hWZmCe

Also see the relevant FAQ entry:
https://golang.org/doc/faq#nil_error

@ix-sean-benoit

This comment has been minimized.

Copy link
Author

@ix-sean-benoit ix-sean-benoit commented Jul 2, 2019

It is definitely possible to implement an io.Reader using a nil pointer.

To clarify my intention with this bug: if I pass in a nil *bytes.Buffer, *bytes.Reader, or *strings.Reader, then I expect it to not panic due to nil pointer dereference in the following if block (from https://golang.org/src/net/http/request.go?#L832):

	if body != nil {
		switch v := body.(type) {
		case *bytes.Buffer:
			req.ContentLength = int64(v.Len())
			buf := v.Bytes()
			req.GetBody = func() (io.ReadCloser, error) {
				r := bytes.NewReader(buf)
				return ioutil.NopCloser(r), nil
			}
		case *bytes.Reader:
			req.ContentLength = int64(v.Len())
			snapshot := *v
			req.GetBody = func() (io.ReadCloser, error) {
				r := snapshot
				return ioutil.NopCloser(&r), nil
			}
		case *strings.Reader:
			req.ContentLength = int64(v.Len())
			snapshot := *v
			req.GetBody = func() (io.ReadCloser, error) {
				r := snapshot
				return ioutil.NopCloser(&r), nil
			}
		default:
			// This is where we'd set it to -1 (at least
			// if body != NoBody) to mean unknown, but
			// that broke people during the Go 1.8 testing
			// period. People depend on it being 0 I
			// guess. Maybe retry later. See Issue 18117.
		}
		// For client requests, Request.ContentLength of 0
		// means either actually 0, or unknown. The only way
		// to explicitly say that the ContentLength is zero is
		// to set the Body to nil. But turns out too much code
		// depends on NewRequest returning a non-nil Body,
		// so we use a well-known ReadCloser variable instead
		// and have the http package also treat that sentinel
		// variable to mean explicitly zero.
		if req.GetBody != nil && req.ContentLength == 0 {
			req.Body = NoBody
			req.GetBody = func() (io.ReadCloser, error) { return NoBody, nil }
		}
	}

The block is explicitly checking that body is not nil, so the intent is clearly to prevent nil pointer dereferences and similar panics from occurring when these particular types are used.

I think that the following modifications should correct the behaviour which is incorrect (at least, in my opinion) without preventing the use of custom io.Reader implementations that use nil pointers:

	switch v := body.(type) {
	case *bytes.Buffer:
		if v != nil {
			req.ContentLength = int64(v.Len())
			buf := v.Bytes()
			req.GetBody = func() (io.ReadCloser, error) {
				r := bytes.NewReader(buf)
				return ioutil.NopCloser(r), nil
			}
		}
	case *bytes.Reader:
		if v != nil {
			req.ContentLength = int64(v.Len())
			snapshot := *v
			req.GetBody = func() (io.ReadCloser, error) {
				r := snapshot
				return ioutil.NopCloser(&r), nil
			}
		}
	case *strings.Reader:
		if v != nil {
			req.ContentLength = int64(v.Len())
			snapshot := *v
			req.GetBody = func() (io.ReadCloser, error) {
				r := snapshot
				return ioutil.NopCloser(&r), nil
			}
		}
	default:
		// This is where we'd set it to -1 (at least
		// if body != NoBody) to mean unknown, but
		// that broke people during the Go 1.8 testing
		// period. People depend on it being 0 I
		// guess. Maybe retry later. See Issue 18117.
	}

TL;DR for my modifications above: in each of the non-default cases wrap the existing code in a if v != nil block.

I quickly tested this behaviour (playground: https://play.golang.org/p/lKp_LtwlTWC) and it seems to behave as I expected.

@slrz

This comment has been minimized.

Copy link

@slrz slrz commented Jul 2, 2019

The primary point of the existing nil check is to discover whether the (optional) request body was provided. If the body argument wasn't optional, there would be no use for that check.

What's the use case for passing a non-nil io.Reader containing a nil *bytes.Buffer to NewRequest? This most likely signals a bug in the calling code. The proposed changes to NewRequest would just cover it up. The code in question should panic.

edit: don't get too hung up on those special cases. For the sake of this discussion, they can be ignored entirely, I think. A nil *bytes.Buffer is going to panic at the first invocation of its Read method. It is thus not a useful thing to pass around as an io.Reader. NewRequest shouldn't be in the business of trying to predict whether the provided interface value really implements the io.Reader contract. It should just call the Read method.

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Jul 2, 2019

Agreed with @slrz. This is working as designed.

Typed vs. untyped nils can be a bit confusing (see proposal #22729), but conflating the two even further would only make things even more confusing.

@bcmills bcmills closed this Jul 2, 2019
@ix-sean-benoit

This comment has been minimized.

Copy link
Author

@ix-sean-benoit ix-sean-benoit commented Jul 2, 2019

What's the use case for passing a non-nil io.Reader containing a nil *bytes.Buffer to NewRequest? This most likely signals a bug in the calling code.

The use case is if you have a function that uses NewRequest and also accepts an optional "body" parameter. Currently, the wrapper function needs to explicitly check that it was passed a nil and then explicitly pass an untyped nil instead of just always passing along the argument that was passed in.

Snippet illustrating what I mean: https://play.golang.org/p/LA9YJ6nTatq

The proposal @bcmills linked has some changes to the language which would improve this, but it looks like there won't be much action on it until Go 2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.