Skip to content
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.mkdir/mkdirSync hang with {recursive: true} if name is invalid #28599

Closed
jeffrson opened this issue Jul 8, 2019 · 5 comments · Fixed by #29070
Closed

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

jeffrson opened this issue Jul 8, 2019 · 5 comments · Fixed by #29070
Labels
confirmed-bug Issues with confirmed bugs. fs Issues and PRs related to the fs subsystem / file system. windows Issues and PRs related to the Windows platform.

Comments

@jeffrson
Copy link

jeffrson commented Jul 8, 2019

  • Version: 10.16.0, 11.14.0, 12.6.0
  • Platform: Windows 10
  • Subsystem: fs

If the name of the dir to be created is invalid, fs.mkdir never calls back and fs.mkdirSync blocks:

const fs = require('fs')

console.log("1")
fs.mkdir('invalid1:', {recursive: true}, (err) => {
  console.log("is not called")
  if (err) throw err
})

console.log("2")
fs.mkdirSync('invalid2:', {recursive: true})
console.log("is not reached")

BTW, the error message that is reported with {recursive: false} appears a bit strange:

Error: ENOENT: no such file or directory, mkdir 'invalid2:'

Wouldn't it be more convenient to use EPERM, EACCES or ENOTDIR?

@Hakerh400
Copy link
Contributor

Hakerh400 commented Jul 8, 2019

Colons on NTFS files systems on Windows denote file streams (source).

@jeffrson
Copy link
Author

jeffrson commented Jul 8, 2019

The error is the same for "invalid>", "invalid<" oder "invalid|".

@silverwind
Copy link
Contributor

silverwind commented Jul 9, 2019

Those are all invalid characters on NTFS, see here.

Not sure if Node.js should do validation for those (and take a performance hit on every platform). There are modules like this which you can use to validate yourself.

The hang itself should probably be investigated, thought.

@jeffrson
Copy link
Author

Yes - I know this module. However, it is not cross plattform as far as I can tell - ':', '<', '>','|' may be valid on Linux (although not necessarily recommended, I guess). Yes, of course I could distinguish platforms. But I could as well rely on "fs.mkdir" to fail, if something is wrong.

The point is, Node should, if it fails to create the directory for whatever reason, give a reasonable error and, of course, not hang - with { recursive: true } or not.

@Fishrock123 Fishrock123 added fs Issues and PRs related to the fs subsystem / file system. windows Issues and PRs related to the Windows platform. labels Jul 10, 2019
bzoz added a commit to JaneaSystems/libuv that referenced this issue Jul 11, 2019
Makes uv_fs_mkdir return UV_EINVAL for invalid filenames instead of
UV_ENOENT.

Ref: nodejs/node#28599
bzoz added a commit to JaneaSystems/libuv that referenced this issue Jul 11, 2019
Makes uv_fs_mkdir return UV_EINVAL for invalid filenames instead of
UV_ENOENT.

Ref: nodejs/node#28599
@bzoz
Copy link
Contributor

bzoz commented Jul 11, 2019

This is similar to #27198, which was fixed by #27207.

For invalid filenames, uv_fs_mkdir returns UV_ENOENT, which confuses the fs.mkdir function. It constantly creates the parent directory then tries to create the invalid folder.

Made a libuv PR that makes it return UV_EINVAL for invalid filenames. This fixes this issue.

@silverwind silverwind added the confirmed-bug Issues with confirmed bugs. label Jul 11, 2019
bzoz added a commit to libuv/libuv that referenced this issue Jul 16, 2019
Makes uv_fs_mkdir return UV_EINVAL for invalid filenames instead of
UV_ENOENT.

Ref: nodejs/node#28599

PR-URL: #2375
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
cjihrig added a commit to cjihrig/node that referenced this issue Aug 11, 2019
Notable changes:

- UV_FS_O_FILEMAP has been added for faster access to memory
  mapped files on Windows.
- uv_fs_mkdir() now returns UV_EINVAL for invalid filenames
  on Windows. It previously returned UV_ENOENT.
- The uv_fs_statfs() API has been added.
- The uv_os_environ() and uv_os_free_environ() APIs have
  been added.

Fixes: nodejs#28599
Fixes: nodejs#28945
Fixes: nodejs#29008
PR-URL: nodejs#29070
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
targos pushed a commit that referenced this issue Aug 19, 2019
Notable changes:

- UV_FS_O_FILEMAP has been added for faster access to memory
  mapped files on Windows.
- uv_fs_mkdir() now returns UV_EINVAL for invalid filenames
  on Windows. It previously returned UV_ENOENT.
- The uv_fs_statfs() API has been added.
- The uv_os_environ() and uv_os_free_environ() APIs have
  been added.

Fixes: #28599
Fixes: #28945
Fixes: #29008
PR-URL: #29070
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
BethGriggs pushed a commit to BethGriggs/node that referenced this issue Feb 26, 2020
Notable changes:

- UV_FS_O_FILEMAP has been added for faster access to memory
  mapped files on Windows.
- uv_fs_mkdir() now returns UV_EINVAL for invalid filenames
  on Windows. It previously returned UV_ENOENT.
- The uv_fs_statfs() API has been added.
- The uv_os_environ() and uv_os_free_environ() APIs have
  been added.

Fixes: nodejs#28599
Fixes: nodejs#28945
Fixes: nodejs#29008
PR-URL: nodejs#29070
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
BethGriggs pushed a commit that referenced this issue Mar 2, 2020
Notable changes:

- UV_FS_O_FILEMAP has been added for faster access to memory
  mapped files on Windows.
- uv_fs_mkdir() now returns UV_EINVAL for invalid filenames
  on Windows. It previously returned UV_ENOENT.
- The uv_fs_statfs() API has been added.
- The uv_os_environ() and uv_os_free_environ() APIs have
  been added.

Fixes: #28599
Fixes: #28945
Fixes: #29008
PR-URL: #29070
Backport-PR-URL: #31969
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. fs Issues and PRs related to the fs subsystem / file system. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants