GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — buffer: use size_t instead of uint32_t to avoid segmentation fault by Xstoudi · Pull Request #48033 · nodejs/node

Conversation

@Xstoudi

@Xstoudi Xstoudi commented May 16, 2023

Copy link
Copy Markdown
Contributor

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bricss reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@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

@bnoordhuis bnoordhuis left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@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

Xstoudi commented May 16, 2023

Copy link
Copy Markdown
Contributor Author

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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bnoordhuis

bnoordhuis commented May 17, 2023

Copy link
Copy Markdown
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.
Xstoudi reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bnoordhuis bnoordhuis added request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. 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. Only starts once the PR has an approving review. label May 17, 2023
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
lpinca
lpinca approved these changes May 17, 2023
View reviewed changes
targos
targos approved these changes May 17, 2023
View reviewed changes
jasnell
jasnell approved these changes May 17, 2023
View reviewed changes
@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
CI Reliability 2023-05-18 nodejs/reliability#569
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
CI Reliability 2023-05-19 nodejs/reliability#570
Open
21 tasks
JungMinu
JungMinu approved these changes May 19, 2023
View reviewed changes
This was referenced May 20, 2023
CI Reliability 2023-05-20 nodejs/reliability#571
Open
CI Reliability 2023-05-21 nodejs/reliability#572
Open
CI Reliability 2023-05-22 nodejs/reliability#573
Open
CI Reliability 2023-05-23 nodejs/reliability#574
Open
CI Reliability 2023-05-24 nodejs/reliability#575
Open
@Xstoudi

Xstoudi commented Jul 22, 2023

Copy link
Copy Markdown
Contributor Author

Any chance it get merged?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@targos

targos commented Jul 24, 2023

Copy link
Copy Markdown
Member

@nodejs/cpp-reviewers
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
codebytere
codebytere approved these changes Jul 24, 2023
View reviewed changes
@aduh95 aduh95 added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Aug 10, 2023
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. 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
Comment thread src/string_bytes.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.
@github-actions github-actions Bot mentioned this pull request Aug 11, 2023
CI Reliability 2023-08-11 nodejs/reliability#634
Open
1 of 29 tasks
@nodejs-github-bot

nodejs-github-bot commented Dec 23, 2023

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Qard
Qard approved these changes Dec 24, 2023
View reviewed changes
@aduh95 aduh95 added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label May 10, 2024
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label May 10, 2024
@nodejs-github-bot

nodejs-github-bot commented May 10, 2024

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Xstoudi and others added 5 commits May 11, 2024 12:16
@Xstoudi @aduh95
buffer: use size_t instead of uint32_t to avoid segmentation fault
ef83723 
Fixes: nodejs#46836
@Xstoudi @aduh95
fix: fix another potential overflow
855fd36
@Xstoudi @aduh95
fix: remove blankline
ca4471a
@Xstoudi @aduh95
fix: format cpp
98d8590
@jasnell @tniessen @aduh95
Apply suggestions from code review
d80d6ca 
Co-authored-by: Tobias Nießen <[email protected]>
@aduh95
aduh95 force-pushed the fix/segfault-hex-encode branch from e7ba2bf to d80d6ca Compare May 11, 2024 09:16
@nodejs-github-bot

nodejs-github-bot commented May 11, 2024

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
aduh95
aduh95 reviewed May 11, 2024
View reviewed changes
Comment thread src/string_bytes.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.
@aduh95
format_cpp
62f99dc
@nodejs-github-bot

nodejs-github-bot commented May 11, 2024

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@aduh95
aduh95 merged commit 05a941f into nodejs:main May 11, 2024
@aduh95

aduh95 commented May 11, 2024

Copy link
Copy Markdown
Contributor

Landed in 05a941f
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
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]>
@targos targos mentioned this pull request May 13, 2024
v22.2.0 release proposal #52971
Merged
@marco-ippolito marco-ippolito mentioned this pull request Jun 17, 2024
v20.15.0 proposal #53486
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
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]>
soophoo pushed a commit to soophoo/node that referenced this pull request Jun 20, 2024
@Xstoudi @soophoo
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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tniessen tniessen tniessen left review comments
@aduh95 aduh95 aduh95 left review comments
@Qard Qard Qard approved these changes
@bnoordhuis bnoordhuis bnoordhuis approved these changes
@jasnell jasnell jasnell approved these changes
@lpinca lpinca lpinca approved these changes
@codebytere codebytere codebytere approved these changes
@JungMinu JungMinu JungMinu approved these changes
@RaisinTen RaisinTen RaisinTen approved these changes
+1 more reviewer
@targos targos targos approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. 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

Uh oh!

There was an error while loading. Please reload this page.

12 participants

@Xstoudi @bnoordhuis @nodejs-github-bot @targos @aduh95 @Qard @jasnell @lpinca @codebytere @tniessen @JungMinu @RaisinTen
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.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.