This is an automated email from the ASF dual-hosted git repository.
markt-asf pushed a commit to branch 11.0.x
in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/11.0.x by this push:
new db401286f8 Fix concurrency issues with session Store load/save
db401286f8 is described below
commit db401286f86fbb5eeb71f89b8621c2a2cf6500f8
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 26d740168e..670174d6a7 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;
@@ -411,24 +412,30 @@ public class DataSourceStore extends StoreBase {
ClassLoader oldThreadContextCL = context.bind(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("dataSourceStore.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("dataSourceStore.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(sm.getString("dataSourceStore.noObject", id));
}
- } else if (context.getLogger().isDebugEnabled()) {
- contextLog.debug(sm.getString("dataSourceStore.noObject", id));
+ return null;
}
- return null;
+ } finally {
+ readLock.unlock();
}
} finally {
context.unbind(oldThreadContextCL);
@@ -440,7 +447,13 @@ public class DataSourceStore extends StoreBase {
@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;
});
@@ -487,7 +500,13 @@ public class DataSourceStore extends StoreBase {
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();
@@ -514,6 +533,8 @@ public class DataSourceStore extends StoreBase {
}
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 4df5bb1faa..66dab8b072 100644
--- a/java/org/apache/catalina/session/FileStore.java
+++ b/java/org/apache/catalina/session/FileStore.java
@@ -38,7 +38,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;
/**
@@ -73,8 +72,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.
*/
@@ -211,7 +208,7 @@ public final class FileStore extends StoreBase {
ClassLoader oldThreadContextCL = context.bind(null);
try {
- Lock readLock = sessionLocksById.getLock(id).readLock();
+ Lock readLock = getSessionStoreLock(id).readLock();
readLock.lock();
try {
if (!file.exists()) {
@@ -249,7 +246,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()) {
@@ -264,20 +261,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 295fd2ef46..a36ff672d8 100644
--- a/java/org/apache/catalina/session/LocalStrings.properties
+++ b/java/org/apache/catalina/session/LocalStrings.properties
@@ -111,5 +111,6 @@ standardSessionAccessor.nullId=Unable to create Accessor instance as session ID
standardSessionAccessor.nullManager=Unable to create Accessor instance as session manager is 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 ed9181dc20..ee9ef331d3 100644
--- a/java/org/apache/catalina/session/PersistentManagerBase.java
+++ b/java/org/apache/catalina/session/PersistentManagerBase.java
@@ -18,10 +18,9 @@ package org.apache.catalina.session;
import java.io.IOException;
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;
@@ -100,11 +99,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
*/
@@ -113,7 +107,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.
@@ -541,6 +534,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
@@ -558,21 +579,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);
@@ -584,11 +596,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) {
@@ -599,11 +617,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 9b666912d5..5530f203b1 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 4422005e3f..1e2d839035 100644
--- a/java/org/apache/catalina/valves/PersistentValve.java
+++ b/java/org/apache/catalina/valves/PersistentValve.java
@@ -195,6 +195,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 2f6518c370..801a0fd688 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 jakarta.servlet.http.HttpServletRequest;
import jakarta.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 9d91caa26f..7defa4636b 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.