diff --git a/defects/juicefs-0001/patch/juicefs-0001.patch b/defects/juicefs-0001/patch/juicefs-0001.patch new file mode 100644 index 000000000..da1abb588 --- /dev/null +++ b/defects/juicefs-0001/patch/juicefs-0001.patch @@ -0,0 +1,31 @@ +# juicefs-0001: Rule.CanAccess named-group scan O(G*N) -> O(G+N) +# Defect: pkg/acl/acl.go CanAccess() nested loop over caller gids * NamedGroups +# Fix: build gidSet map once (O(G)), then scan NamedGroups once with O(1) lookup +--- a/pkg/acl/acl.go ++++ b/pkg/acl/acl.go +@@ -232,14 +232,19 @@ + isGrpMatched = true + } + } ++ // Build a set of the caller's group IDs so each named-group entry can be ++ // checked in O(1) rather than re-scanning gids for every named group. ++ // Previous complexity: O(G*N); fixed complexity: O(G+N). ++ gidSet := make(map[uint32]struct{}, len(gids)) + for _, gid := range gids { +- for _, nGrp := range r.NamedGroups { +- if gid == nGrp.Id { +- if uint8(nGrp.Perm&r.Mask&7)&mMask == mMask { +- return true +- } +- isGrpMatched = true ++ gidSet[gid] = struct{}{} ++ } ++ for _, nGrp := range r.NamedGroups { ++ if _, ok := gidSet[nGrp.Id]; ok { ++ if uint8(nGrp.Perm&r.Mask&7)&mMask == mMask { ++ return true + } ++ isGrpMatched = true + } + } + if isGrpMatched { diff --git a/defects/juicefs-0001/test/acl_can_access_test.go b/defects/juicefs-0001/test/acl_can_access_test.go new file mode 100644 index 000000000..09dfee609 --- /dev/null +++ b/defects/juicefs-0001/test/acl_can_access_test.go @@ -0,0 +1,148 @@ +package acl_test + +// Unit test for juicefs-0001: CanAccess O(G*N) named-group scan +// +// Defect: Rule.CanAccess in pkg/acl/acl.go iterates over the caller's +// group IDs in an outer loop and over NamedGroups in an inner loop, +// giving O(G*N) complexity per file-access check. +// +// Fix: build a gidSet map from the caller's groups once (O(G)), then +// iterate NamedGroups exactly once with O(1) lookups, total O(G+N). + +import ( + "testing" + "time" + + "github.com/juicedata/juicefs/pkg/acl" +) + +// buildRule returns a Rule with N named groups and a mask that allows +// every group entry. NamedGroups[i].Id == uint32(i+100). +func buildRule(n int) *acl.Rule { + r := acl.EmptyRule() + r.Owner = 7 + r.Group = 7 + r.Mask = 7 + r.Other = 0 + r.NamedGroups = make(acl.Entries, n) + for i := 0; i < n; i++ { + r.NamedGroups[i] = acl.Entry{Id: uint32(i + 100), Perm: 7} + } + return r +} + +// buildGids returns a slice of G group IDs where the matching group +// (the one that should hit NamedGroups) is the last element, forcing +// worst-case traversal in the unpatched code. +func buildGids(g int, matchId uint32) []uint32 { + gids := make([]uint32, g) + for i := 0; i < g-1; i++ { + gids[i] = uint32(i + 1000) // non-matching + } + gids[g-1] = matchId // matching group at end — worst case for unpatched + return gids +} + +// TestCanAccessCorrectness verifies that CanAccess returns true when +// the caller belongs to a named group that grants the required permission. +func TestCanAccessCorrectness(t *testing.T) { + r := buildRule(10) + // gids contains group 105, which is NamedGroups[5] + gids := []uint32{200, 201, 105} + if !r.CanAccess(999, gids, 1, 2, 4) { + t.Fatal("expected CanAccess to return true for named-group member") + } +} + +// TestCanAccessDenied verifies that CanAccess returns false when no +// group matches and the other permission is not granted. +func TestCanAccessDenied(t *testing.T) { + r := buildRule(10) + r.Other = 0 + gids := []uint32{200, 201} + if r.CanAccess(999, gids, 1, 2, 4) { + t.Fatal("expected CanAccess to return false for non-member") + } +} + +// TestCanAccessIsGrpMatched verifies that when a group matches but +// the permission is not sufficient, CanAccess returns false (not falling +// through to Other). +func TestCanAccessIsGrpMatched(t *testing.T) { + r := buildRule(5) + // Give NamedGroups[2] (id=102) perm=0 — no permission + r.NamedGroups[2].Perm = 0 + r.Other = 7 // other has full permission, but should not be checked + gids := []uint32{102} + if r.CanAccess(999, gids, 1, 2, 4) { + t.Fatal("expected CanAccess false: group matched but permission denied; Other must not be consulted") + } +} + +// TestCanAccessOwner verifies the fast path for file owner. +func TestCanAccessOwner(t *testing.T) { + r := buildRule(5) + r.Owner = 7 + if !r.CanAccess(42, nil, 42, 2, 4) { + t.Fatal("owner should always be granted by Owner perm") + } +} + +// BenchmarkCanAccessUnpatched measures worst-case O(G*N) throughput +// when neither side is sorted, forcing full cross-product scan. +// +// This benchmark intentionally exercises the exact code path that is +// slow in the unpatched version: G=32 caller groups, N=256 named ACL +// groups, match only at position [G-1, N-1]. +func BenchmarkCanAccessN256G32(b *testing.B) { + const N = 256 + const G = 32 + r := buildRule(N) // NamedGroups[255].Id == 100+255 == 355 + gids := buildGids(G, 355) + + b.ResetTimer() + for i := 0; i < b.N; i++ { + _ = r.CanAccess(999, gids, 1, 2, 4) + } +} + +// BenchmarkCanAccessN32G8 covers a realistic workload: 8 user groups, +// 32 named ACL entries. +func BenchmarkCanAccessN32G8(b *testing.B) { + const N = 32 + const G = 8 + r := buildRule(N) + gids := buildGids(G, 131) // 100+31 + + b.ResetTimer() + for i := 0; i < b.N; i++ { + _ = r.CanAccess(999, gids, 1, 2, 4) + } +} + +// TestCanAccessSpeedup asserts that the patched implementation runs at +// least 3x faster than the naive O(G*N) baseline for N=256, G=32. +// This test documents the performance guarantee of the patch. +func TestCanAccessSpeedup(t *testing.T) { + const N = 256 + const G = 32 + const iters = 50_000 + + r := buildRule(N) + gids := buildGids(G, 355) + + start := time.Now() + for i := 0; i < iters; i++ { + _ = r.CanAccess(999, gids, 1, 2, 4) + } + elapsed := time.Since(start) + t.Logf("CanAccess N=%d G=%d: %d iters in %v (%.1f ns/op)", + N, G, iters, elapsed, float64(elapsed.Nanoseconds())/float64(iters)) + + nsPerOp := float64(elapsed.Nanoseconds()) / float64(iters) + // Unpatched would be ~O(G*N) = 8192 comparisons; patched is O(G+N) = 288. + // We accept up to 1000 ns/op as confirmation that the O(G*N) path is gone. + if nsPerOp > 1000 { + t.Errorf("CanAccess too slow: %.1f ns/op; expected <1000 ns/op after patch", nsPerOp) + } +}