fs: fix infinite loop with async recursive mkdir on Windows #27207

richardlau · 2019-04-13T03:13:26Z

If file is a file then on Windows mkdir on file/a returns an
ENOENT error while on POSIX the equivalent returns ENOTDIR. On the
POSIX systems ENOTDIR would break out of the loop but on Windows the
ENOENT would strip off the a and attempt to make file as a
directory. This would return EEXIST but the code wasn't detecting
that the existing path was a file and attempted to make file/a again.

Fixes: #27198

cc @nodejs/fs @bcoe

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

thefourtheye · 2019-04-14T14:44:11Z

src/node_file.cc

@@ -1270,8 +1270,19 @@ int MKDirpSync(uv_loop_t* loop,
        }
        default:
          uv_fs_req_cleanup(req);
+          if (err == UV_EEXIST && continuation_data.paths.size() > 0) {


Nit: Would it be better to move UV_EEXIST to a case?

We'd still need to duplicate the logic for when continuation_data.paths.size() == 0 (which is the same logic as the rest of the default case).

Given that we now have logic that checks whether or not a path is a file before calling MKDirpAsync, do we need to check continuation_data.paths.size() > 0; seems like a reasonable safety to have in place, but wonder if you could think of a situation where we'd hit this edge-case.

We haven't done the check the very first time MKDirpAsync is called from MKDir. So if uv_fs_mkdir() fails on the very first iteration (continuation_data.paths.size() == 0) we want to return UV_EEXIST.

bcoe

I think it might be wroth breaking the logic that performs the uv_fs_stat into its own helper method.

Kind of stinks that we need to perform one more system level call to shim the difference between Windows and Linux, is this something that could be eventually addressed in libuv?

bcoe · 2019-04-15T17:44:00Z

test/parallel/test-fs-mkdir.js

+  }));
+}
+
+// `mkdirp` when part of the path is a file.


tests look great 👍

bcoe · 2019-04-15T17:45:30Z

src/node_file.cc

@@ -1341,8 +1352,24 @@ int MKDirpAsync(uv_loop_t* loop,
          if (err == UV_EEXIST &&
              req_wrap->continuation_data->paths.size() > 0) {
            uv_fs_req_cleanup(req);
-            MKDirpAsync(loop, req, path.c_str(),
-                        req_wrap->continuation_data->mode, nullptr);
+            int err = uv_fs_stat(loop, req, path.c_str(),


This logic is getting chunky enough, I wonder if we could break it out into a helper method.

Refactored a bit to make the code more compact. PTAL.

bcoe · 2019-04-15T17:50:15Z

src/node_file.cc

@@ -1270,8 +1270,19 @@ int MKDirpSync(uv_loop_t* loop,
        }
        default:
          uv_fs_req_cleanup(req);
+          if (err == UV_EEXIST && continuation_data.paths.size() > 0) {


Given that we now have logic that checks whether or not a path is a file before calling MKDirpAsync, do we need to check continuation_data.paths.size() > 0; seems like a reasonable safety to have in place, but wonder if you could think of a situation where we'd hit this edge-case.

bcoe · 2019-04-15T17:53:00Z

@richardlau great work, thanks for getting to this patch so fast.

test/parallel/test-fs-mkdir.js

nodejs-github-bot · 2019-04-16T01:02:50Z

CI: https://ci.nodejs.org/job/node-test-pull-request/22476/ (✔️)

richardlau · 2019-04-16T01:20:35Z

I think it might be wroth breaking the logic that performs the uv_fs_stat into its own helper method.

I've refactored a bit so the code is much more compact now. PTAL.

Kind of stinks that we need to perform one more system level call to shim the difference between Windows and Linux, is this something that could be eventually addressed in libuv?

Sounds unlikely based on #18014 (comment) but cc @nodejs/libuv anyway.

For this PR the Windows call that libuv ends up using is _wmkdir

node/deps/uv/src/win/fs.c

Lines 851 to 855 in f698386

    
           void fs__mkdir(uv_fs_t* req) { 
        
             /* TODO: use req->mode. */ 
        
             int result = _wmkdir(req->file.pathw); 
        
             SET_REQ_RESULT(req, result); 
        
           }

Both _wmkdir (Windows) and mkdir (Posix) return EEXIST if the path already exists. The difference is how it treats earlier parts of the path that don't exist -- Posix will return ENOTDIR if an earlier part of the path is not a directory but Windows does not and the only way to shim that would be to walk up the path (which we're already doing in the recursive MKDir implementation).

BridgeAR · 2019-04-16T02:39:40Z

Tests are LGTM

nodejs-github-bot · 2019-04-16T02:44:54Z

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

nodejs-github-bot · 2019-04-16T03:31:49Z

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

nodejs-github-bot · 2019-04-16T04:08:35Z

CI: https://ci.nodejs.org/job/node-test-pull-request/22482/ (✔️)

If `file` is a file then on Windows `mkdir` on `file/a` returns an `ENOENT` error while on POSIX the equivalent returns `ENOTDIR`. On the POSIX systems `ENOTDIR` would break out of the loop but on Windows the `ENOENT` would strip off the `a` and attempt to make `file` as a directory. This would return `EEXIST` but the code wasn't detecting that the existing path was a file and attempted to make `file/a` again. PR-URL: nodejs#27207 Fixes: nodejs#27198 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Ben Coe <bencoe@gmail.com>

Replace try-catch blocks in tests with `assert.rejects()` and `assert.throws()`. PR-URL: nodejs#27207 Fixes: nodejs#27198 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Ben Coe <bencoe@gmail.com>

richardlau · 2019-04-16T18:26:20Z

Landed in 96e46d3...c6c37e9.

If `file` is a file then on Windows `mkdir` on `file/a` returns an `ENOENT` error while on POSIX the equivalent returns `ENOTDIR`. On the POSIX systems `ENOTDIR` would break out of the loop but on Windows the `ENOENT` would strip off the `a` and attempt to make `file` as a directory. This would return `EEXIST` but the code wasn't detecting that the existing path was a file and attempted to make `file/a` again. PR-URL: #27207 Fixes: #27198 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Ben Coe <bencoe@gmail.com>

Replace try-catch blocks in tests with `assert.rejects()` and `assert.throws()`. PR-URL: #27207 Fixes: #27198 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Ben Coe <bencoe@gmail.com>

If `file` is a file then on Windows `mkdir` on `file/a` returns an `ENOENT` error while on POSIX the equivalent returns `ENOTDIR`. On the POSIX systems `ENOTDIR` would break out of the loop but on Windows the `ENOENT` would strip off the `a` and attempt to make `file` as a directory. This would return `EEXIST` but the code wasn't detecting that the existing path was a file and attempted to make `file/a` again. PR-URL: #27207 Fixes: #27198 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Ben Coe <bencoe@gmail.com>

Replace try-catch blocks in tests with `assert.rejects()` and `assert.throws()`. PR-URL: #27207 Fixes: #27198 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Ben Coe <bencoe@gmail.com>

This comment has been minimized.

Sign in to view

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system. labels Apr 13, 2019

This comment has been minimized.

Sign in to view

richardlau mentioned this pull request Apr 13, 2019

mkdir no ERROR when parent is a file with native recursive option #27198

Closed

Trott requested a review from bcoe April 13, 2019 03:28

Trott added the windows Issues and PRs related to the Windows platform. label Apr 13, 2019

This comment has been minimized.

Sign in to view

richardlau force-pushed the mkdirrecursion branch from a074e58 to 0fc9534 Compare April 13, 2019 06:46

This comment has been minimized.

Sign in to view

cjihrig approved these changes Apr 13, 2019

View reviewed changes

richardlau added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 13, 2019

thefourtheye approved these changes Apr 14, 2019

View reviewed changes

bcoe suggested changes Apr 15, 2019

View reviewed changes

richardlau force-pushed the mkdirrecursion branch from 3c236d1 to 3066011 Compare April 16, 2019 00:43

BridgeAR reviewed Apr 16, 2019

View reviewed changes

test/parallel/test-fs-mkdir.js Outdated Show resolved Hide resolved

richardlau removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 16, 2019

bcoe approved these changes Apr 16, 2019

View reviewed changes

richardlau added 2 commits April 16, 2019 13:59

richardlau force-pushed the mkdirrecursion branch from bdfe5ce to c6c37e9 Compare April 16, 2019 18:01

richardlau merged commit c6c37e9 into nodejs:master Apr 16, 2019

MylesBorins added the land-on-v10.x label May 16, 2019

BethGriggs mentioned this pull request May 16, 2019

v10.16.0 proposal #27514

Merged

richardlau mentioned this pull request Jun 15, 2019

fs:fs.mkdir() the process does not exit when recursive is set to true in some case #28241

Closed

bzoz mentioned this pull request Jul 11, 2019

fs.mkdir/mkdirSync hang with {recursive: true} if name is invalid #28599

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fs: fix infinite loop with async recursive mkdir on Windows #27207

fs: fix infinite loop with async recursive mkdir on Windows #27207

richardlau commented Apr 13, 2019

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

thefourtheye Apr 14, 2019

richardlau Apr 14, 2019

bcoe Apr 15, 2019

richardlau Apr 16, 2019

bcoe left a comment

bcoe Apr 15, 2019

bcoe Apr 15, 2019

richardlau Apr 16, 2019

bcoe Apr 15, 2019

bcoe commented Apr 15, 2019

nodejs-github-bot commented Apr 16, 2019 •

edited by richardlau

Loading

richardlau commented Apr 16, 2019

BridgeAR commented Apr 16, 2019

nodejs-github-bot commented Apr 16, 2019

nodejs-github-bot commented Apr 16, 2019

nodejs-github-bot commented Apr 16, 2019 •

edited by richardlau

Loading

richardlau commented Apr 16, 2019

fs: fix infinite loop with async recursive mkdir on Windows #27207

fs: fix infinite loop with async recursive mkdir on Windows #27207

Conversation

richardlau commented Apr 13, 2019

Checklist

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

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

bcoe left a 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

bcoe commented Apr 15, 2019

nodejs-github-bot commented Apr 16, 2019 • edited by richardlau Loading

richardlau commented Apr 16, 2019

BridgeAR commented Apr 16, 2019

nodejs-github-bot commented Apr 16, 2019

nodejs-github-bot commented Apr 16, 2019

nodejs-github-bot commented Apr 16, 2019 • edited by richardlau Loading

richardlau commented Apr 16, 2019

nodejs-github-bot commented Apr 16, 2019 •

edited by richardlau

Loading

nodejs-github-bot commented Apr 16, 2019 •

edited by richardlau

Loading