fix: guard readFieldName lookahead against AIOOBE on trailing backslash - #7833
fix: guard readFieldName lookahead against AIOOBE on trailing backslash#7833waterWang wants to merge 1 commit into
Conversation
JSONReaderASCII.readFieldName() reads bytes[offset + 1] after seeing a backslash,
but the loop guard only covers bytes[offset]. When a field name ends in a backslash
(e.g. `{"\`, `{"a\`), offset + 1 == end, so the lookahead runs off the array and
throws ArrayIndexOutOfBoundsException instead of the expected JSONException.
Add an explicit check: if the backslash is the final byte, throw JSONException
(matching how malformed field names are already handled elsewhere).
|
|
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
Not explored to full depth (tool budget reached): "agent 5": 未执行 mvn test （共享 worktree 未编译， core/target/classes 不存在，规则也禁止在此强制构建），因此没有经验性确认"加了守卫后整套现有测试仍全绿"。; "agent 5": 未实测 JSON.parseObject("{\\\"中文\\\\") 走 UTF16 抛 AIOOBE 的复现——witness: not run —结论来自源码阅读（ toCharArray() 长度恰为 end 、循环不变式 offset < end ），非执行结果。; "agent 5": 未核实 JSONReaderUTF8 的 bytes 是否会被分配得比 end 大（若被 pad，则其 UTF8 表现可能是读到脏字节而非 AIOOBE）。; "agent 3b": did not check whether readFieldNameHashCode() and its readFieldNameHashCode0()/…Error() helpers in the three readers carry the same unguarded lookahead (tex…; "agent 3b": the sibling-path AIOOBE was demonstrated against the locally installed 2.0.65-SNAPSHOT jar plus the HEAD source, not a build of HEAD (no build run in the shared….
— qwen3.8-flash via Qwen Code /review (v0.22.2)
| if (c == '\\') { | ||
| nameEscape = true; | ||
| if (offset + 1 >= end) { | ||
| throw new JSONException(info("illegal input")); |
There was a problem hiding this comment.
[Suggestion] The new throw diverges from this file's established wording for the same condition. JSONReaderASCII already reports "escape ran off the end" as error("invalid escape character EOI") at six sites (lines 147, 213, 284, 546, 1269, 1398), and JSONReader.error(String) is exactly new JSONException(info(message)) — so the added line hand-inlines that factory with a different message, and the commit's stated rationale ("matching how malformed field names are handled elsewhere") does not hold. Measured on this commit: JSON.parseObject("{\"a\\") (Map path) now reports illegal input, offset 2 while JSON.parseObject("{\"a\\", Foo.class) (bean path, line 147) reports invalid escape character EOI, offset 2 for the same one-line document — tests or alert rules that match the established string (16 call sites across core) will not recognize the new branch. Suggest reusing the factory: ```suggestion throw error("invalid escape character EOI"); ``` No test asserts illegal input today (grep over core/src/test returns nothing), so the swap contradicts nothing; and if the UTF16 twin at JSONReaderUTF16.java:1991 gets the same guard in a follow-up, it is worth choosing the wording once for both readers.
— qwen3.8-flash via Qwen Code /review (v0.22.2)
| int c = bytes[offset]; | ||
| if (c == '\\') { | ||
| nameEscape = true; | ||
| if (offset + 1 >= end) { |
There was a problem hiding this comment.
[Suggestion] Nothing in the test suite pins this guard: reverting all three added lines leaves the entire reactor suite green. Measured during this review — the full 15-module mvn test on the PR reports 9606 tests / 0 failures, and the same suite on an identical tree with only these three lines reverted (the file restored to 30dd009d1) reports the exact same 9606 / 0 failures. The behavioral delta is real, though: with the guard, JSON.parseObject("{\"a\\") throws JSONException: illegal input; with it reverted, the same call throws ArrayIndexOutOfBoundsException: Index 4 out of bounds for length 4. readFieldName() is a large method that gets hot-path refactors regularly, so a future edit that drops this branch would silently resurrect the original #7809 symptom — an unchecked ArrayIndexOutOfBoundsException escaping callers' catch (JSONException) — with no CI signal. Suggest adding core/src/test/java/com/alibaba/fastjson2/issues_7000/Issue7809.java (the convention used by the sibling fixes Issue7668/7678/7691): assertThrows(JSONException.class, () -> JSON.parseObject("{\"a\\")) for the inputs {"\, {"a\ and {"a":1,"b\ — asserting the specific type matters here, since ArrayIndexOutOfBoundsException is also a RuntimeException and a loose assertThrows(Exception.class, ...) would stay green on the unfixed build — plus one positive control such as {"a\"b":1} still parsing to key a"b. As the acceptance criterion for that new test, remove these three lines and confirm it goes red with the ArrayIndexOutOfBoundsException.
— qwen3.8-flash via Qwen Code /review (v0.22.2)
3 participants
Fixes #7809
JSONReaderASCII.readFieldName()readsbytes[offset + 1]after seeing a backslash, but the loop guard only coversbytes[offset]. When a field name ends in a backslash,offset + 1 == end, so the lookahead runs off the array and throwsArrayIndexOutOfBoundsExceptioninstead ofJSONException.Add an explicit check: if the backslash is the final byte, throw
JSONException, matching how malformed field names are handled elsewhere.