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..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
@@ -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,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.
+ *
+ * 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();
+ 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();
+ }
+ }
+
}