Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
g0w6y wants to merge 2 commits into
base: main
Choose a base branch
from
+142 −46
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
082d4cb
fix(json): confine StrutsJSONWriter write state to the writing thread
g0w6y Jul 9, 2026
eb5a584
test(json): raise writer concurrency test to 16 threads for reliable …
g0w6y Jul 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
114 changes: 68 additions & 46 deletions plugins/json/src/main/java/org/apache/struts2/json/StrutsJSONWriter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,14 @@
* 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 {

Expand All @@ -84,17 +92,22 @@ public static void clearBeanInfoCaches() {
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;

Expand Down Expand Up @@ -129,18 +142,20 @@ public String write(Object object) throws JSONException {
@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();
}
}

/**
Expand All @@ -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
Expand All @@ -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);
Expand Down Expand Up @@ -213,7 +229,7 @@ protected void process(Object object, Method method) throws JSONException {
processCustom(object, method);
}

this.stack.pop();
state.stack.pop();
}

/**
Expand All @@ -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);

Expand All @@ -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;
Expand All @@ -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);
}
}
Expand Down Expand Up @@ -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);
Expand All @@ -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;
}
Expand All @@ -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) {
Expand All @@ -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;
Expand All @@ -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);
}
}
Expand Down Expand Up @@ -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();
Expand All @@ -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);
}
}
Expand All @@ -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;
Expand All @@ -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);
}
}
Expand Down Expand Up @@ -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);
}

/**
Expand Down Expand Up @@ -759,5 +782,4 @@ public JSONAnnotationFinder invoke() {
return this;
}
}

}
74 changes: 74 additions & 0 deletions plugins/json/src/test/java/org/apache/struts2/json/StrutsJSONWriterTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -315,4 +320,73 @@ 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.
*
* <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();
}
}

}
Loading