Line breaks in filenames cause infinite loops in mkdirs & mkdirsSync

[wrager]

• Operating System: Windows 10 Enterprise 2016
• Node.js version: 6.10.2
• fs-extra version: 4.0.3

It would be nice to have error handling for line breaks in the mkdirs and mkdirsSync methods.

Correct names:

> require('fs-extra').mkdirs('dir_correct');
Promise { <pending> }

> require('fs-extra').mkdirs('dir_correct', (a) => console.log('callback', a));
undefined
> callback null

> require('fs-extra').mkdirsSync('dir_correct');
null

Directories with \n in paths are not created. It's okay, but here we get an infinite loop (at least in mkdirsSync), which is unacceptable in my opinion. In mkdirs with callback, the callback is not called.

> require('fs-extra').mkdirs('dir_incorrect\n');
Promise { <pending> }

> require('fs-extra').mkdirs('dir_incorrect\n', (a) => console.log('callback', a));
undefined

> require('fs-extra').mkdirsSync('dir_incorrect\n');
RangeError: Maximum call stack size exceeded
    at Object.fs.mkdirSync (fs.js:923:18)
    at mkdirsSync (C:\projects\test1\node_modules\fs-extra\lib\mkdirs\mkdirs-sync.js:31:9)
    at mkdirsSync (C:\projects\test1\node_modules\fs-extra\lib\mkdirs\mkdirs-sync.js:37:16)
    at mkdirsSync (C:\projects\test1\node_modules\fs-extra\lib\mkdirs\mkdirs-sync.js:38:9)
    at mkdirsSync (C:\projects\test1\node_modules\fs-extra\lib\mkdirs\mkdirs-sync.js:38:9)
    at mkdirsSync (C:\projects\test1\node_modules\fs-extra\lib\mkdirs\mkdirs-sync.js:38:9)
    at mkdirsSync (C:\projects\test1\node_modules\fs-extra\lib\mkdirs\mkdirs-sync.js:38:9)
    at mkdirsSync (C:\projects\test1\node_modules\fs-extra\lib\mkdirs\mkdirs-sync.js:38:9)
    at mkdirsSync (C:\projects\test1\node_modules\fs-extra\lib\mkdirs\mkdirs-sync.js:38:9)
    at mkdirsSync (C:\projects\test1\node_modules\fs-extra\lib\mkdirs\mkdirs-sync.js:38:9)

[RyanZim]

Huh, these weird cases. What's the result of calling fs.mkdir with a newline in the filename? (when the parent directories exist)

[wrager]

@RyanZim

> require('fs').mkdir('dir_incorrect\n');
undefined
> Error: ENOENT: no such file or directory, mkdir 'C:\projects\test1\dir_incorrect
'
    at Error (native)

[RyanZim]

No wonder it crashes. ENOENT is also the error code we get when the parent doesn't exist, so we respond by trying to create the parent. That causes an infinite loop.

Not sure what's the best way to solve this, but we need to do something.

[manidlou]

What should happen when a newline character exists in the path? error out?

[RyanZim]

@manidlou That's the ugly part. AFAIK, on Windows, newlines aren't allowed. However, they may be allowed on some Linux systems, not sure.

We could either error out on a newline, or else put logic in place that if we get ENOENT after creating the parent, we just throw it. Not sure what's best.

[wrager]

@manidlou @RyanZim It seems to me that you should delegate the handling of incorrect characters to the OS. Not only because operating systems are possible that allow line breaks in paths, but also because other similar characters are possible that we do not know about.

[RyanZim]

@wrager I'm leaning in that direction as well. If only Windoze would throw a sensible error for invalid characters instead of ENOENT... 😬

[manidlou]

@wrager yes delegating to the OS makes sense. @RyanZim you are right! there is inconsistency in error codes that OSes throw!

Not only that, there is a big discussion on what valid file paths should look like! There is this Wikipedia page on Filename on reserved words and characters in different OSes. IMO, dealing with checking for valid file path (that's what I had originally in my mind) is a mess because of noisy OS differences!

put logic in place that if we get ENOENT after creating the parent, we just throw it.

@RyanZim I think that's a good and feasible approach!

So then when that happens, we should remove the parent directory that was just created, right? Since essentially it is a failed operation?!

[RyanZim]

@manidlou Yeah, we probably should. That's gonna be messy, though. I didn't think about that.

[RyanZim]

Well, hopefully nodejs/node#21875 solves this for newer Node versions.

[RyanZim]

Should be fixed since #894.