From 082d4cb1398a9b5db0f76055a22d14bb7eeed3fc Mon Sep 17 00:00:00 2001 From: g0w6y Date: Thu, 9 Jul 2026 21:28:55 +0530 Subject: [PATCH 1/2] 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. --- .../apache/struts2/json/StrutsJSONWriter.java | 114 +++++++++++------- .../struts2/json/StrutsJSONWriterTest.java | 75 ++++++++++++ 2 files changed, 143 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 3918004741..59fadd9b3f 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 @@ * Serializes an object into JavaScript Object Notation (JSON). If cyclic * references are detected they will be nulled out. *

+ * + *

+ * 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. + *

*/ public class StrutsJSONWriter implements JSONWriter { @@ -84,17 +92,22 @@ public static void clearBeanInfoCaches() { BEAN_INFO_CACHE.clear(); } - private final StringBuilder buf = new StringBuilder(); - private final Deque stack = new ArrayDeque<>(); + private static final class WriteState { + private final StringBuilder buf = new StringBuilder(); + private final Deque stack = new ArrayDeque<>(); + private Object root; + private boolean buildExpr = true; + private String exprStack = ""; + private Collection excludeProperties; + private Collection includeProperties; + private boolean excludeNullProperties; + } + + private static final ThreadLocal WRITE_STATE = new ThreadLocal<>(); + private boolean ignoreHierarchy = true; - private Object root; - private boolean buildExpr = true; - private String exprStack = ""; - private Collection excludeProperties; - private Collection 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 String write(Object object) throws JSONException { @Override public String write(Object object, Collection excludeProperties, Collection 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 @@ protected void value(Object object, Method method) throws JSONException { return; } - if (this.stack.contains(object)) { + if (WRITE_STATE.get().stack.contains(object)) { Class clazz = object.getClass(); // cyclic reference @@ -181,7 +196,8 @@ protected void value(Object object, Method method) throws JSONException { * @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 @@ protected void process(Object object, Method method) throws JSONException { processCustom(object, method); } - this.stack.pop(); + state.stack.pop(); } /** @@ -236,12 +252,13 @@ protected void processCustom(Object object, Method method) throws JSONException 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 @@ protected void bean(Object object) throws JSONException { continue; } String expr = null; - if (this.buildExpr) { + if (state.buildExpr) { expr = this.expandExpr(name); if (this.shouldExcludeProperty(expr)) { continue; @@ -282,7 +299,7 @@ protected void bean(Object object) throws JSONException { boolean propertyPrinted = this.add(name, value, accessor, hasData); hasData = hasData || propertyPrinted; - if (this.buildExpr) { + if (state.buildExpr) { this.setExprStack(expr); } } @@ -397,25 +414,28 @@ protected boolean shouldExcludeProperty(PropertyDescriptor prop) throws Security } 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 @@ protected boolean shouldExcludeProperty(String expr) { } } - 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 @@ protected boolean shouldExcludeProperty(String expr) { * 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 @@ protected boolean add(String name, Object value, Method method, boolean hasData) 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 @@ protected void map(Map map, Method method) throws JSONException { 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 @@ private static DateTimeFormatter getDefaultDateTimeFormatter(TemporalAccessor te 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 @@ protected void array(Iterator it, Method method) throws JSONException { } hasData = true; this.value(it.next(), method); - if (this.buildExpr) { + if (state.buildExpr) { this.setExprStack(expr); } } @@ -596,12 +618,13 @@ protected void array(Iterator it, Method method) throws JSONException { 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 @@ protected void array(Object object, Method method) throws JSONException { } hasData = true; this.value(Array.get(object, i), method); - if (this.buildExpr) { + if (state.buildExpr) { this.setExprStack(expr); } } @@ -669,14 +692,14 @@ protected void string(Object obj) { * 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 JSONAnnotationFinder invoke() { 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 acc52dcaa8..1641827ae9 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.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,74 @@ public void testSerializeCalendar() throws Exception { 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. + */ + @Test + public void testConcurrentReuseDoesNotSwapResponsesAcrossWrites() throws Exception { + JSONWriter sharedWriter = new StrutsJSONWriter(); + NamedBean victim = new NamedBean("victim", "VICTIM_SECRET_TOKEN_9f8e7d6c5b4a3210"); + NamedBean attacker = new NamedBean("attacker", "nothing interesting here"); + + int iterations = 50_000; + AtomicInteger leaksObserved = new AtomicInteger(); + CountDownLatch start = new CountDownLatch(1); + ExecutorService pool = Executors.newFixedThreadPool(2); + + pool.submit(() -> { + await(start); + for (int i = 0; i < iterations; i++) { + try { + sharedWriter.write(victim); + } catch (JSONException ignored) { + // the victim call's own outcome isn't what's being measured + } + } + }); + pool.submit(() -> { + await(start); + for (int i = 0; i < iterations; i++) { + try { + String response = sharedWriter.write(attacker); + if (response.contains(victim.getSecret())) { + leaksObserved.incrementAndGet(); + } + } catch (JSONException ignored) { + // a failed serialize under contention isn't itself a leak + } + } + }); + + start.countDown(); + pool.shutdown(); + assertTrue(pool.awaitTermination(120, TimeUnit.SECONDS)); + + assertEquals("a concurrently-written response must never be returned in place of this " + + "request's own response", 0, leaksObserved.get()); + } + + private static void await(CountDownLatch latch) { + try { + latch.await(); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + } + } + } From eb5a584060955d50f46f7f5f502e2c2bc6775fd3 Mon Sep 17 00:00:00 2001 From: g0w6y Date: Thu, 9 Jul 2026 23:08:17 +0530 Subject: [PATCH 2/2] 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. --- .../struts2/json/StrutsJSONWriterTest.java | 59 +++++++++---------- 1 file changed, 29 insertions(+), 30 deletions(-) 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 1641827ae9..2239c5f719 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 @@ -338,48 +338,47 @@ public NamedBean(String owner, String secret) { * 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. + * + *

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.

*/ @Test public void testConcurrentReuseDoesNotSwapResponsesAcrossWrites() throws Exception { JSONWriter sharedWriter = new StrutsJSONWriter(); - NamedBean victim = new NamedBean("victim", "VICTIM_SECRET_TOKEN_9f8e7d6c5b4a3210"); - NamedBean attacker = new NamedBean("attacker", "nothing interesting here"); - - int iterations = 50_000; - AtomicInteger leaksObserved = new AtomicInteger(); + int threadCount = 16; + int iterations = 20_000; + AtomicInteger corrupted = new AtomicInteger(); CountDownLatch start = new CountDownLatch(1); - ExecutorService pool = Executors.newFixedThreadPool(2); - - pool.submit(() -> { - await(start); - for (int i = 0; i < iterations; i++) { - try { - sharedWriter.write(victim); - } catch (JSONException ignored) { - // the victim call's own outcome isn't what's being measured - } - } - }); - pool.submit(() -> { - await(start); - for (int i = 0; i < iterations; i++) { - try { - String response = sharedWriter.write(attacker); - if (response.contains(victim.getSecret())) { - leaksObserved.incrementAndGet(); + 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 } - } catch (JSONException ignored) { - // a failed serialize under contention isn't itself a leak } - } - }); + }); + } start.countDown(); pool.shutdown(); assertTrue(pool.awaitTermination(120, TimeUnit.SECONDS)); - assertEquals("a concurrently-written response must never be returned in place of this " + - "request's own response", 0, leaksObserved.get()); + assertEquals("a concurrently-written response must never overwrite or corrupt this " + + "thread's own response", 0, corrupted.get()); } private static void await(CountDownLatch latch) {