fix: grow strBuf when reading long escaped strings, for issue #7671#7676
fix: grow strBuf when reading long escaped strings, for issue #7671#7676kamilkrzywanski wants to merge 1 commit into
Conversation
wenshao
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. The change is correct and correctly scoped: the hoisted and mid-loop this.strBuf = strBuf assignments are equivalent/safe, and JSONReaderUTF16 is the only reader with this reusable-buffer pattern (JSONReaderUTF8 writes the grown buffer back to its thread-local cache; the ASCII/Str paths don't use strBuf). Build and the new Issue7671 tests pass (verified locally on JDK 17). One non-blocking suggestion inline.
— Qwen Code via Qwen Code /review
| @Test | ||
| public void testStrBufGrow() { |
There was a problem hiding this comment.
[Suggestion] These regression tests don't gate the source change — both pass identically with the two mid-loop this.strBuf = strBuf; assignments reverted. Verified empirically: reverting only the source edits and running Issue7671 gives Tests run: 2, Failures: 0 (test() and testStrBufGrow() both green). The pre-existing else if (stroff > strBuf.length) guard (added in commit bdfe92f for issue #3989) already prevents the original ArrayIndexOutOfBoundsException at the initial arraycopy, and the parsed result is always built from the correctly-grown local strBuf; so on current main this is a buffer-reuse/consistency improvement rather than a live crash fix. In testStrBufGrow the two added lines never even execute: key "b" has a leading run stroff=600 > 512, so the else if grows the buffer, and the total 651 < newCapacity(600,512)=768, so neither mid-loop guard fires. — Concrete cost: a future revert of the two lines would not be caught by these tests. Consider a white-box assertion (e.g. via reflection) that strBuf capacity grows after parsing consecutive long escaped strings, or a short note that this is a reuse/consistency follow-up to #3989 rather than a new crash fix. (The getBytes(UTF_8) path uses JSONReaderUTF8, which writes the grown buffer back to its cache, so it doesn't exercise the changed code.)
— Qwen Code via Qwen Code /review
58b27cf to
9b2c9a7
Compare
JSONReaderUTF16 grew a local strBuf while decoding escaped strings but did not always write the larger array back to this.strBuf. Later reads then re-grew from a stale smaller buffer. Always assign this.strBuf after allocate/grow (including mid-loop) so the reusable buffer stays in sync. The AIOOBE reported in alibaba#7671 on 2.0.61 is already fixed on main by alibaba#3989; this is a buffer-reuse follow-up.
9b2c9a7 to
6f90c9d
Compare
What this PR does / why we need it?
Keep
JSONReaderUTF16.strBufin sync when the local buffer is grown mid-loop while reading escaped strings.Issue #7671 reported
ArrayIndexOutOfBoundsExceptionon 2.0.61 when a long escaped string followed a shorter one that had already initializedstrBuf(e.g.char[512]). The live crash path is already fixed on main by #3989 (else if (stroff > strBuf.length)before the initialarraycopy, released in 2.0.62). This change is a buffer-reuse / consistency follow-up: whenstrBufis resized inside the escape loop, write the grown array back tothis.strBufso later reads reuse the larger buffer instead of re-growing from the stale smaller one.Summary of your change
this.strBufafter initial allocate/grow, and after mid-loop growth inJSONReaderUTF16.readString.Issue3989already covers the AIOOBE case; a black-box parse test would not fail if these mid-loop assignments were reverted.Please indicate you've done the following: