From 8f1bf532f6dda679c9be8b22a673125017868a19 Mon Sep 17 00:00:00 2001
From: Violet Hynes <violet.hynes@hashicorp.com>
Date: Thu, 17 Aug 2023 15:13:28 -0400
Subject: [PATCH] Fix client clone with headers deadlock (#22410)

* Fix client clone with headers deadlock

* Changelog

* Typo
---
 api/client.go       | 32 +++++++++++++++++---------
 api/client_test.go  | 55 +++++++++++++++++++++++++++++++++++++++++++++
 changelog/22410.txt |  3 +++
 3 files changed, 80 insertions(+), 10 deletions(-)
 create mode 100644 changelog/22410.txt

diff --git a/api/client.go b/api/client.go
index 4be03826bb..0fa00c617e 100644
--- a/api/client.go
+++ b/api/client.go
@@ -998,7 +998,9 @@ func (c *Client) Namespace() string {
 func (c *Client) WithNamespace(namespace string) *Client {
 	c2 := *c
 	c2.modifyLock = sync.RWMutex{}
-	c2.headers = c.Headers()
+	c.modifyLock.RLock()
+	c2.headers = c.headersInternal()
+	c.modifyLock.RUnlock()
 	if namespace == "" {
 		c2.ClearNamespace()
 	} else {
@@ -1035,7 +1037,12 @@ func (c *Client) ClearToken() {
 func (c *Client) Headers() http.Header {
 	c.modifyLock.RLock()
 	defer c.modifyLock.RUnlock()
+	return c.headersInternal()
+}
 
+// headersInternal gets the current set of headers used for requests. Must be called
+// with the read modifyLock held.
+func (c *Client) headersInternal() http.Header {
 	if c.headers == nil {
 		return nil
 	}
@@ -1183,24 +1190,28 @@ func (c *Client) CloneTLSConfig() bool {
 // the api.Config struct, such as policy override and wrapping function
 // behavior, must currently then be set as desired on the new client.
 func (c *Client) Clone() (*Client, error) {
+	c.modifyLock.RLock()
+	defer c.modifyLock.RUnlock()
+	c.config.modifyLock.RLock()
+	defer c.config.modifyLock.RUnlock()
 	return c.clone(c.config.CloneHeaders)
 }
 
 // CloneWithHeaders creates a new client similar to Clone, with the difference
-// being that the  headers are always cloned
+// being that the headers are always cloned
 func (c *Client) CloneWithHeaders() (*Client, error) {
+	c.modifyLock.RLock()
+	defer c.modifyLock.RUnlock()
+	c.config.modifyLock.RLock()
+	defer c.config.modifyLock.RUnlock()
 	return c.clone(true)
 }
 
 // clone creates a new client, with the headers being cloned based on the
-// passed in cloneheaders boolean
+// passed in cloneheaders boolean.
+// Must be called with the read lock and config read lock held.
 func (c *Client) clone(cloneHeaders bool) (*Client, error) {
-	c.modifyLock.RLock()
-	defer c.modifyLock.RUnlock()
-
 	config := c.config
-	config.modifyLock.RLock()
-	defer config.modifyLock.RUnlock()
 
 	newConfig := &Config{
 		Address:        config.Address,
@@ -1230,7 +1241,7 @@ func (c *Client) clone(cloneHeaders bool) (*Client, error) {
 	}
 
 	if cloneHeaders {
-		client.SetHeaders(c.Headers().Clone())
+		client.SetHeaders(c.headersInternal().Clone())
 	}
 
 	if config.CloneToken {
@@ -1261,6 +1272,7 @@ func (c *Client) NewRequest(method, requestPath string) *Request {
 	mfaCreds := c.mfaCreds
 	wrappingLookupFunc := c.wrappingLookupFunc
 	policyOverride := c.policyOverride
+	headers := c.headersInternal()
 	c.modifyLock.RUnlock()
 
 	host := addr.Host
@@ -1305,7 +1317,7 @@ func (c *Client) NewRequest(method, requestPath string) *Request {
 		req.WrapTTL = DefaultWrappingLookupFunc(method, lookupPath)
 	}
 
-	req.Headers = c.Headers()
+	req.Headers = headers
 	req.PolicyOverride = policyOverride
 
 	return req
diff --git a/api/client_test.go b/api/client_test.go
index 0de2c17c2b..4226572757 100644
--- a/api/client_test.go
+++ b/api/client_test.go
@@ -738,6 +738,61 @@ func TestClone(t *testing.T) {
 	}
 }
 
+// TestCloneWithHeadersNoDeadlock confirms that the cloning of the client doesn't cause
+// a deadlock.
+// Raised in https://github.com/hashicorp/vault/issues/22393 -- there was a
+// potential deadlock caused by running the problematicFunc() function in
+// multiple goroutines.
+func TestCloneWithHeadersNoDeadlock(t *testing.T) {
+	client, err := NewClient(nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	wg := &sync.WaitGroup{}
+
+	problematicFunc := func() {
+		wg.Add(1)
+		client.SetCloneToken(true)
+		_, err := client.CloneWithHeaders()
+		if err != nil {
+			t.Fatal(err)
+		}
+		wg.Done()
+	}
+
+	for i := 0; i < 1000; i++ {
+		go problematicFunc()
+	}
+	wg.Wait()
+}
+
+// TestCloneNoDeadlock is like TestCloneWithHeadersNoDeadlock but with
+// Clone instead of CloneWithHeaders
+func TestCloneNoDeadlock(t *testing.T) {
+	client, err := NewClient(nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	wg := &sync.WaitGroup{}
+
+	problematicFunc := func() {
+		wg.Add(1)
+		client.SetCloneToken(true)
+		_, err := client.Clone()
+		if err != nil {
+			t.Fatal(err)
+		}
+		wg.Done()
+	}
+
+	for i := 0; i < 1000; i++ {
+		go problematicFunc()
+	}
+	wg.Wait()
+}
+
 func TestSetHeadersRaceSafe(t *testing.T) {
 	client, err1 := NewClient(nil)
 	if err1 != nil {
diff --git a/changelog/22410.txt b/changelog/22410.txt
new file mode 100644
index 0000000000..25fcc3335f
--- /dev/null
+++ b/changelog/22410.txt
@@ -0,0 +1,3 @@
+```release-note:bug
+api/client: Fix deadlock in client.CloneWithHeaders when used alongside other client methods.
+```
\ No newline at end of file