src: fix TextDecoder large-input and error paths - #65634
Open
JosephDoUrden wants to merge 1 commit into
Open
Conversation
ConverterObject::Decode() sized its ICU target buffer as the input length, or the pending byte count when flushing if that is larger, times min_char_size(), times 2. min_char_size() is the minimum number of bytes per character, so multiplying by it inflates the bound instead of tightening it: for UTF-16 (min_char_size() == 2) a 256 MiB input requested 2^30 UChars, which fails ucnv_toUnicode()'s internal targetLimit validation before any input is examined, and the failure was then reported as ERR_ENCODING_INVALID_ENCODED_DATA. Bound the buffer by 2 * (input length + pending bytes) / min_char_size instead: each character consumes at least min_char_size bytes and emits at most one surrogate pair, and bytes carried over from previous chunks complete a character in this one. The request is also clamped to ucnv_toUnicode()'s target-range validation limit of 0x3fffffff UChars, which loses nothing since larger results cannot fit in a V8 string anyway. This decodes every input whose result fits in a V8 string. Also return after a failed StringBytes::Encode() instead of falling through, so the exception it scheduled (such as ERR_STRING_TOO_LONG for results beyond the string limit) is no longer masked by ERR_ENCODING_INVALID_ENCODED_DATA. The `2 *` factor dates to 98ec909, which restored the effective capacity that an earlier targetLimit arithmetic bug had provided by accident. The min_char_size() multiplier itself is older, from ed21cb1. Fixes: nodejs#47645 Refs: nodejs#41026 Refs: nodejs#61559 Signed-off-by: Yusufhan Saçak <yusufhansacak@icloud.com>
Contributor
|
Benchmark GHA (util / text-decoder): https://github.com/nodejs/node/actions/runs/33238297431 🟢 Results
Benchmark results:
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #65634 +/- ##
=======================================
Coverage 90.05% 90.05%
=======================================
Files 754 754
Lines 255722 255720 -2
Branches 48314 48318 +4
=======================================
+ Hits 230281 230289 +8
- Misses 16555 16557 +2
+ Partials 8886 8874 -12
🚀 New features to boost your workflow:
|
Contributor
|
Unfortunately there was an issue in the |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #47645.
TextDecoder('utf-16le').decode(new Uint16Array(2**27))throwsERR_ENCODING_INVALID_ENCODED_DATAeven though the input is valid and the result (134M chars) fits comfortably in a V8 string. Root cause analysis is in the issue thread:ConverterObject::Decode()sizes the ICU target as2 * min_char_size * input lengthUChars.min_char_size()is the minimum number of bytes per character, so multiplying by it inflates the bound instead of tightening it. For UTF-16 that requests 4x the input in UChars, which crossesucnv_toUnicode()'s target range validation (0x3fffffff UChars) at exactly 2**27 elements, and ICU rejects the call before looking at a single byte. The failure then gets reported by the blanket invalid-data throw at the bottom of the function.Two changes, same function:
The bound becomes
2 * (input length + pending bytes) / min_char_size, clamped to the ICU cap. Each character consumes at leastmin_char_sizebytes and emits at most a surrogate pair, and bytes carried over from a previous chunk complete a character in this one, so they count too. For everymin_char_size == 1encoding (all the CJK ones) this is the same as or larger than the old bound, so nothing tightens there. Only utf-16le/be change, from 4x to 1x. After the change the whole representable range decodes: an input of2 * MAX_STRING_LENGTHbytes produces exactly aMAX_STRING_LENGTHstring.Second,
Decode()now returns after a failedStringBytes::Encode()instead of falling through, so the exception Encode scheduled (ERR_STRING_TOO_LONGfor oversized results) is no longer replaced byERR_ENCODING_INVALID_ENCODED_DATA. Boundary behaviour measured locally:2 * MAX_STRING_LENGTHbytes decodes,+2bytes throwsERR_STRING_TOO_LONG, lone surrogate withfatal: truestill throwsERR_ENCODING_INVALID_ENCODED_DATA.On the interaction with #61559 / #61041: I know the direction is to move TextDecoder off ICU, and #61559 would take utf-16 out of this path entirely. But big5, euc-jp, euc-kr, gb18030, shift_jis and iso-2022-jp keep routing through
ConverterObject::Decode(), and the error-path fix applies to all of them, so this stands regardless of when the fast-path work lands. Happy to rebase if #61559 moves first.Tests are in pummel because of the working set (~1.6 GiB and ~3 GiB peaks, both under the existing
test-buffer-large-size-*ceiling). The streaming case uses an odd byte split so a code unit actually stays pending across the chunk boundary, and compares content, not just length. Everything ran locally on both a small-icu and a full-icu build: the two pummel tests (including the gb18030ERR_STRING_TOO_LONGcase, which needs full-icu),parallel/test-whatwg-encoding*andparallel/test-icu-*, plus an A/B sweep against an unpatched build of the same tree (91 sizes per endianness, no behaviour change other than the fixed cases) and a mid-character gb18030 streaming split to exercise the pending-bytes term on amin_char_size == 1encoding.Known remaining limitation: results that could never fit in a string (more than 0x3fffffff UChars, i.e. inputs over ~2 GiB) still surface the blanket error rather than
ERR_STRING_TOO_LONG. Fixing that needs a chunked conversion loop; it did not seem worth the extra risk in this change.AI disclosure: I used an AI coding agent for parts of the investigation and drafting. I verified the root cause against the ICU and Node sources myself, and every number in this description comes from runs on my own machine.