This is an automated email from the ASF dual-hosted git repository.
lukaszlenart pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/struts.git
The following commit(s) were added to refs/heads/main by this push:
new 18955b98a WW-5644 fix(json): confine StrutsJSONWriter write state to
the writing thread (#1776)
18955b98a is described below
commit 18955b98a4c05fd697711007988893916140c92d
Author: ⳕⲛτⲉⲅⲥⲉⳏτⲟⲅ 🕵🏻 <[email protected]>
AuthorDate: Tue Jul 14 12:19:07 2026 +0530
WW-5644 fix(json): confine StrutsJSONWriter write state to the writing
thread (#1776)
* fix(json): confine StrutsJSONWriter write state to the writing thread
JSONUtil obtains its JSONWriter once via @Inject and reuses that same
instance across every concurrent response handled by that JSONResult/
JSONInterceptor configuration. StrutsJSONWriter kept its output buffer,
cyclic-reference stack, root object, and expression-path state
(buf/stack/root/buildExpr/exprStack/excludeProperties/
includeProperties/excludeNullProperties) as plain instance fields, all
reset in place at the start of write().
Two concurrent write() calls on the same instance therefore race on
that reset: one call's in-progress buffer can be wiped and overwritten
by a second, unrelated concurrent call before the first call reads it
back via buf.toString(), so one request's serialized JSON can be
returned as a completely different, concurrently-served request's
response body.
Move buf/stack/root/buildExpr/exprStack/excludeProperties/
includeProperties/excludeNullProperties into a WriteState confined to
a ThreadLocal, scoped to a single write() call. Method signatures and
behavior are otherwise unchanged so existing StrutsJSONWriter
subclasses keep working; ignoreHierarchy/dateFormat/enumAsBean/
excludeProxyProperties stay as plain instance fields since they are
set to the same value on every call for a given writer configuration
and are safe to share.
* test(json): raise writer concurrency test to 16 threads for reliable repro
Verified independently that the 2-thread version can miss the race on
machines with more cores than contending threads (with no CPU
contention, the OS scheduler has no need to preempt either thread
mid-call, so the corruption window is rarely hit): 0 reproductions in
8 reruns against unpatched code on a 10-core machine. Sixteen threads
reproduced it reliably (44,646/320,000 corrupted responses against
unpatched StrutsJSONWriter), and confirmed zero corruption against the
fix under the same load.
---------
Co-authored-by: g0w6y <[email protected]>
---
.../org/apache/struts2/json/StrutsJSONWriter.java | 114 ++++++++++++---------
.../apache/struts2/json/StrutsJSONWriterTest.java | 74 +++++++++++++
2 files changed, 142 insertions(+), 46 deletions(-)
diff --git
a/plugins/json/src/main/java/org/apache/struts2/json/StrutsJSONWriter.java
b/plugins/json/src/main/java/org/apache/struts2/json/StrutsJSONWriter.java
index 391800474..59fadd9b3 100644
--- a/plugins/json/src/main/java/org/apache/struts2/json/StrutsJSONWriter.java
+++ b/plugins/json/src/main/java/org/apache/struts2/json/StrutsJSONWriter.java
@@ -66,6 +66,14 @@ import java.util.regex.Pattern;
* Serializes an object into JavaScript Object Notation (JSON). If cyclic
* references are detected they will be nulled out.
* </p>
+ *
+ * <p>
+ * A single StrutsJSONWriter instance is shared across all concurrent
responses handled by a given
+ * JSONResult/JSONInterceptor (it is injected once, not created per request),
so the output buffer,
+ * cycle-detection stack and other per-write state are kept in a {@link
ThreadLocal}, not instance
+ * fields -- otherwise two concurrent {@link #write(Object)} calls would
corrupt each other's output,
+ * including returning one request's serialized data as another, unrelated
request's response.
+ * </p>
*/
public class StrutsJSONWriter implements JSONWriter {
@@ -84,17 +92,22 @@ public class StrutsJSONWriter implements JSONWriter {
BEAN_INFO_CACHE.clear();
}
- private final StringBuilder buf = new StringBuilder();
- private final Deque<Object> stack = new ArrayDeque<>();
+ private static final class WriteState {
+ private final StringBuilder buf = new StringBuilder();
+ private final Deque<Object> stack = new ArrayDeque<>();
+ private Object root;
+ private boolean buildExpr = true;
+ private String exprStack = "";
+ private Collection<Pattern> excludeProperties;
+ private Collection<Pattern> includeProperties;
+ private boolean excludeNullProperties;
+ }
+
+ private static final ThreadLocal<WriteState> WRITE_STATE = new
ThreadLocal<>();
+
private boolean ignoreHierarchy = true;
- private Object root;
- private boolean buildExpr = true;
- private String exprStack = "";
- private Collection<Pattern> excludeProperties;
- private Collection<Pattern> includeProperties;
private DateFormat dateFormat;
private boolean enumAsBean = ENUM_AS_BEAN_DEFAULT;
- private boolean excludeNullProperties;
private boolean excludeProxyProperties;
private ProxyService proxyService;
@@ -129,18 +142,20 @@ public class StrutsJSONWriter implements JSONWriter {
@Override
public String write(Object object, Collection<Pattern> excludeProperties,
Collection<Pattern> includeProperties, boolean
excludeNullProperties) throws JSONException {
- this.excludeNullProperties = excludeNullProperties;
- this.buf.setLength(0);
- this.stack.clear();
- this.root = object;
- this.exprStack = "";
- this.buildExpr = ((excludeProperties != null) &&
!excludeProperties.isEmpty())
+ WriteState state = new WriteState();
+ state.excludeNullProperties = excludeNullProperties;
+ state.root = object;
+ state.buildExpr = ((excludeProperties != null) &&
!excludeProperties.isEmpty())
|| ((includeProperties != null) &&
!includeProperties.isEmpty());
- this.excludeProperties = excludeProperties;
- this.includeProperties = includeProperties;
- this.value(object, null);
-
- return this.buf.toString();
+ state.excludeProperties = excludeProperties;
+ state.includeProperties = includeProperties;
+ WRITE_STATE.set(state);
+ try {
+ this.value(object, null);
+ return state.buf.toString();
+ } finally {
+ WRITE_STATE.remove();
+ }
}
/**
@@ -156,7 +171,7 @@ public class StrutsJSONWriter implements JSONWriter {
return;
}
- if (this.stack.contains(object)) {
+ if (WRITE_STATE.get().stack.contains(object)) {
Class<?> clazz = object.getClass();
// cyclic reference
@@ -181,7 +196,8 @@ public class StrutsJSONWriter implements JSONWriter {
* @throws JSONException in case of error during serialize
*/
protected void process(Object object, Method method) throws JSONException {
- this.stack.push(object);
+ WriteState state = WRITE_STATE.get();
+ state.stack.push(object);
if (object instanceof Class) {
this.string(object);
@@ -213,7 +229,7 @@ public class StrutsJSONWriter implements JSONWriter {
processCustom(object, method);
}
- this.stack.pop();
+ state.stack.pop();
}
/**
@@ -236,12 +252,13 @@ public class StrutsJSONWriter implements JSONWriter {
protected void bean(Object object) throws JSONException {
this.add("{");
+ WriteState state = WRITE_STATE.get();
BeanInfo info;
try {
Class<?> clazz = excludeProxyProperties ?
proxyService.ultimateTargetClass(object) : object.getClass();
- info = ((object == this.root) && this.ignoreHierarchy)
+ info = ((object == state.root) && this.ignoreHierarchy)
? getBeanInfoIgnoreHierarchy(clazz)
: getBeanInfo(clazz);
@@ -267,7 +284,7 @@ public class StrutsJSONWriter implements JSONWriter {
continue;
}
String expr = null;
- if (this.buildExpr) {
+ if (state.buildExpr) {
expr = this.expandExpr(name);
if (this.shouldExcludeProperty(expr)) {
continue;
@@ -282,7 +299,7 @@ public class StrutsJSONWriter implements JSONWriter {
boolean propertyPrinted = this.add(name, value, accessor,
hasData);
hasData = hasData || propertyPrinted;
- if (this.buildExpr) {
+ if (state.buildExpr) {
this.setExprStack(expr);
}
}
@@ -397,25 +414,28 @@ public class StrutsJSONWriter implements JSONWriter {
}
protected String expandExpr(int i) {
- return this.exprStack + "[" + i + "]";
+ return WRITE_STATE.get().exprStack + "[" + i + "]";
}
protected String expandExpr(String property) {
- if (this.exprStack.isEmpty()) {
+ String exprStack = WRITE_STATE.get().exprStack;
+ if (exprStack.isEmpty()) {
return property;
}
- return this.exprStack + "." + property;
+ return exprStack + "." + property;
}
protected String setExprStack(String expr) {
- String s = this.exprStack;
- this.exprStack = expr;
+ WriteState state = WRITE_STATE.get();
+ String s = state.exprStack;
+ state.exprStack = expr;
return s;
}
protected boolean shouldExcludeProperty(String expr) {
- if (this.excludeProperties != null) {
- for (Pattern pattern : this.excludeProperties) {
+ WriteState state = WRITE_STATE.get();
+ if (state.excludeProperties != null) {
+ for (Pattern pattern : state.excludeProperties) {
if (pattern.matcher(expr).matches()) {
if (LOG.isDebugEnabled()) {
LOG.debug("Ignoring property because of exclude rule:
" + expr);
@@ -425,8 +445,8 @@ public class StrutsJSONWriter implements JSONWriter {
}
}
- if (this.includeProperties != null) {
- for (Pattern pattern : this.includeProperties) {
+ if (state.includeProperties != null) {
+ for (Pattern pattern : state.includeProperties) {
if (pattern.matcher(expr).matches()) {
return false;
}
@@ -443,7 +463,7 @@ public class StrutsJSONWriter implements JSONWriter {
* Add name/value pair to buffer
*/
protected boolean add(String name, Object value, Method method, boolean
hasData) throws JSONException {
- if (excludeNullProperties && value == null) {
+ if (WRITE_STATE.get().excludeNullProperties && value == null) {
return false;
}
if (hasData) {
@@ -462,24 +482,25 @@ public class StrutsJSONWriter implements JSONWriter {
protected void map(Map<?, ?> map, Method method) throws JSONException {
this.add("{");
+ WriteState state = WRITE_STATE.get();
Iterator<?> it = map.entrySet().iterator();
boolean warnedNonString = false; // one report per map
boolean hasData = false;
while (it.hasNext()) {
Map.Entry<?, ?> entry = (Map.Entry<?, ?>) it.next();
- if (excludeNullProperties && entry.getValue() == null) {
+ if (state.excludeNullProperties && entry.getValue() == null) {
continue;
}
Object key = entry.getKey();
if (key == null) {
- LOG.error("Cannot build expression for null key in {}",
exprStack);
+ LOG.error("Cannot build expression for null key in {}",
state.exprStack);
continue;
}
String expr = null;
- if (this.buildExpr) {
+ if (state.buildExpr) {
expr = this.expandExpr(key.toString());
if (this.shouldExcludeProperty(expr)) {
continue;
@@ -499,7 +520,7 @@ public class StrutsJSONWriter implements JSONWriter {
this.value(key.toString(), method);
this.add(":");
this.value(entry.getValue(), method);
- if (this.buildExpr) {
+ if (state.buildExpr) {
this.setExprStack(expr);
}
}
@@ -566,10 +587,11 @@ public class StrutsJSONWriter implements JSONWriter {
protected void array(Iterator<?> it, Method method) throws JSONException {
this.add("[");
+ WriteState state = WRITE_STATE.get();
boolean hasData = false;
for (int i = 0; it.hasNext(); i++) {
String expr = null;
- if (this.buildExpr) {
+ if (state.buildExpr) {
expr = this.expandExpr(i);
if (this.shouldExcludeProperty(expr)) {
it.next();
@@ -582,7 +604,7 @@ public class StrutsJSONWriter implements JSONWriter {
}
hasData = true;
this.value(it.next(), method);
- if (this.buildExpr) {
+ if (state.buildExpr) {
this.setExprStack(expr);
}
}
@@ -596,12 +618,13 @@ public class StrutsJSONWriter implements JSONWriter {
protected void array(Object object, Method method) throws JSONException {
this.add("[");
+ WriteState state = WRITE_STATE.get();
int length = Array.getLength(object);
boolean hasData = false;
for (int i = 0; i < length; ++i) {
String expr = null;
- if (this.buildExpr) {
+ if (state.buildExpr) {
expr = this.expandExpr(i);
if (this.shouldExcludeProperty(expr)) {
continue;
@@ -613,7 +636,7 @@ public class StrutsJSONWriter implements JSONWriter {
}
hasData = true;
this.value(Array.get(object, i), method);
- if (this.buildExpr) {
+ if (state.buildExpr) {
this.setExprStack(expr);
}
}
@@ -669,14 +692,14 @@ public class StrutsJSONWriter implements JSONWriter {
* Add object to buffer
*/
protected void add(Object obj) {
- this.buf.append(obj);
+ WRITE_STATE.get().buf.append(obj);
}
/*
* Add char to buffer
*/
protected void add(char c) {
- this.buf.append(c);
+ WRITE_STATE.get().buf.append(c);
}
/**
@@ -759,5 +782,4 @@ public class StrutsJSONWriter implements JSONWriter {
return this;
}
}
-
}
diff --git
a/plugins/json/src/test/java/org/apache/struts2/json/StrutsJSONWriterTest.java
b/plugins/json/src/test/java/org/apache/struts2/json/StrutsJSONWriterTest.java
index acc52dcaa..2239c5f71 100644
---
a/plugins/json/src/test/java/org/apache/struts2/json/StrutsJSONWriterTest.java
+++
b/plugins/json/src/test/java/org/apache/struts2/json/StrutsJSONWriterTest.java
@@ -39,6 +39,11 @@ import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.TimeZone;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicInteger;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
@@ -315,4 +320,73 @@ public class StrutsJSONWriterTest {
assertTrue(json.contains("\"calendar\":\"2012-12-23T10:10:10\""));
}
+ public static class NamedBean {
+ private final String owner;
+ private final String secret;
+
+ public NamedBean(String owner, String secret) {
+ this.owner = owner;
+ this.secret = secret;
+ }
+
+ public String getOwner() { return owner; }
+ public String getSecret() { return secret; }
+ }
+
+ /**
+ * A single StrutsJSONWriter instance is injected once into
JSONUtil/JSONResult and reused
+ * across every concurrent response handled by that result, so write()
must be safe to call
+ * concurrently from multiple threads on the same instance without one
call's output buffer
+ * leaking into -- or being overwritten by -- another call's output.
+ *
+ * <p>Uses a higher thread count than the minimum needed to demonstrate
the bug: with only two
+ * threads on a machine with many cores, the OS scheduler has no need to
preempt either thread
+ * mid-call, so the race window is rarely hit and the test can pass even
against the unpatched
+ * code. Sixteen threads contending for the same instance reproduces the
corruption reliably.</p>
+ */
+ @Test
+ public void testConcurrentReuseDoesNotSwapResponsesAcrossWrites() throws
Exception {
+ JSONWriter sharedWriter = new StrutsJSONWriter();
+ int threadCount = 16;
+ int iterations = 20_000;
+ AtomicInteger corrupted = new AtomicInteger();
+ CountDownLatch start = new CountDownLatch(1);
+ ExecutorService pool = Executors.newFixedThreadPool(threadCount);
+
+ for (int t = 0; t < threadCount; t++) {
+ String secret = "SECRET_" + t + "_9f8e7d6c5b4a3210";
+ NamedBean own = new NamedBean("user" + t, secret);
+ pool.submit(() -> {
+ await(start);
+ for (int i = 0; i < iterations; i++) {
+ try {
+ String response = sharedWriter.write(own);
+ if (!response.contains(secret)) {
+ // this thread's own response doesn't even contain
its own data:
+ // the shared buffer was corrupted or overwritten
by another thread
+ corrupted.incrementAndGet();
+ }
+ } catch (JSONException ignored) {
+ // a failed serialize under contention is not itself
the thing measured here
+ }
+ }
+ });
+ }
+
+ start.countDown();
+ pool.shutdown();
+ assertTrue(pool.awaitTermination(120, TimeUnit.SECONDS));
+
+ assertEquals("a concurrently-written response must never overwrite or
corrupt this " +
+ "thread's own response", 0, corrupted.get());
+ }
+
+ private static void await(CountDownLatch latch) {
+ try {
+ latch.await();
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ }
+ }
+
}
