Fix race condition on customSearchPath slice closes #4792 (#4837)

* Add test for #4792: customSearchPath data race

This test demonstrates the data race on the customSearchPath slice when
accessed concurrently from multiple goroutines without synchronization.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>

* Fix #4792: Add mutex protection for customSearchPath slice

The customSearchPath slice was being accessed concurrently from multiple
goroutines without synchronization, causing data races. This fix adds a
dedicated mutex (searchPathMutex) to protect all reads and writes to the
customSearchPath and searchPathPrefix fields.

Changes:
- Added searchPathMutex field to DbClient struct
- Initialize searchPathMutex in NewDbClient constructor
- Protected all customSearchPath writes in SetRequiredSessionSearchPath
- Protected all customSearchPath reads in GetRequiredSessionSearchPath
- Protected all customSearchPath reads in GetCustomSearchPath
- Fixed logging in ensureSessionSearchPath to use already-fetched value
- Updated test to initialize searchPathMutex for proper testing

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>

---------

Co-authored-by: Claude <noreply@anthropic.com>
This commit is contained in:
Nathan Wallace
2025-11-16 11:51:41 -05:00
committed by GitHub
parent 3cfbb59dc5
commit 189a1e38a2
3 changed files with 69 additions and 4 deletions

View File

@@ -54,6 +54,8 @@ type DbClient struct {
// if a custom search path or a prefix is used, store it here
customSearchPath []string
searchPathPrefix []string
// allows locked access to customSearchPath and searchPathPrefix
searchPathMutex *sync.Mutex
// the default user search path
userSearchPath []string
// disable timing - set whilst in process of querying the timing
@@ -71,6 +73,7 @@ func NewDbClient(ctx context.Context, connectionString string, opts ...ClientOpt
parallelSessionInitLock: semaphore.NewWeighted(constants.MaxParallelClientInits),
sessions: make(map[uint32]*db_common.DatabaseSession),
sessionsMutex: &sync.Mutex{},
searchPathMutex: &sync.Mutex{},
connectionString: connectionString,
}

View File

@@ -29,9 +29,6 @@ func (c *DbClient) SetRequiredSessionSearchPath(ctx context.Context) error {
// default required path to user search path
requiredSearchPath := c.userSearchPath
// store custom search path and search path prefix
c.searchPathPrefix = searchPathPrefix
// if a search path was passed, use that
if len(configuredSearchPath) > 0 {
requiredSearchPath = configuredSearchPath
@@ -43,6 +40,12 @@ func (c *DbClient) SetRequiredSessionSearchPath(ctx context.Context) error {
requiredSearchPath = db_common.EnsureInternalSchemaSuffix(requiredSearchPath)
// if either configuredSearchPath or searchPathPrefix are set, store requiredSearchPath as customSearchPath
c.searchPathMutex.Lock()
defer c.searchPathMutex.Unlock()
// store custom search path and search path prefix
c.searchPathPrefix = searchPathPrefix
if len(configuredSearchPath)+len(searchPathPrefix) > 0 {
c.customSearchPath = requiredSearchPath
} else {
@@ -75,6 +78,9 @@ func (c *DbClient) loadUserSearchPath(ctx context.Context, connection *pgx.Conn)
// GetRequiredSessionSearchPath implements Client
func (c *DbClient) GetRequiredSessionSearchPath() []string {
c.searchPathMutex.Lock()
defer c.searchPathMutex.Unlock()
if c.customSearchPath != nil {
return c.customSearchPath
}
@@ -83,6 +89,9 @@ func (c *DbClient) GetRequiredSessionSearchPath() []string {
}
func (c *DbClient) GetCustomSearchPath() []string {
c.searchPathMutex.Lock()
defer c.searchPathMutex.Unlock()
return c.customSearchPath
}
@@ -107,7 +116,7 @@ func (c *DbClient) ensureSessionSearchPath(ctx context.Context, session *db_comm
}
// so we need to set the search path
log.Printf("[TRACE] session search path will be updated to %s", strings.Join(c.customSearchPath, ","))
log.Printf("[TRACE] session search path will be updated to %s", strings.Join(requiredSearchPath, ","))
err := db_common.ExecuteSystemClientCall(ctx, session.Connection.Conn(), func(ctx context.Context, tx pgx.Tx) error {
_, err := tx.Exec(ctx, fmt.Sprintf("set search_path to %s", strings.Join(db_common.PgEscapeSearchPath(requiredSearchPath), ",")))

View File

@@ -1,6 +1,7 @@
package db_client
import (
"context"
"sync"
"testing"
@@ -166,3 +167,55 @@ func TestDbClient_SessionConnectionNilSafety(t *testing.T) {
// Session is created with nil connection initially
assert.Nil(t, session.Connection, "New session should have nil connection initially")
}
// TestDbClient_SessionSearchPathUpdatesThreadSafe verifies that concurrent access
// to customSearchPath does not cause data races.
// Reference: https://github.com/turbot/steampipe/issues/4792
//
// This test simulates concurrent goroutines accessing and modifying the customSearchPath
// slice. Without proper synchronization, this causes a data race.
//
// Run with: go test -race -run TestDbClient_SessionSearchPathUpdatesThreadSafe
func TestDbClient_SessionSearchPathUpdatesThreadSafe(t *testing.T) {
// Create a DbClient with the fields we need for testing
client := &DbClient{
customSearchPath: []string{"public", "internal"},
userSearchPath: []string{"public"},
searchPathMutex: &sync.Mutex{},
}
// Number of concurrent operations to test
const numGoroutines = 100
var wg sync.WaitGroup
wg.Add(numGoroutines * 3)
// Simulate concurrent readers calling GetRequiredSessionSearchPath
for i := 0; i < numGoroutines; i++ {
go func() {
defer wg.Done()
_ = client.GetRequiredSessionSearchPath()
}()
}
// Simulate concurrent readers calling GetCustomSearchPath
for i := 0; i < numGoroutines; i++ {
go func() {
defer wg.Done()
_ = client.GetCustomSearchPath()
}()
}
// Simulate concurrent writers calling SetRequiredSessionSearchPath
// This is the most dangerous operation as it modifies the slice
for i := 0; i < numGoroutines; i++ {
go func() {
defer wg.Done()
ctx := context.Background()
// This will write to customSearchPath
_ = client.SetRequiredSessionSearchPath(ctx)
}()
}
wg.Wait()
}