Merge cockroachdb#108032

108032: sql: Make tochar.FormatCache thread safe r=otan,rafiss a=ecwall

Fixes cockroachdb#95424

`FormatCache.lookup()` previously wrapped calls to `cache.UnorderedCache.Get()` in `RWMutex.RLock()`/`RWMutex.RUnlock()` which allowed for data races because `UnorderedCache.Get()` can modify the state of its LRU cache.

This PR fixes the race condition by changing the `RWMutex` to a `Mutex` to handle cases where the LRU cache is modified.

Release note (bug fix): Fix nil pointer dereference caused by race condition when using to_char builtin.

Co-authored-by: Evan Wall <[email protected]>

pkg/util/tochar/cache.go

@@ -20,8 +20,9 @@ const maxCacheKeySize = 100
 // FormatCache is a cache used to store parsing info used for to_char.
 // It is thread safe, and is safe to use by `nil` caches.
 type FormatCache struct {
+	// mu must be a Mutex, not a RWMutex because Get can modify the LRU cache.
 	mu struct {
-		syncutil.RWMutex
+		syncutil.Mutex
 		cache *cache.UnorderedCache
 	}
 }
@@ -41,8 +42,8 @@ func NewFormatCache(size int) *FormatCache {
 func (pc *FormatCache) lookup(fmtString string) []formatNode {
 	if pc != nil && len(fmtString) <= maxCacheKeySize {
 		if ret, ok := func() ([]formatNode, bool) {
-			pc.mu.RLock()
-			defer pc.mu.RUnlock()
+			pc.mu.Lock()
+			defer pc.mu.Unlock()
 			ret, ok := pc.mu.cache.Get(fmtString)
 			if ok {
 				return ret.([]formatNode), true

pkg/util/tochar/cache_test.go

@@ -40,3 +40,17 @@ func TestFormatCache(t *testing.T) {
 	_, ok = c.mu.cache.Get("HH24")
 	require.True(t, ok)
 }
+
+// TestFormatCacheLookupThreadSafe is non-deterministic. Flakes indicate that
+// FormatCache.lookup is not thread safe.
+// See https://github.com/cockroachdb/cockroach/issues/95424
+func TestFormatCacheLookupThreadSafe(t *testing.T) {
+	formats := []string{"HH12", "HH24", "MI"}
+	c := NewFormatCache(len(formats) - 1)
+	for i := 0; i < 100_000; i++ {
+		go func(i int) {
+			format := formats[i%len(formats)]
+			c.lookup(format)
+		}(i)
+	}
+}