fix: copy sets before sorting in SetEquals, SetEqualsInt and Set2DEquals

util/util.go

@@ -153,57 +153,70 @@ func ParamsToString(s ...string) string {
 }
 
 // SetEquals determines whether two string sets are identical.
+// Neither argument is modified: these are usually answers the enforcer handed
+// back, which alias its own state, so sorting them in place would reorder the
+// caller's data as a side effect of asking a question about it.
 func SetEquals(a []string, b []string) bool {
 	if len(a) != len(b) {
 		return false
 	}
 
-	sort.Strings(a)
-	sort.Strings(b)
+	sortedA := append([]string(nil), a...)
+	sortedB := append([]string(nil), b...)
+	sort.Strings(sortedA)
+	sort.Strings(sortedB)
 
-	for i, v := range a {
-		if v != b[i] {
+	for i, v := range sortedA {
+		if v != sortedB[i] {
 			return false
 		}
 	}
 	return true
 }
 
-// SetEquals determines whether two int sets are identical.
+// SetEqualsInt determines whether two int sets are identical.
+// Neither argument is modified, for the same reason as SetEquals.
 func SetEqualsInt(a []int, b []int) bool {
 	if len(a) != len(b) {
 		return false
 	}
 
-	sort.Ints(a)
-	sort.Ints(b)
+	sortedA := append([]int(nil), a...)
+	sortedB := append([]int(nil), b...)
+	sort.Ints(sortedA)
+	sort.Ints(sortedB)
 
-	for i, v := range a {
-		if v != b[i] {
+	for i, v := range sortedA {
+		if v != sortedB[i] {
 			return false
 		}
 	}
 	return true
 }
 
 // Set2DEquals determines whether two string slice sets are identical.
+// Neither argument is modified, rows included: GetPermissionsForUser and its
+// siblings hand back the model's own rules, and sorting a row in place
+// permuted the fields of a stored policy rule.
 func Set2DEquals(a [][]string, b [][]string) bool {
 	if len(a) != len(b) {
 		return false
 	}
 
-	var aa []string
-	for _, v := range a {
-		sort.Strings(v)
-		aa = append(aa, strings.Join(v, ", "))
-	}
-	var bb []string
-	for _, v := range b {
-		sort.Strings(v)
-		bb = append(bb, strings.Join(v, ", "))
+	return SetEquals(sortedRows(a), sortedRows(b))
+}
+
+// sortedRows renders each row as a string with its fields in sorted order,
+// working on a copy of the row.
+func sortedRows(rows [][]string) []string {
+	rendered := make([]string, 0, len(rows))
+	for _, row := range rows {
+		sorted := append([]string(nil), row...)
+		sort.Strings(sorted)
+		rendered = append(rendered, strings.Join(sorted, ", "))
 	}
 
-	return SetEquals(aa, bb)
+	return rendered
 }
 
 // JoinSlice joins a string and a slice into a new slice.

util/util_test.go

@@ -279,3 +279,63 @@ func TestEscapeStringLiterals(t *testing.T) {
 	testEscapeStringLiterals(t, `'hello'`, `'hello'`)
 	testEscapeStringLiterals(t, `"world"`, `"world"`)
 }
+
+// The Set* comparisons answer a question about their arguments and must not
+// rewrite them. They used to sort in place, and since the enforcer hands back
+// slices that alias its own policy, comparing an answer permuted the stored
+// rule - see the enforcer level regression test TestSet2DEqualsKeepsPolicyIntact.
+func TestSetComparisonsDoNotModifyArguments(t *testing.T) {
+	strs := []string{"c", "a", "b"}
+	other := []string{"b", "c", "a"}
+	if !SetEquals(strs, other) {
+		t.Error("SetEquals should hold for the same members in another order")
+	}
+	if !ArrayEquals(strs, []string{"c", "a", "b"}) {
+		t.Errorf("SetEquals reordered its first argument: %v", strs)
+	}
+	if !ArrayEquals(other, []string{"b", "c", "a"}) {
+		t.Errorf("SetEquals reordered its second argument: %v", other)
+	}
+
+	ints := []int{3, 1, 2}
+	otherInts := []int{2, 3, 1}
+	if !SetEqualsInt(ints, otherInts) {
+		t.Error("SetEqualsInt should hold for the same members in another order")
+	}
+	if ints[0] != 3 || ints[1] != 1 || ints[2] != 2 {
+		t.Errorf("SetEqualsInt reordered its first argument: %v", ints)
+	}
+	if otherInts[0] != 2 || otherInts[1] != 3 || otherInts[2] != 1 {
+		t.Errorf("SetEqualsInt reordered its second argument: %v", otherInts)
+	}
+
+	rules := [][]string{{"zoe", "data1", "read"}, {"amy", "data2", "write"}}
+	want := [][]string{{"amy", "data2", "write"}, {"zoe", "data1", "read"}}
+	if !Set2DEquals(rules, want) {
+		t.Error("Set2DEquals should hold for the same rules in another order")
+	}
+	if !ArrayEquals(rules[0], []string{"zoe", "data1", "read"}) {
+		t.Errorf("Set2DEquals reordered a row of its first argument: %v", rules)
+	}
+	if !ArrayEquals(want[1], []string{"zoe", "data1", "read"}) {
+		t.Errorf("Set2DEquals reordered a row of its second argument: %v", want)
+	}
+
+	// The comparisons still discriminate after the change.
+	if SetEquals([]string{"a", "b"}, []string{"a", "c"}) {
+		t.Error("SetEquals should not hold for different members")
+	}
+	if SetEqualsInt([]int{1, 2}, []int{1, 3}) {
+		t.Error("SetEqualsInt should not hold for different members")
+	}
+	if Set2DEquals(rules, [][]string{{"zoe", "data1", "read"}, {"amy", "data2", "read"}}) {
+		t.Error("Set2DEquals should not hold for different rules")
+	}
+}
+
+// TestLRUCachePutOverwritesAnExistingKey pins CAS-8: Put on a key already in
+// the cache reuses that key's node for its links, so the new value has to be
+// written onto it. Re-linking alone left the cache answering with the value the
+// key was first stored with, and every later Put for that key was discarded
+// without a word. Both the plain and the synchronised cache are driven, because
+// the synchronised one delegates and would inherit the defect silently.