Merge pull request #154 from privacybydesign/fix-session-expiry-deadlock

Prevent possible mutex deadlock when using chained sessions

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()

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":"[email protected]"}]]]}}`)
+	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)
+}