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

Panic on [email protected]/request.go:99 #268

Closed
sponeil opened this issue Jul 5, 2023 · 1 comment · Fixed by #272
Closed

Panic on [email protected]/request.go:99 #268

sponeil opened this issue Jul 5, 2023 · 1 comment · Fixed by #272
Assignees

Comments

@sponeil
Copy link

sponeil commented Jul 5, 2023

Encountered on an older versions of Go and go-openapi, but reproduced it with current versions:
panic: runtime error: invalid memory address or nil pointer dereference
[signal 0xc0000005 code=0x0 addr=0x28 pc=0x90de85]

goroutine 157 [running]:
github.com/go-openapi/runtime.(*peekingReader).Read(0xc000b7f7c0?, {0xc0005a1d3e?, 0x740a7e?, 0xc0005597a0?})
C:/Users/devbuild/go/pkg/mod/github.com/go-openapi/[email protected]/request.go:99 +0x25
net/http.(*transferWriter).probeRequestBody.func1({0x28c3bab8, 0xc000171c80})
C:/Script/Go-1.20/src/net/http/transfer.go:211 +0x6e
created by net/http.(*transferWriter).probeRequestBody
C:/Script/Go-1.20/src/net/http/transfer.go:208 +0x12e

Panics in last line of peekingReader.Read because p.underlying is nil:
func (p *peekingReader) Read(d []byte) (int, error) { if p == nil { return 0, io.EOF } return p.underlying.Read(d) }

The only place I can find p.underlying set to nil is in Close:
func (p *peekingReader) Close() error { p.underlying = nil if p.orig != nil { return p.orig.Close() } return nil }

Request: Can you please add a check to peekingReader.Read() to return an error when p.underlying is nil? Nested/hidden interfaces and goroutines can make it very difficult to follow the trail of breadcrumbs to determine the root cause, and an error would be so much more friendly than a panic here. Perhaps io.ErrUnexpectedEOF, as it is unexpected?

TIA

@fredbi
Copy link
Member

fredbi commented Dec 7, 2023

@sponeil yes it looks like you're reading on an already closed reader.
I agree that the panic is not super nice.
Let's add some checks to return io.EOF when reading from a closed reader and an error when closing an already closed reader (like os.File)

fredbi added a commit to fredbi/runtime that referenced this issue Dec 7, 2023
@fredbi fredbi self-assigned this Dec 7, 2023
fredbi added a commit to fredbi/runtime that referenced this issue Dec 8, 2023
fredbi added a commit that referenced this issue Dec 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants