Skip to content

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

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
Merged
aduh95 merged 6 commits into from
May 11, 2024
Merged

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

aduh95 merged 6 commits into from
May 11, 2024

Conversation

@Xstoudi
Copy link
Contributor

@XstoudiXstoudi 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-botnodejs-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
@XstoudiXstoudiforce-pushed the fix/segfault-hex-encode branch from 1c62637 to ed1b2fdCompareMay 16, 2023 11:53
@XstoudiXstoudi changed the title fix: use size_t instead of uint_32_t to avoid segmentation faultbuffer: use size_t instead of uint_32_t to avoid segmentation faultMay 16, 2023
bnoordhuis
bnoordhuis approved these changes May 16, 2023
View reviewed changes
Copy link
Member

@bnoordhuisbnoordhuis 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.

@XstoudiXstoudiforce-pushed the fix/segfault-hex-encode branch from ed1b2fd to 73805bbCompareMay 16, 2023 20:50
@XstoudiXstoudi changed the title buffer: use size_t instead of uint_32_t to avoid segmentation faultbuffer: use size_t instead of uint32_t to avoid segmentation faultMay 16, 2023
@Xstoudi
Copy link
ContributorAuthor

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

bnoordhuis commented May 17, 2023

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.

@bnoordhuisbnoordhuis 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-actionsgithub-actionsbot 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-actionsgithub-actionsbot mentioned this pull request May 18, 2023
Open
20 tasks
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@github-actionsgithub-actionsbot mentioned this pull request May 19, 2023
Open
21 tasks
This was referenced May 20, 2023
Open
Open
Open
Open
Open
@Xstoudi
Copy link
ContributorAuthor

Xstoudi commented Jul 22, 2023

Any chance it get merged?

@targos
Copy link
Member

targos commented Jul 24, 2023

@nodejs/cpp-reviewers

@aduh95aduh95 added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 10, 2023
@github-actionsgithub-actionsbot 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
@github-actionsgithub-actionsbot mentioned this pull request Aug 11, 2023
Open
29 tasks
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Dec 23, 2023

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

@aduh95aduh95 added the request-ci Add this label to start a Jenkins CI on a PR. label May 10, 2024
@github-actionsgithub-actionsbot 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 commented May 10, 2024

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

@aduh95aduh95force-pushed the fix/segfault-hex-encode branch from e7ba2bf to d80d6caCompareMay 11, 2024 09:16
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented May 11, 2024

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

aduh95
aduh95 reviewed May 11, 2024
View reviewed changes
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented May 11, 2024

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

@aduh95aduh95 merged commit 05a941f into nodejs:mainMay 11, 2024
@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 <[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]>
@targostargos 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 <[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-ippolitomarco-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 <[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
@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 <[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]>
ebouye pushed a commit to ebouye/node that referenced this pull request Jun 20, 2024
@Xstoudi@ebouye
buffer: use size_t instead of uint32_t to avoid segmentation fault
6a50607
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
@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 <[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 freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tniessentniessentniessen left review comments

@aduh95aduh95aduh95 left review comments

@QardQardQard approved these changes

@bnoordhuisbnoordhuisbnoordhuis approved these changes

@jasnelljasnelljasnell approved these changes

@lpincalpincalpinca approved these changes

@codebyterecodebyterecodebytere approved these changes

@targostargostargos approved these changes

@JungMinuJungMinuJungMinu approved these changes

@RaisinTenRaisinTenRaisinTen approved these changes

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.bufferIssues 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

12 participants

@Xstoudi@bnoordhuis@nodejs-github-bot@targos@aduh95@Qard@jasnell@lpinca@codebytere@tniessen@JungMinu@RaisinTen