[
https://issues.apache.org/jira/browse/WW-3576?focusedWorklogId=1048116&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-1048116
]
ASF GitHub Bot logged work on WW-3576:
--------------------------------------
Author: ASF GitHub Bot
Created on: 18/Oct/26 14:06
Start Date: 18/Oct/26 14:06
Worklog Time Spent: 10m
Work Description: Copilot commented on code in PR #2042:
URL: https://github.com/apache/struts/pull/2042#discussion_r4307326144
##########
core/src/main/java/org/apache/struts2/interceptor/exec/SessionSandboxReconciler.java:
##########
@@ -0,0 +1,121 @@
+/*
+ * 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.struts2.interceptor.exec;
+
+import jakarta.servlet.http.HttpServletRequest;
+import jakarta.servlet.http.HttpSession;
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
+import org.apache.struts2.action.BackgroundSessionConflictAware;
+import org.apache.struts2.action.SessionConflict;
+
+import java.util.LinkedHashSet;
+import java.util.List;
+import java.util.Objects;
+import java.util.Set;
+
+/**
+ * Applies the writes an execAndWait background process logged in its {@link
SandboxedSessionMap} to the
+ * session of the polling request. Writes are discarded when that session is
not the one the process started
+ * with. A key changed elsewhere since the snapshot keeps its current value
unless the action implements
+ * {@link BackgroundSessionConflictAware}.
+ */
+public final class SessionSandboxReconciler {
+
+ private static final Logger LOG =
LogManager.getLogger(SessionSandboxReconciler.class);
+
+ private SessionSandboxReconciler() {
+ }
+
+ /**
+ * Applies the sandbox's writes to the session of the given request. Must
be called on that request's thread.
+ *
+ * @param sandbox the sandbox the background process worked on
+ * @param request the polling request that saw the process finish
+ * @param action the background process's action, consulted when it
implements
+ * {@link BackgroundSessionConflictAware}
+ */
+ public static void reconcile(SandboxedSessionMap sandbox,
HttpServletRequest request, Object action) {
+ HttpSession real = request.getSession(false);
+ if (real == null || !real.getId().equals(sandbox.getStartSessionId()))
{
+ LOG.debug("Discarding the session writes of background process
[{}]: its session is gone", sandbox.getProcessName());
+ return;
+ }
+ List<SessionOperation> operations = sandbox.getOperations();
+ int lastInvalidate = lastInvalidateIndex(operations);
+ if (lastInvalidate >= 0) {
+ real.invalidate();
+ HttpSession fresh = request.getSession(true);
+ for (String key : touchedKeys(operations.subList(lastInvalidate +
1, operations.size()))) {
+ write(fresh, key, sandbox.get(key));
+ }
+ return;
+ }
+ for (String key : touchedKeys(operations)) {
+ reconcileKey(sandbox, real, key, action);
+ }
+ }
+
+ private static void reconcileKey(SandboxedSessionMap sandbox, HttpSession
real, String key, Object action) {
+ Object current = real.getAttribute(key);
+ Object snapshot = sandbox.getSnapshotValue(key);
+ Object background = sandbox.get(key);
+ if (Objects.equals(current, snapshot)) {
+ write(real, key, background);
Review Comment:
The conflict check and write are not atomic with ordinary request writes.
Another request can update this key after getAttribute() reads the snapshot
value but before write() stores the background value. That update is then lost
without invoking conflict resolution. The interceptor's
synchronized(httpSession) block does not prevent this because SessionMap
mutations do not acquire that monitor. Use a shared per-session synchronization
protocol for reconciliation and framework mutations, and add a deterministic
test for this interleaving.
##########
core/src/main/java/org/apache/struts2/interceptor/exec/SandboxedSessionMap.java:
##########
@@ -0,0 +1,171 @@
+/*
+ * 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.struts2.interceptor.exec;
+
+import jakarta.servlet.http.HttpSession;
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
+import org.apache.struts2.dispatcher.SessionMap;
+
+import java.io.Serial;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.Enumeration;
+import java.util.HashSet;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.Callable;
+import java.util.concurrent.atomic.AtomicBoolean;
+
+/**
+ * The session view an execAndWait background process works on: a shallow
snapshot of the session taken at
+ * construction plus an ordered log of the process's own writes. It never
touches the live session; the writes are
+ * applied to it later by {@link SessionSandboxReconciler}.
+ */
+@SuppressWarnings("java:S2160") // equal by contents, as every Map
+public class SandboxedSessionMap extends SessionMap {
+
+ @Serial
+ private static final long serialVersionUID = 2907451127739228064L;
+
+ private static final Logger LOG =
LogManager.getLogger(SandboxedSessionMap.class);
+
+ private final transient Map<String, Object> snapshot = new
LinkedHashMap<>();
+ private final transient Map<String, Object> view = new LinkedHashMap<>();
+ private final transient List<SessionOperation> operations = new
ArrayList<>();
+ private final transient String startSessionId;
+ private final transient String processName;
+ private final transient AtomicBoolean escapeWarned = new AtomicBoolean();
+
+ /**
+ * Snapshots the attributes of the given session. Must be called on the
request thread that owns the session.
+ *
+ * @param session the live session to snapshot
+ * @param processName the name of the background process, used in log
messages
+ */
+ public SandboxedSessionMap(HttpSession session, String processName) {
+ super();
+ this.startSessionId = session.getId();
+ this.processName = processName;
+ Enumeration<String> names = session.getAttributeNames();
+ while (names.hasMoreElements()) {
+ String name = names.nextElement();
+ snapshot.put(name, session.getAttribute(name));
+ }
+ view.putAll(snapshot);
+ }
+
+ <T> T runInSandbox(Callable<T> work) throws Exception {
+ return callWithLiveWriteListener(this::warnEscapeOnce, work);
+ }
+
+ private void warnEscapeOnce() {
+ if (escapeWarned.compareAndSet(false, true)) {
+ LOG.warn("Background process [{}] wrote to the request session,
bypassing its session sandbox; "
+ + "use ActionContext.getContext().getSession() or
SessionAware", processName);
+ }
+ }
+
+ String getStartSessionId() {
+ return startSessionId;
+ }
+
+ String getProcessName() {
+ return processName;
+ }
+
+ synchronized List<SessionOperation> getOperations() {
+ return Collections.unmodifiableList(new ArrayList<>(operations));
+ }
+
+ synchronized Object getSnapshotValue(String key) {
+ return snapshot.get(key);
+ }
+
+ @Override
+ public synchronized Object get(Object key) {
+ return view.get(asKey(key));
+ }
+
+ /**
+ * Records the write in the sandbox; a {@code null} value is recorded as a
removal.
+ *
+ * @param key the name of the session attribute
+ * @param value the value to set
+ * @return the previous value in the sandbox
+ */
+ @Override
+ public synchronized Object put(String key, Object value) {
+ if (value == null) {
+ return remove(key);
+ }
+ operations.add(new SessionOperation.Put(key, value));
+ return view.put(key, value);
+ }
+
+ @Override
+ public synchronized Object remove(Object key) {
+ String name = asKey(key);
+ operations.add(new SessionOperation.Remove(name));
+ return view.remove(name);
+ }
+
+ @Override
+ public synchronized boolean containsKey(Object key) {
+ return view.get(asKey(key)) != null;
+ }
+
+ @Override
+ public synchronized Set<Entry<String, Object>> entrySet() {
+ Set<Entry<String, Object>> result = new HashSet<>();
+ for (Map.Entry<String, Object> current : view.entrySet()) {
+ String key = current.getKey();
+ result.add(new SimpleEntry<>(key, current.getValue()) {
+ @Override
+ public Object setValue(Object value) {
+ return put(key, value);
Review Comment:
This updates the sandbox but leaves the SimpleEntry's stored value
unchanged. After entry.setValue("new"), entry.getValue() still returns the old
value, violating the Map.Entry contract. Update the entry itself as well as
recording the sandbox write, and extend testEntrySetValueIsLogged to check the
retained entry's value and setValue's return value.
##########
core/src/main/java/org/apache/struts2/interceptor/ExecuteAndWaitInterceptor.java:
##########
@@ -352,6 +362,15 @@ protected String doIntercept(ActionInvocation
actionInvocation) throws Exception
}
}
+ private void writeBackSessionSandbox(BackgroundProcess bp, ActionContext
context) {
+ bp.getSessionSandbox().ifPresent(sandbox -> {
+ SessionSandboxReconciler.reconcile(sandbox,
ServletActionContext.getRequest(), bp.getAction());
+ if (bp.getAction() instanceof SessionAware sessionAware) {
+ sessionAware.withSession(context.getSession());
Review Comment:
When the sandbox logs invalidate() followed by a put, reconciliation
populates a replacement HttpSession, but context.getSession() still pins the
invalidated session. Reinjecting that map makes SessionAware and
result-rendering reads return null for the new attributes until a put repins
it. Refresh both the polling context's session map and its
DispatcherConstants.SESSION helper before reinjection when reconciliation
replaces the session. Add an interceptor-level invalidate-then-put test that
reads the attributes during result rendering.
##########
core/src/main/java/org/apache/struts2/interceptor/exec/SessionSandboxReconciler.java:
##########
@@ -0,0 +1,121 @@
+/*
+ * 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.struts2.interceptor.exec;
+
+import jakarta.servlet.http.HttpServletRequest;
+import jakarta.servlet.http.HttpSession;
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
+import org.apache.struts2.action.BackgroundSessionConflictAware;
+import org.apache.struts2.action.SessionConflict;
+
+import java.util.LinkedHashSet;
+import java.util.List;
+import java.util.Objects;
+import java.util.Set;
+
+/**
+ * Applies the writes an execAndWait background process logged in its {@link
SandboxedSessionMap} to the
+ * session of the polling request. Writes are discarded when that session is
not the one the process started
+ * with. A key changed elsewhere since the snapshot keeps its current value
unless the action implements
+ * {@link BackgroundSessionConflictAware}.
+ */
+public final class SessionSandboxReconciler {
+
+ private static final Logger LOG =
LogManager.getLogger(SessionSandboxReconciler.class);
+
+ private SessionSandboxReconciler() {
+ }
+
+ /**
+ * Applies the sandbox's writes to the session of the given request. Must
be called on that request's thread.
+ *
+ * @param sandbox the sandbox the background process worked on
+ * @param request the polling request that saw the process finish
+ * @param action the background process's action, consulted when it
implements
+ * {@link BackgroundSessionConflictAware}
+ */
+ public static void reconcile(SandboxedSessionMap sandbox,
HttpServletRequest request, Object action) {
+ HttpSession real = request.getSession(false);
+ if (real == null || !real.getId().equals(sandbox.getStartSessionId()))
{
+ LOG.debug("Discarding the session writes of background process
[{}]: its session is gone", sandbox.getProcessName());
+ return;
+ }
+ List<SessionOperation> operations = sandbox.getOperations();
+ int lastInvalidate = lastInvalidateIndex(operations);
+ if (lastInvalidate >= 0) {
+ real.invalidate();
+ HttpSession fresh = request.getSession(true);
+ for (String key : touchedKeys(operations.subList(lastInvalidate +
1, operations.size()))) {
+ write(fresh, key, sandbox.get(key));
+ }
+ return;
+ }
+ for (String key : touchedKeys(operations)) {
+ reconcileKey(sandbox, real, key, action);
+ }
+ }
+
+ private static void reconcileKey(SandboxedSessionMap sandbox, HttpSession
real, String key, Object action) {
+ Object current = real.getAttribute(key);
Review Comment:
Another request can invalidate the session after the ID check, causing
getAttribute() or a subsequent write to throw IllegalStateException.
SessionMap.invalidate() does not acquire the interceptor's HttpSession monitor.
The completion poll then fails after its background-process key has already
been removed. Handle invalidation around the Servlet API calls and discard
remaining writes without creating a replacement session or swallowing
exceptions from the conflict hook. Add a regression test that invalidates the
session between the ID check and attribute access.
##########
core/src/main/java/org/apache/struts2/interceptor/exec/StrutsBackgroundProcess.java:
##########
@@ -91,6 +100,55 @@ public BackgroundProcess prepare() {
return this;
}
+ @Override
+ public BackgroundProcess withSessionSandbox(SandboxedSessionMap sandbox) {
+ this.sessionSandbox = sandbox;
+ this.sandboxContext =
copyOf(invocation.getInvocationContext().getContextMap());
+ return this;
+ }
+
+ @Override
+ public Optional<SandboxedSessionMap> getSessionSandbox() {
+ return Optional.ofNullable(sessionSandbox);
+ }
+
+ private String invokeAction() throws Exception {
+ if (sessionSandbox == null) {
+ return invocation.invokeActionOnly();
+ }
+ return sessionSandbox.runInSandbox(invocation::invokeActionOnly);
+ }
+
+ private void installSessionSandbox() {
+ if (sessionSandbox == null) {
+ return;
+ }
+
ActionContext.bind(ActionContext.of(backgroundContextMap()).withSession(sessionSandbox));
+ if (invocation.getAction() instanceof SessionAware sessionAware) {
+ sessionAware.withSession(sessionSandbox);
+ }
+ }
+
+ private Map<String, Object> backgroundContextMap() {
+ ActionContext bound = ActionContext.getContext();
+ if (bound == null || bound.getContextMap() ==
invocation.getInvocationContext().getContextMap()) {
+ return sandboxContext;
Review Comment:
A beforeInvocation() override that calls super.beforeInvocation() and then
edits ActionContext still satisfies this identity check. Its edits, such as a
locale change, are discarded when the earlier handoff snapshot is installed. In
the default beforeInvocation(), bind ActionContext.of(sandboxContext) when
sessionSandbox is present; retain the existing binding otherwise. This lets
super-calling overrides edit the isolated context the action will use. Add a
test for that override alongside the existing custom-context test.
Issue Time Tracking
-------------------
Worklog Id: (was: 1048116)
Time Spent: 2h (was: 1h 50m)
> SessionMap is not thread-safe
> -----------------------------
>
> Key: WW-3576
> URL: https://issues.apache.org/jira/browse/WW-3576
> Project: Struts 2
> Issue Type: Bug
> Components: Other
> Affects Versions: 2.2.1
> Reporter: Sylvain Veyrié
> Assignee: Lukasz Lenart
> Priority: Major
> Fix For: 7.5.0
>
> Time Spent: 2h
> Remaining Estimate: 0h
>
> Searching for a bug after some stress test (Exception on "getAttribute":
> already invalidated session), I reviewed SessionMap.java.
> Following WW-239, SessionMap has been made thread-safe.
> All methods are like this :
> {code:java}
> public void doSomething() {
> if (session == null) {
> return;
> }
>
> synchronized (session) {
> session.doSometing();
> }
> }
> {code}
> For example:
> {code:java}
> public void invalidate() {
> if (session == null) {
> return;
> }
>
> synchronized (session) {
> session.invalidate();
> session = null;
> entries = null;
> }
> }
> {code}
> IMHO this is not thread-safe. With the example of invalidate(), if there is a
> context switch just before the synchronized, the nullity is no more checked.
> If another invalidate() is called at least a NPE can be thrown. There are
> probably other side-effects like my exception problem.
> As now Struts 2 only supports Java 5+, there is two ways to fix it :
> * use a double-check-locking (DCL) and set session "volatile" (easy way)
> * use java.util.concurrent instead of synchronized keyword
> If you agree and choose one of those fixes, I can provide a patch.
> For the moment, I don't know if my bug is resolved if we directly use javax
> session, without this wrapper.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
