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: fix spawnSync CHECK when SIGKILL fails #31768

Closed
wants to merge 1 commit into from

Conversation

bnoordhuis
Copy link
Member

We might not have sufficient privileges to signal the child process
so don't make assumptions about the return value of uv_process_kill().

Example:

node -e 'child_process.spawnSync("sudo", ["ls"], { maxBuffer: 1 })'

No test because:

  1. The test needs to run as root (can't invoke sudo), and

  2. The parent needs to drop privileges but can't, because
    then the child process won't have sufficient privileges.

Fixes: #31747

We might not have sufficient privileges to signal the child process
so don't make assumptions about the return value of `uv_process_kill()`.

Example:

    node -e 'child_process.spawnSync("sudo", ["ls"], { maxBuffer: 1 })'

No test because:

1. The test needs to run as root (can't invoke sudo), and

2. The parent needs to drop privileges but can't, because
   then the child process won't have sufficient privileges.

Fixes: nodejs#31747
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. child_process Issues and PRs related to the child_process subsystem. labels Feb 13, 2020
@bnoordhuis bnoordhuis added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 15, 2020
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

addaleax pushed a commit that referenced this pull request Mar 11, 2020
We might not have sufficient privileges to signal the child process
so don't make assumptions about the return value of `uv_process_kill()`.

Example:

    node -e 'child_process.spawnSync("sudo", ["ls"], { maxBuffer: 1 })'

No test because:

1. The test needs to run as root (can't invoke sudo), and

2. The parent needs to drop privileges but can't, because
   then the child process won't have sufficient privileges.

Fixes: #31747

PR-URL: #31768
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: David Carlier <[email protected]>
@addaleax
Copy link
Member

Landed in d09e1da

@addaleax addaleax closed this Mar 11, 2020
MylesBorins pushed a commit that referenced this pull request Mar 11, 2020
We might not have sufficient privileges to signal the child process
so don't make assumptions about the return value of `uv_process_kill()`.

Example:

    node -e 'child_process.spawnSync("sudo", ["ls"], { maxBuffer: 1 })'

No test because:

1. The test needs to run as root (can't invoke sudo), and

2. The parent needs to drop privileges but can't, because
   then the child process won't have sufficient privileges.

Fixes: #31747

PR-URL: #31768
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: David Carlier <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Mar 12, 2020
targos pushed a commit that referenced this pull request Apr 22, 2020
We might not have sufficient privileges to signal the child process
so don't make assumptions about the return value of `uv_process_kill()`.

Example:

    node -e 'child_process.spawnSync("sudo", ["ls"], { maxBuffer: 1 })'

No test because:

1. The test needs to run as root (can't invoke sudo), and

2. The parent needs to drop privileges but can't, because
   then the child process won't have sufficient privileges.

Fixes: #31747

PR-URL: #31768
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: David Carlier <[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. c++ Issues and PRs that require attention from people who are familiar with C++. child_process Issues and PRs related to the child_process subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

child_process: spawnSync crashes trying to terminate setuid child because of maxBuffer exceeded
7 participants