-
Notifications
You must be signed in to change notification settings - Fork 30k
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: move ToNamespacedPath
to c++
#52135
Conversation
Review requested:
|
003590b
to
2d120ca
Compare
cc @nodejs/platform-windows Any idea where this error/warning is thrown from? |
AFAICT the changes are implying that |
getValidatedPath() result is called with |
d93f610
to
5c62bf2
Compare
Sorry, I read the changes wrong. It doesn't affect js-side Lines 2820 to 2823 in d93f610
Which mirrors current (might be bugged on Windows if called with long prefix) js implementation. |
This should land first #52148. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@RafaelGSS the function signature is not 1 to 1. We're always using BufferValue in node:fs. |
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 |
8ab9cf7
to
a611c98
Compare
hey @huseyinacacak-janea, sorry for the late response. I've updated the pull-request with your recommendations. |
Co-Authored-By: Daniel Lemire <daniel@lemire.me>
@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 |
})); | ||
} | ||
|
||
// Monkey-patching path module should also not allow path traversal. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think its not needed but on top of that It fails because win32 path resolves return "." for cwd, but previously it didn't.
@@ -316,7 +316,7 @@ if (common.isMainThread && (common.isLinux || common.isOSX)) { | |||
const p = fs.mkdirSync(pathname, common.mustNotMutateObjectDeep({ recursive: true })); | |||
assert.strictEqual(fs.existsSync(pathname), true); | |||
assert.strictEqual(fs.statSync(pathname).isDirectory(), true); | |||
assert.strictEqual(p, firstPathCreated); | |||
assert.strictEqual(p, path.toNamespacedPath(firstPathCreated)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Landed in 399eb33 |
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>
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>
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>
This is a step forward to move all
node:fs
implementation to C++. Since we now have support forwin32.pathResolve()
in C++, we can move forward with movingToNamespacedPath
functions to C++.In a follow up pull-request, I'll move
FromNamespacedPath()
from node_url.cc to path.cccc @nodejs/fs @RafaelGSS