From 6ed6dd8db56f382b8500852f2d1e3e0b3e4c6136 Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Tue, 13 Oct 2026 09:38:01 +0000 Subject: [PATCH] policy/v2: stop ViaRoutesForPeer appending into pm.pol.Grants Updates #3515 (cherry picked from commit 5cadfa16eb0bbeee3232bd9dae9c7c51f9be61f1) --- hscontrol/policy/v2/policy.go | 4 +- .../policy/v2/policy_concurrency_test.go | 50 +++++++++++++++++++ 2 files changed, 53 insertions(+), 1 deletion(-) diff --git a/hscontrol/policy/v2/policy.go b/hscontrol/policy/v2/policy.go index 804a9848b..3f0444de6 100644 --- a/hscontrol/policy/v2/policy.go +++ b/hscontrol/policy/v2/policy.go @@ -1241,7 +1241,9 @@ func (pm *PolicyManager) ViaRoutesForPeer(viewer, peer types.NodeView) types.Via return result } - grants := pm.pol.Grants + // Clip so the appends below allocate: callers share pm.pol under RLock, + // and writing into its spare capacity races between them. + grants := slices.Clip(pm.pol.Grants) for _, acl := range pm.pol.ACLs { grants = append(grants, aclToGrants(acl)...) } diff --git a/hscontrol/policy/v2/policy_concurrency_test.go b/hscontrol/policy/v2/policy_concurrency_test.go index b4b759f59..026c9f665 100644 --- a/hscontrol/policy/v2/policy_concurrency_test.go +++ b/hscontrol/policy/v2/policy_concurrency_test.go @@ -2,6 +2,7 @@ package v2 import ( "fmt" + "slices" "sync" "testing" @@ -101,3 +102,52 @@ func TestPolicyManagerConcurrentReads(t *testing.T) { wg.Wait() } + +// TestViaRoutesForPeerDoesNotWritePolicyGrants guards against +// ViaRoutesForPeer appending ACL-derived grants into the backing array of +// pm.pol.Grants. Readers hold only the shared RLock, so any spare capacity +// in that slice becomes a write/write race between concurrent callers. +func TestViaRoutesForPeerDoesNotWritePolicyGrants(t *testing.T) { + users := types.Users{{Name: "user1", Email: "user1@headscale.net"}} + + a := node("a", "100.64.0.1", "fd7a:115c:a1e0::1", users[0]) + a.ID = 1 + b := node("b", "100.64.0.2", "fd7a:115c:a1e0::2", users[0]) + b.ID = 2 + nodes := types.Nodes{a, b} + + // The via grant keeps ViaRoutesForPeer past its no-via early return. + policy := `{ + "tagOwners": {"tag:router": ["user1@headscale.net"]}, + "grants": [ + {"src": ["*"], "dst": ["*"], "ip": ["tcp:22"]}, + {"src": ["*"], "dst": ["10.0.0.0/8"], "ip": ["*"], "via": ["tag:router"]} + ], + "acls": [ + {"action": "accept", "src": ["*"], "dst": ["*:*"]} + ] + }` + + pm, err := NewPolicyManager([]byte(policy), users, nodes.ViewSlice()) + require.NoError(t, err) + + // Guarantee spare capacity instead of relying on the JSON decoder's + // growth policy. + pm.pol.Grants = slices.Grow(pm.pol.Grants, 4) + spare := pm.pol.Grants[len(pm.pol.Grants):cap(pm.pol.Grants)] + + var wg sync.WaitGroup + for range 8 { + wg.Go(func() { + for range 50 { + pm.ViaRoutesForPeer(a.View(), b.View()) + } + }) + } + + wg.Wait() + + for i, g := range spare { + assert.Zero(t, g, "spare slot %d of pm.pol.Grants was written", i) + } +}