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.

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

lib: fix realpathSync resolving to invalid path (#54200) #54458

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

lib: fix realpathSync resolving to invalid path (#54200) #54458

wants to merge 15 commits into from

Conversation

CGQAQ
Copy link
Contributor

@CGQAQ CGQAQ commented Aug 20, 2024 •
edited
Loading

This PR fixes that fs.realpathSync will resolve to invalid path in some situation, specifically the input is /dev/stdin on Linux

Fixed: #54200

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/startup

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Aug 20, 2024
@CGQAQ CGQAQ changed the title fix: can't execute /dev/stdin on linux (#54200) cli: fix can't execute /dev/stdin on linux (#54200) Aug 20, 2024
@juanarbol juanarbol added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 20, 2024
@juanarbol
Copy link
Member

Hey! Thanks!

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 20, 2024
@nodejs-github-bot
Copy link
Collaborator

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

MoLow
MoLow reviewed Aug 20, 2024
View reviewed changes
src/node.cc Outdated Show resolved Hide resolved
@codecov Codecov
Copy link

codecov bot commented Aug 20, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.33%. Comparing base (561bc87) to head (d6063ae).
Report is 25 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff            @@
##             main   #54458    +/-   ##
========================================
  Coverage   87.32%   87.33%            
========================================
  Files         648      649     +1     
  Lines      182359   182611   +252     
  Branches    34981    35038    +57     
========================================
+ Hits       159239   159475   +236     
- Misses      16386    16396    +10     
- Partials     6734     6740     +6
Files Coverage Δ
lib/fs.js 93.28% <100.00%> (+0.01%) ⬆️

... and 49 files with indirect coverage changes

@CGQAQ CGQAQ requested a review from MoLow August 20, 2024 05:55
@RedYetiDev RedYetiDev added the linux Issues and PRs related to the Linux platform. label Aug 20, 2024
RedYetiDev
RedYetiDev reviewed Aug 20, 2024
View reviewed changes
src/node.cc Outdated Show resolved Hide resolved
@CGQAQ CGQAQ changed the title cli: fix can't execute /dev/stdin on linux (#54200) lib: fix can't execute /dev/stdin on linux (#54200) Aug 21, 2024
@CGQAQ CGQAQ changed the title lib: fix can't execute /dev/stdin on linux (#54200) lib: fix realpathSync resolving to invalid path (#54200) Aug 21, 2024
@CGQAQ CGQAQ requested a review from RedYetiDev August 21, 2024 04:41
RedYetiDev
RedYetiDev reviewed Aug 21, 2024
View reviewed changes
test/parallel/test-linux-dev-stdin.js Outdated Show resolved Hide resolved
test/parallel/test-linux-dev-stdin.js Outdated Show resolved Hide resolved
test/parallel/test-linux-dev-stdin.js Outdated Show resolved Hide resolved
test/parallel/test-linux-dev-stdin.js Outdated Show resolved Hide resolved
@CGQAQ CGQAQ requested a review from RedYetiDev August 22, 2024 01:02
RedYetiDev
RedYetiDev reviewed Aug 22, 2024
View reviewed changes
Copy link
Member

@RedYetiDev RedYetiDev left a comment

Choose a reason for hiding this comment

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

👍!

A few tiny nitpicks, but otherwise I think this looks good (but I'm not a core collaborator)

lib/fs.js Outdated Show resolved Hide resolved
lib/fs.js Outdated Show resolved Hide resolved
test/parallel/test-linux-dev-stdin.js Outdated Show resolved Hide resolved
@RedYetiDev RedYetiDev added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Aug 22, 2024
@jasnell jasnell requested a review from anonrig September 8, 2024 04:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RedYetiDev RedYetiDev RedYetiDev left review comments

@Ethan-Arrowood Ethan-Arrowood Ethan-Arrowood left review comments

@MoLow MoLow Awaiting requested review from MoLow

@anonrig anonrig Awaiting requested review from anonrig

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. linux Issues and PRs related to the Linux platform. needs-ci PRs that need a full CI run.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

can't execute /dev/stdin on linux
6 participants
@CGQAQ @nodejs-github-bot @juanarbol @MoLow @Ethan-Arrowood @RedYetiDev