From 5d5c1b955f38f7788110f5d2d86e547262b53f1a Mon Sep 17 00:00:00 2001 From: urso Date: Mon, 25 May 2020 22:27:53 +0200 Subject: [PATCH 1/3] Improve thread safety of fingerprint processor The fingerprint processor used to reuse state between calls, but not being threadsafe means that the processor might get into an invalid state if it is used by multiple go-routines by accident, creating invalid, non-reproducible hash values. This change does not reuse the hash state between runs. Running the benchmarks seems to suggest that we don't create any extra allocations with this change (tested with go1.13): $ go test -bench . -benchmem goos: darwin goarch: amd64 pkg: github.com/elastic/beats/v7/libbeat/processors/fingerprint BenchmarkHashMethods/sha384-4 1000000000 0.123 ns/op 0 B/op 0 allocs/op BenchmarkHashMethods/sha512-4 1000000000 0.125 ns/op 0 B/op 0 allocs/op BenchmarkHashMethods/xxhash-4 1000000000 0.0539 ns/op 0 B/op 0 allocs/op BenchmarkHashMethods/md5-4 1000000000 0.0925 ns/op 0 B/op 0 allocs/op BenchmarkHashMethods/sha1-4 1000000000 0.112 ns/op 0 B/op 0 allocs/op BenchmarkHashMethods/sha256-4 1000000000 0.134 ns/op 0 B/op 0 allocs/op --- libbeat/processors/fingerprint/fingerprint.go | 17 ++++--- .../fingerprint/fingerprint_test.go | 50 ++++++++++++++++++- 2 files changed, 59 insertions(+), 8 deletions(-) diff --git a/libbeat/processors/fingerprint/fingerprint.go b/libbeat/processors/fingerprint/fingerprint.go index 90d051cec77a..8de73c65f365 100644 --- a/libbeat/processors/fingerprint/fingerprint.go +++ b/libbeat/processors/fingerprint/fingerprint.go @@ -19,8 +19,8 @@ package fingerprint import ( "fmt" - "hash" "io" + "sync" "time" "github.com/elastic/beats/v7/libbeat/beat" @@ -39,7 +39,8 @@ const processorName = "fingerprint" type fingerprint struct { config Config fields []string - hash hash.Hash + hash hashMethod + mu sync.Mutex } // New constructs a new fingerprint processor. @@ -49,12 +50,15 @@ func New(cfg *common.Config) (processors.Processor, error) { return nil, makeErrConfigUnpack(err) } - fields := common.MakeStringSet(config.Fields...) + // The fields array must be sorted, to guarantee that we always + // get the same hash for a similar set of configured keys. + // The call `ToSlice` always returns a sorted slice. + fields := common.MakeStringSet(config.Fields...).ToSlice() p := &fingerprint{ config: config, - hash: config.Method(), - fields: fields.ToSlice(), + hash: config.Method, + fields: fields, } return p, nil @@ -62,8 +66,7 @@ func New(cfg *common.Config) (processors.Processor, error) { // Run enriches the given event with fingerprint information func (p *fingerprint) Run(event *beat.Event) (*beat.Event, error) { - hashFn := p.hash - hashFn.Reset() + hashFn := p.hash() err := p.writeFields(hashFn, event.Fields) if err != nil { diff --git a/libbeat/processors/fingerprint/fingerprint_test.go b/libbeat/processors/fingerprint/fingerprint_test.go index 6274af7c0b29..957ab589876c 100644 --- a/libbeat/processors/fingerprint/fingerprint_test.go +++ b/libbeat/processors/fingerprint/fingerprint_test.go @@ -24,11 +24,56 @@ import ( "time" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "github.com/elastic/beats/v7/libbeat/beat" "github.com/elastic/beats/v7/libbeat/common" ) +func TestWithConfig(t *testing.T) { + cases := map[string]struct { + config common.MapStr + input common.MapStr + want string + }{ + "hello world": { + config: common.MapStr{ + "fields": []string{"message"}, + }, + input: common.MapStr{ + "message": "hello world", + }, + want: "50110bbfc1757f21caacc966b33f5ea2235c4176739447e0b3285dec4e1dd2a4", + }, + "with string escaping": { + config: common.MapStr{ + "fields": []string{"message"}, + }, + input: common.MapStr{ + "message": `test message "hello world"`, + }, + want: "14a0364b79acbe4c78dd5e77db2c93ae8c750518b32581927d50b3eef407184e", + }, + } + + for name, test := range cases { + t.Run(name, func(t *testing.T) { + config := common.MustNewConfigFrom(test.config) + p, err := New(config) + require.NoError(t, err) + + testEvent := &beat.Event{ + Timestamp: time.Now(), + Fields: test.input.Clone(), + } + newEvent, err := p.Run(testEvent) + v, err := newEvent.GetValue("fingerprint") + assert.NoError(t, err) + assert.Equal(t, test.want, v) + }) + } +} + func TestHashMethods(t *testing.T) { testFields := common.MapStr{ "field1": "foo", @@ -393,7 +438,10 @@ func BenchmarkHashMethods(b *testing.B) { b.Run(method, func(b *testing.B) { b.ResetTimer() for _, e := range events { - p.Run(&e) + _, err := p.Run(&e) + if err != nil { + b.Fatal(err) + } } }) } From b500732a79013c4da6de14c857d39266ee15e539 Mon Sep 17 00:00:00 2001 From: urso Date: Mon, 25 May 2020 22:36:36 +0200 Subject: [PATCH 2/3] oops --- libbeat/processors/fingerprint/fingerprint.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/libbeat/processors/fingerprint/fingerprint.go b/libbeat/processors/fingerprint/fingerprint.go index 8de73c65f365..3028f7a20fe7 100644 --- a/libbeat/processors/fingerprint/fingerprint.go +++ b/libbeat/processors/fingerprint/fingerprint.go @@ -20,7 +20,6 @@ package fingerprint import ( "fmt" "io" - "sync" "time" "github.com/elastic/beats/v7/libbeat/beat" @@ -40,7 +39,6 @@ type fingerprint struct { config Config fields []string hash hashMethod - mu sync.Mutex } // New constructs a new fingerprint processor. From aba010afd423476232f618d66846a8e50c1d9ce3 Mon Sep 17 00:00:00 2001 From: urso Date: Mon, 25 May 2020 22:37:27 +0200 Subject: [PATCH 3/3] changelog --- CHANGELOG.next.asciidoc | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.next.asciidoc b/CHANGELOG.next.asciidoc index fda61aacbbe3..7d5bb7bacd48 100644 --- a/CHANGELOG.next.asciidoc +++ b/CHANGELOG.next.asciidoc @@ -113,6 +113,7 @@ https://github.com/elastic/beats/compare/v7.0.0-alpha2...master[Check the HEAD d - Fix `keystore add` hanging under Windows. {issue}18649[18649] {pull}18654[18654] - Fix an issue where error messages are not accurate in mapstriface. {issue}18662[18662] {pull}18663[18663] - Fix regression in `add_kubernetes_metadata`, so configured `indexers` and `matchers` are used if defaults are not disabled. {issue}18481[18481] {pull}18818[18818] +- Fix potential race condition in fingerprint processor. {pull}18738[18738] *Auditbeat*