diff --git a/http/httphi/README.md b/http/httphi/README.md index d91b6c7..23c363c 100644 --- a/http/httphi/README.md +++ b/http/httphi/README.md @@ -48,3 +48,8 @@ for { Runnable server over raw Linux sockets, plus query, form and multipart handlers: [`example_test.go`](./example_test.go). + + +## Naming + +Gonna be honest with y'all. I initially wanted it to be named `httplo` until I saw I could write `httphi.MethHead` with a small change. \ No newline at end of file diff --git a/http/httphi/exchange_test.go b/http/httphi/exchange_test.go index 26824ea..173176f 100644 --- a/http/httphi/exchange_test.go +++ b/http/httphi/exchange_test.go @@ -317,7 +317,9 @@ func TestHandleHTTP10Served(t *testing.T) { // No registered handler must yield 404, not an empty response. func TestHandleNoHandler(t *testing.T) { var sm MuxSlice - sm.Handle("GET /", func(ex *Exchange) { t.Error("handler must not run") }) + // "/{$}" is the root and nothing else; a bare "/" is a catch-all that would + // match /nowhere too, see [SetPathValues]. + sm.Handle("GET /{$}", func(ex *Exchange) { t.Error("handler must not run") }) conn := serve(t, "GET /nowhere HTTP/1.1\r\nHost: h\r\n\r\n", &sm) const want = "HTTP/1.1 404 Not Found\r\n\r\n" if got := conn.ViewWritten(); got != want { diff --git a/http/httphi/mux.go b/http/httphi/mux.go index 30de2e9..d639cd5 100644 --- a/http/httphi/mux.go +++ b/http/httphi/mux.go @@ -66,6 +66,7 @@ func Handle(exch *Exchange, mux Mux, backoff lneto.BackoffStrategy) error { // Mux on the request path: the query string is the handler's business. path := reqhdr.RequestPath() meth := reqhdr.Method() + clear(exch.pathValues) matchedPattern, handler := mux.LookupHandler(MethodFromBytes(meth), path, exch.pathValues) if handler != nil { exch.matchedPattern = matchedPattern @@ -124,17 +125,30 @@ var pathSeparator = []byte{'/'} // Unlike ServeMux, segments are compared and bound raw, so "/users/{id}" binds // "x%2Fy" and not "x/y". Which paths match is unaffected. Bound values alias // requestPath rather than copy it. +// +// Values are bound while walking, before the match is known, so on failure +// SetPathValues clears what it bound. A [Mux] may then try patterns in turn +// without a matching one inheriting values from one that failed. func SetPathValues(dstPathVals []PathValue, pattern string, requestPath []byte) (matched, pathValSliceTooShort bool) { + n, matched, pathValSliceTooShort := setPathValues(dstPathVals, pattern, requestPath) + if !matched { + clear(dstPathVals[:n]) + } + return matched, pathValSliceTooShort +} + +// setPathValues is [SetPathValues] reporting how many values it bound, so its +// caller can discard them when the pattern turns out not to match. +func setPathValues(dstPathVals []PathValue, pattern string, requestPath []byte) (n int, matched, pathValSliceTooShort bool) { if len(pattern) == 0 || pattern[0] != '/' || len(requestPath) == 0 || requestPath[0] != '/' { - return false, false + return n, false, false } pattern, requestPath = pattern[1:], requestPath[1:] - n := 0 for { if len(pattern) == 0 { // Nothing left after a slash: an anonymous "..." taking the rest, // which is why "/files/" matches "/files/a/b" and "/" matches all. - return true, false + return n, true, false } patSeg, patRest, patMore := strings.Cut(pattern, "/") reqSeg, reqRest, reqMore := bytes.Cut(requestPath, pathSeparator) @@ -143,40 +157,40 @@ func SetPathValues(dstPathVals []PathValue, pattern string, requestPath []byte) case isWildcard && name == "$": // Matches the end of the path and nothing else, so it must be the // last segment of the pattern and leave no path behind. - return !patMore && len(requestPath) == 0, false + return n, !patMore && len(requestPath) == 0, false case isWildcard && isMulti: // Takes the remainder including slashes, possibly empty. if name != "" { if n >= len(dstPathVals) { - return false, true + return n, false, true } dstPathVals[n] = PathValue{Key: name, Value: requestPath} n++ } - return true, false + return n, true, false case isWildcard: if len(reqSeg) == 0 { - return false, false // One segment means a non-empty one. + return n, false, false // One segment means a non-empty one. } if n >= len(dstPathVals) { - return false, true + return n, false, true } dstPathVals[n] = PathValue{Key: name, Value: reqSeg} n++ default: if b2s(reqSeg) != patSeg { - return false, false + return n, false, false } } if patMore != reqMore { // One side has a further segment and the other does not, so // "/health" misses "/health/" and "/files/" misses "/files". - return false, false + return n, false, false } else if !patMore { - return true, false // Both spent on the same segment. + return n, true, false // Both spent on the same segment. } pattern, requestPath = patRest, reqRest } @@ -221,8 +235,10 @@ func (sm *MuxSlice) LookupHandler(method Method, path []byte, dstPathVals []Path if endpoint.method != MethUndefined && endpoint.method != method { continue } - // Method matches. - if endpoint.pathVals > 0 { + // Method matches. A pattern ending in '/' is a wildcard despite binding no + // values: the trailing slash is an anonymous "{...}", so it must go + // through the matcher and not a literal compare, see [SetPathValues]. + if endpoint.pathVals > 0 || strings.HasSuffix(endpoint.path, "/") { if ok, _ := SetPathValues(dstPathVals, endpoint.path, path); ok { return endpoint.path, endpoint.handler } diff --git a/http/httphi/mux_test.go b/http/httphi/mux_test.go index e2b61f9..f9911eb 100644 --- a/http/httphi/mux_test.go +++ b/http/httphi/mux_test.go @@ -224,3 +224,87 @@ func TestExchangePathValueClearedBetweenRequests(t *testing.T) { t.Errorf("want no path value on a literal route, got id=%q from the previous request", leaked) } } + +// A lookup tries each endpoint in turn and [SetPathValues] binds as it walks, so +// a pattern that binds values and then fails must not leave them behind for the +// pattern that does match: a handler would read a wildcard no matched pattern has. +func TestMuxSliceNoStaleBindingsAcrossCandidates(t *testing.T) { + var sm MuxSlice + sm.Reset(2) + sm.Handle("/a/{x}/{y}/z", func(ex *Exchange) { t.Error("non-matching handler ran") }) + var gotP, gotX, gotY string + sm.Handle("/a/{p}/b", func(ex *Exchange) { + gotP = string(ex.PathValue("p")) + gotX = string(ex.PathValue("x")) + gotY = string(ex.PathValue("y")) + ex.WriteHeader(200) + }) + + exch := new(Exchange) + exch.Configure(ExchangeConfig{ + RawBuf: make([]byte, 2048), RequestBufferLim: 1024, + NumHeaderKVCap: defaultNumHeaderKVCap, MaxPathValues: sm.MaxPathValues(), + }) + conn := newConn("GET /a/1/b HTTP/1.1\r\nHost: h\r\n\r\n") + conn.Hangup() + if !exch.Acquire(conn) { + t.Fatal("fresh exchange failed to acquire") + } + if err := Handle(exch, &sm, nopBackoff); err != nil { + t.Fatal(err) + } + if gotP != "1" { + t.Errorf("want p=1 from the matched pattern, got %q", gotP) + } + if gotX != "" || gotY != "" { + t.Errorf("want no x/y from the failed candidate, got x=%q y=%q", gotX, gotY) + } +} + +// A pattern ending in '/' carries no brace but is still a wildcard: the trailing +// slash is an anonymous "{...}", see [SetPathValues]. MuxSlice must route it +// through the same matcher rather than comparing the path literally. +func TestMuxSliceTrailingSlashPattern(t *testing.T) { + for _, test := range []struct { + pattern string + path string + want bool + }{ + {pattern: "/files/", path: "/files/a/b", want: true}, + {pattern: "/files/", path: "/files/", want: true}, + {pattern: "/files/", path: "/files", want: false}, + {pattern: "/files/", path: "/other/a", want: false}, + {pattern: "/", path: "/anything/at/all", want: true}, + {pattern: "/", path: "/", want: true}, + // Without the trailing slash a pattern stays literal. + {pattern: "/files", path: "/files/a", want: false}, + {pattern: "/files", path: "/files", want: true}, + } { + t.Run(test.pattern+"__"+test.path, func(t *testing.T) { + var sm MuxSlice + sm.Reset(1) + var served bool + sm.Handle(test.pattern, func(ex *Exchange) { served = true; ex.WriteHeader(200) }) + // MuxSlice must agree with the matcher it delegates to. + if ok, _ := SetPathValues(nil, test.pattern, []byte(test.path)); ok != test.want { + t.Fatalf("SetPathValues disagrees with the table: got %v", ok) + } + exch := new(Exchange) + exch.Configure(ExchangeConfig{ + RawBuf: make([]byte, 2048), RequestBufferLim: 1024, + NumHeaderKVCap: defaultNumHeaderKVCap, MaxPathValues: sm.MaxPathValues(), + }) + conn := newConn("GET " + test.path + " HTTP/1.1\r\nHost: h\r\n\r\n") + conn.Hangup() + if !exch.Acquire(conn) { + t.Fatal("fresh exchange failed to acquire") + } + if err := Handle(exch, &sm, nopBackoff); err != nil { + t.Fatal(err) + } + if served != test.want { + t.Errorf("want served=%v, got %v", test.want, served) + } + }) + } +} diff --git a/http/httphi/router.go b/http/httphi/router.go index d72b361..7e4177f 100644 --- a/http/httphi/router.go +++ b/http/httphi/router.go @@ -184,7 +184,8 @@ func (r *Router) Configure(cfg RouterConfig) error { r.respBuf = cfg.ResponseHeaderMinBufferSize r.mux = cfg.Mux r.log = cfg.Logger - r.maxPathValues = cfg.Mux.MaxPathValues() + maxPathValues := cfg.Mux.MaxPathValues() + r.maxPathValues = maxPathValues r.normalizeKeys = cfg.NormalizeOutgoingKeys // Freelist entries were sized by the outgoing configuration: recycling one // would serve a request with buffer limits cfg never asked for. @@ -208,7 +209,7 @@ func (r *Router) Configure(cfg RouterConfig) error { r.exchs = r.exchs[:numgoro] rawBuflen := cfg.RequestHeaderBufferSize + cfg.ResponseHeaderMinBufferSize internal.SliceReuse(&r.globbuf, numgoro*rawBuflen) - maxPathValues := cfg.Mux.MaxPathValues() + for i := range numgoro { // TODO exchange buffer alloc goff := i * rawBuflen diff --git a/http/httphi/router_test.go b/http/httphi/router_test.go index b797e63..88aff7c 100644 --- a/http/httphi/router_test.go +++ b/http/httphi/router_test.go @@ -247,7 +247,9 @@ func TestRouterMux(t *testing.T) { sm MuxSlice router Router ) - sm.Handle("GET /", staticPage(t, "root")) + // "/{$}" is the root alone: a bare "/" is a catch-all and would serve + // "root" for /page and /nowhere as well, see [SetPathValues]. + sm.Handle("GET /{$}", staticPage(t, "root")) sm.Handle("GET /page", staticPage(t, "page")) sm.Handle("/any", staticPage(t, "any")) // No method: matches any. configSynchronousRouter(t, &router, bufferSize, &sm)