Apache/struts
1
0
Fork 0
mirror of https://github.com/apache/struts.git synced 2026-08-05 14:47:09 +00:00
Code Issues Packages Projects Releases Wiki Activity

WW-5644 fix(json): confine StrutsJSONWriter write state to the writing thread (#1776)

Browse Source 
* 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 <g0w6y@users.noreply.github.com>
This commit is contained in:
ⳕⲛτⲉⲅⲥⲉⳏτⲟⲅ 🕵🏻
2026-07-14 12:19:07 +05:30
committed by GitHub
parent 75a285a106
commit 18955b98a4
2 changed files with 142 additions and 46 deletions
Download Patch File Download Diff File Expand all files Collapse all files
plugins/json/src/main/java/org/apache/struts2/json/StrutsJSONWriter.java
+68 -46
View File
@@ -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;
}
}
}
plugins/json/src/test/java/org/apache/struts2/json/StrutsJSONWriterTest.java
+74
View File
@@ -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();
}
}
}