net/http: passing in nil *bytes.Reader to NewRequest panics

[omusil24]

Go version

go version go1.23.4 darwin/arm64

Output of go env in your module/workspace:

.

What did you do?

Run a code similar to the following minimal example:

package main

import (
	"bytes"
	"encoding/json"
	"fmt"
	"net/http"
)

func doRequest(reqBody interface{}) {
	var bodyReader *bytes.Reader
	if reqBody != nil {
		payloadBytes, err := json.Marshal(reqBody)
		if err != nil {
			return
		}
		bodyReader = bytes.NewReader(payloadBytes)
	}

	_, err := http.NewRequest("POST", "localhost:8080/abc", bodyReader)
	if err != nil {
		return
	}
	fmt.Println("ok")
}

func main() {
	// request body might be different values, here we just pass nil directly as an example
	doRequest(nil)
}

What did you see happen?

Running the code panics:

% go run main.go
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x2 addr=0x18 pc=0x10242bc10]

goroutine 1 [running]:
bytes.(*Reader).Len(...)
	/usr/local/go/src/bytes/reader.go:27
net/http.NewRequestWithContext({0x1024cf3e8, 0x1025f2540}, {0x10242dfa3?, 0x140000021c0?}, {0x10243091d?, 0x12?}, {0x1024ce508, 0x0})
	/usr/local/go/src/net/http/request.go:942 +0x400
net/http.NewRequest(...)
	/usr/local/go/src/net/http/request.go:872
main.doRequest({0x0?, 0x0?})
	/Users/user/test/main.go:20 +0xcc
main.main()
	/Users/user/test/main.go:28 +0x24
exit status 2

What did you expect to see?

The code should not panic and print "ok".

[seankhliao]

I think this will be a won't fix. nil bytes.Readers aren't valid https://go.dev/play/p/FPPAzZHiumv, net/http should not need to account for that.

[omusil24]

I think this will be a won't fix. nil bytes.Readers aren't valid https://go.dev/play/p/FPPAzZHiumv, net/http should not need to account for that.

@seankhliao What do you mean they aren't valid? They're nil, they're not invalid. Yeah, clearly using a nil-ptr like you're doing in the example is going to result in nil-ptr dereference. That's fine. But an intuitive code that a lot of programmers are writing should be handled and not result in panics.

Most people (who aren't familiar with exact Golang internals) would expect passing in a nil pointer to a base library function to work exactly the same as passing in nil. This is clearly a common issue that has been reported over the years lots of times:

• sending nil body of type *bytes.Buffer in http.NewRequest() function is causing panic #26666
• net/http: NewRequest panics if body is typed nil #32897
• net/http: invalid memory address or nil pointer dereference when body is a *Buffer nil #44327
• net/http.NewRequest panics on nil body #52578
• https://stackoverflow.com/questions/39606037/golang-passing-nil-to-function-that-accepts-an-interface

Why couldn't the net/http package handle this common issue? Literally just adding an if-clause would fix it.

[seankhliao]

a nil pointer in an interface is not nil, it's the same underlying issue as https://go.dev/doc/faq#nil_error

in any case, this is a programmer error that can never be valid, and panicking is the standard behavior rather than silently doing the wrong thing.

[omusil24]

... and panicking is the standard behavior rather than silently doing the wrong thing.

I get what you mean, but clearly whoever passes in nil *bytes.Buffer, *bytes.Reader or *strings.Reader to NewRequest meant for it to be equivalent to passing in nil, as that's what you would intuitively expect, so I wouldn't call this the wrong thing necessarily.

You have special code for handling these three types in the function, so why not add an extra if to handle a common use-case/usage of the function? Otherwise people are going to bump into this issue again and again, wasting time figuring out where's the problem.

[ianlancetaylor]

It's much easier to explain the API of a function as "this is what happens if body != nil" than it is to explain "this is what happens if body != nil or body is *bytes.Buffer, *bytes.Reader or *strings.Reader and the value of the pointer is nil".

Yes, the different between a nil pointer and a nil interface is is an unfortunate complexity in the Go language, but the fix should not be to complicate the API of library functions.

[omusil24]

Yes, the different between a nil pointer and a nil interface is is an unfortunate complexity in the Go language, but the fix should not be to complicate the API of library functions.

It does not change (and especially it does not complicate) the API, it just makes it safer IMHO.

Or, if doing this change is a problem, then at least adding a line or two to the documentation of the NewRequest and NewRequestWithContext would be very helpful. To explain that passing in nil *bytes.Buffer, *bytes.Reader or *strings.Reader values is not equal to passing in nil and will cause a panic.

[ianlancetaylor]

It changes the API because, to avoid confusion, we must document it, test it, and maintain it.

I don't mind adding documentation but I don't know what it should say. For better or worse, we must assume that anybody using Go is familiar with https://go.dev/doc/faq#nil_error. We definitely do not want to repeat that for every function that takes an interface argument.