-
-
Notifications
You must be signed in to change notification settings - Fork 36.6k
src: fix TextDecoder large-input and error paths #65634
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
JosephDoUrden
wants to merge
1
commit into
nodejs:main
Choose a base branch
from
JosephDoUrden:fix/textdecoder-icu-error-path
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+166
−12
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
68 changes: 68 additions & 0 deletions
68
test/pummel/test-whatwg-encoding-custom-textdecoder-large.js
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,68 @@ | ||
| 'use strict'; | ||
| const common = require('../common'); | ||
|
|
||
| // Input large enough that the old 4x target bound exceeded ICU's | ||
| // 0x3fffffff UChar limit; also needs more than a 32-bit heap. | ||
| common.skipIf32Bits(); | ||
|
|
||
| if (!common.hasIntl) | ||
| common.skip('missing Intl'); | ||
|
|
||
| // Peak RSS is around 1.6 GiB: the input, the ICU target buffer, and two | ||
| // result strings. | ||
| if (require('os').totalmem() < 8 * 2 ** 30) | ||
| common.skip('less than 8 GiB of total memory'); | ||
|
|
||
| const assert = require('assert'); | ||
|
|
||
| const size = 2 ** 27; | ||
|
|
||
| let input; | ||
|
|
||
| try { | ||
| input = Buffer.allocUnsafe(size * 2); | ||
| } catch (e) { | ||
| if ( | ||
| e.code === 'ERR_MEMORY_ALLOCATION_FAILED' || | ||
| /Array buffer allocation failed/.test(e.message) | ||
| ) { | ||
| common.skip('insufficient space for Buffer.allocUnsafe'); | ||
| } | ||
|
|
||
| throw e; | ||
| } | ||
|
|
||
| // Non-uniform repeating pattern of A, a U+1F600 surrogate pair and 中, | ||
| // written as explicit little-endian bytes so the input is identical on | ||
| // big-endian hosts. Corrupted or misplaced output cannot match it. | ||
| input.fill(Buffer.from([0x41, 0x00, 0x3D, 0xD8, 0x00, 0xDE, 0x2D, 0x4E])); | ||
|
|
||
| const decoder = new TextDecoder('utf-16le'); | ||
|
|
||
| // 2 ** 27 UTF-16 code units used to fail with | ||
| // ERR_ENCODING_INVALID_ENCODED_DATA because the target buffer request | ||
| // exceeded ICU's internal targetLimit validation. | ||
| // Refs: https://github.com/nodejs/node/issues/47645 | ||
| const result = decoder.decode(input); | ||
| assert.strictEqual(result.length, size); | ||
| assert.strictEqual(result[0], 'A'); | ||
| assert.strictEqual(result[1], '\uD83D'); | ||
| assert.strictEqual(result[2], '\uDE00'); | ||
| assert.strictEqual(result[size / 2], 'A'); | ||
| assert.strictEqual(result[size - 1], '中'); | ||
|
|
||
| // Guard against over-correction: one code unit below the failure boundary | ||
| // decodes at HEAD too and must keep doing so. The truncation removes the | ||
| // trailing 中, so it does not split a surrogate pair. | ||
| assert.strictEqual(decoder.decode(input.subarray(0, size * 2 - 2)).length, | ||
| size - 1); | ||
|
|
||
| // Streaming with an odd byte split lands mid-code-unit, so one byte stays | ||
| // pending in the converter across the chunk boundary. The full content is | ||
| // compared against the non-streaming result, so any corruption at the | ||
| // boundary fails the test. | ||
| const split = 2 ** 26 + 1; | ||
| const streamed = decoder.decode(input.subarray(0, split), { stream: true }) + | ||
| decoder.decode(input.subarray(split)); | ||
| assert.strictEqual(streamed.length, result.length); | ||
| assert.strictEqual(streamed, result); |
79 changes: 79 additions & 0 deletions
79
test/pummel/test-whatwg-encoding-custom-textdecoder-toolong.js
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,79 @@ | ||
| 'use strict'; | ||
| const common = require('../common'); | ||
|
|
||
| // The working set is around 3 GiB, far beyond a 32-bit heap. | ||
| common.skipIf32Bits(); | ||
|
|
||
| if (!common.hasIntl) | ||
| common.skip('missing Intl'); | ||
|
|
||
| // Peak RSS is around 3 GiB: a 1 GiB input, a 2 GiB ICU target buffer, and | ||
| // a transient 1 GiB StringBytes copy. | ||
| if (require('os').totalmem() < 8 * 2 ** 30) | ||
| common.skip('less than 8 GiB of total memory'); | ||
|
|
||
| const assert = require('assert'); | ||
| const kStringMaxLength = require('buffer').constants.MAX_STRING_LENGTH; | ||
|
|
||
| function allocOrSkip(bytes) { | ||
| try { | ||
| return Buffer.allocUnsafe(bytes); | ||
| } catch (e) { | ||
| if ( | ||
| e.code === 'ERR_MEMORY_ALLOCATION_FAILED' || | ||
| /Array buffer allocation failed/.test(e.message) | ||
| ) { | ||
| common.skip('insufficient space for Buffer.allocUnsafe'); | ||
| } | ||
|
|
||
| throw e; | ||
| } | ||
| } | ||
|
|
||
| function assertThrowsTooLong(fn) { | ||
| assert.throws(fn, (e) => { | ||
| // Constrained machines can fail the 1 GiB copy StringBytes makes while | ||
| // building the string, before the length limit is reached. | ||
| if (e.code === 'ERR_MEMORY_ALLOCATION_FAILED') | ||
| common.skip('insufficient memory for the StringBytes copy'); | ||
|
|
||
| assert.strictEqual(e.code, 'ERR_STRING_TOO_LONG'); | ||
| return true; | ||
| }); | ||
| } | ||
|
|
||
| { | ||
| // One UTF-16 code unit beyond the maximum string length: the decode | ||
| // completes inside ICU but the resulting kStringMaxLength + 1 characters | ||
| // cannot be materialised as a string, which must surface as | ||
| // ERR_STRING_TOO_LONG rather than ERR_ENCODING_INVALID_ENCODED_DATA. | ||
| // The ICU target buffer request is 2 * (size / 2) = size UChars, which | ||
| // must stay <= 0x3fffffff (ICU's targetLimit cap) for the conversion to | ||
| // run at all; size = 2 * kStringMaxLength + 2 = 1073741778 satisfies | ||
| // that. | ||
| const size = 2 * kStringMaxLength + 2; | ||
| const input = allocOrSkip(size); | ||
| input.fill(0x20); | ||
| assertThrowsTooLong(() => new TextDecoder('utf-16le').decode(input)); | ||
| } | ||
|
|
||
| { | ||
| // Same limit through a min_char_size() == 1 encoding: pure-ASCII input | ||
| // of kStringMaxLength + 1 bytes decodes to kStringMaxLength + 1 | ||
| // characters. The 2x target bound is clamped to ICU's cap, so only the | ||
| // output length decides the outcome. gb18030 needs full-icu; skip the | ||
| // case silently on small-icu builds (the utf-16le case above ran). | ||
| let decoder; | ||
| try { | ||
| decoder = new TextDecoder('gb18030'); | ||
| } catch (e) { | ||
| if (e.code !== 'ERR_ENCODING_NOT_SUPPORTED') | ||
| throw e; | ||
| } | ||
|
|
||
| if (decoder !== undefined) { | ||
| const input = allocOrSkip(kStringMaxLength + 1); | ||
| input.fill(0x41); | ||
| assertThrowsTooLong(() => decoder.decode(input)); | ||
| } | ||
| } |
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't be hardcoding a magic number that we don't own if there isn't a compelling reason to do so. Why not just leave it to ICU to throw back an error if this limit is violated?