On Tue, Oct 13, 2026 at 6:56 PM Mark Thomas <[email protected]> wrote: > > I think this needs to be reverted. > > I think the problem this commit is trying to solve can only occur if two > separate sessions end up with the same ID. If that happens we have much > bigger issues.
Yes, I pushed back using this argument (session id collisions are impossible), but the scenario is actually not really about that [the testcase is though]. > Also, the PersistentManager has to remove by ID. It can't remove by > identity since as a session restored from the store will be a different > instance every time it is restored. ### [MEDIUM] TOCTOU race: background store expiration can evict a live session that is swapped in concurrently, losing the session - File: /home/opencode/code-review/tomcat/java/org/apache/catalina/session/StoreBase.java:156-189 (unlocked `isLoaded` check + `expire()`), interacting with /home/opencode/code-review/tomcat/java/org/apache/catalina/session/PersistentManagerBase.java:576-643 (`swapIn`/`reactivateLoadedSession`) and /home/opencode/code-review/tomcat/java/org/apache/catalina/session/ManagerBase.java:774-792 (`remove` deletes by session ID, not by object identity). - Description: Sequence: (1) background thread in `StoreBase.processExpires()` loads a swapped-out session copy C from the Store, determines it is expired, and checks `isLoaded(key)` (StoreBase.java:170-181) - a plain, lock-free `sessions.get(key)` - which returns `false`; (2) concurrently, the owner's next request calls `PersistentManagerBase.findSession()` -> `swapIn()`, which takes the per-ID store write lock, loads the session L from the still-present store entry and adds it to the `sessions` map (PersistentManagerBase.java:628-643); (3) the background thread then calls `session.expire()` on copy C; `expire()` first runs the application's `sessionDestroyed` listeners (StandardSession.java:610-640 - an arbitrarily long window), then `manager.remove(C, true)` executes `sessions.remove(C.getIdInternal())` (ManagerBase.java:789-791) which removes whatever object is currently mapped to that ID - i.e. the live session L - and `PersistentManagerBase.remove()` (PersistentManagerBase.java:449-456) additionally deletes the store entry. Result: L is orphaned (still referenced by the in-flight request, now invisible to the Manager) and the store backup is gone, so the next request for that ID cannot recover the session - the user is logged out with state lost, and the app received a `sessionDestroyed` for a session that was still active. - Verification: Confirmed `sessions.remove(id)` is key-based (ManagerBase.java:789-791), `PersistentManagerBase.remove` removes the store entry by ID (PersistentManagerBase.java:449-456), and `isLoaded` (PersistentManagerBase.java:220-229) performs no locking; `swapIn` holds the per-ID store write lock only around its own load/add, and `processExpires()` takes no store lock around the isLoaded decision, so no mutual exclusion exists between the two paths. The window spans the whole `sessionDestroyed` listener invocation, so it is not negligible. - Proposed fix: Make the decision and the removal atomic per session ID, e.g. in `StoreBase.processExpires()` acquire `manager.getStore().getSessionStoreLock(key).writeLock()` around the `isLoaded` check/`expire()`/`remove(key)` block, and in `ManagerBase.remove(Session, boolean)` only remove when the mapped object is the same instance (`sessions.remove(id, session)`), so a concurrently swapped-in session can never be evicted by a stale copy. But since it is so unlikely, I am ok with reverting and seeing what the next review says. So I'll do that. It would need to come up with a better testcase. Rémy > Mark > > > On 07/10/2026 17:48, [email protected] wrote: > > This is an automated email from the ASF dual-hosted git repository. > > > > rmaucher pushed a commit to branch main > > in repository https://gitbox.apache.org/repos/asf/tomcat.git > > > > commit e44f30dc6eb435254cea39acfc1f0333481634e1 > > Author: opencode <[email protected]> > > AuthorDate: Wed Oct 7 17:30:51 2026 +0200 > > > > Remove sessions from the manager map by instance identity > > > > ManagerBase.remove(Session, boolean) removed the map entry keyed by the > > session ID regardless of which instance the map currently held. A stale > > copy of a session, such as one deserialized from a Store, could there- > > fore evict a different, live instance that had the same ID, losing it > > without any of its own state being expired. > > > > Use the two argument ConcurrentHashMap removal so the entry is only > > removed when the map still holds the session instance passed in. > > --- > > java/org/apache/catalina/session/ManagerBase.java | 7 +- > > .../session/TestPersistentManagerBase.java | 105 > > +++++++++++++++++++++ > > 2 files changed, 111 insertions(+), 1 deletion(-) > > > > diff --git a/java/org/apache/catalina/session/ManagerBase.java > > b/java/org/apache/catalina/session/ManagerBase.java > > index 16bb91b071..6ba65d4873 100644 > > --- a/java/org/apache/catalina/session/ManagerBase.java > > +++ b/java/org/apache/catalina/session/ManagerBase.java > > @@ -787,7 +787,12 @@ public abstract class ManagerBase extends > > LifecycleMBeanBase implements Manager > > } > > > > if (session.getIdInternal() != null) { > > - sessions.remove(session.getIdInternal()); > > + // Only remove the entry if the map still holds this exact > > session > > + // instance. This prevents a stale copy of a session (for > > example, > > + // an expired copy loaded from a store) from removing a live > > + // session with the same ID that was loaded into the map in the > > + // meantime. > > + sessions.remove(session.getIdInternal(), session); > > } > > } > > > > diff --git > > a/test/org/apache/catalina/session/TestPersistentManagerBase.java > > b/test/org/apache/catalina/session/TestPersistentManagerBase.java > > new file mode 100644 > > index 0000000000..1d266f8d1e > > --- /dev/null > > +++ b/test/org/apache/catalina/session/TestPersistentManagerBase.java > > @@ -0,0 +1,105 @@ > > +/* > > + * Licensed to the Apache Software Foundation (ASF) under one or more > > + * contributor license agreements. See the NOTICE file distributed with > > + * this work for additional information regarding copyright ownership. > > + * The ASF licenses this file to You under the Apache License, Version 2.0 > > + * (the "License"); you may not use this file except in compliance with > > + * the License. You may obtain a copy of the License at > > + * > > + * http://www.apache.org/licenses/LICENSE-2.0 > > + * > > + * Unless required by applicable law or agreed to in writing, software > > + * distributed under the License is distributed on an "AS IS" BASIS, > > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. > > + * See the License for the specific language governing permissions and > > + * limitations under the License. > > + */ > > +package org.apache.catalina.session; > > + > > +import java.io.File; > > + > > +import org.junit.Assert; > > +import org.junit.Test; > > + > > +import org.apache.catalina.Session; > > +import org.apache.catalina.startup.ExpandWar; > > +import org.apache.tomcat.unittest.TesterContext; > > +import org.apache.tomcat.unittest.TesterHost; > > +import org.apache.tomcat.unittest.TesterServletContext; > > + > > +public class TestPersistentManagerBase { > > + > > + private static final String SESSION_ID = "testerSessionId"; > > + > > + /* > > + * Removing a stale copy of a session must not evict a different > > session > > + * instance with the same ID from the manager. > > + */ > > + @Test > > + public void testRemoveStaleCopyDoesNotEvictLiveSession() throws > > Exception { > > + File dir = new File("SESS_TEMP_STALE_REMOVE"); > > + ExpandWar.delete(dir); > > + try { > > + PersistentManager manager = new PersistentManager(); > > + > > + FileStore store = new FileStore(); > > + store.setDirectory(dir.getAbsolutePath()); > > + manager.setStore(store); > > + > > + TesterContext context = new TesterContext(); > > + context.setServletContext(new TesterServletContext()); > > + context.setParent(new TesterHost()); > > + manager.setContext(context); > > + > > + manager.start(); > > + > > + Session live = manager.createSession(SESSION_ID); > > + > > + // A different instance carrying the same session ID, not > > + // registered with the manager, as produced by loading a copy > > from > > + // the store before it is swapped in > > + StandardSession stale = new StandardSession(manager); > > + stale.id = SESSION_ID; > > + > > + manager.remove(stale, false); > > + > > + // Before the fix, the removal was based on the session ID > > alone > > + // and removed the live session from the manager > > + Session current = manager.findSession(SESSION_ID); > > + Assert.assertSame(live, current); > > + } finally { > > + ExpandWar.delete(dir); > > + } > > + } > > + > > + /* > > + * Control for the test above: removing the instance that is in the map > > + * does remove the session. > > + */ > > + @Test > > + public void testRemoveLiveInstanceRemovesSession() throws Exception { > > + File dir = new File("SESS_TEMP_LIVE_REMOVE"); > > + ExpandWar.delete(dir); > > + try { > > + PersistentManager manager = new PersistentManager(); > > + > > + FileStore store = new FileStore(); > > + store.setDirectory(dir.getAbsolutePath()); > > + manager.setStore(store); > > + > > + TesterContext context = new TesterContext(); > > + context.setServletContext(new TesterServletContext()); > > + context.setParent(new TesterHost()); > > + manager.setContext(context); > > + > > + manager.start(); > > + > > + Session session = manager.createSession(SESSION_ID); > > + manager.remove(session, false); > > + > > + Assert.assertNull(manager.findSession(SESSION_ID)); > > + } finally { > > + ExpandWar.delete(dir); > > + } > > + } > > +} > > > > > > --------------------------------------------------------------------- > > To unsubscribe, e-mail: [email protected] > > For additional commands, e-mail: [email protected] > > > --------------------------------------------------------------------- > To unsubscribe, e-mail: [email protected] > For additional commands, e-mail: [email protected] > --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
