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,lib: handle invalid stdio configuration gracefully #55942

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

juanarbol
Copy link
Member

Fixes an issue where malformed or unexpected stdio configurations could cause crashes or undefined behavior during child process spawning. This patch ensures robust validation of stdio entries:

Fixes: #55932


I don't think we should patch user-space misconfiguration. This is a rare case and, could be extrapolated to different issues. I never faced an issue due to proto mutation. But I personally don't think we should patch that (except for the native layer.)

Fixes an issue where malformed or unexpected stdio configurations
could cause crashes or undefined behavior during child process
spawning. This patch ensures robust validation of stdio entries:

Fixes: nodejs#55932
Signed-off-by: Juan José Arboleda <soyjuanarbol@gmail.com>
@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. needs-ci PRs that need a full CI run. labels Nov 21, 2024
Copy link

codecov bot commented Nov 21, 2024

Codecov Report

Attention: Patch coverage is 69.23077% with 4 lines in your changes missing coverage. Please review.

Project coverage is 88.00%. Comparing base (f270462) to head (e079e6e).
Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
lib/internal/child_process.js 42.85% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #55942      +/-   ##
==========================================
- Coverage   88.49%   88.00%   -0.50%     
==========================================
  Files         653      653              
  Lines      187728   187741      +13     
  Branches    36182    35875     -307     
==========================================
- Hits       166136   165221     -915     
- Misses      14814    15700     +886     
- Partials     6778     6820      +42     
Files with missing lines Coverage Δ
src/process_wrap.cc 84.81% <100.00%> (+0.49%) ⬆️
lib/internal/child_process.js 93.87% <42.85%> (-1.11%) ⬇️

... and 91 files with indirect coverage changes

---- 🚨 Try these New Features:

return;
}

// Refs: https://github.com/nodejs/node/issues/55932
Copy link
Member

Choose a reason for hiding this comment

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

In addition to the link, a short description of the issue being tested would be helpful so folks don't have to actually follow the link to see what this is about.

@targos
Copy link
Member

targos commented Nov 24, 2024

I'm fine with improving robustness wrt userland prototype changes, but I don't think we should add tests for that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FATAL ERROR: v8::ToLocalChecked Empty MaybeLocal
4 participants