Skip to content
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

src: replace unreachable code with static_assert #46209

Merged

Conversation

tniessen
Copy link
Member

This function base64-decodes a given JavaScript string to obtain the secret key, whose length must not exceed INT_MAX. However, because JavaScript strings are limited to v8::String::kMaxLength chars and because base64 decoding never yields more bytes than input chars, the size of the decoded key must be strictly less than v8::String::kMaxLength bytes. Therefore, it is sufficient to statically assert that String::kMaxLength <= INT_MAX (which is always true because String::kMaxLength itself is an int).

Aside from being unreachable, Coverity considers the current code "suspicious" because it indicates that buffers larger than INT_MAX might actually be allocated.

This function base64-decodes a given JavaScript string to obtain the
secret key, whose length must not exceed INT_MAX. However, because
JavaScript strings are limited to v8::String::kMaxLength chars and
because base64 decoding never yields more bytes than input chars, the
size of the decoded key must be strictly less than
v8::String::kMaxLength bytes. Therefore, it is sufficient to statically
assert that String::kMaxLength <= INT_MAX (which is always true because
String::kMaxLength itself is an int).

Aside from being unreachable, Coverity considers the current code
"suspicious" because it indicates that buffers larger than INT_MAX might
actually be allocated.
@tniessen tniessen added crypto Issues and PRs related to the crypto subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. labels Jan 14, 2023
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/crypto

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Jan 14, 2023
@panva panva added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 17, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 17, 2023
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot
Copy link
Collaborator

@panva panva added the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 17, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 17, 2023
@nodejs-github-bot nodejs-github-bot merged commit 15d673d into nodejs:main Jan 17, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in 15d673d

RafaelGSS pushed a commit to RafaelGSS/node that referenced this pull request Jan 17, 2023
This function base64-decodes a given JavaScript string to obtain the
secret key, whose length must not exceed INT_MAX. However, because
JavaScript strings are limited to v8::String::kMaxLength chars and
because base64 decoding never yields more bytes than input chars, the
size of the decoded key must be strictly less than
v8::String::kMaxLength bytes. Therefore, it is sufficient to statically
assert that String::kMaxLength <= INT_MAX (which is always true because
String::kMaxLength itself is an int).

Aside from being unreachable, Coverity considers the current code
"suspicious" because it indicates that buffers larger than INT_MAX might
actually be allocated.

PR-URL: nodejs#46209
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
tniessen added a commit to tniessen/node that referenced this pull request Jan 18, 2023
This function divides an unsigned 32-bit integer by 8, effectively
right-shifting it by three bits, so the result must be less than
INT_MAX.

Refs: nodejs#46209
tniessen added a commit to tniessen/node that referenced this pull request Jan 18, 2023
This function divides an unsigned 32-bit integer by 8, effectively
right-shifting it by three bits, so the result must be less than
INT_MAX.

Refs: nodejs#46209
RafaelGSS pushed a commit that referenced this pull request Jan 20, 2023
This function base64-decodes a given JavaScript string to obtain the
secret key, whose length must not exceed INT_MAX. However, because
JavaScript strings are limited to v8::String::kMaxLength chars and
because base64 decoding never yields more bytes than input chars, the
size of the decoded key must be strictly less than
v8::String::kMaxLength bytes. Therefore, it is sufficient to statically
assert that String::kMaxLength <= INT_MAX (which is always true because
String::kMaxLength itself is an int).

Aside from being unreachable, Coverity considers the current code
"suspicious" because it indicates that buffers larger than INT_MAX might
actually be allocated.

PR-URL: #46209
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
@RafaelGSS RafaelGSS mentioned this pull request Jan 20, 2023
nodejs-github-bot pushed a commit that referenced this pull request Jan 21, 2023
This function divides an unsigned 32-bit integer by 8, effectively
right-shifting it by three bits, so the result must be less than
INT_MAX.

Refs: #46209
PR-URL: #46250
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
juanarbol pushed a commit that referenced this pull request Jan 26, 2023
This function base64-decodes a given JavaScript string to obtain the
secret key, whose length must not exceed INT_MAX. However, because
JavaScript strings are limited to v8::String::kMaxLength chars and
because base64 decoding never yields more bytes than input chars, the
size of the decoded key must be strictly less than
v8::String::kMaxLength bytes. Therefore, it is sufficient to statically
assert that String::kMaxLength <= INT_MAX (which is always true because
String::kMaxLength itself is an int).

Aside from being unreachable, Coverity considers the current code
"suspicious" because it indicates that buffers larger than INT_MAX might
actually be allocated.

PR-URL: #46209
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
@juanarbol juanarbol mentioned this pull request Jan 28, 2023
juanarbol pushed a commit that referenced this pull request Jan 31, 2023
This function base64-decodes a given JavaScript string to obtain the
secret key, whose length must not exceed INT_MAX. However, because
JavaScript strings are limited to v8::String::kMaxLength chars and
because base64 decoding never yields more bytes than input chars, the
size of the decoded key must be strictly less than
v8::String::kMaxLength bytes. Therefore, it is sufficient to statically
assert that String::kMaxLength <= INT_MAX (which is always true because
String::kMaxLength itself is an int).

Aside from being unreachable, Coverity considers the current code
"suspicious" because it indicates that buffers larger than INT_MAX might
actually be allocated.

PR-URL: #46209
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
ruyadorno pushed a commit that referenced this pull request Feb 1, 2023
This function divides an unsigned 32-bit integer by 8, effectively
right-shifting it by three bits, so the result must be less than
INT_MAX.

Refs: #46209
PR-URL: #46250
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
juanarbol pushed a commit that referenced this pull request Mar 3, 2023
This function divides an unsigned 32-bit integer by 8, effectively
right-shifting it by three bits, so the result must be less than
INT_MAX.

Refs: #46209
PR-URL: #46250
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
juanarbol pushed a commit that referenced this pull request Mar 5, 2023
This function divides an unsigned 32-bit integer by 8, effectively
right-shifting it by three bits, so the result must be less than
INT_MAX.

Refs: #46209
PR-URL: #46250
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants