fs: fix infinite loop with async recursive mkdir on Windows by richardlau · Pull Request #27207 · nodejs/node (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation27 Commits2 Checks0 Files changed

Conversation

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]({{ revealButtonHref }})

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

This comment has been minimized.

nodejs-github-bot added c++

Issues and PRs that require attention from people who are familiar with C++.

Issues and PRs related to the fs subsystem / file system.

labels

Apr 13, 2019

This comment has been minimized.

Trott added the windows

Issues and PRs related to the Windows platform.

label

Apr 13, 2019

This comment has been minimized.

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

@@ -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) {

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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).

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

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.

Contributor

@bcoe bcoe left a comment

Choose a reason for hiding this comment

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

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?

}));
}

// `mkdirp` when part of the path is a file.

Choose a reason for hiding this comment

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

tests look great 👍

@@ -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(),

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

@@ -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) {

Choose a reason for hiding this comment

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

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

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

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

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).

bcoe approved these changes Apr 16, 2019

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

This was referenced

Apr 23, 2019

MylesBorins pushed a commit that referenced this pull request

May 16, 2019

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

MylesBorins pushed a commit that referenced this pull request

May 16, 2019

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

MylesBorins pushed a commit that referenced this pull request

May 16, 2019

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

MylesBorins pushed a commit that referenced this pull request

May 16, 2019

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 was referenced

May 29, 2019

bzoz mentioned this pull request

Jul 11, 2019

Labels

c++

Issues and PRs that require attention from people who are familiar with C++.

Issues and PRs related to the fs subsystem / file system.

windows

Issues and PRs related to the Windows platform.