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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 6 commits into from
May 11, 2024
Merged

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

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 force-pushed the fix/segfault-hex-encode branch from 1c62637 to ed1b2fd Compare May 16, 2023 11:53
@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
bnoordhuis
bnoordhuis approved these changes May 16, 2023
View reviewed changes
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 force-pushed the fix/segfault-hex-encode branch from ed1b2fd to 73805bb Compare May 16, 2023 20:50
@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.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@github-actions github-actions bot mentioned this pull request May 18, 2023
Open
20 tasks
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@github-actions github-actions bot mentioned this pull request May 19, 2023
Open
21 tasks
This was referenced May 20, 2023
Open
Open
Open
Open
Open
@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.

Sign in to view
tniessen
tniessen reviewed Aug 10, 2023
View reviewed changes
src/string_bytes.cc Outdated Show resolved Hide resolved
@github-actions github-actions bot mentioned this pull request Aug 11, 2023
Open
29 tasks
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/56485/

@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

CI: https://ci.nodejs.org/job/node-test-pull-request/59081/

@aduh95 aduh95 force-pushed the fix/segfault-hex-encode branch from e7ba2bf to d80d6ca Compare May 11, 2024 09:16
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/59090/

aduh95
aduh95 reviewed May 11, 2024
View reviewed changes
src/string_bytes.cc Outdated Show resolved Hide resolved
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/59091/

@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
@Xstoudi @targos
buffer: use size_t instead of uint32_t to avoid segmentation fault
a5d63f9 
Fixes: #46836
PR-URL: #48033
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Reviewed-By: Shelley Vohr <shelley.vohr@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
@targos targos mentioned this pull request May 13, 2024
Merged
marco-ippolito pushed a commit that referenced this pull request Jun 17, 2024
@Xstoudi @marco-ippolito
buffer: use size_t instead of uint32_t to avoid segmentation fault
baa6b55 
Fixes: #46836
PR-URL: #48033
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Reviewed-By: Shelley Vohr <shelley.vohr@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
@marco-ippolito marco-ippolito mentioned this pull request Jun 17, 2024
Merged
marco-ippolito pushed a commit that referenced this pull request Jun 17, 2024
@Xstoudi @marco-ippolito
buffer: use size_t instead of uint32_t to avoid segmentation fault
67b157a 
Fixes: #46836
PR-URL: #48033
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Reviewed-By: Shelley Vohr <shelley.vohr@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
marco-ippolito pushed a commit that referenced this pull request Jun 17, 2024
@Xstoudi @marco-ippolito
buffer: use size_t instead of uint32_t to avoid segmentation fault
8d628c3 
Fixes: #46836
PR-URL: #48033
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Reviewed-By: Shelley Vohr <shelley.vohr@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
sophoniie pushed a commit to sophoniie/node that referenced this pull request Jun 20, 2024
@Xstoudi @sophoniie
buffer: use size_t instead of uint32_t to avoid segmentation fault
6a50607 
Fixes: nodejs#46836
PR-URL: nodejs#48033
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Reviewed-By: Shelley Vohr <shelley.vohr@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
bmeck pushed a commit to bmeck/node that referenced this pull request Jun 22, 2024
@Xstoudi @bmeck
buffer: use size_t instead of uint32_t to avoid segmentation fault
89ad851 
Fixes: nodejs#46836
PR-URL: nodejs#48033
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Reviewed-By: Shelley Vohr <shelley.vohr@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aduh95 aduh95 aduh95 left review comments

@tniessen tniessen tniessen left review comments

@RaisinTen RaisinTen RaisinTen approved these changes

@JungMinu JungMinu JungMinu approved these changes

@targos targos targos approved these changes

@codebytere codebytere codebytere approved these changes

@lpinca lpinca lpinca approved these changes

@jasnell jasnell jasnell approved these changes

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@Qard Qard Qard approved these changes

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

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