From 189a1e38a2a54fab8e0b207dbe7c750b3f1669df Mon Sep 17 00:00:00 2001 From: Nathan Wallace Date: Sun, 16 Nov 2025 11:51:41 -0500 Subject: [PATCH] Fix race condition on customSearchPath slice closes #4792 (#4837) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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 * 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 --------- Co-authored-by: Claude --- pkg/db/db_client/db_client.go | 3 ++ pkg/db/db_client/db_client_search_path.go | 17 +++++-- pkg/db/db_client/db_client_session_test.go | 53 ++++++++++++++++++++++ 3 files changed, 69 insertions(+), 4 deletions(-) diff --git a/pkg/db/db_client/db_client.go b/pkg/db/db_client/db_client.go index caf2e6221..1ca11af14 100644 --- a/pkg/db/db_client/db_client.go +++ b/pkg/db/db_client/db_client.go @@ -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, } diff --git a/pkg/db/db_client/db_client_search_path.go b/pkg/db/db_client/db_client_search_path.go index 95100360d..ceef2e92e 100644 --- a/pkg/db/db_client/db_client_search_path.go +++ b/pkg/db/db_client/db_client_search_path.go @@ -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), ","))) diff --git a/pkg/db/db_client/db_client_session_test.go b/pkg/db/db_client/db_client_session_test.go index 985781ebd..b9572eebb 100644 --- a/pkg/db/db_client/db_client_session_test.go +++ b/pkg/db/db_client/db_client_session_test.go @@ -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() +}