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 75a285a10 WW-5643 fix(json): confine StrutsJSONReader parse state to
the parsing thread (#1775)
75a285a10 is described below
commit 75a285a1065313e082d4ecf4e678211ee65331e0
Author: ⳕⲛτⲉⲅⲥⲉⳏτⲟⲅ 🕵🏻 <[email protected]>
AuthorDate: Tue Jul 14 12:18:46 2026 +0530
WW-5643 fix(json): confine StrutsJSONReader parse state to the parsing
thread (#1775)
* fix(json): confine StrutsJSONReader parse state to the parsing thread
JSONInterceptor obtains its JSONReader once via @Inject and reuses that
same instance across every concurrent request handled by that
interceptor. StrutsJSONReader kept its parse cursor, token buffer and
nesting-depth counter (used to enforce maxDepth/maxElements/
maxStringLength/maxKeyLength) as plain instance fields, so two
concurrent read() calls on the same instance tore each other's state:
one request's depth counter could be decremented by an unrelated
concurrent request finishing its own parse, letting payloads deeper
than the configured maxDepth through, and the shared character cursor
and string/number buffer let fragments of one request's JSON body leak
into a different, concurrently-parsed request's result.
Move the cursor, current character, token, buffer and depth into a
ParseState confined to a ThreadLocal, scoped to a single read() call.
Method signatures and behavior are otherwise unchanged so existing
StrutsJSONReader subclasses keep working; the limit fields
(maxElements/maxDepth/maxStringLength/maxKeyLength) stay as plain
instance fields since they are set to the same value on every call for
a given interceptor configuration and are safe to share.
* test(json): raise reader 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 both symptoms reliably against unpatched StrutsJSONReader
(81 cross-thread data leaks and 79 maxDepth bypasses out of 160,000
attempts), and confirmed zero of either against the fix under the
same load. Combined the two prior tests into one, since both symptoms
come from the same shared parse state and are naturally checked
together per thread.
---------
Co-authored-by: g0w6y <[email protected]>
---
.../org/apache/struts2/json/StrutsJSONReader.java | 134 ++++++++++++---------
.../apache/struts2/json/StrutsJSONReaderTest.java | 81 +++++++++++++
2 files changed, 159 insertions(+), 56 deletions(-)
diff --git
a/plugins/json/src/main/java/org/apache/struts2/json/StrutsJSONReader.java
b/plugins/json/src/main/java/org/apache/struts2/json/StrutsJSONReader.java
index 530a479cc..96593d2b9 100644
--- a/plugins/json/src/main/java/org/apache/struts2/json/StrutsJSONReader.java
+++ b/plugins/json/src/main/java/org/apache/struts2/json/StrutsJSONReader.java
@@ -30,6 +30,13 @@ import java.util.Map;
* Deserializes an object from a JSON string with configurable limits
* to prevent denial-of-service attacks via malicious payloads.
* </p>
+ *
+ * <p>
+ * A single StrutsJSONReader instance is shared across all concurrent requests
handled by a given
+ * JSONInterceptor (it is injected once, not created per request), so the
cursor, token buffer and
+ * nesting depth of an in-progress parse are kept in a {@link ThreadLocal},
not instance fields --
+ * otherwise two concurrent {@link #read(String)} calls would corrupt each
other's parse state.
+ * </p>
*/
public class StrutsJSONReader implements JSONReader {
private static final Object OBJECT_END = new Object();
@@ -47,16 +54,20 @@ public class StrutsJSONReader implements JSONReader {
't', '\t'
);
- private CharacterIterator it;
- private char c;
- private Object token;
- private final StringBuilder buf = new StringBuilder();
+ private static final class ParseState {
+ private CharacterIterator it;
+ private char c;
+ private Object token;
+ private final StringBuilder buf = new StringBuilder();
+ private int depth;
+ }
+
+ private static final ThreadLocal<ParseState> PARSE_STATE = new
ThreadLocal<>();
private int maxElements = DEFAULT_MAX_ELEMENTS;
private int maxDepth = DEFAULT_MAX_DEPTH;
private int maxStringLength = DEFAULT_MAX_STRING_LENGTH;
private int maxKeyLength = DEFAULT_MAX_KEY_LENGTH;
- private int depth;
@Override
public void setMaxElements(int maxElements) {
@@ -79,92 +90,99 @@ public class StrutsJSONReader implements JSONReader {
}
protected char next() {
- this.c = this.it.next();
+ ParseState state = PARSE_STATE.get();
+ state.c = state.it.next();
- return this.c;
+ return state.c;
}
protected void skipWhiteSpace() {
- while (Character.isWhitespace(this.c)) {
+ while (Character.isWhitespace(PARSE_STATE.get().c)) {
this.next();
}
}
@Override
public Object read(String string) throws JSONException {
- this.it = new StringCharacterIterator(string);
- this.c = this.it.first();
- this.depth = 0;
-
- return this.read();
+ ParseState state = new ParseState();
+ state.it = new StringCharacterIterator(string);
+ state.c = state.it.first();
+ PARSE_STATE.set(state);
+ try {
+ return this.read();
+ } finally {
+ PARSE_STATE.remove();
+ }
}
protected Object read() throws JSONException {
+ ParseState state = PARSE_STATE.get();
Object ret;
this.skipWhiteSpace();
- if (this.c == '"') {
+ if (state.c == '"') {
this.next();
ret = this.string('"');
- } else if (this.c == '\'') {
+ } else if (state.c == '\'') {
this.next();
ret = this.string('\'');
- } else if (this.c == '[') {
+ } else if (state.c == '[') {
this.next();
ret = this.array();
- } else if (this.c == ']') {
+ } else if (state.c == ']') {
ret = ARRAY_END;
this.next();
- } else if (this.c == ',') {
+ } else if (state.c == ',') {
ret = COMMA;
this.next();
- } else if (this.c == '{') {
+ } else if (state.c == '{') {
this.next();
ret = this.object();
- } else if (this.c == '}') {
+ } else if (state.c == '}') {
ret = OBJECT_END;
this.next();
- } else if (this.c == ':') {
+ } else if (state.c == ':') {
ret = COLON;
this.next();
- } else if ((this.c == 't') && (this.next() == 'r') && (this.next() ==
'u') && (this.next() == 'e')) {
+ } else if ((state.c == 't') && (this.next() == 'r') && (this.next() ==
'u') && (this.next() == 'e')) {
ret = Boolean.TRUE;
this.next();
- } else if ((this.c == 'f') && (this.next() == 'a') && (this.next() ==
'l') && (this.next() == 's')
+ } else if ((state.c == 'f') && (this.next() == 'a') && (this.next() ==
'l') && (this.next() == 's')
&& (this.next() == 'e')) {
ret = Boolean.FALSE;
this.next();
- } else if ((this.c == 'n') && (this.next() == 'u') && (this.next() ==
'l') && (this.next() == 'l')) {
+ } else if ((state.c == 'n') && (this.next() == 'u') && (this.next() ==
'l') && (this.next() == 'l')) {
ret = null;
this.next();
- } else if (Character.isDigit(this.c) || (this.c == '-')) {
+ } else if (Character.isDigit(state.c) || (state.c == '-')) {
ret = this.number();
} else {
throw buildInvalidInputException();
}
- this.token = ret;
+ state.token = ret;
return ret;
}
protected Map<String, Object> object() throws JSONException {
- if (this.depth >= this.maxDepth) {
+ ParseState state = PARSE_STATE.get();
+ if (state.depth >= this.maxDepth) {
throw new JSONException("JSON object nesting exceeds maximum
allowed depth ("
+ this.maxDepth + "). Use " + JSONConstants.JSON_MAX_DEPTH
+ " to increase the limit.");
}
- this.depth++;
+ state.depth++;
try {
Map<String, Object> ret = new HashMap<>();
Object next = this.read();
if (next != OBJECT_END) {
String key = (String) next;
validateKeyLength(key);
- while (this.token != OBJECT_END) {
+ while (state.token != OBJECT_END) {
this.read(); // should be a colon
- if (this.token != OBJECT_END) {
+ if (state.token != OBJECT_END) {
if (ret.size() >= this.maxElements) {
throw new JSONException("JSON object exceeds
maximum allowed elements ("
+ this.maxElements + "). Use " +
JSONConstants.JSON_MAX_ELEMENTS + " to increase the limit.");
@@ -187,7 +205,7 @@ public class StrutsJSONReader implements JSONReader {
return ret;
} finally {
- this.depth--;
+ state.depth--;
}
}
@@ -199,21 +217,22 @@ public class StrutsJSONReader implements JSONReader {
}
protected JSONException buildInvalidInputException() {
- return new JSONException("Input string is not well formed JSON
(invalid char " + this.c + ")");
+ return new JSONException("Input string is not well formed JSON
(invalid char " + PARSE_STATE.get().c + ")");
}
protected List<Object> array() throws JSONException {
- if (this.depth >= this.maxDepth) {
+ ParseState state = PARSE_STATE.get();
+ if (state.depth >= this.maxDepth) {
throw new JSONException("JSON array nesting exceeds maximum
allowed depth ("
+ this.maxDepth + "). Use " + JSONConstants.JSON_MAX_DEPTH
+ " to increase the limit.");
}
- this.depth++;
+ state.depth++;
try {
List<Object> ret = new ArrayList<>();
Object value = this.read();
- while (this.token != ARRAY_END) {
+ while (state.token != ARRAY_END) {
if (ret.size() >= this.maxElements) {
throw new JSONException("JSON array exceeds maximum
allowed elements ("
+ this.maxElements + "). Use " +
JSONConstants.JSON_MAX_ELEMENTS + " to increase the limit.");
@@ -230,31 +249,32 @@ public class StrutsJSONReader implements JSONReader {
return ret;
} finally {
- this.depth--;
+ state.depth--;
}
}
protected Object number() throws JSONException {
- this.buf.setLength(0);
+ ParseState state = PARSE_STATE.get();
+ state.buf.setLength(0);
boolean toDouble = false;
- if (this.c == '-') {
+ if (state.c == '-') {
this.add();
}
this.addDigits();
- if (this.c == '.') {
+ if (state.c == '.') {
toDouble = true;
this.add();
this.addDigits();
}
- if ((this.c == 'e') || (this.c == 'E')) {
+ if ((state.c == 'e') || (state.c == 'E')) {
toDouble = true;
this.add();
- if ((this.c == '+') || (this.c == '-')) {
+ if ((state.c == '+') || (state.c == '-')) {
this.add();
}
@@ -263,13 +283,13 @@ public class StrutsJSONReader implements JSONReader {
if (toDouble) {
try {
- return Double.parseDouble(this.buf.toString());
+ return Double.parseDouble(state.buf.toString());
} catch (NumberFormatException e) {
throw buildInvalidInputException();
}
} else {
try {
- return Long.parseLong(this.buf.toString());
+ return Long.parseLong(state.buf.toString());
} catch (NumberFormatException e) {
throw buildInvalidInputException();
}
@@ -277,16 +297,17 @@ public class StrutsJSONReader implements JSONReader {
}
protected Object string(char quote) throws JSONException {
- this.buf.setLength(0);
+ ParseState state = PARSE_STATE.get();
+ state.buf.setLength(0);
- while ((this.c != quote) && (this.c != CharacterIterator.DONE)) {
- if (this.c == '\\') {
+ while ((state.c != quote) && (state.c != CharacterIterator.DONE)) {
+ if (state.c == '\\') {
this.next();
- if (this.c == 'u') {
+ if (state.c == 'u') {
this.add(this.unicode());
} else {
- Character value = escapes.get(this.c);
+ Character value = escapes.get(state.c);
if (value != null) {
this.add(value);
@@ -295,7 +316,7 @@ public class StrutsJSONReader implements JSONReader {
} else {
this.add();
}
- if (this.buf.length() > this.maxStringLength) {
+ if (state.buf.length() > this.maxStringLength) {
throw new JSONException("JSON string exceeds maximum allowed
length ("
+ this.maxStringLength + "). Use " +
JSONConstants.JSON_MAX_STRING_LENGTH + " to increase the limit.");
}
@@ -303,32 +324,33 @@ public class StrutsJSONReader implements JSONReader {
this.next();
- return this.buf.toString();
+ return state.buf.toString();
}
protected void add(char cc) {
- this.buf.append(cc);
+ PARSE_STATE.get().buf.append(cc);
this.next();
}
protected void add() {
- this.add(this.c);
+ this.add(PARSE_STATE.get().c);
}
protected void addDigits() {
- while (Character.isDigit(this.c)) {
+ while (Character.isDigit(PARSE_STATE.get().c)) {
this.add();
}
}
protected char unicode() {
+ ParseState state = PARSE_STATE.get();
int value = 0;
for (int i = 0; i < 4; ++i) {
value = switch (this.next()) {
- case '0', '1', '2', '3', '4', '5', '6', '7', '8', '9' ->
(value << 4) + (this.c - '0');
- case 'a', 'b', 'c', 'd', 'e', 'f' -> (value << 4) + (this.c -
'W');
- case 'A', 'B', 'C', 'D', 'E', 'F' -> (value << 4) + (this.c -
'7');
+ case '0', '1', '2', '3', '4', '5', '6', '7', '8', '9' ->
(value << 4) + (state.c - '0');
+ case 'a', 'b', 'c', 'd', 'e', 'f' -> (value << 4) + (state.c -
'W');
+ case 'A', 'B', 'C', 'D', 'E', 'F' -> (value << 4) + (state.c -
'7');
default -> value;
};
}
diff --git
a/plugins/json/src/test/java/org/apache/struts2/json/StrutsJSONReaderTest.java
b/plugins/json/src/test/java/org/apache/struts2/json/StrutsJSONReaderTest.java
index 48c69c865..827711c61 100644
---
a/plugins/json/src/test/java/org/apache/struts2/json/StrutsJSONReaderTest.java
+++
b/plugins/json/src/test/java/org/apache/struts2/json/StrutsJSONReaderTest.java
@@ -22,6 +22,11 @@ import org.junit.Test;
import java.util.List;
import java.util.Map;
+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.assertNotNull;
@@ -156,4 +161,80 @@ public class StrutsJSONReaderTest {
var ex = assertThrows(JSONException.class, () -> reader.read(json));
assertTrue(ex.getMessage().contains("maximum allowed depth"));
}
+
+ /**
+ * A single StrutsJSONReader instance is injected once into
JSONUtil/JSONInterceptor and reused
+ * across every concurrent request handled by that interceptor, so read()
must be safe to call
+ * concurrently from multiple threads on the same instance without one
call's parse state
+ * (cursor, token buffer, depth counter) leaking into another's -- both as
a maxDepth bypass and
+ * as one request's data appearing inside a different, concurrently-parsed
request's result.
+ *
+ * <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 both
symptoms reliably.</p>
+ */
+ @Test
+ public void
testConcurrentReuseDoesNotBypassMaxDepthOrLeakDataAcrossParses() throws
Exception {
+ int maxDepth = 5;
+ var reader = new StrutsJSONReader();
+ reader.setMaxDepth(maxDepth);
+
+ String overDepthPayload = nestedObject(maxDepth + 1);
+ int threadCount = 16;
+ int iterations = 10_000;
+ AtomicInteger overDepthAccepted = new AtomicInteger();
+ AtomicInteger crossThreadLeaks = new AtomicInteger();
+ CountDownLatch start = new CountDownLatch(1);
+ ExecutorService pool = Executors.newFixedThreadPool(threadCount);
+
+ for (int t = 0; t < threadCount; t++) {
+ String marker = "MARKER_" + t + "_9f8e7d6c5b4a3210";
+ String ownPayload = "{\"account\":\"user" + t + "\",\"secret\":\""
+ marker + "\"}";
+ pool.submit(() -> {
+ await(start);
+ for (int i = 0; i < iterations; i++) {
+ try {
+ Object result = reader.read(ownPayload);
+ if (!(result instanceof Map) ||
!marker.equals(((Map<?, ?>) result).get("secret"))) {
+ // this thread's own parse doesn't even contain
its own data: the shared
+ // cursor/buffer was corrupted or overwritten by
another thread
+ crossThreadLeaks.incrementAndGet();
+ }
+ } catch (JSONException ignored) {
+ // a failed parse under contention is not itself the
thing measured here
+ }
+ try {
+ reader.read(overDepthPayload);
+ overDepthAccepted.incrementAndGet();
+ } catch (JSONException expected) {
+ // must always be rejected: depth is one more than
maxDepth
+ }
+ }
+ });
+ }
+
+ start.countDown();
+ pool.shutdown();
+ assertTrue(pool.awaitTermination(120, TimeUnit.SECONDS));
+
+ assertEquals("an over-depth payload must never be accepted, even when
the reader instance " +
+ "is shared with concurrent unrelated parses", 0,
overDepthAccepted.get());
+ assertEquals("a concurrently-parsed request's data must never appear
in place of this " +
+ "thread's own parsed result", 0, crossThreadLeaks.get());
+ }
+
+ private static void await(CountDownLatch latch) {
+ try {
+ latch.await();
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ }
+ }
+
+ private static String nestedObject(int depth) {
+ StringBuilder sb = new StringBuilder();
+
sb.append("{\"a\":".repeat(depth)).append("1").append("}".repeat(depth));
+ return sb.toString();
+ }
}
