fix: make fallback handler reachable again when no routes match

[ydylla]

This is my try for a fix of #207. If all routes signaled they can definitely not match (even with more data) we call the fallback handler directly. This allows configs that use the listener wrapper to use the caddy http app as fallback again (in most situations). See the "listener wrapper fallback" config in #209 for an example.
But this requires discipline from matcher authors to only return false, nil if they are sure a match is impossible. If a match is potentially possible with more data they must return false, layer4.ErrConsumedAllPrefetchedBytes. I fixed the http matcher which was the only one affected by this currently.

It's a draft for now because it needs more testing and a version of an endless loop is back. Can be triggered with the "endless loop bug" config from #209. I have to think about how to solve it or if we can accept that broken configs do broken things.

@WeidiDeng feel free to try it with your listener wrapper configs.

[mholt]

Thank you @ydylla . I like the ring approach.

@WeidiDeng Do you think you could take a look and see what you think?

Also, does this conflict with #208 or is this separate?

[WeidiDeng]

@mholt That exploit regarding infinite loop discussed on slack is not fixed, and it's incompatible with my patch.

[ydylla]

I think I found a good solution to the endless matching bug. By removing one of the 3 loops completely and not resetting the i of the prefetch for loop. Not resetting i is done by the use of a labeled continue (I just learned that Go can do this 🥳). With this prefetch is correctly called and blocks after a proxy_protocol handler fails to consume the available data. Now no loop exists that is unbound, which should help mitigate possible future bugs.
I tested all configs from #209 and all work correctly.

@mholt Regarding the ring: I think I want to remove it again. A single routeIdx int also works and we can save some allocations by removing it:

replace-ring.diff

diff --git a/layer4/routes.go b/layer4/routes.go
index 8afebf1..c996765 100644
--- a/layer4/routes.go
+++ b/layer4/routes.go
@@ -15,7 +15,6 @@
 package layer4
 
 import (
-	"container/ring"
 	"encoding/json"
 	"errors"
 	"fmt"
@@ -103,11 +102,7 @@ func (routes RouteList) Compile(logger *zap.Logger, matchingTimeout time.Duratio
 	return HandlerFunc(func(cx *Connection) error {
 		deadline := time.Now().Add(matchingTimeout)
 
-		routesIdxRing := ring.New(len(routes))
-		for i := 0; i < len(routes); i++ {
-			routesIdxRing.Value = i
-			routesIdxRing = routesIdxRing.Next()
-		}
+		routeIdx := -1 // init with -1 because before first use we increment it
 
 		notMatchingRoutes := make(map[int]struct{}, len(routes))
 	router:
@@ -135,13 +130,15 @@ func (routes RouteList) Compile(logger *zap.Logger, matchingTimeout time.Duratio
 				}
 			}
 
-			// Use a ring to try routes in a strictly circular fashion.
+			// Use a wrapping routeIdx similar to a container/ring to try routes in a strictly circular fashion.
 			// After a match continue with the routes after the matched one, instead of starting at the beginning.
 			// This is done for backwards compatibility with configs written before the "Non blocking matchers & matching timeout" rewrite.
 			// See https://github.com/mholt/caddy-l4/pull/192 and https://github.com/mholt/caddy-l4/pull/192#issuecomment-2143681952.
 			for j := 0; j < len(routes); j++ {
-				routeIdx := routesIdxRing.Value.(int)
-				routesIdxRing = routesIdxRing.Next()
+				routeIdx++
+				if routeIdx >= len(routes) {
+					routeIdx = 0
+				}
 
 				// Skip routes that signaled they definitely can not match
 				if _, ok := notMatchingRoutes[routeIdx]; ok {

Let me know if you think it is worth it or we should keep the ring to better convey the intent.
We could probably also pool the notMatchingRoutes maps like we do with the matching buffers.

[mholt]

@ydylla I like the simplicity of that proposed patch! I'd be down for ripping out the ring, especially if comments explain what is happening.

[ydylla]

@mholt I have removed the ring and also fixed the new matchers from master to not swallow the ErrConsumedAllPrefetchedBytes error. Which is now required like I mentioned in the initial post.

[ydylla]

Superseded by #208

[mholt]

Thanks for your hard work on this 👍