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: Server panics on nil connection #37663

Open
jefferai opened this issue Mar 4, 2020 · 6 comments
Open

net/http: Server panics on nil connection #37663

jefferai opened this issue Mar 4, 2020 · 6 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@jefferai
Copy link

jefferai commented Mar 4, 2020

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

Go 1.14

Does this issue reproduce with the latest release?

Yes, as well as 1.13.8.

What did you do?

When using a custom net.Listener there was a case where a nil net.Conn could be returned, along with a nil error. The listen function doesn't check whether or not the connection coming from Accept is nil: https://golang.org/src/net/http/server.go?s=90476:90522#L2901

This can lead to a hard to track down panic, because if this happens e.g. on program shutdown whether you actually see the panic or not may be timing-dependent.

I should note that the net.Listener documentation does not say that it is required to return a non-nil net.Conn if err is nil.

What did you expect to see?

A proper nil check.

What did you see instead?

A panic.

@bradfitz
Copy link
Contributor

bradfitz commented Mar 4, 2020

Go only documents unusual cases.

If a func returns (T, error), that means by default that exactly 1 of T or error must be non-nil. If both can be non-nil or both can be nil, then we document it.

@bradfitz
Copy link
Contributor

bradfitz commented Mar 4, 2020

I don't think there's anything to do here. Maybe we could panic with a more helpful string, but I'd still panic.

@bradfitz bradfitz changed the title net/http: Server panic's on nil connection net/http: Server panics on nil connection Mar 4, 2020
@jefferai
Copy link
Author

jefferai commented Mar 4, 2020

If a func returns (T, error), that means by default that exactly 1 of T or error must be non-nil. If both can be non-nil or both can be nil, then we document it.

Can I just ask, where is that in the spec? I haven't read the spec lately, it's been a few years, but I don't remember seeing that and some grepping through the spec right now doesn't pull anything up.

@ianlancetaylor
Copy link
Contributor

It's not in the spec. The language permits returning multiple non-zero values. But the convention is that when returning an error the other values are returned as the zero values for their type. Functions that do not follow that convention are expected to document that fact. There is no need to document the fact that a function does follow that convention.

@bradfitz
Copy link
Contributor

bradfitz commented Mar 5, 2020

There is no need to document the fact that a function does follow that convention.

In fact, it's actively harmful, because then people wonder why some places document it and some don't. We've actually removed redundant documentation in the past because it was causing such confusion elsewhere.

@jefferai
Copy link
Author

jefferai commented Mar 6, 2020

There's a difference between "this is what I see a lot of places" and "this is a convention that is expected to be followed". If this isn't in the spec, you are relying on the user's memory and/or experience and/or for them to never actually just make mistakes and have bugs (as happened in this case) in order to not trip up the stdlib with an iron-clad but undocumented convention you are expected to follow. Maybe I'm alone here but it feels pretty user-hostile to require developers to just "know convention", especially for a language as generally well-specified as Go.

Regardless, I don't even really see what all of that that has to do with this report. My custom listener had a bug and it triggered a panic. Wouldn't it be better to do a nil check and return an error than panic on a nil pointer? Defense in depth and all.

@dmitshur dmitshur added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Mar 9, 2020
@dmitshur dmitshur added this to the Backlog milestone Mar 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

4 participants