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

Jump to bottom

io/ioutil: disallow nil readers in NopCloser #33409

Open
palsivertsen opened this issue Aug 1, 2019 · 2 comments
Open

io/ioutil: disallow nil readers in NopCloser #33409

palsivertsen opened this issue Aug 1, 2019 · 2 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Backlog

Comments

@palsivertsen
Copy link

Not sure if this is worth fixing, but thought I should create an issue in case anyone else experience the same issue.

What did you do?

If you create a new http.Request, feed it a ioutil.NopCloser with a nil reader and try to send the request using a http.Client, you'll get a panic with a stack trace from another thread.

package main

import (
	"io"
	"io/ioutil"
	"log"
	"net/http"
)

func main() {
	post(nil)
	log.Print("post nil ok")
	post(ioutil.NopCloser(nil))
	log.Print("post nil nop closer ok")
}

func post(body io.Reader) {
	req, err := http.NewRequest(http.MethodPost, "https://example.com", body)
	if err != nil {
		log.Fatal(err)
	}

	c := http.Client{}
	_, derr := c.Do(req)
	if derr != nil {
		log.Fatal(derr)
	}
}
$ go run .
2019/08/01 16:25:19 post nil ok
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x18 pc=0x4b142e]

goroutine 14 [running]:
io/ioutil.(*nopCloser).Read(0xc00028b470, 0xc00050a000, 0x4000, 0x4000, 0x7fb4ddb98678, 0xc00028b470, 0x7fb4ddb98678)
	<autogenerated>:1 +0x2e
net/http.(*http2clientStream).writeRequestBody(0xc0002f5900, 0x7fb4ddb98658, 0xc00028b470, 0x7fb4ddb98678, 0xc00028b470, 0x0, 0x0)
	/usr/local/go/src/net/http/h2_bundle.go:7664 +0x521
net/http.(*http2Transport).getBodyWriterState.func1()
	/usr/local/go/src/net/http/h2_bundle.go:8878 +0xc2
created by net/http.http2bodyWriterState.scheduleBodyWrite
	/usr/local/go/src/net/http/h2_bundle.go:8925 +0xa3
exit status 2

What did you expect to see?

ioutil.NopCloser(nil) should return nil
OR
c.Do(req) should panic with stack trace leading back to this call
OR
ioutil.NopCloser(nil) should panic with stack trace leading back to this call

What did you see instead?

Panic from an internal go routine created the http package.

Does this issue reproduce with the latest release (go1.12.7)?

Yes. (tested 1.12.6 and 1.12.7)

System details

go version go1.12.6 linux/amd64
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/pal/.cache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/pal/projects/go"
GOPROXY=""
GORACE=""
GOROOT="/usr/local/go"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
GOROOT/bin/go version: go version go1.12.6 linux/amd64
GOROOT/bin/go tool compile -V: compile version go1.12.6
uname -sr: Linux 4.15.0-54-generic
Distributor ID:	Ubuntu
Description:	Ubuntu 16.04.6 LTS
Release:	16.04
Codename:	xenial
/lib/x86_64-linux-gnu/libc.so.6: GNU C Library (Ubuntu GLIBC 2.23-0ubuntu11) stable release version 2.23, by Roland McGrath et al.
@ianlancetaylor ianlancetaylor changed the title Improvement to ioutil: Disallow nil readers in NopCloser io/ioutil: disallow nil readers in NopCloser Aug 1, 2019
@ianlancetaylor ianlancetaylor added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Aug 1, 2019
@ianlancetaylor ianlancetaylor added this to the Go1.14 milestone Aug 1, 2019
@ianlancetaylor
Copy link
Contributor

Seems like it would be more or less OK for ioutil.NopCloser(nil) to panic. It's hard to see how that would be meaningful. Although it's also not clear to me that this is worth fixing.

@palsivertsen
Copy link
Author

It's hard to see how that would be meaningful.

In this case a panic from ioutil.NopCloser(nil) would produce a stack trace back to the third line in the main function (post(ioutil.NopCloser(nil))) instead of:

io/ioutil.(*nopCloser).Read(0xc00028b470, 0xc00050a000, 0x4000, 0x4000, 0x7fb4ddb98678, 0xc00028b470, 0x7fb4ddb98678)
	<autogenerated>:1 +0x2e
net/http.(*http2clientStream).writeRequestBody(0xc0002f5900, 0x7fb4ddb98658, 0xc00028b470, 0x7fb4ddb98678, 0xc00028b470, 0x0, 0x0)
	/usr/local/go/src/net/http/h2_bundle.go:7664 +0x521
net/http.(*http2Transport).getBodyWriterState.func1()
	/usr/local/go/src/net/http/h2_bundle.go:8878 +0xc2
created by net/http.http2bodyWriterState.scheduleBodyWrite
	/usr/local/go/src/net/http/h2_bundle.go:8925 +0xa3

@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
3 participants
@rsc @ianlancetaylor @palsivertsen