From 0c824f11e98df8e3b2d5e1f2657d58424d6168dc Mon Sep 17 00:00:00 2001
From: Robert Zaremba <robert@zaremba.ch>
Date: Thu, 25 Mar 2021 14:51:34 +0100
Subject: [PATCH] fix race condition on address.String (#8997)

---
 types/address.go           | 29 ++++++++++----------
 types/address_race_test.go | 55 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 69 insertions(+), 15 deletions(-)
 create mode 100644 types/address_race_test.go

diff --git a/types/address.go b/types/address.go
index 1a20091033..a730e5120c 100644
--- a/types/address.go
+++ b/types/address.go
@@ -78,11 +78,11 @@ const (
 var (
 	// AccAddress.String() is expensive and if unoptimized dominantly showed up in profiles,
 	// yet has no mechanisms to trivially cache the result given that AccAddress is a []byte type.
-	accAddrMu     sync.RWMutex
+	accAddrMu     sync.Mutex
 	accAddrCache  *simplelru.LRU
-	consAddrMu    sync.RWMutex
+	consAddrMu    sync.Mutex
 	consAddrCache *simplelru.LRU
-	valAddrMu     sync.RWMutex
+	valAddrMu     sync.Mutex
 	valAddrCache  *simplelru.LRU
 )
 
@@ -268,13 +268,13 @@ func (aa AccAddress) String() string {
 	}
 
 	var key = conv.UnsafeBytesToStr(aa)
-	accAddrMu.RLock()
+	accAddrMu.Lock()
+	defer accAddrMu.Unlock()
 	addr, ok := accAddrCache.Get(key)
-	accAddrMu.RUnlock()
 	if ok {
 		return addr.(string)
 	}
-	return cacheBech32Addr(GetConfig().GetBech32AccountAddrPrefix(), aa, accAddrCache, key, &accAddrMu)
+	return cacheBech32Addr(GetConfig().GetBech32AccountAddrPrefix(), aa, accAddrCache, key)
 }
 
 // Format implements the fmt.Formatter interface.
@@ -418,13 +418,13 @@ func (va ValAddress) String() string {
 	}
 
 	var key = conv.UnsafeBytesToStr(va)
-	valAddrMu.RLock()
+	valAddrMu.Lock()
+	defer valAddrMu.Unlock()
 	addr, ok := valAddrCache.Get(key)
-	valAddrMu.RUnlock()
 	if ok {
 		return addr.(string)
 	}
-	return cacheBech32Addr(GetConfig().GetBech32ValidatorAddrPrefix(), va, valAddrCache, key, &valAddrMu)
+	return cacheBech32Addr(GetConfig().GetBech32ValidatorAddrPrefix(), va, valAddrCache, key)
 }
 
 // Format implements the fmt.Formatter interface.
@@ -573,13 +573,13 @@ func (ca ConsAddress) String() string {
 	}
 
 	var key = conv.UnsafeBytesToStr(ca)
-	consAddrMu.RLock()
+	consAddrMu.Lock()
+	defer consAddrMu.Unlock()
 	addr, ok := consAddrCache.Get(key)
-	consAddrMu.RUnlock()
 	if ok {
 		return addr.(string)
 	}
-	return cacheBech32Addr(GetConfig().GetBech32ConsensusAddrPrefix(), ca, consAddrCache, key, &consAddrMu)
+	return cacheBech32Addr(GetConfig().GetBech32ConsensusAddrPrefix(), ca, consAddrCache, key)
 }
 
 // Bech32ifyAddressBytes returns a bech32 representation of address bytes.
@@ -725,13 +725,12 @@ func addressBytesFromHexString(address string) ([]byte, error) {
 	return hex.DecodeString(address)
 }
 
-func cacheBech32Addr(prefix string, addr []byte, cache *simplelru.LRU, cacheKey string, m sync.Locker) string {
+// cacheBech32Addr is not concurrency safe. Concurrent access to cache causes race condition.
+func cacheBech32Addr(prefix string, addr []byte, cache *simplelru.LRU, cacheKey string) string {
 	bech32Addr, err := bech32.ConvertAndEncode(prefix, addr)
 	if err != nil {
 		panic(err)
 	}
-	m.Lock()
 	cache.Add(cacheKey, bech32Addr)
-	m.Unlock()
 	return bech32Addr
 }
diff --git a/types/address_race_test.go b/types/address_race_test.go
new file mode 100644
index 0000000000..eb87a9a9bf
--- /dev/null
+++ b/types/address_race_test.go
@@ -0,0 +1,55 @@
+package types_test
+
+import (
+	"encoding/binary"
+	"fmt"
+	"testing"
+	"time"
+
+	"github.com/cosmos/cosmos-sdk/types"
+	"github.com/stretchr/testify/require"
+)
+
+// generates AccAddress with `prefix` and calls String method
+func addressStringCaller(require *require.Assertions, prefix byte, max uint32, cancel chan bool, done chan<- bool) {
+	bz := make([]byte, 5) // prefix + 4 bytes for uint
+	bz[0] = prefix
+	for i := uint32(0); ; i++ {
+		if i >= max {
+			i = 0
+		}
+		select {
+		case <-cancel:
+			done <- true
+			return
+		default:
+			binary.BigEndian.PutUint32(bz[1:], i)
+			str := types.AccAddress(bz).String()
+			require.True(str != "")
+		}
+
+	}
+}
+
+func (s *addressTestSuite) TestAddressRace() {
+	fmt.Println("starting test")
+	if testing.Short() {
+		s.T().Skip("AddressRace test is not short")
+	}
+	workers := 4
+	done := make(chan bool, workers)
+	cancel := make(chan bool)
+	for i := byte(1); i <= 2; i++ { // workes which will loop in first 100 addresses
+		go addressStringCaller(s.Require(), i, 100, cancel, done)
+	}
+	for i := byte(1); i <= 2; i++ { // workes which will generate 1e6 new addresses
+		go addressStringCaller(s.Require(), i, 1000000, cancel, done)
+	}
+	<-time.After(time.Millisecond * 30)
+	close(cancel)
+
+	// cleanup
+	for i := 0; i < 4; i++ {
+		<-done
+	}
+}