Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions regexp.go
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,9 @@ func newRouteRegexp(tpl string, typ regexpType, options routeRegexpOptions) (*ro
if errCompile != nil {
return nil, errCompile
}
if reg == nil {
return nil, fmt.Errorf("mux: regexp compilation returned nil for pattern %q", patternStr)
}

// Check for capturing groups which used to work in older versions
if reg.NumSubexp() != len(idxs)/2 {
Expand Down Expand Up @@ -186,7 +189,14 @@ type routeRegexp struct {
}

// Match matches the regexp against the URL host or path.
// Returns false (no match) if the receiver or compiled regexp is nil.
// This guard prevents panics from a nil *regexp.Regexp, which has been
// observed in production (gorilla#749). A no-match response is preferable
// to crashing the server on an unexpected nil state.
func (r *routeRegexp) Match(req *http.Request, match *RouteMatch) bool {
if r == nil || r.regexp == nil {
return false
}
if r.regexpType == regexpTypeHost {
host := getHost(req)
if r.wildcardHostPort {
Expand Down
40 changes: 40 additions & 0 deletions regexp_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package mux
import (
"net/url"
"reflect"
"regexp"
"strconv"
"strings"
"testing"
Expand Down Expand Up @@ -65,6 +66,45 @@ func Test_findFirstQueryKey(t *testing.T) {
}
}

// Test_routeRegexp_NilRegexp verifies that Match returns false without
// panicking when called on a nil receiver or one with a nil compiled regexp.
// This guards against gorilla#749 where a nil *regexp.Regexp panicked
// during Router.ServeHTTP in production.
// Test_newRouteRegexp_NilRegexp verifies that newRouteRegexp returns
// an error when RegexpCompileFunc returns nil without error.
// This prevents a nil regexp from being created.
func Test_newRouteRegexp_NilRegexp(t *testing.T) {
original := RegexpCompileFunc
RegexpCompileFunc = func(string) (*regexp.Regexp, error) {
return nil, nil
}
defer func() { RegexpCompileFunc = original }()

_, err := newRouteRegexp("/{id}", regexpTypePath, routeRegexpOptions{})
if err == nil {
t.Fatal("expected error when RegexpCompileFunc returns nil, got nil")
}
if !strings.Contains(err.Error(), "returned nil") {
t.Errorf("expected 'returned nil' in error, got: %s", err)
}
}

func Test_routeRegexp_NilRegexp(t *testing.T) {
req := newRequest("GET", "/")

// Nil receiver
var nilR *routeRegexp
if nilR.Match(req, &RouteMatch{}) {
t.Fatal("nil routeRegexp should not match")
}

// Non-nil receiver with nil regexp (manually constructed, bypassing constructor)
r := &routeRegexp{regexpType: regexpTypePath}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question as I left on the other PR;

This is manually constructing the routeRegexp, which is a type that's non-exported and has a constructor;

mux/regexp.go

Lines 31 to 41 in db9d1d0

// newRouteRegexp parses a route template and returns a routeRegexp,
// used to match a host, a path or a query string.
//
// It will extract named variables, assemble a regexp to be matched, create
// a "reverse" template to build URLs and compile regexps to validate variable
// values used in URL building.
//
// Previously we accepted only Python-like identifiers for variable
// names ([a-zA-Z_][a-zA-Z0-9_]*), but currently the only restriction is that
// name and pattern can't be empty, and names can't contain a colon.
func newRouteRegexp(tpl string, typ regexpType, options routeRegexpOptions) (*routeRegexp, error) {

I'm not sure if fixing this by adding guards is the right thing to do; the related ticket indicates that either something is broken elsewhere, or that the code is used incorrectly, leading to the panic (which may actually be the correct thing to do for such cases);

if r.Match(req, &RouteMatch{}) {
t.Fatal("routeRegexp with nil regexp should not match")
}
}

func Benchmark_findQueryKey(b *testing.B) {
tests := []string{
"a=1&b=2",
Expand Down