This is an automated email from the ASF dual-hosted git repository.
markt-asf pushed a commit to branch 10.1.x
in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/10.1.x by this push:
new 35d9237fd9 Fix concurrency issues with session Store load/save
35d9237fd9 is described below
commit 35d9237fd97d6917be6dc00fca82a26dccbb40bf
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 2bcad9e9b4..5bca390cab 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;
@@ -412,24 +413,30 @@ public class DataSourceStore extends StoreBase {
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("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(Globals.IS_SECURITY_ENABLED,
oldThreadContextCL);
@@ -441,7 +448,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;
});
@@ -488,7 +501,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();
@@ -515,6 +534,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 9fdb05803d..1e06927fa3 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 a5277251db..3afb6abc29 100644
--- a/java/org/apache/catalina/session/LocalStrings.properties
+++ b/java/org/apache/catalina/session/LocalStrings.properties
@@ -104,5 +104,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 a792077228..30c4172c09 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 eeabcbc9ad..e2fe4dae82 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 c3668ec235..7a98704fa3 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -128,6 +128,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="Cluster">
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
