saml icon indicating copy to clipboard operation
saml copied to clipboard

Fix IDP deadlock due to recursive locking

Open agis opened this issue 2 years ago • 0 comments

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

agis avatar Feb 14 '24 11:02 agis