Merge pull request #1174 from baranowb/UNDERTOW-1869_v2

[UNDERTOW-1869] fix concurrency on session creation
undertow-io · Sep 28, 2021 · 9d12c9e · 9d12c9e
2 parents c6569f4 + 30c0dbb
commit 9d12c9e
Showing 1 changed file with 35 additions and 19 deletions.
diff --git a/core/src/main/java/io/undertow/server/session/InMemorySessionManager.java b/core/src/main/java/io/undertow/server/session/InMemorySessionManager.java
@@ -52,6 +52,9 @@
  */
 public class InMemorySessionManager implements SessionManager, SessionManagerStatistics {
 
+    // place holder for sessions during id creation, it prevents collision of session ids in race condition scenarios where two sessions are being created with the same id
+    private static final SessionImpl PLACE_HOLDER_SESSION = new SessionImpl(null, null, null, null, null, null, 0);
+
     private final AttachmentKey<SessionImpl> NEW_SESSION = AttachmentKey.create(SessionImpl.class);
 
     private final SessionIdGenerator sessionIdGenerator;
@@ -145,6 +148,9 @@ public Session createSession(final HttpServerExchange serverExchange, final Sess
                 while (sessions.size() >= maxSize && !evictionQueue.isEmpty()) {
 
                     String key = evictionQueue.poll();
+                    if(key == null) {
+                        break;
+                    }
                     UndertowLogger.REQUEST_LOGGER.debugf("Removing session %s as max size has been hit", key);
                     SessionImpl toRemove = sessions.get(key);
                     if (toRemove != null) {
@@ -161,24 +167,14 @@ public Session createSession(final HttpServerExchange serverExchange, final Sess
         if (config == null) {
             throw UndertowMessages.MESSAGES.couldNotFindSessionCookieConfig();
         }
+
         String sessionID = config.findSessionId(serverExchange);
-        if (sessionID == null) {
-            int count = 0;
-            while (sessionID == null) {
-                sessionID = sessionIdGenerator.createSessionId();
-                if (sessions.containsKey(sessionID)) {
-                    sessionID = null;
-                }
-                if (count++ == 100) {
-                    //this should never happen
-                    //but we guard against pathalogical session id generators to prevent an infinite loop
-                    throw UndertowMessages.MESSAGES.couldNotGenerateUniqueSessionId();
-                }
-            }
-        } else {
-            if (sessions.containsKey(sessionID)) {
+        if (sessionID != null) {
+            if (!saveSessionID(sessionID))
                 throw UndertowMessages.MESSAGES.sessionWithIdAlreadyExists(sessionID);
-            }
+            // else: succeeded to use requested session id
+        } else {
+            sessionID = createAndSaveNewID();
         }
         Object evictionToken;
         if (evictionQueue != null) {
@@ -189,7 +185,7 @@ public Session createSession(final HttpServerExchange serverExchange, final Sess
         final SessionImpl session = new SessionImpl(this, sessionID, config, serverExchange.getIoThread(), serverExchange.getConnection().getWorker(), evictionToken, defaultSessionTimeout);
 
         UndertowLogger.SESSION_LOGGER.debugf("Created session with id %s for exchange %s", sessionID, serverExchange);
-        sessions.put(sessionID, session);
+        saveSession(sessionID, session);
         config.setSessionId(serverExchange, session.getId());
         session.bumpTimeout();
         sessionListeners.sessionCreated(session, serverExchange);
@@ -210,6 +206,26 @@ public Session createSession(final HttpServerExchange serverExchange, final Sess
         return session;
     }
 
+    private boolean saveSessionID(String sessionID) {
+        return this.sessions.putIfAbsent(sessionID, PLACE_HOLDER_SESSION) == null;
+    }
+
+    private String createAndSaveNewID() {
+        for (int i = 0; i < 100; i++) {
+            final String sessionID = sessionIdGenerator.createSessionId();
+            if (saveSessionID(sessionID))
+                return sessionID;
+        }
+        //this should 'never' happen
+        //but we guard against pathalogical session id generators to prevent an infinite loop
+        throw UndertowMessages.MESSAGES.couldNotGenerateUniqueSessionId();
+    }
+
+    private void saveSession(String savedID, SessionImpl session) {
+        final SessionImpl placeHolder = sessions.put(savedID, session);
+        assert(placeHolder == PLACE_HOLDER_SESSION);
+    }
+
     @Override
     public Session getSession(final HttpServerExchange serverExchange, final SessionConfig config) {
         if (serverExchange != null) {
@@ -620,10 +636,10 @@ public SessionManager getSessionManager() {
         @Override
         public String changeSessionId(final HttpServerExchange exchange, final SessionConfig config) {
             final String oldId = sessionId;
-            String newId = sessionManager.sessionIdGenerator.createSessionId();
+            String newId = sessionManager.createAndSaveNewID();
             this.sessionId = newId;
             if(!invalid) {
-                sessionManager.sessions.put(newId, this);
+                sessionManager.saveSession(newId, this);
                 config.setSessionId(exchange, this.getId());
             }
             sessionManager.sessions.remove(oldId);