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: panic when a nil HandlerFunc is passed to http.HandleFunc #24297

Closed
wangsong93 opened this issue Mar 7, 2018 · 8 comments

Comments

Projects
None yet
5 participants
@wangsong93
Copy link

commented Mar 7, 2018

Please answer these questions before submitting your issue. Thanks!

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

1.9.2

Does this issue reproduce with the latest release?

yes

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

amd64 linux

What did you do?

package main

import "net/http"

func main() {
    http.HandleFunc("/", nil)
    http.ListenAndServe(":8080", nil)
}

What did you expect to see?

when I go run this file, the nil error appears not in compilation time but in run time. The reason is that http.Handler(http.HandlerFunc(nil)) == nil is false. BUT use reflect we can know the value of http.Handler(http.HandlerFunc(nil)) is nil, so the error should be stopped in compilation time by fix Handle function.

What did you see instead?

@davecheney

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2018

@namusyaka namusyaka changed the title when the 2nd parameter of net/http.HandleFunc(string, func(ResponseWriter, *Request)) is nil, the error cannot appear in compile time. net/http: when the 2nd parameter of net/http.HandleFunc(string, func(ResponseWriter, *Request)) is nil, the error cannot appear in compile time. Mar 7, 2018

@namusyaka

This comment has been minimized.

Copy link
Member

commented Mar 7, 2018

I guess he would like to say that it is a matter of panic occurring after the mux actually received the request. It will be something like the following error:

http: panic serving [::1]:51880: runtime error: invalid memory address or nil pointer dereference

And it is possible to generate a more descriptive panic by actually checking whether it is nil at the timing when HandleFunc is called without using reflect. It would be something like:

diff --git a/src/net/http/server.go b/src/net/http/server.go
index a7ba753bf5..04a0444140 100644
--- a/src/net/http/server.go
+++ b/src/net/http/server.go
@@ -2358,6 +2358,9 @@ func (mux *ServeMux) Handle(pattern string, handler Handler) {

 // HandleFunc registers the handler function for the given pattern.
 func (mux *ServeMux) HandleFunc(pattern string, handler func(ResponseWriter, *Request)) {
+       if handler == nil {
+               panic("net/http: handler must not be nil")
+       }
        mux.Handle(pattern, HandlerFunc(handler))
 }

/cc @bradfitz @tombergan

@andybons andybons changed the title net/http: when the 2nd parameter of net/http.HandleFunc(string, func(ResponseWriter, *Request)) is nil, the error cannot appear in compile time. net/http: panic when a nil HandlerFunc is passed to http.HandleFunc Mar 7, 2018

@andybons

This comment has been minimized.

Copy link
Member

commented Mar 7, 2018

We won’t be able to catch this at compile time. @namusyaka’s fix seems reasonable unless I’m missing something.

@andybons andybons added this to the Go1.11 milestone Mar 7, 2018

@wangsong93

This comment has been minimized.

Copy link
Author

commented Mar 8, 2018

I mean how to reproduce the panic in following code even if the 2nd parameter is nil

func (mux *ServeMux) Handle(pattern string, handler Handler) {
	mux.mu.Lock()
	defer mux.mu.Unlock()

	if pattern == "" {
		panic("http: invalid pattern " + pattern)
	}
        // if reflect.ValueOf(handler).IsNil() { // this can be a solution
	if handler == nil {
		panic("http: nil handler")
	}
	if mux.m[pattern].explicit {
		panic("http: multiple registrations for " + pattern)
	}
@namusyaka

This comment has been minimized.

Copy link
Member

commented Mar 8, 2018

I'm going to send a CL about this problem in a few days.

@namusyaka

This comment has been minimized.

Copy link
Member

commented Mar 8, 2018

@wangsong93 I don't think it is a solution about the issue. Since it is called on every request, it should not be an essential solution.
In order to solve the issue you reported, it is enough to check the second argument when http.HandleFunc is called.

@wangsong93

This comment has been minimized.

Copy link
Author

commented Mar 8, 2018

@namusyaka yes, your solution is better and I agree. My solution is just used to verify my idea. Nil parameters should be stopped as earlier as possible.

@namusyaka namusyaka self-assigned this Mar 8, 2018

@gopherbot

This comment has been minimized.

Copy link

commented Mar 8, 2018

Change https://golang.org/cl/99575 mentions this issue: net/http: improved to detect nil Handler when calling (*ServeMux)HandleFunc

@gopherbot gopherbot closed this in 7d654af Mar 8, 2018

@golang golang locked and limited conversation to collaborators Mar 8, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.