From 563374034b8f5b51f805a992937d087559d0f4a7 Mon Sep 17 00:00:00 2001
From: Sietse Ringers <mail@sietseringers.net>
Date: Mon, 31 May 2021 16:29:13 +0200
Subject: [PATCH 1/2] fix: prevent possible mutex deadlock when using chained
 sessions

When periodically iterating over sessions to see if they need to be expired,
the `memorySessionStore.deleteExpired()` method of the IRMA server locks the
session store mutex, and while that is locked it additionally locks each
session mutex when handling it. If a session is currently being handled by an
endpoint which also locks the session mutex, `deleteExpired()` therefore has to
wait for the handler to finish dealing with the session. However, when that
session attempts to starts a new session (as part of a session chain), it tries
to lock the session store mutex again, which was already locked: a deadlock.

We solve this by making `deleteExired()` not request a session lock while the
entire store is also locked: the store and session locking are now done
sequentially. That way, even when a session is locked by an endpoint in the
loop over the sessions in `deleteExpired()`, that session is free to write new
sessions to the session store.
---
 server/irmaserver/sessions.go | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/server/irmaserver/sessions.go b/server/irmaserver/sessions.go
index 740377ff6..9c7138273 100644
--- a/server/irmaserver/sessions.go
+++ b/server/irmaserver/sessions.go
@@ -113,10 +113,15 @@ func (s *memorySessionStore) deleteExpired() {
 	// First check which sessions have expired
 	// We don't need a write lock for this yet, so postpone that for actual deleting
 	s.RLock()
-	expired := make([]string, 0, len(s.requestor))
+	toCheck := make(map[string]*session, len(s.requestor))
 	for token, session := range s.requestor {
-		session.Lock()
+		toCheck[token] = session
+	}
+	s.RUnlock()
 
+	expired := make([]string, 0, len(toCheck))
+	for token, session := range toCheck {
+		session.Lock()
 		timeout := maxSessionLifetime
 		if session.status == server.StatusInitialized && session.rrequest.Base().ClientTimeout != 0 {
 			timeout = time.Duration(session.rrequest.Base().ClientTimeout) * time.Second
@@ -134,7 +139,6 @@ func (s *memorySessionStore) deleteExpired() {
 		}
 		session.Unlock()
 	}
-	s.RUnlock()
 
 	// Using a write lock, delete the expired sessions
 	s.Lock()

From af8024a09e02919a15fd0cfaff6d7fa38c2d3fcf Mon Sep 17 00:00:00 2001
From: Ivar Derksen <ivar.derksen@sidn.nl>
Date: Thu, 3 Jun 2021 14:08:23 +0200
Subject: [PATCH 2/2] Add unit test for deadlock bug in memory session store

---
 server/irmaserver/sessions_test.go | 51 ++++++++++++++++++++++++++++++
 1 file changed, 51 insertions(+)
 create mode 100644 server/irmaserver/sessions_test.go

diff --git a/server/irmaserver/sessions_test.go b/server/irmaserver/sessions_test.go
new file mode 100644
index 000000000..f2b1775a7
--- /dev/null
+++ b/server/irmaserver/sessions_test.go
@@ -0,0 +1,51 @@
+package irmaserver
+
+import (
+	irma "github.com/privacybydesign/irmago"
+	"github.com/privacybydesign/irmago/server"
+	"github.com/sirupsen/logrus"
+	"github.com/stretchr/testify/require"
+	"testing"
+	"time"
+)
+
+func TestMemoryStoreNoDeadlock(t *testing.T) {
+	logger := logrus.New()
+	logger.Level = logrus.FatalLevel
+	s, err := New(&server.Configuration{Logger: logger})
+	require.NoError(t, err)
+	defer s.Stop()
+
+	req, err := server.ParseSessionRequest(`{"request":{"@context":"https://irma.app/ld/request/disclosure/v2","context":"AQ==","nonce":"MtILupG0g0J23GNR1YtupQ==","devMode":true,"disclose":[[[{"type":"test.test.email.email","value":"example@example.com"}]]]}}`)
+	require.NoError(t, err)
+	session := s.newSession(irma.ActionDisclosing, req)
+
+	session.Lock()
+	deletingCompleted := false
+	addingCompleted := false
+	// Make sure the deleting continues on completion such that the test itself will not hang.
+	defer func() {
+		session.Unlock()
+		time.Sleep(100 * time.Millisecond)
+		require.True(t, deletingCompleted)
+	}()
+
+	go func() {
+		s.sessions.deleteExpired()
+		deletingCompleted = true
+	}()
+
+	// Make sure the goroutine above is running
+	time.Sleep(100 * time.Millisecond)
+
+	// Make a new session; this involves adding it to the memory session store.
+	go func() {
+		_ = s.newSession(irma.ActionDisclosing, req)
+		addingCompleted = true
+	}()
+
+	// Check whether the IRMA server doesn't hang
+	time.Sleep(100 * time.Millisecond)
+	require.True(t, addingCompleted)
+	require.False(t, deletingCompleted)
+}