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

fs: fix readdir and opendir recursive with unknown file types #49603

Merged
merged 1 commit into from
Sep 11, 2023
Merged

fs: fix readdir and opendir recursive with unknown file types #49603

merged 1 commit into from
Sep 11, 2023

Conversation

lux01
Copy link
Contributor

@lux01 lux01 commented Sep 11, 2023

If the libuv operations invoked by readdir/opendir return uv_dirent_t values where the type is UV_DIRENT_UNKNOWN then a further lstat is issued to fully construct the Dirent values. In the recursive versions of these functions, the path parameter was incorrectly assumed to be the path to the entry when it should be the path to the directory containing the entry.

Fixes #49499.

@nodejs-github-bot nodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Sep 11, 2023
@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Sep 11, 2023
If the libuv operations invoked by `readdir`/`opendir` return
`uv_dirent_t` values where the `type` is `UV_DIRENT_UNKNOWN` then a
further `lstat` is issued to fully construct the `Dirent` values. In the
recursive versions of these functions, the `path` parameter was
incorrectly assumed to be the path to the entry when it should be the
path to the directory containing the entry.

Fixes #49499.
@richardlau richardlau added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 11, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 11, 2023
@nodejs-github-bot
Copy link
Collaborator

@anonrig
Copy link
Member

anonrig commented Sep 11, 2023

cc @Ethan-Arrowood

Copy link
Contributor

@Ethan-Arrowood Ethan-Arrowood left a comment

Choose a reason for hiding this comment

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

lgtm

@Ethan-Arrowood
Copy link
Contributor

Great investigation and fix @lux01 . Thank you very much!

Copy link
Member

@anonrig anonrig left a comment

Choose a reason for hiding this comment

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

lgtm

@MoLow
Copy link
Member

MoLow commented Sep 11, 2023

@ruyadorno this seems like a serious bug and is ready for landing, perhaps it is a good idea to land this into v18?

@MoLow MoLow added fast-track PRs that do not need to wait for 48 hours to land. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Sep 11, 2023
@github-actions
Copy link
Contributor

Fast-track has been requested by @MoLow. Please 👍 to approve.

@lux01
Copy link
Contributor Author

lux01 commented Sep 11, 2023

To fix this on the v18.x branch I needed #48698 so that would need back porting too (if it's not already marked as such).

@MoLow MoLow added the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 11, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 11, 2023
@nodejs-github-bot nodejs-github-bot merged commit d8b378c into nodejs:main Sep 11, 2023
33 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in d8b378c

@ruyadorno
Copy link
Member

@MoLow it hasn't landed on time yet for the v20 release and I prefer not to rush it to v18 before it's out in a current release line. 😊 thanks for the heads up though!

ruyadorno pushed a commit that referenced this pull request Sep 28, 2023
If the libuv operations invoked by `readdir`/`opendir` return
`uv_dirent_t` values where the `type` is `UV_DIRENT_UNKNOWN` then a
further `lstat` is issued to fully construct the `Dirent` values. In the
recursive versions of these functions, the `path` parameter was
incorrectly assumed to be the path to the entry when it should be the
path to the directory containing the entry.

Fixes #49499.

PR-URL: #49603
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
This was referenced Sep 28, 2023
alexfernandez pushed a commit to alexfernandez/node that referenced this pull request Nov 1, 2023
If the libuv operations invoked by `readdir`/`opendir` return
`uv_dirent_t` values where the `type` is `UV_DIRENT_UNKNOWN` then a
further `lstat` is issued to fully construct the `Dirent` values. In the
recursive versions of these functions, the `path` parameter was
incorrectly assumed to be the path to the entry when it should be the
path to the directory containing the entry.

Fixes nodejs#49499.

PR-URL: nodejs#49603
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
@aduh95 aduh95 added the dont-land-on-v18.x PRs that should not land on the v18.x-staging branch and should not be released in v18.x. label Dec 3, 2023
@aduh95
Copy link
Contributor

aduh95 commented Dec 3, 2023

This introduced a breaking change for dirent.path (and should have been marked as semver-major PRs that contain breaking changes and should be released in the next major version. IMO), marking it as dont-land-on-v18.x.

sonsurim pushed a commit to sonsurim/node that referenced this pull request Jul 21, 2024
An error occurred in the test/sequential/test-fs-opendir-recursive.js file
after changing the code. This was resolved by referring to the code resolved
in PR nodejs#49603.

Refs: nodejs#49603
sonsurim pushed a commit to sonsurim/node that referenced this pull request Jul 21, 2024
An error occurred in the test/sequential/test-fs-opendir-recursive.js file
after changing the code. This was resolved by referring to the code resolved
in PR nodejs#49603.

Refs: nodejs#49603
sonsurim pushed a commit to sonsurim/node that referenced this pull request Jul 21, 2024
An error occurred in the test/sequential/test-fs-opendir-recursive.js file
after changing the code. This was resolved by referring to the code resolved
in PR nodejs#49603.

Refs: nodejs#49603
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. dont-land-on-v18.x PRs that should not land on the v18.x-staging branch and should not be released in v18.x. fast-track PRs that do not need to wait for 48 hours to land. fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Regression in fs.readdirSync on AIX introduced in 18.17.0
8 participants