fix: guard against ArrayIndexOutOfBounds in char2 for non-hex \x escape digits - #7829
fix: guard against ArrayIndexOutOfBounds in char2 for non-hex \x escape digits#7829waterWang wants to merge 1 commit into
Conversation
…pe digits DIGITS2 only covers 0..'f'. A \x escape whose digit falls outside that table (or a byte >= 0x80 arriving as a negative int in the byte-based readers) previously indexed DIGITS2 out of bounds, crashing with ArrayIndexOutOfBoundsException instead of parsing or raising JSONException. Introduce digit2() which returns 0 for characters outside the table, the same value the table already gives to in-range non-hex characters, keeping existing semantics. Fixes alibaba#7810.
|
|
xhon-pelushi
left a comment
There was a problem hiding this comment.
Built this locally (JDK 21, mvn -pl core). The fix in JSONReader.char2() is correct and I'd like to see it land — but the test file does not compile, and once it does, two of its seven tests fail. One of those failures is a real bug that this PR doesn't fix.
The fix itself checks out
DIGITS2 has 103 entries (0..'f'), and in-range non-hex characters are already mapped to 0 (JSONFactory.java:142-150), so "out-of-range characters get the value the table already gives to in-range non-hex characters" is accurate, not just plausible.
Measured on master vs this branch:
| input | master | this PR |
|---|---|---|
"\xzz" |
AIOOBE: Index 122 out of bounds for length 103 |
U+0000 |
"\xgg" |
AIOOBE: Index 103 |
U+0000 |
"\x{{" |
AIOOBE: Index 123 |
U+0000 |
"\x\u00ff\u00ff" |
AIOOBE: Index -1 |
U+0000 |
"\x\u0080\u0080" |
AIOOBE: Index -128 |
U+0000 |
{"\xzz":1} (field name) |
AIOOBE: Index 122 |
{"\u0000":1} |
{"a":["\xzz"]} (nested) |
AIOOBE: Index 122 |
{"a":["\u0000"]} |
"\x41" / "\x2f" |
U+0041 / U+002F |
unchanged |
Fixing the shared helper rather than the ~11 call sites is the right call — char2 is reached from 10 places in JSONReaderASCII plus JSONReaderUTF16:980, all passing raw bytes[...]/chars[...].
I also checked whether not throwing is the wrong choice, since char1() does reject invalid escapes via char1Error. I think you're right not to throw: the closer analogue is the \u path, and IOUtils.hexDigit4(byte[], int) does branchless SWAR nibble extraction with no hex validation at all, so silently decoding non-hex digits is the established behaviour for hex escapes. Worth keeping in mind that this is a deliberate choice, not an oversight.
On the AGENTS.md codeSize rule — the refactor is fine, and slightly better than before:
master: char2 = 16 bytes
this PR: char2 = 14 bytes (array loads moved out)
this PR: digit2 = 22 bytes
Both are under the 35-byte C1 MaxInlineSize threshold, so nothing loses inlinability, and this is the \x escape path rather than the hot scan loop. No performance objection from me.
Blocking: the test file doesn't compile
mvn -pl core test fails before running anything:
Issue7810.java:[41,63] cannot find symbol
symbol: method get(java.lang.String)
location: class java.lang.Object
Issue7810.java:[42,59] cannot find symbol
symbol: method keySet()
location: class java.lang.Object
JSON.parse returns Object, so lines 41-42 need a cast plus import com.alibaba.fastjson2.JSONObject;:
assertEquals("\u0000", ((JSONObject) JSON.parse("{\"a\":\"\\xzz\"}")).get("a"));
assertEquals("\u0000", ((JSONObject) JSON.parse("{\"\\xzz\":1}")).keySet().iterator().next());Separately, mvn validate fails checkstyle:
Issue7810.java:[1] (misc) NewlineAtEndOfFile: File does not end with a newline.
The blob really does end with 0x7d — I checked it through the GitHub API rather than trusting my checkout, so it isn't a local artifact.
After making it compile, 2 of 7 tests fail
Tests run: 7, Failures: 2, Errors: 0
testAllContainerPaths (line 40) compares a scalar to a JSONArray's JSON text:
assertEquals("\u0000", JSON.parse("[\"\\xzz\"]").toString());
// expected: <\u0000> but was: <["\u0000"]>JSON.parse("[\"\\xzz\"]") is a JSONArray, so toString() is ["\u0000"]. Probably wants ((JSONArray) JSON.parse(...)).get(0).
testTruncatedEscapeStillThrows (line 70) is the interesting one — it fails on this branch:
assertThrows(JSONException.class, () -> JSON.parse("{\"\\"));
// Unexpected exception type thrown, expected: <JSONException>
// but was: <java.lang.ArrayIndexOutOfBoundsException>That is a genuine remaining bug, not a bad assertion. A trailing backslash still throws AIOOBE on both master and this branch, but only in field-name position:
master this PR
{"\ (field name) AIOOBE: Index 3 out of bounds for length 3 same
{"ab\ (field name) AIOOBE: Index 5 out of bounds for length 5 same
["\ (array) JSONException: invalid escape character EOI same
"\ (bare) JSONException: invalid escape character EOI same
{"a":"\ (value) JSONException: invalid escape character EOI same
So the field-name escape path is missing the end-of-input guard that the value path has. It's the same class as this PR but a different site, and it's adjacent to what #7827 is fixing in JSONReaderASCII.readFieldName() — though that PR targets the whitespace loops, not the escape loop, so I don't think it covers this. Either fold the field-name guard in here, or drop this assertion and file it separately; leaving a failing test in isn't an option.
Summary
The char2 change is good and I'd merge it. Before that, please: add the JSONObject import and casts, add the trailing newline, fix the line-40 assertion, and decide what to do about line 70 — mvn validate and mvn test both need to pass, per AGENTS.md.
I only exercised core on JDK 21 / x86-64 Linux, and only the tests named above rather than the full core suite.
中文版
在本地构建并验证过(JDK 21,mvn -pl core)。JSONReader.char2() 的修复是正确的,我认为应该合入 —— 但测试文件无法编译;修好编译错误后,7 个测试中有 2 个失败,其中一个暴露了本 PR 未修复的真实缺陷。
修复本身是正确的
DIGITS2 共 103 项(0 到 'f'),且范围内的非十六进制字符本来就映射为 0(JSONFactory.java:142-150)。因此"越界字符取与范围内非十六进制字符相同的值"这一说法是经过核实的。
master 与本分支的对比:"\xzz" 在 master 上抛 AIOOBE: Index 122 out of bounds for length 103,本 PR 返回 U+0000;"\xgg"(Index 103)、"\x{{"(Index 123)、"\x\u00ff\u00ff"(Index -1)、"\x\u0080\u0080"(Index -128)、字段名 {"\xzz":1}(Index 122)、嵌套 {"a":["\xzz"]}(Index 122)同理。"\x41"、"\x2f" 等合法十六进制转义行为不变。
修改共享的辅助方法而不是逐个修改约 11 个调用点是正确的选择:char2 在 JSONReaderASCII 中有 10 处调用,另有 JSONReaderUTF16:980,全部直接传入 bytes[...] / chars[...]。
我也考虑过"是否应该抛异常",因为 char1() 通过 char1Error 拒绝非法转义。我认为不抛是对的:更接近的参照是 \u 路径,IOUtils.hexDigit4(byte[], int) 使用无分支的 SWAR 位运算提取,完全不校验十六进制字符。所以对十六进制转义静默解码本来就是既有行为。这是有意的取舍,值得记录下来。
关于 AGENTS.md 的 codeSize 规则:char2 从 16 字节降到 14 字节,新增的 digit2 为 22 字节,二者都低于 C1 MaxInlineSize 的 35 字节阈值,内联不受影响;而且这属于 \x 转义路径,不是热扫描循环。性能上没有问题。
阻塞问题:测试文件无法编译
mvn -pl core test 在运行任何测试前就失败:Issue7810.java:[41,63] 和 [42,59] 报 cannot find symbol,因为 JSON.parse 返回 Object。需要加 import com.alibaba.fastjson2.JSONObject; 并强制转型。
另外 mvn validate 的 checkstyle 失败:Issue7810.java:[1] (misc) NewlineAtEndOfFile: File does not end with a newline.。我通过 GitHub API 核对过该 blob 确实以 0x7d 结尾,不是本地检出造成的。
修好编译后仍有 2 个测试失败
Tests run: 7, Failures: 2, Errors: 0
testAllContainerPaths(第 40 行):把标量与JSONArray的 JSON 文本比较,expected: <\u0000> but was: <["\u0000"]>。应改为((JSONArray) JSON.parse(...)).get(0)。testTruncatedEscapeStillThrows(第 70 行):这是真正的缺陷,不是断言写错。JSON.parse("{\"\\")在 master 和本分支上都抛ArrayIndexOutOfBoundsException(Index 3 out of bounds for length 3),而数组位置["\、裸字符串"\、值位置{"a":"\都能正确抛出JSONException。也就是说字段名的转义路径缺少值路径已有的输入结束保护。这与本 PR 属于同一类问题但位于不同位置,和 #7827 修改的readFieldName相邻,但那个 PR 针对的是空白跳过循环而非转义循环,应该覆盖不到这里。建议要么在本 PR 中一并加上字段名的保护,要么删掉该断言并另开 issue —— 但不能留下一个失败的测试。
小结
char2 的改动很好,建议合入。合入前请:补上 JSONObject 的 import 与转型、补上文件末尾换行、修正第 40 行断言,并决定第 70 行如何处理。按 AGENTS.md 的要求,mvn validate 与 mvn test 都需要通过。
我只在 JDK 21 / x86-64 Linux 上验证了 core 模块,且只运行了上述测试,未跑完整的 core 测试套件。
Problem
A
\xescape whose digit falls outside the hex table crashes withArrayIndexOutOfBoundsExceptioninstead of parsing or raisingJSONException.JSONReader.char2()indexesDIGITS2directly with the raw character:DIGITS2only covers0..'f'. Any escape character above that — or a byte>= 0x80read as a signed byte in the byte-based readers (arriving as a negative int) — indexes out of bounds:Fix
Add
digit2()which returns0for characters outside the table — the same valueDIGITS2already gives to in-range non-hex characters — keeping existing semantics for"\x::"/"\x@@"(which already decode to\u0000) intact.Verified that
"\x41"->A,"\xff"->\u00ff,"\x00"->\u0000are unchanged.Fixes #7810