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

buffer: use size_t instead of uint32_t to avoid segmentation fault #48033

Merged
merged 6 commits into from
May 11, 2024

Conversation

Xstoudi
Copy link
Contributor

@Xstoudi Xstoudi commented May 16, 2023

Fixes: #46836

If tests are needed for this, I'll need some guidance about how to do it as I don't know how to test if something produce a segfault or not.

@nodejs-github-bot nodejs-github-bot added buffer Issues and PRs related to the buffer subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels May 16, 2023
@Xstoudi Xstoudi changed the title fix: use size_t instead of uint_32_t to avoid segmentation fault buffer: use size_t instead of uint_32_t to avoid segmentation fault May 16, 2023
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's spelled uint32_t but LGTM apart from that. :-)

I spotted another bug while looking at the surrounding code: that CHECK(dlen >= slen * 2) a few lines up doesn't guard against overflow. Maybe good for a follow-up PR.

@Xstoudi Xstoudi changed the title buffer: use size_t instead of uint_32_t to avoid segmentation fault buffer: use size_t instead of uint32_t to avoid segmentation fault May 16, 2023
@Xstoudi
Copy link
Contributor Author

Xstoudi commented May 16, 2023

Commit message amended.

I could fix that bug directly if needed but I'm not sure to understand in which case would it let an overflow happen?

@bnoordhuis
Copy link
Member

Imagine size_t is 32 bits and slen == 0x80000001, then slen * 2 wraps around to 2.

As a bug it's mostly hypothetical but it's not entirely in the realm of the impossible.

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

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@Xstoudi
Copy link
Contributor Author

Xstoudi commented Jul 22, 2023

Any chance it get merged?

@targos
Copy link
Member

targos commented Jul 24, 2023

@nodejs/cpp-reviewers

@aduh95 aduh95 added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 10, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 10, 2023
@nodejs-github-bot

This comment was marked as outdated.

src/string_bytes.cc Outdated Show resolved Hide resolved
@nodejs-github-bot
Copy link
Collaborator

@aduh95 aduh95 added the request-ci Add this label to start a Jenkins CI on a PR. label May 10, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 10, 2024
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

src/string_bytes.cc Outdated Show resolved Hide resolved
@nodejs-github-bot
Copy link
Collaborator

@aduh95 aduh95 merged commit 05a941f into nodejs:main May 11, 2024
50 checks passed
@aduh95
Copy link
Contributor

aduh95 commented May 11, 2024

Landed in 05a941f

targos pushed a commit that referenced this pull request May 11, 2024
Fixes: #46836
PR-URL: #48033
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: Shelley Vohr <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
marco-ippolito pushed a commit that referenced this pull request Jun 17, 2024
Fixes: #46836
PR-URL: #48033
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: Shelley Vohr <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
marco-ippolito pushed a commit that referenced this pull request Jun 17, 2024
Fixes: #46836
PR-URL: #48033
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: Shelley Vohr <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
marco-ippolito pushed a commit that referenced this pull request Jun 17, 2024
Fixes: #46836
PR-URL: #48033
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: Shelley Vohr <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
EliphazBouye pushed a commit to EliphazBouye/node that referenced this pull request Jun 20, 2024
Fixes: nodejs#46836
PR-URL: nodejs#48033
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: Shelley Vohr <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
bmeck pushed a commit to bmeck/node that referenced this pull request Jun 22, 2024
Fixes: nodejs#46836
PR-URL: nodejs#48033
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: Shelley Vohr <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. buffer Issues and PRs related to the buffer subsystem. c++ Issues and PRs that require attention from people who are familiar with C++.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Integer overflow in StringBytes::Encode hex leads to a crash or an infinite loop