This is an automated email from the ASF dual-hosted git repository.
markt-asf pushed a commit to branch 9.0.x
in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/9.0.x by this push:
new 2d575916b0 Fix concurrency issues with session Store load/save
2d575916b0 is described below
commit 2d575916b0a7a97bd4b9d0e3b93fcc264dc9c85f
Author: Mark Thomas <[email protected]>
AuthorDate: Fri Aug 21 18:33:36 2026 +0100
Fix concurrency issues with session Store load/save
---
java/org/apache/catalina/Store.java | 24 +++++++
.../apache/catalina/session/DataSourceStore.java | 55 +++++++++++-----
java/org/apache/catalina/session/FileStore.java | 17 ++---
.../catalina/session/LocalStrings.properties | 1 +
.../catalina/session/PersistentManagerBase.java | 77 +++++++++++++---------
java/org/apache/catalina/session/StoreBase.java | 13 ++++
.../apache/catalina/valves/PersistentValve.java | 5 ++
.../catalina/session/TestPersistentManager.java | 8 +++
webapps/docs/changelog.xml | 8 +++
9 files changed, 152 insertions(+), 56 deletions(-)
diff --git a/java/org/apache/catalina/Store.java b/java/org/apache/catalina/Store.java
index c2cb99c572..5e2e2a7b0b 100644
--- a/java/org/apache/catalina/Store.java
+++ b/java/org/apache/catalina/Store.java
@@ -19,6 +19,8 @@ package org.apache.catalina;
import java.beans.PropertyChangeListener;
import java.io.IOException;
+import java.util.concurrent.locks.ReadWriteLock;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
/**
@@ -136,4 +138,26 @@ public interface Store {
* @exception IOException if an input/output error occurs
*/
void save(Session session) throws IOException;
+
+
+ /**
+ * Obtain the session store lock for the session with the given identifier.
+ * <p>
+ * Sub-classes of StoreBase use this lock as necessary. External users of the Store must obtain a write lock before
+ * changing the session identifier. More generally, external users of the store must obtain a write lock before
+ * manipulating the session in any way that changes the mapping from session object to session identifier.
+ * <p>
+ * Implementations of this interface <b>MUST</b> provide an implementation of this method else any change in session
+ * identifier, e.g. on authentication, may result in inconsistent data being held in the store.
+ * <p>
+ * Prior to Tomcat 12, the default implementation always returns a new {@link ReadWriteLock} which will not provide
+ * any concurrency protection. From Tomcat 12, an {@link UnsupportedOperationException} is thrown.
+ *
+ * @param sessionId the session identifier
+ *
+ * @return The lock for the given session identifier
+ */
+ default ReadWriteLock getSessionStoreLock(String sessionId) {
+ return new ReentrantReadWriteLock();
+ }
}
diff --git a/java/org/apache/catalina/session/DataSourceStore.java b/java/org/apache/catalina/session/DataSourceStore.java
index 22130fa66d..168f863d81 100644
--- a/java/org/apache/catalina/session/DataSourceStore.java
+++ b/java/org/apache/catalina/session/DataSourceStore.java
@@ -30,6 +30,7 @@ import java.sql.ResultSet;
import java.sql.SQLException;
import java.util.ArrayList;
import java.util.List;
+import java.util.concurrent.locks.Lock;
import javax.naming.Context;
import javax.naming.InitialContext;
@@ -129,24 +130,30 @@ public class DataSourceStore extends JDBCStore {
ClassLoader oldThreadContextCL = context.bind(Globals.IS_SECURITY_ENABLED, null);
try (PreparedStatement preparedLoadSql = conn.prepareStatement(loadSql)) {
- preparedLoadSql.setString(1, id);
- preparedLoadSql.setString(2, getName());
- try (ResultSet rst = preparedLoadSql.executeQuery()) {
- if (rst.next()) {
- try (ObjectInputStream ois = getObjectInputStream(rst.getBinaryStream(2))) {
- if (contextLog.isTraceEnabled()) {
- contextLog.trace(sm.getString(getStoreName() + ".loading", id, sessionTable));
+ Lock readLock = getSessionStoreLock(id).readLock();
+ readLock.lock();
+ try {
+ preparedLoadSql.setString(1, id);
+ preparedLoadSql.setString(2, getName());
+ try (ResultSet rst = preparedLoadSql.executeQuery()) {
+ if (rst.next()) {
+ try (ObjectInputStream ois = getObjectInputStream(rst.getBinaryStream(2))) {
+ if (contextLog.isTraceEnabled()) {
+ contextLog.trace(sm.getString(getStoreName() + ".loading", id, sessionTable));
+ }
+
+ StandardSession _session = (StandardSession) manager.createEmptySession();
+ _session.readObjectData(ois);
+ _session.setManager(manager);
+ return _session;
}
-
- StandardSession _session = (StandardSession) manager.createEmptySession();
- _session.readObjectData(ois);
- _session.setManager(manager);
- return _session;
+ } else if (context.getLogger().isDebugEnabled()) {
+ contextLog.debug(getStoreName() + ": No persisted data object found");
}
- } else if (context.getLogger().isDebugEnabled()) {
- contextLog.debug(getStoreName() + ": No persisted data object found");
+ return null;
}
- return null;
+ } finally {
+ readLock.unlock();
}
} finally {
context.unbind(Globals.IS_SECURITY_ENABLED, oldThreadContextCL);
@@ -158,7 +165,13 @@ public class DataSourceStore extends JDBCStore {
@Override
public void remove(String id) throws IOException {
withRetry(conn -> {
- remove(id, conn);
+ Lock writeLock = getSessionStoreLock(id).writeLock();
+ writeLock.lock();
+ try {
+ remove(id, conn);
+ } finally {
+ writeLock.unlock();
+ }
return null;
});
@@ -205,7 +218,13 @@ public class DataSourceStore extends JDBCStore {
sessionDataCol + ", " + sessionValidCol + ", " + sessionMaxInactiveCol + ", " + sessionLastAccessedCol +
") VALUES (?, ?, ?, ?, ?, ?)";
- synchronized (session) {
+ String sessionId = session.getIdInternal();
+ Lock writeLock = getSessionStoreLock(sessionId).writeLock();
+ writeLock.lock();
+ try {
+ if (!sessionId.equals(session.getIdInternal())) {
+ throw new IOException(sm.getString("store.inconsistentSessionID", sessionId, session.getIdInternal()));
+ }
// First serialize session
ByteArrayOutputStream bos = new ByteArrayOutputStream();
@@ -232,6 +251,8 @@ public class DataSourceStore extends JDBCStore {
}
return null;
});
+ } finally {
+ writeLock.unlock();
}
if (manager.getContext().getLogger().isTraceEnabled()) {
diff --git a/java/org/apache/catalina/session/FileStore.java b/java/org/apache/catalina/session/FileStore.java
index a8589fdfcd..bdf168824a 100644
--- a/java/org/apache/catalina/session/FileStore.java
+++ b/java/org/apache/catalina/session/FileStore.java
@@ -39,7 +39,6 @@ import org.apache.catalina.Session;
import org.apache.juli.logging.Log;
import org.apache.juli.logging.LogFactory;
import org.apache.tomcat.util.ExceptionUtils;
-import org.apache.tomcat.util.concurrent.KeyedReentrantReadWriteLock;
import org.apache.tomcat.util.res.StringManager;
/**
@@ -74,8 +73,6 @@ public final class FileStore extends StoreBase {
*/
private File directoryFile = null;
- private KeyedReentrantReadWriteLock sessionLocksById = new KeyedReentrantReadWriteLock();
-
/**
* Name to register for this Store, used for logging.
*/
@@ -212,7 +209,7 @@ public final class FileStore extends StoreBase {
ClassLoader oldThreadContextCL = context.bind(Globals.IS_SECURITY_ENABLED, null);
try {
- Lock readLock = sessionLocksById.getLock(id).readLock();
+ Lock readLock = getSessionStoreLock(id).readLock();
readLock.lock();
try {
if (!file.exists()) {
@@ -250,7 +247,7 @@ public final class FileStore extends StoreBase {
.trace(sm.getString(getStoreName() + ".removing", id, file.getAbsolutePath()));
}
- Lock writeLock = sessionLocksById.getLock(id).writeLock();
+ Lock writeLock = getSessionStoreLock(id).writeLock();
writeLock.lock();
try {
if (file.exists() && !file.delete()) {
@@ -265,20 +262,24 @@ public final class FileStore extends StoreBase {
@Override
public void save(Session session) throws IOException {
// Open an output stream to the specified pathname, if any
- File file = file(session.getIdInternal());
+ String sessionId = session.getIdInternal();
+ File file = file(sessionId);
if (file == null) {
return;
}
if (manager.getContext().getLogger().isTraceEnabled()) {
manager.getContext().getLogger()
- .trace(sm.getString(getStoreName() + ".saving", session.getIdInternal(), file.getAbsolutePath()));
+ .trace(sm.getString(getStoreName() + ".saving", sessionId, file.getAbsolutePath()));
}
File tempFile = new File(file.getAbsolutePath() + ".tmp");
- Lock writeLock = sessionLocksById.getLock(session.getIdInternal()).writeLock();
+ Lock writeLock = getSessionStoreLock(sessionId).writeLock();
writeLock.lock();
try {
+ if (!sessionId.equals(session.getIdInternal())) {
+ throw new IOException(sm.getString("store.inconsistentSessionID", sessionId, session.getIdInternal()));
+ }
try (FileOutputStream fos = new FileOutputStream(tempFile);
ObjectOutputStream oos = new ObjectOutputStream(new BufferedOutputStream(fos))) {
((StandardSession) session).writeObjectData(oos);
diff --git a/java/org/apache/catalina/session/LocalStrings.properties b/java/org/apache/catalina/session/LocalStrings.properties
index 6669906484..f4562c6970 100644
--- a/java/org/apache/catalina/session/LocalStrings.properties
+++ b/java/org/apache/catalina/session/LocalStrings.properties
@@ -105,5 +105,6 @@ standardSession.setAttribute.ise=setAttribute: Session [{0}] has already been in
standardSession.setAttribute.namenull=setAttribute: name parameter cannot be null
store.expireFail=Error processing session expiration for key [{0}]
+store.inconsistentSessionID=The session ID has changed from [{0}] to [{1}] during the write process
store.keysFail=Error getting keys
store.removeFail=Error removing key [{0}]
diff --git a/java/org/apache/catalina/session/PersistentManagerBase.java b/java/org/apache/catalina/session/PersistentManagerBase.java
index 7b56a953dc..286f0e7b63 100644
--- a/java/org/apache/catalina/session/PersistentManagerBase.java
+++ b/java/org/apache/catalina/session/PersistentManagerBase.java
@@ -21,10 +21,9 @@ import java.security.AccessController;
import java.security.PrivilegedActionException;
import java.security.PrivilegedExceptionAction;
import java.util.Arrays;
-import java.util.HashMap;
import java.util.HashSet;
-import java.util.Map;
import java.util.Set;
+import java.util.concurrent.locks.Lock;
import org.apache.catalina.Lifecycle;
import org.apache.catalina.LifecycleException;
@@ -174,11 +173,6 @@ public abstract class PersistentManagerBase extends ManagerBase implements Store
protected int maxIdleSwap = -1;
- /**
- * Sessions currently being swapped in and the associated locks
- */
- private final Map<String,Object> sessionSwapInLocks = new HashMap<>();
-
/*
* Session that is currently getting swapped in to prevent loading it more than once concurrently
*/
@@ -187,7 +181,6 @@ public abstract class PersistentManagerBase extends ManagerBase implements Store
// ------------------------------------------------------------- Properties
-
/**
* Indicates how many seconds old a session can get, after its last use in a request, before it should be backed up
* to the store. {@code -1} means sessions are not backed up.
@@ -640,6 +633,34 @@ public abstract class PersistentManagerBase extends ManagerBase implements Store
// ------------------------------------------------------ Protected Methods
+ @Override
+ protected void changeSessionId(Session session, String newId, boolean notifySessionListeners,
+ boolean notifyContainerListeners) {
+
+ Store store = getStore();
+ if (store == null) {
+ super.changeSessionId(session, newId, notifySessionListeners, notifyContainerListeners);
+ return;
+ }
+
+ String oldId = session.getIdInternal();
+
+ Lock oldWriteLock = store.getSessionStoreLock(oldId).writeLock();
+ oldWriteLock.lock();
+ try {
+ Lock newWriteLock = store.getSessionStoreLock(newId).writeLock();
+ newWriteLock.lock();
+ try {
+ super.changeSessionId(session, newId, notifySessionListeners, notifyContainerListeners);
+ } finally {
+ newWriteLock.unlock();
+ }
+ } finally {
+ oldWriteLock.unlock();
+ }
+ }
+
+
/**
* Look for a session in the Store and, if found, restore it in the Manager's list of active sessions if
* appropriate. The session will be removed from the Store after swapping in, but will not be added to the active
@@ -657,21 +678,12 @@ public abstract class PersistentManagerBase extends ManagerBase implements Store
return null;
}
- Object swapInLock;
-
- /*
- * The purpose of this sync and these locks is to make sure that a session is only loaded once. It doesn't
- * matter if the lock is removed and then another thread enters this method and tries to load the same session.
- * That thread will re-create a swapIn lock for that session, quickly find that the session is already in
- * sessions, use it and carry on.
- */
- synchronized (this) {
- swapInLock = sessionSwapInLocks.computeIfAbsent(id, k -> new Object());
- }
-
Session session;
- synchronized (swapInLock) {
+ Lock writeLock = getStore().getSessionStoreLock(id).writeLock();
+ writeLock.lock();
+ try {
+
// First check to see if another thread has loaded the session into
// the manager
session = sessions.get(id);
@@ -683,11 +695,17 @@ public abstract class PersistentManagerBase extends ManagerBase implements Store
session = loadSessionFromStore(id);
sessionToSwapIn.set(session);
- if (session != null && !session.isValid()) {
- log.error(sm.getString("persistentManager.swapInInvalid", id));
- session.expire();
- removeSession(id);
- session = null;
+ if (session != null) {
+ if (!session.isValid()) {
+ log.error(sm.getString("persistentManager.swapInInvalid", id));
+ session.expire();
+ removeSession(id);
+ session = null;
+ } else if (!session.getIdInternal().equals(id)) {
+ log.error(sm.getString("persistentManager.swapInInvalid", id));
+ removeSession(id);
+ session = null;
+ }
}
if (session != null) {
@@ -698,11 +716,8 @@ public abstract class PersistentManagerBase extends ManagerBase implements Store
sessionToSwapIn.remove();
}
}
- }
-
- // Make sure the lock is removed
- synchronized (this) {
- sessionSwapInLocks.remove(id);
+ } finally {
+ writeLock.unlock();
}
return session;
diff --git a/java/org/apache/catalina/session/StoreBase.java b/java/org/apache/catalina/session/StoreBase.java
index 5cdd5d6faa..eaa4ab56b5 100644
--- a/java/org/apache/catalina/session/StoreBase.java
+++ b/java/org/apache/catalina/session/StoreBase.java
@@ -22,6 +22,7 @@ import java.io.BufferedInputStream;
import java.io.IOException;
import java.io.InputStream;
import java.io.ObjectInputStream;
+import java.util.concurrent.locks.ReadWriteLock;
import org.apache.catalina.LifecycleException;
import org.apache.catalina.LifecycleState;
@@ -31,6 +32,7 @@ import org.apache.catalina.Store;
import org.apache.catalina.util.CustomObjectInputStream;
import org.apache.catalina.util.LifecycleBase;
import org.apache.catalina.util.ToStringUtil;
+import org.apache.tomcat.util.concurrent.KeyedReentrantReadWriteLock;
import org.apache.tomcat.util.res.StringManager;
/**
@@ -67,6 +69,11 @@ public abstract class StoreBase extends LifecycleBase implements Store {
*/
protected Manager manager;
+ /*
+ * Locks used to control concurrent access to session for persistence
+ */
+ private KeyedReentrantReadWriteLock sessionLocksById = new KeyedReentrantReadWriteLock();
+
// ------------------------------------------------------------- Properties
@@ -95,6 +102,12 @@ public abstract class StoreBase extends LifecycleBase implements Store {
// --------------------------------------------------------- Public Methods
+ @Override
+ public ReadWriteLock getSessionStoreLock(String sessionId) {
+ return sessionLocksById.getLock(sessionId);
+ }
+
+
@Override
public void addPropertyChangeListener(PropertyChangeListener listener) {
support.addPropertyChangeListener(listener);
diff --git a/java/org/apache/catalina/valves/PersistentValve.java b/java/org/apache/catalina/valves/PersistentValve.java
index d50a73968d..3e855becfd 100644
--- a/java/org/apache/catalina/valves/PersistentValve.java
+++ b/java/org/apache/catalina/valves/PersistentValve.java
@@ -196,6 +196,11 @@ public class PersistentValve extends ValveBase {
}
session.expire();
store.remove(sessionId);
+ } else if (!session.getIdInternal().equals(sessionId)) {
+ if (containerLog.isTraceEnabled()) {
+ containerLog.trace("session swapped in has wrong session ID");
+ }
+ store.remove(sessionId);
} else {
session.setManager(manager);
// session.setId(sessionId); Only if new ???
diff --git a/test/org/apache/catalina/session/TestPersistentManager.java b/test/org/apache/catalina/session/TestPersistentManager.java
index 320234d09a..dc5cdec308 100644
--- a/test/org/apache/catalina/session/TestPersistentManager.java
+++ b/test/org/apache/catalina/session/TestPersistentManager.java
@@ -17,6 +17,8 @@
package org.apache.catalina.session;
import java.util.concurrent.atomic.AtomicInteger;
+import java.util.concurrent.locks.ReadWriteLock;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpSessionEvent;
@@ -116,6 +118,12 @@ public class TestPersistentManager {
return timedOutSession(manager, sessionExpireCounter);
}
}).anyTimes();
+ EasyMock.expect(mockStore.getSessionStoreLock(EasyMock.anyString())).andAnswer(new IAnswer<ReadWriteLock>() {
+ @Override
+ public ReadWriteLock answer() throws Throwable {
+ return new ReentrantReadWriteLock();
+ }
+ }).anyTimes();
EasyMock.replay(mockStore);
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index 6e64dcc2c3..be5e4076cd 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -140,6 +140,14 @@
value) in the <code>RemoteIpFilter</code> and
<code>RemoteIpValve</code>. (markt)
</add>
+ <fix>
+ Fix potential concurrency issues when loading/saving sessions from/to a
+ session store. Custom Store implementations that do not extend StoreBase
+ must implement the new <code>getSessionStoreLock()</code> method of the
+ <code>Store</code> interface to ensure concurrency protection. The
+ default method implementation provided only provides the pre-fix
+ functionality. (markt)
+ </fix>
</changelog>
</subsection>
<subsection name="Coyote">
lmpx.com only provides a reader for public news (NNTP) servers. It is not
affiliated with the servers or forums shown here and is not responsible for
the content of articles, which is written by their respective authors.