intl: fix limit calculation - #41026
Merged
Merged
Conversation
nodejs-github-bot
added
c++
Member
Author
|
This is where I see that utf-16 may take 1 or 2 chars - https://en.wikipedia.org/wiki/UTF-16 @jasnell from history I can see you touched this code last and even though the code related to this was simply copied, maybe you have the best context to know if what I'm thinking from my look today makes sense? |
Coverity reported that the use of sizeof along with pointer arithmetic was likely an error as the pointer arithmetic would already be accounting for the size of what the pointer points to. Looking at the code that looked right but removing the extra sizeOf caused tests to fail. Looking more closely it seems like we were not allocating a big enough buffer but the extra sizeof was allowing us to convert even though it might have been corrupting memory. Signed-off-by: Michael Dawson <mdawson@devrus.com>
mhdawson
force-pushed
the
coverity-7
branch
3 times, most recently
from
November 29, 2021 23:20
0f736b0 to
54f24fb
Compare
aduh95
approved these changes
Dec 3, 2021
aduh95
added
author ready
github-actions
Bot
removed
the
request-ci
Collaborator
Collaborator
This was referenced Dec 4, 2021
Collaborator
This was referenced Dec 8, 2021
Collaborator
Collaborator
Collaborator
Collaborator
aduh95
added
the
commit-queue
nodejs-github-bot
removed
the
commit-queue
Collaborator
|
Landed in 98ec909 |
This was referenced Dec 11, 2021
46 tasks
danielleadams
pushed a commit
that referenced
this pull request
Dec 14, 2021
Coverity reported that the use of sizeof along with pointer arithmetic was likely an error as the pointer arithmetic would already be accounting for the size of what the pointer points to. Looking at the code that looked right but removing the extra sizeOf caused tests to fail. Looking more closely it seems like we were not allocating a big enough buffer but the extra sizeof was allowing us to convert even though it might have been corrupting memory. Signed-off-by: Michael Dawson <mdawson@devrus.com> PR-URL: #41026 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
This was referenced Dec 15, 2021
danielleadams
pushed a commit
that referenced
this pull request
Jan 31, 2022
Coverity reported that the use of sizeof along with pointer arithmetic was likely an error as the pointer arithmetic would already be accounting for the size of what the pointer points to. Looking at the code that looked right but removing the extra sizeOf caused tests to fail. Looking more closely it seems like we were not allocating a big enough buffer but the extra sizeof was allowing us to convert even though it might have been corrupting memory. Signed-off-by: Michael Dawson <mdawson@devrus.com> PR-URL: #41026 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
danielleadams
pushed a commit
that referenced
this pull request
Jan 31, 2022
Coverity reported that the use of sizeof along with pointer arithmetic was likely an error as the pointer arithmetic would already be accounting for the size of what the pointer points to. Looking at the code that looked right but removing the extra sizeOf caused tests to fail. Looking more closely it seems like we were not allocating a big enough buffer but the extra sizeof was allowing us to convert even though it might have been corrupting memory. Signed-off-by: Michael Dawson <mdawson@devrus.com> PR-URL: #41026 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
danielleadams
pushed a commit
that referenced
this pull request
Feb 1, 2022
Coverity reported that the use of sizeof along with pointer arithmetic was likely an error as the pointer arithmetic would already be accounting for the size of what the pointer points to. Looking at the code that looked right but removing the extra sizeOf caused tests to fail. Looking more closely it seems like we were not allocating a big enough buffer but the extra sizeof was allowing us to convert even though it might have been corrupting memory. Signed-off-by: Michael Dawson <mdawson@devrus.com> PR-URL: #41026 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Merged
JosephDoUrden
added a commit
to JosephDoUrden/node
that referenced
this pull request
Aug 30, 2026
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>
JosephDoUrden
added a commit
to JosephDoUrden/node
that referenced
this pull request
Sep 2, 2026
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 String::kMaxLength + 1 UChars, one extra for a leading BOM that the success path strips: a result that overflows the clamped buffer cannot become a V8 string even after the strip, so it is reported as ERR_STRING_TOO_LONG, the same error StringBytes::Encode() throws for oversized results. This decodes every input whose result fits in a V8 string; inputs above ICU's 2 GiB single-call source limit keep their existing error behaviour. 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>
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.
Coverity reported that the use of sizeof along with pointer
arithmetic was likely an error as the pointer arithmetic
would already be accounting for the size of what the
pointer points to.
Looking at the code that looked right but removing the
extra sizeOf caused tests to fail.
Looking more closely it seems like we were not allocating
a big enough buffer but the extra sizeof was allowing
us to convert even though it might have been corrupting
memory.
Signed-off-by: Michael Dawson mdawson@devrus.com