httpcaddyfile: Fix panic in automation policy consolidation

[francislavoie]

Fixes #4101

The panic:

panic: runtime error: index out of range [-1]

goroutine 1 [running]:
github.com/caddyserver/caddy/v2/caddyconfig/httpcaddyfile.consolidateAutomationPolicies(0xc00052e800, 0x3, 0x40, 0x0, 0x0, 0xc0006e0de0)
	caddyconfig/httpcaddyfile/tlsapp.go:498 +0xc4a
github.com/caddyserver/caddy/v2/caddyconfig/httpcaddyfile.ServerType.buildTLSApp(0xc0001f25a0, 0x5, 0x5, 0xc0006bb800, 0x0, 0x0, 0x0, 0xc000b7af60, 0x0, 0x0, ...)
	caddyconfig/httpcaddyfile/tlsapp.go:356 +0xed8
github.com/caddyserver/caddy/v2/caddyconfig/httpcaddyfile.ServerType.Setup(0xc0007a1000, 0x43, 0x80, 0xc0006bb800, 0xc0001e5080, 0x5, 0x8, 0x0, 0xc0006bb9b0, 0x0)
	caddyconfig/httpcaddyfile/httptype.go:218 +0x1095
github.com/caddyserver/caddy/v2/caddyconfig/caddyfile.Adapter.Adapt(0x1a26a40, 0x2414708, 0xc000374840, 0xb5, 0x2b5, 0xc0006bb800, 0xc0003ba360, 0x62000000016fda40, 0xc000bdf598, 0x626ade64124565c6, ...)
	caddyconfig/caddyfile/adapter.go:50 +0x162
github.com/caddyserver/caddy/v2/cmd.loadConfig(0x7ffd96f44ec0, 0x14, 0x7ffd96f44edf, 0x9, 0xc000194330, 0x403e33, 0xc000bdfac0, 0x756ea1a0777002, 0x3026626bc76bb325, 0x40e3b8, ...)
	cmd/main.go:177 +0x644
github.com/caddyserver/caddy/v2/cmd.cmdRun(0xc0001843c0, 0x0, 0x0, 0x0)
	cmd/commandfuncs.go:206 +0x805
github.com/caddyserver/caddy/v2/cmd.Main()
	cmd/main.go:85 +0x248
main.main()
	caddy/main.go:12 +0x25

The problem is that i gets decremented to -1, which makes the next iteration of the loop try to access aps[-1], out of range.

I think this fix is good enough but I might have missed some subtleties.

I confirmed that the adapt test I added also triggers the panic before applying the code changes.

[mholt]

Thanks, you beat me to it!

I will want to look into this too, since I'm not sure either of us 100% understand what the correct logic should be (yet). I just might need a day or ... three.

[mholt]

I'm actually wondering if decrementing i here is the bug, not decrementing it past 0... hmm.

[mholt]

For reference, this decrement was added in 7846bc1.

[mholt]

Thanks for the test case -- I had a chance to look at this more, and this patch makes all the tests (including your new one) pass:

diff --git a/caddyconfig/httpcaddyfile/tlsapp.go b/caddyconfig/httpcaddyfile/tlsapp.go
index 45ba9d21..d14d2335 100644
--- a/caddyconfig/httpcaddyfile/tlsapp.go
+++ b/caddyconfig/httpcaddyfile/tlsapp.go
@@ -491,13 +491,14 @@ func consolidateAutomationPolicies(aps []*caddytls.AutomationPolicy) []*caddytls
 	}
 
 	// remove or combine duplicate policies
+outer:
 	for i := 0; i < len(aps); i++ {
 		// compare only with next policies; we sorted by specificity so we must not delete earlier policies
 		for j := i + 1; j < len(aps); j++ {
 			// if they're exactly equal in every way, just keep one of them
 			if reflect.DeepEqual(aps[i], aps[j]) {
 				aps = append(aps[:j], aps[j+1:]...)
-				i--
+				j--
 				break
 			}
 
@@ -524,6 +525,7 @@ func consolidateAutomationPolicies(aps []*caddytls.AutomationPolicy) []*caddytls
 					if automationPolicyShadows(i, aps) >= j {
 						aps = append(aps[:i], aps[i+1:]...)
 						i--
+						continue outer
 					}
 				} else {
 					// avoid repeated subjects

What do you think?

It occurred to me that even if i is decremented below 0, it should always be incremented again before using it, but that happens in the outer loop, not the inner one where the decrementing happens.

[francislavoie]

That sounds reasonable 👍 I'll adjust

[francislavoie]

golangci-lint detected that j-- is ineffectual (the one changed from i--) because we break immediately after. Removed that line altogether instead.

[mholt]

Kewl, let's give this a try.