collections: cache data in FindAll()

internal/collections/concat.go

@@ -17,6 +17,7 @@ import (
 type ConcatCollection struct {
 	data     []collection.Collection
 	variable variables.RuleVariable
+	cache    concatCache
 }
 
 var _ collection.Collection = &ConcatCollection{}
@@ -30,11 +31,11 @@ func NewConcatCollection(variable variables.RuleVariable, data ...collection.Col
 
 // FindAll returns all matches for all collections
 func (c *ConcatCollection) FindAll() []types.MatchData {
-	var res []types.MatchData
-	for _, d := range c.data {
-		res = append(res, replaceVariable(c.variable, d.FindAll())...)
+	parts := make([][]types.MatchData, len(c.data))
+	for i, d := range c.data {
+		parts[i] = d.FindAll()
 	}
-	return res
+	return c.cache.concat(c.variable, parts)
 }
 
 // Name returns the name for the current CollectionconcatCollection
@@ -46,6 +47,7 @@ func (c *ConcatCollection) Name() string {
 type ConcatKeyed struct {
 	data     []collection.Keyed
 	variable variables.RuleVariable
+	cache    concatCache
 }
 
 var _ collection.Keyed = &ConcatKeyed{}
@@ -77,6 +79,11 @@ func (c *ConcatKeyed) FindRegex(key *regexp.Regexp) []types.MatchData {
 
 // FindString returns a slice of MatchData for the string
 func (c *ConcatKeyed) FindString(key string) []types.MatchData {
+	if key == "" {
+		// Children answer an empty key with their shared FindAll result,
+		// which must not be relabelled in place below.
+		return c.FindAll()
+	}
 	var res []types.MatchData
 	for _, d := range c.data {
 		res = append(res, replaceVariable(c.variable, d.FindString(key))...)
@@ -86,20 +93,74 @@ func (c *ConcatKeyed) FindString(key string) []types.MatchData {
 
 // FindAll returns all matches for all collections
 func (c *ConcatKeyed) FindAll() []types.MatchData {
-	var res []types.MatchData
-	for _, d := range c.data {
-		res = append(res, replaceVariable(c.variable, d.FindAll())...)
+	parts := make([][]types.MatchData, len(c.data))
+	for i, d := range c.data {
+		parts[i] = d.FindAll()
 	}
-	return res
+	return c.cache.concat(c.variable, parts)
 }
 
 // Name returns the name for the current CollectionconcatCollection
 func (c *ConcatKeyed) Name() string {
 	return c.variable.Name()
 }
 
+// concatCache reuses the concatenation while every part is the very same
+// slice as last time, which is the case when the parts come from unchanged
+// caching collections. Holding the old parts keeps their memory alive, so a
+// new slice can never reuse the same address.
+type concatCache struct {
+	parts [][]types.MatchData
+	res   []types.MatchData
+}
+
+func sameSlice(a, b []types.MatchData) bool {
+	return len(a) == len(b) && (len(a) == 0 || &a[0] == &b[0])
+}
+
+func (c *concatCache) concat(v variables.RuleVariable, parts [][]types.MatchData) []types.MatchData {
+	if c.parts != nil && len(c.parts) == len(parts) {
+		hit := true
+		for i := range parts {
+			if !sameSlice(c.parts[i], parts[i]) {
+				hit = false
+				break
+			}
+		}
+		if hit {
+			return c.res
+		}
+	}
+
+	n := 0
+	for _, p := range parts {
+		n += len(p)
+	}
+	var res []types.MatchData
+	if n > 0 {
+		// Parts may be shared by their collection: copy rather than relabel them in place.
+		buf := make([]corazarules.MatchData, n)
+		res = make([]types.MatchData, n)
+		i := 0
+		for _, p := range parts {
+			for _, m := range p {
+				buf[i] = corazarules.MatchData{
+					Variable_: v,
+					Key_:      m.Key(),
+					Value_:    m.Value(),
+				}
+				res[i] = &buf[i]
+				i++
+			}
+		}
+	}
+	c.parts, c.res = parts, res
+	return res
+}
+
 // replaceVariable ensures a returned match references the variable of a concatenated variable,
-// not original one.
+// not original one. It relabels md in place, so md must be a fresh slice owned by the caller,
+// never a shared FindAll result.
 func replaceVariable(v variables.RuleVariable, md []types.MatchData) []types.MatchData {
 	for _, m := range md {
 		m.(*corazarules.MatchData).Variable_ = v

internal/collections/map.go

@@ -22,6 +22,17 @@ type Map struct {
 	// summing len(values) across every key on each call made checkArgumentLimit
 	// (called once per Add) quadratic in the number of distinct keys.
 	totalValues int
+	// version changes on every mutation, so views can tell when to rebuild.
+	version uint64
+	// all caches FindAll: rules ask for the same collection over and over
+	// within a phase. Callers must not modify it.
+	all        []types.MatchData
+	allVersion uint64
+}
+
+func (c *Map) touch() {
+	c.version++
+	c.all = nil
 }
 
 var _ collection.Map = &Map{}
@@ -121,8 +132,12 @@ func (c *Map) FindString(key string) []types.MatchData {
 	return result
 }
 
-// FindAll returns all map elements.
+// FindAll returns all map elements. The result is shared until the next
+// mutation of the map, callers must not modify it.
 func (c *Map) FindAll() []types.MatchData {
+	if c.all != nil && c.allVersion == c.version {
+		return c.all
+	}
 	n := 0
 	for _, data := range c.data {
 		n += len(data)
@@ -144,11 +159,13 @@ func (c *Map) FindAll() []types.MatchData {
 			i++
 		}
 	}
+	c.all, c.allVersion = result, c.version
 	return result
 }
 
 // Add adds a new key-value pair to the map.
 func (c *Map) Add(key string, value string) {
+	c.touch()
 	aVal := keyValue{key: key, value: value}
 	if !c.isCaseSensitive {
 		key = strings.ToLower(key)
@@ -159,6 +176,7 @@ func (c *Map) Add(key string, value string) {
 
 // Sets the value of a key with the array of strings passed. If the key already exists, it will be overwritten.
 func (c *Map) Set(key string, values []string) {
+	c.touch()
 	originalKey := key
 	if !c.isCaseSensitive {
 		key = strings.ToLower(key)
@@ -179,6 +197,7 @@ func (c *Map) Set(key string, values []string) {
 
 // SetIndex sets the value of a key at the specified index. If the key already exists, it will be overwritten.
 func (c *Map) SetIndex(key string, index int, value string) {
+	c.touch()
 	originalKey := key
 	if !c.isCaseSensitive {
 		key = strings.ToLower(key)
@@ -200,6 +219,7 @@ func (c *Map) SetIndex(key string, index int, value string) {
 
 // Remove removes a key/value from the map.
 func (c *Map) Remove(key string) {
+	c.touch()
 	if !c.isCaseSensitive {
 		key = strings.ToLower(key)
 	}
@@ -217,6 +237,7 @@ func (c *Map) Name() string {
 
 // Reset removes all key/value pairs from the map.
 func (c *Map) Reset() {
+	c.touch()
 	for k := range c.data {
 		delete(c.data, k)
 	}

internal/collections/map_test.go

@@ -340,3 +340,30 @@ func BenchmarkTxSetGet(b *testing.B) {
 	})
 	b.ReportAllocs()
 }
+
+// FindAll caches its result until the next mutation: every mutator must
+// invalidate it, or rules see stale data for the rest of the phase.
+func TestMapFindAllInvalidation(t *testing.T) {
+	tests := []struct {
+		name   string
+		mutate func(c *Map)
+		want   []string
+	}{
+		{name: "no mutation", mutate: func(*Map) {}, want: []string{"v1", "v2"}},
+		{name: "Add", mutate: func(c *Map) { c.Add("k3", "v3") }, want: []string{"v1", "v2", "v3"}},
+		{name: "Set", mutate: func(c *Map) { c.Set("k1", []string{"new"}) }, want: []string{"new", "v2"}},
+		{name: "SetIndex", mutate: func(c *Map) { c.SetIndex("k1", 0, "new") }, want: []string{"new", "v2"}},
+		{name: "Remove", mutate: func(c *Map) { c.Remove("k1") }, want: []string{"v2"}},
+		{name: "Reset", mutate: func(c *Map) { c.Reset() }, want: nil},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			c := NewMap(variables.ArgsGet)
+			c.Add("k1", "v1")
+			c.Add("k2", "v2")
+			assertUnorderedValuesMatch(t, c.FindAll(), "v1", "v2")
+			tt.mutate(c)
+			assertUnorderedValuesMatch(t, c.FindAll(), tt.want...)
+		})
+	}
+}

internal/collections/named.go

@@ -98,6 +98,9 @@ func (c *NamedCollection) String() string {
 type NamedCollectionNames struct {
 	variable   variables.RuleVariable
 	collection *NamedCollection
+	// all caches FindAll while the underlying collection is unchanged.
+	all        []types.MatchData
+	allVersion uint64
 }
 
 func (c *NamedCollectionNames) FindRegex(key *regexp.Regexp) []types.MatchData {
@@ -153,6 +156,9 @@ func (c *NamedCollectionNames) Get(key string) []string {
 }
 
 func (c *NamedCollectionNames) FindAll() []types.MatchData {
+	if c.all != nil && c.allVersion == c.collection.version {
+		return c.all
+	}
 	n := 0
 	for _, data := range c.collection.data {
 		n += len(data)
@@ -174,6 +180,7 @@ func (c *NamedCollectionNames) FindAll() []types.MatchData {
 			i++
 		}
 	}
+	c.all, c.allVersion = res, c.collection.version
 	return res
 }
 

internal/collections/named_test.go

@@ -122,4 +122,9 @@ func TestNames(t *testing.T) {
 	if len(r) != 0 {
 		t.Errorf("Error finding nonexistent regex, got %d instead of 0", len(r))
 	}
+
+	assertUnorderedValuesMatch(t, names.FindAll(), "key", "key2", "key2")
+	// FindAll is cached: a mutation of the underlying collection must show up.
+	c.Add("key3", "value4")
+	assertUnorderedValuesMatch(t, names.FindAll(), "key", "key2", "key2", "key3")
 }

internal/corazawaf/transaction.go

@@ -681,27 +681,33 @@ func (tx *Transaction) GetField(rv ruleVariableParams) []types.MatchData {
 		matches = col.FindAll()
 	}
 
-	// in the most common scenario filteredMatches length will be
-	// the same as matches length, so we avoid allocating per result.
-	// We reuse the matches slice to store filtered results avoiding extra allocation.
-	filteredCount := 0
-	for _, c := range matches {
-		isException := false
-		lkey := strings.ToLower(c.Key())
-		for _, ex := range rv.Exceptions {
-			// KeyStr is only meaningful without KeyRx: a regex exception from ctl
-			// carries an empty KeyStr, which would otherwise match a key named "".
-			if (ex.KeyRx != nil && ex.KeyRx.MatchString(lkey)) || (ex.KeyRx == nil && (ex.KeyStr == "" || strings.ToLower(ex.KeyStr) == lkey)) {
-				isException = true
-				break
+	// Matches may be shared with the collection: only copy when an exception
+	// actually removes something.
+	if len(rv.Exceptions) > 0 {
+		var filtered []types.MatchData
+		for i, c := range matches {
+			isException := false
+			lkey := strings.ToLower(c.Key())
+			for _, ex := range rv.Exceptions {
+				// KeyStr is only meaningful without KeyRx: a regex exception from ctl
+				// carries an empty KeyStr, which would otherwise match a key named "".
+				if (ex.KeyRx != nil && ex.KeyRx.MatchString(lkey)) || (ex.KeyRx == nil && (ex.KeyStr == "" || strings.ToLower(ex.KeyStr) == lkey)) {
+					isException = true
+					break
+				}
+			}
+			switch {
+			case isException && filtered == nil:
+				filtered = make([]types.MatchData, i, len(matches))
+				copy(filtered, matches[:i])
+			case !isException && filtered != nil:
+				filtered = append(filtered, c)
 			}
 		}
-		if !isException {
-			matches[filteredCount] = c
-			filteredCount++
+		if filtered != nil {
+			matches = filtered
 		}
 	}
-	matches = matches[:filteredCount]
 
 	if rv.Count {
 		count := len(matches)

testing/engine/variables.go

@@ -177,3 +177,39 @@ SecRule ARGS:key2[] "@contains newValue" "id:11,phase:2,pass,setvar:'tx.macro_ex
 SecRule ARGS_POST:key2[name] "@contains PaYlOaD" "id:12,phase:2,pass,log,logdata:'Message from rule 12: %{MATCHED_VAR_NAME}, macro expansion: %{ARGS_POST.key2[name]}'"
 `,
 })
+
+// Collections share their FindAll result between rules within a phase, so
+// one rule's view of a collection must not leak into the next rule's.
+var _ = profile.RegisterProfile(profile.Profile{
+	Meta: profile.Meta{
+		Author:      "Sebastien Blot",
+		Description: "Rules reading the same collection do not affect each other",
+		Enabled:     true,
+		Name:        "variables_shared_collections.yaml",
+	},
+	Tests: []profile.Test{
+		{
+			Title: "concatenated and excepted collections",
+			Stages: []profile.Stage{
+				{
+					Stage: profile.SubStage{
+						Input: profile.StageInput{
+							URI: "/?a=x&b=y&c=y&d=y&e=y&f=y&g=y&h=y",
+						},
+						Output: profile.ExpectedOutput{
+							TriggeredRules: []int{2, 3, 4, 5},
+							LogContains:    `[msg "child ARGS_GET:a"]`,
+						},
+					},
+				},
+			},
+		},
+	},
+	Rules: `
+SecRuleEngine On
+SecRule ARGS "@streq x" "id:2,phase:1,pass,log"
+SecRule ARGS_GET "@streq x" "id:3,phase:1,pass,log,msg:'child %{MATCHED_VAR_NAME}'"
+SecRule ARGS_GET|!ARGS_GET:a "@streq y" "id:4,phase:1,pass,log"
+SecRule ARGS_GET "@streq x" "id:5,phase:1,pass,log"
+`,
+})