fs,win: fix bug in paths with trailing slashes #54160

huseyinacacak-janea · 2024-08-01T12:02:20Z

This PR fixes the inconsistency across platforms by throwing the same exception on Windows as on Linux.

I've taken the logic and the code in the referenced PR and modified it slightly to allow it to run with the current implementation of Node. Thanks @lundibundi.

Fixes: #17801
Refs: #33831

codecov · 2024-08-01T13:10:04Z

Codecov Report

Attention: Patch coverage is 98.41270% with 1 line in your changes missing coverage. Please review.

Project coverage is 87.10%. Comparing base (01cf9bc) to head (6a733cc).
Report is 736 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/internal/fs/utils.js	97.56%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #54160      +/-   ##
==========================================
+ Coverage   87.07%   87.10%   +0.03%     
==========================================
  Files         643      647       +4     
  Lines      181583   181816     +233     
  Branches    34886    34895       +9     
==========================================
+ Hits       158114   158375     +261     
+ Misses      16751    16745       -6     
+ Partials     6718     6696      -22

Files with missing lines	Coverage Δ
lib/fs.js	`93.22% <100.00%> (-0.11%)`	⬇️
lib/internal/fs/promises.js	`97.55% <100.00%> (ø)`
lib/internal/fs/streams.js	`93.46% <100.00%> (ø)`
lib/internal/fs/utils.js	`95.12% <97.56%> (+0.07%)`	⬆️

... and 82 files with indirect coverage changes

Fixes: nodejs#17801 Refs: nodejs#33831

anonrig · 2024-08-01T16:52:57Z

lib/internal/fs/utils.js

@@ -709,14 +709,30 @@ const validateOffsetLengthWrite = hideStackFrames(
  },
 );

-const validatePath = hideStackFrames((path, propName = 'path') => {
+const validatePath = hideStackFrames((path, propName = 'path', isFile) => {


Can you add a JSDoc to this function and adding a small comment that explains the behavior of this new flag?

Suggested change

const validatePath = hideStackFrames((path, propName = 'path', isFile) => {

const validatePath = hideStackFrames((path, propName = 'path', expectFile) => {

anonrig · 2024-08-01T16:53:22Z

lib/internal/fs/utils.js

+        code: 'ERR_FS_EISDIR',
+        message: 'is a directory',
+        path,
+        syscall: 'validatePath',


Suggested change

syscall: 'validatePath',

This is not a syscall.

I've applied the fix you suggested, but as you can see the CI doesn't accept throwing an error without a syscall. I didn't get this on Windows, only Linux.
This function is being called from a number of functions that contain different syscalls. Do you have any idea what I can write here?

I think this syscall should be the name of the fs caller function. Maybe passing it as an argument would be the correct call here? For example, if fs.cpSync() is throwing this error, syscall should be cp. IMHO, Ideally we should throw this error from C++.

Thanks for the review. Fixed.

lib/internal/fs/utils.js

anonrig · 2024-08-06T13:31:26Z

lib/internal/fs/utils.js

+ * @throws {TypeError} Throws if the path is not a string, Uint8Array, or URL without null bytes.
+ * @throws {Error} Throws if the path does not meet the expected type (file).
+ */
+const validatePath = hideStackFrames((path, propName = 'path', expectFile) => {


maybe we should go for options instead of expectFile and pass { expectFile: true, syscall: 'cp' }?

And just check for options != null in 729?

nodejs-github-bot · 2024-08-11T17:45:04Z

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

anonrig · 2024-08-11T17:47:48Z

cc @nodejs/fs @nodejs/performance

mcollina

lgtm

mcollina · 2024-08-14T15:23:06Z

I have a rough feeling that we might be better to land this as semver-major, or at the bare minimum to not backport to v20 and v18. Wdyt?

RafaelGSS · 2024-08-14T16:03:23Z

I have a rough feeling that we might be better to land this as semver-major, or at the bare minimum to not backport to v20 and v18. Wdyt?

That's my feeling too. I'd not land it on 22 too.

Honestly, I'm not confident this design is good/desirable. I can see people forgetting to pass such parameters in a future fs API.

huseyinacacak-janea · 2024-08-21T07:26:42Z

Is there anything else I can do to help this PR move forward?

mcollina · 2024-08-29T04:48:36Z

@anonrig @RafaelGSS PTAL

huseyinacacak-janea · 2024-09-30T12:46:23Z

Can someone take a look at this PR?

nodejs-github-bot · 2024-10-10T11:14:57Z

Landed in 00b2f07

aduh95 · 2024-10-10T17:33:12Z

This landed without a recent CI run, resulting in failing linter on main.

ShenHongFei · 2024-10-14T07:20:27Z

@huseyinacacak-janea @anonrig This change will cause webpack on windows to fail to save cache files.
[webpack.cache.PackFileCacheStrategy/webpack.FileSystemInfo] Error snapshotting file timestamp hash combination of T:\: SystemError [ERR_FS_EISDIR]: Path is a directory: read returned ERR_FS_EISDIR (is a directory) D:\.

Remove the following lines of code and recompile node.js to restore to normal. I'm not sure if the problem lies with webpack or here.

  if (options?.expectFile) {
    const lastCharacter = path[path.length - 1];
    if (
      lastCharacter === '/' || lastCharacter === 47 ||
      (isWindows && (lastCharacter === '\\' || lastCharacter === 92))
    ) {
      throw new ERR_FS_EISDIR({
        code: 'ERR_FS_EISDIR',
        message: 'is a directory',
        path,
        syscall: options.syscall,
        errno: ERR_FS_EISDIR,
      });
    }
  }

I wrote an example project that can reproduce this error

https://github.com/ShenHongFei/webpack-bug

webpack error log

    [webpack.cache.PackFileCacheStrategy] No pack exists at T:\webpack-bug\node_modules\.cache\webpack\default-production.pack: Error: ENOENT: no such file or directory, stat 'T:\webpack-bug\node_modules\.cache\webpack\default-production\index.pack'
<t> [webpack.cache.PackFileCacheStrategy] restore cache container: 30.6804 ms
    [webpack.cache.PackFileCacheStrategy] Pack got invalid because of write to: Compilation/modules|T:\webpack-bug\node_modules\.pnpm\ts-loader@9.5.1_typescript@5.6.3_webpack@5.95.0\node_modules\ts-loader\index.js??ruleSet[1].rules[0]!T:\webpack-bug\entry.ts
    [webpack.cache.PackFileCacheStrategy] Storing pack...
    [webpack.cache.PackFileCacheStrategy] Capturing build dependencies... (T:\webpack-bug\node_modules\.pnpm\webpack@5.95.0\node_modules\webpack\lib\, T:\webpack-bug\tsconfig.json, T:\webpack-bug\node_modules\.pnpm\ts-loader@9.5.1_typescript@5.6.3_webpack@5.95.0\node_modules\ts-loader\index.js)
<t> [webpack.cache.PackFileCacheStrategy] resolve build dependencies: 533.4064 ms
    [webpack.cache.PackFileCacheStrategy/webpack.FileSystemInfo] Error snapshotting file timestamp hash combination of T:\: SystemError [ERR_FS_EISDIR]: Path is a directory: read returned ERR_FS_EISDIR (is a directory) T:\
        at readFile (node:fs:389:5)
        at go$readFile (T:\webpack-bug\node_modules\.pnpm\graceful-fs@4.2.11\node_modules\graceful-fs\graceful-fs.js:118:14)
        at Object.readFile (T:\webpack-bug\node_modules\.pnpm\graceful-fs@4.2.11\node_modules\graceful-fs\graceful-fs.js:115:12)
        at FileSystemInfo._readFileHash (T:\webpack-bug\node_modules\.pnpm\webpack@5.95.0\node_modules\webpack\lib\FileSystemInfo.js:3268:11)
        at T:\webpack-bug\node_modules\.pnpm\webpack@5.95.0\node_modules\webpack\lib\util\AsyncQueue.js:324:10
        at Hook.eval [as callAsync] (eval at create (T:\webpack-bug\node_modules\.pnpm\tapable@2.2.1\node_modules\tapable\lib\HookCodeFactory.js:33:10), <anonymous>:6:1)
        at AsyncQueue._startProcessing (T:\webpack-bug\node_modules\.pnpm\webpack@5.95.0\node_modules\webpack\lib\util\AsyncQueue.js:314:26)
        at AsyncQueue._ensureProcessing (T:\webpack-bug\node_modules\.pnpm\webpack@5.95.0\node_modules\webpack\lib\util\AsyncQueue.js:290:9)
        at process.processImmediate (node:internal/timers:511:21)
<t> [webpack.cache.PackFileCacheStrategy] snapshot build dependencies: 3.4601 ms
<w> [webpack.cache.PackFileCacheStrategy] Caching failed for pack: Error: Unable to snapshot resolve dependencies
    [webpack.cache.PackFileCacheStrategy] Error: Unable to snapshot resolve dependencies
        at T:\webpack-bug\node_modules\.pnpm\webpack@5.95.0\node_modules\webpack\lib\cache\PackFileCacheStrategy.js:1385:13
        at jobError (T:\webpack-bug\node_modules\.pnpm\webpack@5.95.0\node_modules\webpack\lib\FileSystemInfo.js:2232:5)
        at T:\webpack-bug\node_modules\.pnpm\webpack@5.95.0\node_modules\webpack\lib\FileSystemInfo.js:2314:10
        at T:\webpack-bug\node_modules\.pnpm\webpack@5.95.0\node_modules\webpack\lib\FileSystemInfo.js:3344:13
        at T:\webpack-bug\node_modules\.pnpm\webpack@5.95.0\node_modules\webpack\lib\util\AsyncQueue.js:373:5
        at Hook.eval [as callAsync] (eval at create (T:\webpack-bug\node_modules\.pnpm\tapable@2.2.1\node_modules\tapable\lib\HookCodeFactory.js:33:10), <anonymous>:6:1)
        at Hook.CALL_ASYNC_DELEGATE [as _callAsync] (T:\webpack-bug\node_modules\.pnpm\tapable@2.2.1\node_modules\tapable\lib\Hook.js:18:14)
        at AsyncQueue._handleResult (T:\webpack-bug\node_modules\.pnpm\webpack@5.95.0\node_modules\webpack\lib\util\AsyncQueue.js:343:21)
        at T:\webpack-bug\node_modules\.pnpm\webpack@5.95.0\node_modules\webpack\lib\util\AsyncQueue.js:330:10
        at Hook.eval [as callAsync] (eval at create (T:\webpack-bug\node_modules\.pnpm\tapable@2.2.1\node_modules\tapable\lib\HookCodeFactory.js:33:10), <anonymous>:6:1)

blexrob · 2024-10-30T12:09:01Z

This also causes trouble for emscripten: emscripten-core/emscripten#22812

Fixes: nodejs#17801 Refs: nodejs#33831 PR-URL: nodejs#54160 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>

…6417 Looks like a nodejs bug, introduced in v23.0.0 - commit hash 00b2f07f It should be fixed in v23.2.0 - dd9b6833 bug: nodejs/node#54160 fix: nodejs/node#55527 I am able to replicate this on v23.1.0.

nodejs-github-bot added fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. labels Aug 1, 2024

huseyinacacak-janea force-pushed the huseyin-5301-inconsistent-fs-module branch from 31da37f to c93a41e Compare August 1, 2024 13:46

fs,win: fix bug in paths with trailing slashes

733558f

Fixes: nodejs#17801 Refs: nodejs#33831

huseyinacacak-janea force-pushed the huseyin-5301-inconsistent-fs-module branch from c93a41e to 733558f Compare August 1, 2024 14:42

anonrig reviewed Aug 1, 2024

View reviewed changes

fixup! fs,win: fix bug in paths with trailing slashes

797c55c

anonrig reviewed Aug 6, 2024

View reviewed changes

fixup! fs,win: fix bug in paths with trailing slashes

6a733cc

jasnell added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 11, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 11, 2024

mcollina approved these changes Aug 14, 2024

View reviewed changes

mcollina added the semver-major PRs that contain breaking changes and should be released in the next major version. label Aug 29, 2024

StefanStojanovic mentioned this pull request Oct 10, 2024

2024-10-16, Version 23.0.0 (Current) #55338

Merged

anonrig approved these changes Oct 10, 2024

View reviewed changes

anonrig added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 10, 2024

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 10, 2024

nodejs-github-bot merged commit 00b2f07 into nodejs:main Oct 10, 2024
59 checks passed

RafaelGSS mentioned this pull request Oct 10, 2024

lib: runtime deprecate SlowBuffer #55175

Merged

aduh95 mentioned this pull request Oct 10, 2024

fs: fix linter issue #55353

Merged

rvagg mentioned this pull request Oct 25, 2024

Revert "fs,win: fix bug in paths with trailing slashes" #55527

Merged

merceyz mentioned this pull request Oct 25, 2024

esbuild-wasm hangs on Node 23 evanw/esbuild#3951

Open

RafaelGSS mentioned this pull request Oct 30, 2024

Enforce a more strict policy for semver-major releases nodejs/Release#1054

Closed

juanarbol mentioned this pull request Oct 31, 2024

Regression in fs.readFile #55609

Closed

jeremybobbin mentioned this pull request Dec 5, 2024

prevent reading directories as files when building fs cache #16417 webpack/webpack#19036

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fs,win: fix bug in paths with trailing slashes #54160

fs,win: fix bug in paths with trailing slashes #54160

huseyinacacak-janea commented Aug 1, 2024

codecov bot commented Aug 1, 2024 •

edited

Loading

anonrig Aug 1, 2024

huseyinacacak-janea Aug 6, 2024

anonrig Aug 1, 2024

anonrig Aug 1, 2024

huseyinacacak-janea Aug 6, 2024

anonrig Aug 6, 2024

huseyinacacak-janea Aug 9, 2024

anonrig Aug 6, 2024

anonrig Aug 6, 2024

huseyinacacak-janea Aug 9, 2024

nodejs-github-bot commented Aug 11, 2024

anonrig commented Aug 11, 2024

mcollina left a comment

mcollina commented Aug 14, 2024

RafaelGSS commented Aug 14, 2024 •

edited

Loading

huseyinacacak-janea commented Aug 21, 2024

mcollina commented Aug 29, 2024

huseyinacacak-janea commented Sep 30, 2024

nodejs-github-bot commented Oct 10, 2024

aduh95 commented Oct 10, 2024

ShenHongFei commented Oct 14, 2024 •

edited

Loading

blexrob commented Oct 30, 2024

	const validatePath = hideStackFrames((path, propName = 'path', isFile) => {
	const validatePath = hideStackFrames((path, propName = 'path', expectFile) => {

fs,win: fix bug in paths with trailing slashes #54160

fs,win: fix bug in paths with trailing slashes #54160

Conversation

huseyinacacak-janea commented Aug 1, 2024

codecov bot commented Aug 1, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nodejs-github-bot commented Aug 11, 2024

anonrig commented Aug 11, 2024

mcollina left a comment

Choose a reason for hiding this comment

mcollina commented Aug 14, 2024

RafaelGSS commented Aug 14, 2024 • edited Loading

huseyinacacak-janea commented Aug 21, 2024

mcollina commented Aug 29, 2024

huseyinacacak-janea commented Sep 30, 2024

nodejs-github-bot commented Oct 10, 2024

aduh95 commented Oct 10, 2024

ShenHongFei commented Oct 14, 2024 • edited Loading

blexrob commented Oct 30, 2024

codecov bot commented Aug 1, 2024 •

edited

Loading

RafaelGSS commented Aug 14, 2024 •

edited

Loading

ShenHongFei commented Oct 14, 2024 •

edited

Loading