Fix race conditions in censor's querywriter (#468)

* control concurrent access to cached queries with mutex to avoid race conditions * use currently maintained and new version of boltdb to fix race condition issues in tests related to go1.14 pointer checks and mentioned on github etcd-io/bbolt#187 * * use race detector for golang unit tests * remove extra test run with GODEBUG=tls13=1 due to env variable was removed and unsupported since go1.14 https://go.dev/doc/go1.14
cossacklabs · Dec 1, 2021 · 7333406 · 7333406
1 parent c943d92
commit 7333406
Show file tree

Hide file tree

Showing 11 changed files with 41 additions and 25 deletions.
diff --git a/.circleci/check_gotest.sh b/.circleci/check_gotest.sh
@@ -7,6 +7,7 @@
 
 OLD_PATH="$PATH"
 TEST_BUILD_TAGS=${TEST_BUILD_TAGS:-}
+TEST_EXTRA_BUILD_FLAGS=${TEST_EXTRA_BUILD_FLAGS:-}
 
 if [ -z "$GO_VERSIONS" ]; then
     # extract default Go version from $GOROOT
@@ -27,17 +28,10 @@ for go_version in $GO_VERSIONS; do
     echo "GOROOT=$GOROOT"
     echo "PATH=$PATH"
 
-    go test -v -tags="${TEST_BUILD_TAGS}" ./...;
+    go test -tags="${TEST_BUILD_TAGS}" ${TEST_EXTRA_BUILD_FLAGS} ./...;
     status="$?"
     if [[ "${status}" != "0" ]]; then
-        echo "$version-tls12" >> "$FILEPATH_ERROR_FLAG";
-    fi
-
-    # test with supported tls1.3
-    GODEBUG="tls13=1" go test -v -tags="${TEST_BUILD_TAGS}" github.com/cossacklabs/acra/...;
-    status="$?"
-    if [[ "${status}" != "0" ]]; then
-        echo "$version-tls13" >> "$FILEPATH_ERROR_FLAG";
+        echo "$version" >> "$FILEPATH_ERROR_FLAG";
     fi
 done
 

diff --git a/.circleci/config.yml b/.circleci/config.yml
@@ -31,10 +31,20 @@ jobs:
       - run: .circleci/check_golint.sh
       - run: .circleci/check_misspell.sh
       - run: .circleci/check_ineffassign.sh
+        # run go test with race detector only with latest version because it is slower and we expect that latest version
+        # has the newest and all existing algorithms for race detection
       - run:
           command: .circleci/check_gotest.sh
           environment:
             - TEST_BUILD_TAGS: "integration,boltdb,redis,vault"
+            - TEST_EXTRA_BUILD_FLAGS: "-race"
+            - GO_VERSIONS: 1.17.3
+        # and run all other versions without race detector
+      - run:
+          command: .circleci/check_gotest.sh
+          environment:
+            - GO_VERSIONS: 1.15.2 1.16.9
+            - TEST_BUILD_TAGS: "integration,boltdb,redis,vault"
       # check python wrapper
       - run: PYTHONPATH=`pwd`/wrappers/python python3 wrappers/python/acrawriter/tests.py
 

diff --git a/CHANGELOG_DEV.md b/CHANGELOG_DEV.md
@@ -1,3 +1,11 @@
+## 0.91.0 - 2021-12-01
+### Changed
+- wrap `acra-censor`'s query writers' manipulations of cached queries with a mutex to avoid race conditions
+- tests run with `-race` flag to detect race conditions
+- changed BoltDB dependency from old `github.com/boltdb/boltdb` to `go.etcd.io/bbolt` that doesn't have race condition
+  issues related to updated memory checks in go1.14
+
+
 ## 0.91.0 - 2021-11-25
 ### Changed
 - `acra-censor's` query writer now can track amount of skipped queries and allows configuration of serialization 

diff --git a/acra-censor/common/logging_logic.go b/acra-censor/common/logging_logic.go
@@ -45,7 +45,7 @@ type QueryWriter struct {
 	Queries              []*QueryInfo
 	logStorage           LogStorage
 	queryIndex           int
-	mutex                *sync.RWMutex
+	mutex                sync.RWMutex
 	signalBackgroundExit chan bool
 	signalWriteQuery     chan string
 	signalShutdown       chan os.Signal
@@ -61,7 +61,6 @@ func NewFileQueryWriter(filePath string) (*QueryWriter, error) {
 	// create writer
 	writer := &QueryWriter{
 		queryIndex:           0,
-		mutex:                &sync.RWMutex{},
 		serializationTimeout: DefaultSerializationTimeout,
 		serializationTicker:  time.NewTicker(DefaultSerializationTimeout),
 		logger:               log.WithField("internal_object", "querywriter"),
@@ -90,8 +89,8 @@ func NewFileQueryWriter(filePath string) (*QueryWriter, error) {
 
 // WalkQueries walks through each query and perform some action on it
 func (queryWriter *QueryWriter) WalkQueries(visitor func(query *QueryInfo) error) error {
-	queryWriter.mutex.Lock()
-	defer queryWriter.mutex.Unlock()
+	queryWriter.mutex.RLock()
+	defer queryWriter.mutex.RUnlock()
 	for _, query := range queryWriter.Queries {
 		if err := visitor(query); err != nil {
 			return err
@@ -102,9 +101,9 @@ func (queryWriter *QueryWriter) WalkQueries(visitor func(query *QueryInfo) error
 
 // DumpQueries writes all queries into file
 func (queryWriter *QueryWriter) DumpQueries() error {
-	queryWriter.mutex.Lock()
+	queryWriter.mutex.RLock()
 	rawData := queryWriter.serializeQueries(queryWriter.Queries)
-	queryWriter.mutex.Unlock()
+	queryWriter.mutex.RUnlock()
 	if err := queryWriter.logStorage.WriteAll(rawData); err != nil {
 		queryWriter.logger.WithError(err).WithField(logging.FieldKeyEventCode, logging.EventCodeErrorCensorIOError).Errorln("Can't dump queries to storage")
 		return err
@@ -181,14 +180,16 @@ func (queryWriter *QueryWriter) readStoredQueries() error {
 		queryWriter.logger.WithError(err).WithField(logging.FieldKeyEventCode, logging.EventCodeErrorCensorIOError).Errorln("Can't read stored queries")
 		return err
 	}
+	queryWriter.mutex.Lock()
 	queryWriter.Queries = q
 	queryWriter.queryIndex = len(q)
+	queryWriter.mutex.Unlock()
 	return nil
 }
 
 func (queryWriter *QueryWriter) dumpBufferedQueries() error {
-	queryWriter.mutex.Lock()
-	defer queryWriter.mutex.Unlock()
+	queryWriter.mutex.RLock()
+	defer queryWriter.mutex.RUnlock()
 
 	if len(queryWriter.Queries) != 0 {
 		partialRawData := queryWriter.serializeQueries(queryWriter.Queries[queryWriter.queryIndex:])
@@ -240,6 +241,8 @@ func (queryWriter *QueryWriter) serializeQueries(queries []*QueryInfo) []byte {
 }
 
 func (queryWriter *QueryWriter) captureQuery(query string) {
+	queryWriter.mutex.Lock()
+	defer queryWriter.mutex.Unlock()
 	//skip already captured queries
 	for _, queryInfo := range queryWriter.Queries {
 		if strings.EqualFold(queryInfo.RawQuery, query) {

diff --git a/cmd/acra-server/acra-server.go b/cmd/acra-server/acra-server.go
@@ -70,8 +70,8 @@ import (
 	pgDialect "github.com/cossacklabs/acra/sqlparser/dialect/postgresql"
 	"github.com/cossacklabs/acra/utils"
 
-	"github.com/boltdb/bolt"
 	log "github.com/sirupsen/logrus"
+	bolt "go.etcd.io/bbolt"
 )
 
 var restartSignalsChannel chan os.Signal

diff --git a/cmd/acra-tokens/tokens/token-storage.go b/cmd/acra-tokens/tokens/token-storage.go
@@ -21,11 +21,11 @@ import (
 	"flag"
 	"os"
 
-	"github.com/boltdb/bolt"
 	"github.com/cossacklabs/acra/cmd"
 	tokenCommon "github.com/cossacklabs/acra/pseudonymization/common"
 	tokenStorage "github.com/cossacklabs/acra/pseudonymization/storage"
 	log "github.com/sirupsen/logrus"
+	bolt "go.etcd.io/bbolt"
 )
 
 // CommonTokenStorageParameters is a mix-in of command line parameters for token storage construction.

diff --git a/cmd/acra-translator/acra-translator.go b/cmd/acra-translator/acra-translator.go
@@ -26,7 +26,6 @@ import (
 	"errors"
 	"flag"
 	"fmt"
-	"github.com/boltdb/bolt"
 	"github.com/cossacklabs/acra/cmd"
 	"github.com/cossacklabs/acra/cmd/acra-translator/common"
 	_ "github.com/cossacklabs/acra/cmd/acra-translator/docs"
@@ -47,6 +46,7 @@ import (
 	common2 "github.com/cossacklabs/acra/pseudonymization/common"
 	"github.com/cossacklabs/acra/pseudonymization/storage"
 	"github.com/cossacklabs/acra/utils"
+	bolt "go.etcd.io/bbolt"
 	_ "net/http/pprof"
 	"os"
 	"os/signal"

diff --git a/go.mod b/go.mod
@@ -5,7 +5,6 @@ go 1.14
 require (
 	contrib.go.opencensus.io/exporter/jaeger v0.2.1
 	github.com/alecthomas/template v0.0.0-20190718012654-fb15b899a751
-	github.com/boltdb/bolt v1.3.1
 	github.com/cossacklabs/themis/gothemis v0.13.1
 	github.com/gin-gonic/gin v1.7.2
 	github.com/go-redis/redis/v7 v7.0.1
@@ -21,6 +20,7 @@ require (
 	github.com/swaggo/files v0.0.0-20190704085106-630677cd5c14
 	github.com/swaggo/gin-swagger v1.3.0
 	github.com/swaggo/swag v1.5.1
+	go.etcd.io/bbolt v1.3.6
 	go.opencensus.io v0.22.4
 	golang.org/x/crypto v0.0.0-20210711020723-a769d52b0f97
 	golang.org/x/net v0.0.0-20211123203042-d83791d6bcd9

diff --git a/go.sum b/go.sum
@@ -61,8 +61,6 @@ github.com/beorn7/perks v1.0.0/go.mod h1:KWe93zE9D1o94FZ5RNwFwVgaQK1VOXiVxmqh+Ce
 github.com/beorn7/perks v1.0.1 h1:VlbKKnNfV8bJzeqoa4cOKqO6bYr3WgKZxO8Z16+hsOM=
 github.com/beorn7/perks v1.0.1/go.mod h1:G2ZrVWU2WbWT9wwq4/hrbKbnv/1ERSJQ0ibhJ6rlkpw=
 github.com/bgentry/speakeasy v0.1.0/go.mod h1:+zsyZBPWlz7T6j88CTgSN5bM796AkVf0kBD4zp0CCIs=
-github.com/boltdb/bolt v1.3.1 h1:JQmyP4ZBrce+ZQu0dY660FMfatumYDLun9hBCUVIkF4=
-github.com/boltdb/bolt v1.3.1/go.mod h1:clJnj/oiGkjum5o1McbSZDSLxVThjynRyGBgiAx27Ps=
 github.com/casbin/casbin/v2 v2.1.2/go.mod h1:YcPU1XXisHhLzuxH9coDNf2FbKpjGlbCg3n9yuLkIJQ=
 github.com/cenkalti/backoff v2.2.1+incompatible h1:tNowT99t7UNflLxfYYSlKYsBpXdEet03Pg2g16Swow4=
 github.com/cenkalti/backoff v2.2.1+incompatible/go.mod h1:90ReRw6GdpyfrHakVjL/QHaoyV4aDUVVkXQJJJ3NXXM=
@@ -521,6 +519,8 @@ github.com/urfave/cli v1.22.1/go.mod h1:Gos4lmkARVdJ6EkW0WaNv/tZAAMe9V7XWyB60NtX
 github.com/xiang90/probing v0.0.0-20190116061207-43a291ad63a2/go.mod h1:UETIi67q53MR2AWcXfiuqkDkRtnGDLqkBTpCHuJHxtU=
 github.com/yuin/goldmark v1.1.25/go.mod h1:3hX8gzYuyVAZsxl0MRgGTJEmQBFcNTphYh9decYSb74=
 go.etcd.io/bbolt v1.3.3/go.mod h1:IbVyRI1SCnLcuJnV2u8VeU0CEYM7e686BmAb1XKL+uU=
+go.etcd.io/bbolt v1.3.6 h1:/ecaJf0sk1l4l6V4awd65v2C3ILy7MSj+s/x1ADCIMU=
+go.etcd.io/bbolt v1.3.6/go.mod h1:qXsaaIqmgQH0T+OPdb99Bf+PKfBBQVAdyD6TY9G8XM4=
 go.etcd.io/etcd v0.0.0-20191023171146-3cf2f69b5738/go.mod h1:dnLIgRNXwCJa5e+c6mIZCrds/GIG4ncV9HhK5PX7jPg=
 go.opencensus.io v0.20.1/go.mod h1:6WKK9ahsWS3RSO+PY9ZHZUfv2irvY6gN279GOPZjmmk=
 go.opencensus.io v0.20.2/go.mod h1:6WKK9ahsWS3RSO+PY9ZHZUfv2irvY6gN279GOPZjmmk=
@@ -667,6 +667,7 @@ golang.org/x/sys v0.0.0-20200323222414-85ca7c5b95cd/go.mod h1:h1NjWce9XRLGQEsW7w
 golang.org/x/sys v0.0.0-20200331124033-c3d80250170d/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
 golang.org/x/sys v0.0.0-20200615200032-f1bc736245b1/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
 golang.org/x/sys v0.0.0-20200625212154-ddb9806d33ae/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
+golang.org/x/sys v0.0.0-20200923182605-d9f96fdee20d/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
 golang.org/x/sys v0.0.0-20201119102817-f84b799fce68/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
 golang.org/x/sys v0.0.0-20210330210617-4fbd30eecc44/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
 golang.org/x/sys v0.0.0-20210423082822-04245dca01da/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=

diff --git a/pseudonymization/storage/boltdb.go b/pseudonymization/storage/boltdb.go
@@ -19,8 +19,8 @@ package storage
 import (
 	"time"
 
-	"github.com/boltdb/bolt"
 	"github.com/cossacklabs/acra/pseudonymization/common"
+	bolt "go.etcd.io/bbolt"
 )
 
 type boltdbStorage struct {

diff --git a/pseudonymization/storage/boltdb_test.go b/pseudonymization/storage/boltdb_test.go
@@ -24,7 +24,7 @@ import (
 	"os"
 	"testing"
 
-	"github.com/boltdb/bolt"
+	bolt "go.etcd.io/bbolt"
 )
 
 func TestBoltdbStorage(t *testing.T) {