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: limit GetProcessTitle() result to 1MB #35492

Closed
wants to merge 2 commits into from

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Oct 4, 2020

GetProcessTitle() otherwise runs an infinite loop when
uv_setup_argv() has not been called (yet). This is a problem
e.g. in assertions from static constructors, which run before
main() and thus before argc and argv become available.

To solve that, do not allocate more than 1MB of storage for the
title and bail out if we reach that point.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

`GetProcessTitle()` otherwise runs an infinite loop when
`uv_setup_argv()` has not been called (yet). This is a problem
e.g. in assertions from static constructors, which run before
`main()` and thus before `argc` and `argv` become available.

To solve that, do not allocate more than 1MB of storage for the
title and bail out if we reach that point.
@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Oct 4, 2020
@addaleax addaleax added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Oct 4, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 4, 2020
@nodejs-github-bot
Copy link
Collaborator

Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

LGTM. An accompanying comment might be helpful though.

@addaleax
Copy link
Member Author

addaleax commented Oct 4, 2020

@cjihrig Added a comment 👍

@addaleax addaleax added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 4, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 4, 2020
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

addaleax added a commit that referenced this pull request Oct 6, 2020
`GetProcessTitle()` otherwise runs an infinite loop when
`uv_setup_argv()` has not been called (yet). This is a problem
e.g. in assertions from static constructors, which run before
`main()` and thus before `argc` and `argv` become available.

To solve that, do not allocate more than 1MB of storage for the
title and bail out if we reach that point.

PR-URL: #35492
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@addaleax
Copy link
Member Author

addaleax commented Oct 6, 2020

Landed in 18af0b0

@addaleax addaleax closed this Oct 6, 2020
@addaleax addaleax deleted the process-title-bailout branch October 6, 2020 08:30
@danielleadams danielleadams mentioned this pull request Oct 6, 2020
danielleadams pushed a commit that referenced this pull request Oct 6, 2020
`GetProcessTitle()` otherwise runs an infinite loop when
`uv_setup_argv()` has not been called (yet). This is a problem
e.g. in assertions from static constructors, which run before
`main()` and thus before `argc` and `argv` become available.

To solve that, do not allocate more than 1MB of storage for the
title and bail out if we reach that point.

PR-URL: #35492
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
joesepi pushed a commit to joesepi/node that referenced this pull request Jan 8, 2021
`GetProcessTitle()` otherwise runs an infinite loop when
`uv_setup_argv()` has not been called (yet). This is a problem
e.g. in assertions from static constructors, which run before
`main()` and thus before `argc` and `argv` become available.

To solve that, do not allocate more than 1MB of storage for the
title and bail out if we reach that point.

PR-URL: nodejs#35492
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
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. 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.

7 participants