From 644b3278c8f828d835c42781db827e1627282744 Mon Sep 17 00:00:00 2001 From: Sebastien Blot Date: Fri, 2 Oct 2026 16:30:28 +0200 Subject: [PATCH 1/2] collections: cache data in FindAll() --- internal/collections/concat.go | 79 ++++++++++++++++++++++++++---- internal/collections/map.go | 23 ++++++++- internal/collections/map_test.go | 27 ++++++++++ internal/collections/named.go | 7 +++ internal/collections/named_test.go | 5 ++ internal/corazawaf/transaction.go | 40 ++++++++------- testing/engine/variables.go | 47 ++++++++++++++++++ 7 files changed, 201 insertions(+), 27 deletions(-) diff --git a/internal/collections/concat.go b/internal/collections/concat.go index f54489e0d..1a5a392ff 100644 --- a/internal/collections/concat.go +++ b/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,11 +93,11 @@ 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 @@ -98,8 +105,62 @@ 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 diff --git a/internal/collections/map.go b/internal/collections/map.go index c285cfe1e..73b382563 100644 --- a/internal/collections/map.go +++ b/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) } diff --git a/internal/collections/map_test.go b/internal/collections/map_test.go index 42b904ef1..9af03bc47 100644 --- a/internal/collections/map_test.go +++ b/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...) + }) + } +} diff --git a/internal/collections/named.go b/internal/collections/named.go index 6d6c922f4..da36c0dc0 100644 --- a/internal/collections/named.go +++ b/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 } diff --git a/internal/collections/named_test.go b/internal/collections/named_test.go index 8138d3407..baae256ef 100644 --- a/internal/collections/named_test.go +++ b/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") } diff --git a/internal/corazawaf/transaction.go b/internal/corazawaf/transaction.go index 500726adc..f8b98e267 100644 --- a/internal/corazawaf/transaction.go +++ b/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) diff --git a/testing/engine/variables.go b/testing/engine/variables.go index abe8f6a44..0a58c5ef0 100644 --- a/testing/engine/variables.go +++ b/testing/engine/variables.go @@ -177,3 +177,50 @@ 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{1, 2, 3, 4, 5}, + LogContains: `[msg "concat ARGS:a"]`, + }, + }, + }, + { + 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{ + LogContains: `[msg "child ARGS_GET:a"]`, + }, + }, + }, + }, + }, + }, + Rules: ` +SecRuleEngine On +SecRule ARGS_GET "@streq x" "id:1,phase:1,pass,log" +SecRule ARGS "@streq x" "id:2,phase:1,pass,log,msg:'concat %{MATCHED_VAR_NAME}'" +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" +`, +}) From e8817c6178d7afcff9ef3c26bf9690fb439b94e0 Mon Sep 17 00:00:00 2001 From: Sebastien Blot Date: Fri, 2 Oct 2026 17:00:33 +0200 Subject: [PATCH 2/2] fix tests --- testing/engine/variables.go | 17 +++-------------- 1 file changed, 3 insertions(+), 14 deletions(-) diff --git a/testing/engine/variables.go b/testing/engine/variables.go index 0a58c5ef0..04284832b 100644 --- a/testing/engine/variables.go +++ b/testing/engine/variables.go @@ -197,18 +197,8 @@ var _ = profile.RegisterProfile(profile.Profile{ URI: "/?a=x&b=y&c=y&d=y&e=y&f=y&g=y&h=y", }, Output: profile.ExpectedOutput{ - TriggeredRules: []int{1, 2, 3, 4, 5}, - LogContains: `[msg "concat ARGS:a"]`, - }, - }, - }, - { - 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{ - LogContains: `[msg "child ARGS_GET:a"]`, + TriggeredRules: []int{2, 3, 4, 5}, + LogContains: `[msg "child ARGS_GET:a"]`, }, }, }, @@ -217,8 +207,7 @@ var _ = profile.RegisterProfile(profile.Profile{ }, Rules: ` SecRuleEngine On -SecRule ARGS_GET "@streq x" "id:1,phase:1,pass,log" -SecRule ARGS "@streq x" "id:2,phase:1,pass,log,msg:'concat %{MATCHED_VAR_NAME}'" +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"