Skip to content

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

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
Open
CGQAQ wants to merge 15 commits into
base: main
Choose a base branch
from
Open

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

CGQAQ 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

nodejs-github-bot commented Aug 20, 2024

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

juanarbol commented Aug 20, 2024

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

nodejs-github-bot commented Aug 20, 2024

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

MoLow
MoLow reviewed Aug 20, 2024
View reviewed changes
@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
@avivkeller avivkeller added the linux Issues and PRs related to the Linux platform. label Aug 20, 2024
avivkeller
avivkeller reviewed Aug 20, 2024
View reviewed changes
@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 avivkeller August 21, 2024 04:41
avivkeller
avivkeller reviewed Aug 21, 2024
View reviewed changes
@CGQAQ CGQAQ requested a review from avivkeller August 22, 2024 01:02
avivkeller
avivkeller reviewed Aug 22, 2024
View reviewed changes
Copy link
Member

@avivkeller avivkeller 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)

@avivkeller avivkeller 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

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

@avivkeller avivkeller avivkeller left review comments

@MoLow MoLow Awaiting requested review from MoLow

@anonrig anonrig Awaiting requested review from anonrig

At least 1 approving review is required to merge this pull request.

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 @avivkeller