From 91716e8856a3a1723129577d6dfb5352d06fc837 Mon Sep 17 00:00:00 2001 From: Agis Anastasopoulos Date: Wed, 14 Feb 2024 13:22:48 +0200 Subject: [PATCH] Fix IDP deadlock due to recursive locking While using the IDP server for integration tests, I've experienced a deadlock after issuing many concurrent requests to the following endpoints: - PUT /services/:id - GET /sso Apparently, the issue is that the IDP code attempts to acquires locks on the Server.idpConfigMu recursively: 1. The /sso HTTP handler calls Server.idpConfigMu.RLock() 2. Before releasing the above lock, this same handler eventually calls s.GetServiceProvider, which in turn attempts to acquire the same lock by calling Server.idpConfigMu.RLock() 3. Concurrent requests to `PUT /services/:id` acquire a write lock by calling Server.idpConfigMu.Lock() Step (2) is a recursive lock, which won't work, as stated by the documentation of sync.RWLock[1]: If a goroutine holds a RWMutex for reading and another goroutine might call Lock, no goroutine should expect to be able to acquire a read lock until the initial read lock is released. In particular, this prohibits recursive read locking. This is to ensure that the lock eventually becomes available; a blocked Lock call excludes new readers from acquiring the lock. This patch fixes the issue by removing the unnecessary lock acquired in step (1). This was redundant since GetServiceProvider() already takes care of serializing access to the s.serviceProviders map, which is the resource in question. [1] https://golang.org/pkg/sync/#RWMutex --- samlidp/samlidp.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/samlidp/samlidp.go b/samlidp/samlidp.go index 13ca10b9..95cd85fe 100644 --- a/samlidp/samlidp.go +++ b/samlidp/samlidp.go @@ -93,8 +93,6 @@ func (s *Server) InitializeHTTP() { s.IDP.ServeMetadata(w, r) }) mux.Handle("/sso", func(w http.ResponseWriter, r *http.Request) { - s.idpConfigMu.RLock() - defer s.idpConfigMu.RUnlock() s.IDP.ServeSSO(w, r) })