FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fs: move `ToNamespacedPath` to c++ by anonrig · Pull Request #52135 · nodejs/node · GitHub

/ node Public

fs: move ToNamespacedPath to c++ - #52135

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
anonrig:simplify-fs
Jun 20, 2024
ConversationCommits (1)ChecksFiles changed
Merged

fs: move ToNamespacedPath to c++#52135
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
anonrig:simplify-fs

Conversation

anonrig commented Mar 18, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

This is a step forward to move all node:fs implementation to C++. Since we now have support for win32.pathResolve() in C++, we can move forward with moving ToNamespacedPath functions to C++.

In a follow up pull-request, I'll move FromNamespacedPath() from node_url.cc to path.cc

cc @nodejs/fs @RafaelGSS

nodejs-github-bot commented Mar 18, 2024

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/url

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Mar 18, 2024
anonrig force-pushed the simplify-fs branch 2 times, most recently from 003590b to 2d120ca Compare March 18, 2024 15:16
richardlau added fs Issues and PRs related to the fs subsystem / file system. windows Issues and PRs related to the Windows platform. labels Mar 18, 2024

anonrig commented Mar 18, 2024

Copy link
Copy Markdown
Member Author

cc @nodejs/platform-windows Any idea where this error/warning is thrown from?

lemire reviewed Mar 18, 2024
View reviewed changes
Comment thread src/path.cc Outdated
lemire reviewed Mar 18, 2024
View reviewed changes
Comment thread src/path.cc Outdated

LiviaMedeiros commented Mar 18, 2024

Copy link
Copy Markdown
Member

AFAICT the changes are implying that getValidatedPath() should do path.ToNamespacedPath() internally, but there's no changes to getValidatedPath() itself in this PR. Does it happen implicitly? If yes, on which step?

lemire reviewed Mar 18, 2024
View reviewed changes
Comment thread src/path.cc Outdated
lemire reviewed Mar 18, 2024
View reviewed changes
Comment thread src/path.h Outdated
lemire reviewed Mar 18, 2024
View reviewed changes
Comment thread src/path.cc Outdated

anonrig commented Mar 18, 2024

Copy link
Copy Markdown
Member Author

AFAICT the changes are implying that getValidatedPath() should do path.ToNamespacedPath() internally, but there's no changes to getValidatedPath() itself in this PR. Does it happen implicitly? If yes, on which step?

getValidatedPath() result is called with toNamespacedPath() to ensure Windows long paths are handled correctly. I didn't quite understand your comment, but this pull request basically replaces toNamespacedPath(getValidatedPath(input)) with getValidatedPath(input) and calls toNamespacedPath from C++

anonrig force-pushed the simplify-fs branch 2 times, most recently from d93f610 to 5c62bf2 Compare March 18, 2024 18:08

LiviaMedeiros commented Mar 18, 2024

Copy link
Copy Markdown
Member

getValidatedPath() result is called with toNamespacedPath() to ensure Windows long paths are handled correctly. I didn't quite understand your comment, but this pull request basically replaces toNamespacedPath(getValidatedPath(input)) with getValidatedPath(input) and calls toNamespacedPath from C++

Sorry, I read the changes wrong. It doesn't affect js-side toNamespacedPath() itself but adds cpp-side version that is used in cpp implementations for each fs method.
We might need to doublecheck that every method uses this. Of course, it's non-blocking concern: I'm just pointing out that after this change we'll have to check cpp side instead of js.
Right now I'm only seeing that Mkdtemp lacks it here: https://github.com/nodejs/node/blob/d93f610046d589e9b081935205b82a3a4ab97089/src/node_file.cc#L2820-L2823
Which mirrors current (might be bugged on Windows if called with long prefix) js implementation.

anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 18, 2024
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 18, 2024

nodejs-github-bot commented Mar 18, 2024

Copy link
Copy Markdown
Collaborator

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

RafaelGSS commented Mar 19, 2024

Copy link
Copy Markdown
Member

This should land first #52148.

RafaelGSS reviewed Mar 19, 2024
View reviewed changes

RafaelGSS left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

As @joyeecheung suggested on my initial PR for node::pathResolve, I suggest you do the same for path.toNamespacePath. Change the bindings in a different PR and run the tests to see if it works.

Example: #51295

anonrig commented Mar 19, 2024

Copy link
Copy Markdown
Member Author

@RafaelGSS the function signature is not 1 to 1. We're always using BufferValue in node:fs.

joyeecheung commented Mar 19, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

I think this is different from #51295 in that, the other PR only used the C++ version in a not very well-tested branch (only when permission is enabled) so if it's broken, we may not be able to know due to the low test coverage. This PR however changed most fs bindings so if it's broken, it's probably more likely that some tests would fail on Windows.

That said I think we already don't have enough test cases to check the kind of paths that are need to be handled specially by toNamespacedPath(). Perhaps it would be useful to check the cases in #51097 too

anonrig force-pushed the simplify-fs branch 2 times, most recently from 8ab9cf7 to a611c98 Compare March 21, 2024 14:57

anonrig commented Jun 17, 2024

Copy link
Copy Markdown
Member Author

@RafaelGSS can you look into the permission model changes? I removed an unnecessary test since we don't use path.toNamespacedPath anymore in JS world, and updated several paths and surrounded them with path.toNamespacedPath()

anonrig requested a review from RafaelGSS June 17, 2024 23:51

nodejs-github-bot commented Jun 17, 2024

Copy link
Copy Markdown
Collaborator

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

RafaelGSS reviewed Jun 18, 2024
View reviewed changes
Comment thread test/fixtures/permission/fs-traversal.js
}));
}

// Monkey-patching path module should also not allow path traversal.

RafaelGSS Jun 18, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Regardless if we are moving the implementation to C++ I believe path.toNamespacedPath will always exist through an API right? If so, I don't think we should remove this test, it won't fail anyway.

anonrig Jun 18, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I think its not needed but on top of that It fails because win32 path resolves return "." for cwd, but previously it didn't.

Comment thread test/parallel/test-fs-mkdir.js
assert.strictEqual(fs.existsSync(pathname), true);
assert.strictEqual(fs.statSync(pathname).isDirectory(), true);
assert.strictEqual(p, firstPathCreated);
assert.strictEqual(p, path.toNamespacedPath(firstPathCreated));

RafaelGSS Jun 18, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Can you explain why we are changing the comparison to include the path.toNamespacedPath? Did tmpdir.resolve API change somehow? If so, isn't it a breaking change?

anonrig Jun 18, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Technically it isn't since both paths are the same and from Windows perspective having a path with namespaced value does not really have any difference with without the prefix.

PS: Namespaced paths are used to avoid "path is too long" errors on Windows. If you look into this file, you'll see that we already check the value surrounded by toNamespacedPath in several places already. I feel like this is more complete.

Comment thread src/node_file.cc

nodejs-github-bot commented Jun 18, 2024

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Jun 19, 2024

Copy link
Copy Markdown
Collaborator

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

anonrig requested review from jasnell and mcollina June 19, 2024 22:49
anonrig added the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 20, 2024
nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 20, 2024
nodejs-github-bot merged commit 399eb33 into nodejs:main Jun 20, 2024

nodejs-github-bot commented Jun 20, 2024

Copy link
Copy Markdown
Collaborator

Landed in 399eb33

targos pushed a commit that referenced this pull request Jun 20, 2024
fs: move ToNamespacedPath to c++
105b006 
Co-Authored-By: Daniel Lemire <daniel@lemire.me>
PR-URL: #52135
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>
Reviewed-By: Daniel Lemire <daniel@lemire.me>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
soophoo pushed a commit to soophoo/node that referenced this pull request Jun 20, 2024
fs: move ToNamespacedPath to c++
74e5916 
Co-Authored-By: Daniel Lemire <daniel@lemire.me>
PR-URL: nodejs#52135
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>
Reviewed-By: Daniel Lemire <daniel@lemire.me>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos mentioned this pull request Jun 25, 2024
Merged
Karliz24 mentioned this pull request Jul 9, 2024
Open
RafaelGSS mentioned this pull request Jul 29, 2024
Closed
lpinca mentioned this pull request Aug 1, 2024
Closed
2 tasks

tniessen commented Aug 5, 2024

Copy link
Copy Markdown
Member

@anonrig This PR might have broken a few things, PTAL at #54161.

This was referenced Jan 23, 2025
Closed
Open
Closed
aduh95 removed the baking-for-lts PRs that need to wait before landing in a LTS release. label Feb 3, 2025
legendecas mentioned this pull request May 28, 2025
Merged
joyeecheung mentioned this pull request Jun 5, 2025
Closed
Renegade334 mentioned this pull request Dec 31, 2025
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

RafaelGSS RafaelGSS approved these changes
lemire lemire approved these changes
jasnell jasnell approved these changes
GeoffreyBooth GeoffreyBooth approved these changes
H4ad H4ad approved these changes
mcollina Awaiting requested review from mcollina

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. fs Issues and PRs related to the fs subsystem / file system. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. windows Issues and PRs related to the Windows platform.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Back | FazBrowse Home | New Git URL