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,doc,test: open reserved characters under win32 #13875

Closed

Conversation

XadillaX
Copy link
Contributor

@XadillaX XadillaX commented Jun 22, 2017

Explain the behavior of fs.open() under win32 that file path contains
some characters and add some test cases for them.

  • < (less than)
  • > (greater than)
  • : (colon)
  • " (double quote)
  • / (forward slash)
  • \ (backslash)
  • | (vertical bar or pipe)
  • ? (question mark)
  • * (asterisk)

Refs: #13868
Refs: https://msdn.microsoft.com/en-us/library/windows/desktop/aa365247(v=vs.85).aspx
Refs: https://msdn.microsoft.com/en-us/library/windows/desktop/bb540537.aspx

Checklist
  • make -j4 test (UNIX)
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

fs, doc, test

@nodejs-github-bot nodejs-github-bot added errors Issues and PRs related to JavaScript errors originated in Node.js core. fs Issues and PRs related to the fs subsystem / file system. labels Jun 22, 2017
lib/fs.js Outdated
if (longPath.indexOf(':', 6) !== -1) {
const err = new errors.Error('ERR_INVALID_WIN32_PATH', longPath);
if (req) {
return process.nextTick(function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure this extra closure is needed. Perhaps it could just be process.nextTick(req.oncomplete, err).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mscdex sometimes req.oncomplete depends on req, eg.

  var context = new ReadFileContext(callback, options.encoding);
  context.isUserFd = isFd(path); // file descriptor ownership
  var req = new FSReqWrap();
  req.context = context;
  req.oncomplete = readFileAfterOpen;

...

function readFileAfterOpen(err, fd) {
  var context = this.context;

  ...
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. Perhaps we could at least factor out the callback then, like:

process.nextTick(onWinOpenErrorNT, req, err);

// ...

function onWinOpenErrorNT(req, err) {
  req.oncomplete(err);
}

@mscdex
Copy link
Contributor

mscdex commented Jun 22, 2017

I wonder if having two separate implementations, one for posix and one for Windows, would help any performance-wise? I think it definitely would on node v6.x, which still uses Crankshaft and would possibly allow the posix version to be inlineable.

@XadillaX
Copy link
Contributor Author

XadillaX commented Jun 22, 2017

@mscdex Now only one call stack is added, and an if statement added. I think it won't slow down too much.

lib/fs.js Outdated
if (isWindows) {
// \\?\c:\somepath\1:path
// \\?\UNC\somepath\2:path
if (longPath.indexOf(':', 6) !== -1) {
Copy link
Contributor

@mscdex mscdex Jun 22, 2017

Choose a reason for hiding this comment

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

If we're going to start explicitly checking for these, perhaps we should be checking for all of the reserved characters, either here in JS or at the libuv layer (via a flag of some kind if not enforced all the time for Windows).

EDIT: I just saw your comment about the other reserved characters. I'm wondering if we actually test for those on Windows nodes in CI...

@silverwind
Copy link
Contributor

Did you test other invalid characters and/or filenames (like com, prn)? Windows has quite peculiar rules on which filenames are considered valid, ideally I think we should test them all:

https://msdn.microsoft.com/en-us/library/windows/desktop/aa365247(v=vs.85).aspx#Naming_Conventions

@XadillaX
Copy link
Contributor Author

Did you test other invalid characters and/or filenames (like com, prn)? Windows has quite peculiar rules on which filenames are considered valid, ideally I think we should test them all:

https://msdn.microsoft.com/en-us/library/windows/desktop/aa365247(v=vs.85).aspx#Naming_Conventions

Not yet. image

I will try it tomorrow, now it's 1:11 a.m. at my location and I'm going to sleep.

@XadillaX
Copy link
Contributor Author

@mscdex @silverwind Sorry, I'm going to take a vacation for one week and now I don't have a Windows computer at my hand. So maybe I will continue to work for this PR after one week.

lib/fs.js Outdated
stringToFlags(options.flag || 'r'),
0o666,
req);
openWrap(path, stringToFlags(options.flags || 'r'), 0o666, req);
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe the stringToFlags() calls should be pushed into openWrap() too.

lib/fs.js Outdated
// \\?\UNC\somepath\2:path
if (longPath.indexOf(':', 6) !== -1) {
const err = new errors.Error('ERR_INVALID_WIN32_PATH', longPath);
if (req) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Style nit: single line if statements shouldn't have curly braces.

@@ -187,6 +187,7 @@ E('ERR_SOCKET_BAD_PORT', 'Port should be > 0 and < 65536');
E('ERR_SOCKET_DGRAM_NOT_RUNNING', 'Not running');
E('ERR_V8BREAKITERATOR', 'full ICU data not installed. ' +
'See https://github.com/nodejs/node/wiki/Intl');
E('ERR_INVALID_WIN32_PATH', (path) => `Invalid path under win32: '${path}'`);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the errors in this file are sorted by error code.

@@ -0,0 +1,76 @@
// Copyright Joyent, Inc. and other Node contributors.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think you have to add this for new files.


const isWindows = process.platform === 'win32';

if (!isWindows) return;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you use common.skip() here.

});

OK.forEach((path) => {
fs.writeFile(path, 'world', { encoding: 'utf8' }, function(err) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add common.mustCall() to the readFile() and writeFile() callbacks in this file.


FAIL.forEach((path) => {
fs.readFile(path, { encoding: 'utf8' }, function(err) {
assert.ok(/Invalid path under win32/.test(err.message));
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use ^ and $ to match the full error message.

@silverwind
Copy link
Contributor

I agree with @mscdex, if we do this checking, it's better done in libuv.

Of course, it's also possible to delegate the task of checking if a filename is valid for a platform to the user, for example through modules like https://github.com/sindresorhus/valid-filename.

@XadillaX
Copy link
Contributor Author

@silverwind To be honest, I think libuv's PR progress is much slower than Node.js'.

I will continue to work for other reserved names by implement a checking function next week after my vacation.

@refack
Copy link
Contributor

refack commented Jun 24, 2017

I will continue to work for other reserved names by implement a checking function next week after my vacation.

Enjoy 🍹

@bzoz
Copy link
Contributor

bzoz commented Jun 27, 2017

This file:smth is an NTFS feature, using > etc in filenames is already correctly handled by Node. Please see my comment in the original issue..

@bzoz
Copy link
Contributor

bzoz commented Jun 27, 2017

Oh, forgot to add that I'm -1 on this.

@refack
Copy link
Contributor

refack commented Jun 27, 2017

Oh, forgot to add that I'm -1 on this.

@bzoz It has an element of surprise (alternative streams I personally keep forgetting about). Should we at least document this? Or maybe add special syntax for?

@bzoz
Copy link
Contributor

bzoz commented Jun 27, 2017

IMHO we should document this. Special syntax would be surprising for everyone that already knows this feature. It would also add maintenance burden.

@XadillaX
Copy link
Contributor Author

@bzoz so what I should do is just to document it?

@bzoz
Copy link
Contributor

bzoz commented Jun 27, 2017

I guess so.

The streams feature of NTFS is surprising, we can mention it in the docs and add a link to MSDN: https://msdn.microsoft.com/en-us/library/windows/desktop/bb540537.aspx

@XadillaX
Copy link
Contributor Author

I'll update this issue to document this feature after I'm back.

@XadillaX XadillaX force-pushed the fix/fs-colon-in-path-under-windows branch from 9498ec8 to e90f5cb Compare July 3, 2017 09:10
@XadillaX XadillaX changed the title fs: fs.write wrong behavior under win32 fs,doc,test: open reserved characters under win32 Jul 3, 2017
@XadillaX
Copy link
Contributor Author

XadillaX commented Jul 3, 2017

@cjihrig, @refack, @bzoz, I've updated this PR to explain the behavior.

@XadillaX XadillaX force-pushed the fix/fs-colon-in-path-under-windows branch from 86dc587 to c1c5d57 Compare July 3, 2017 09:43
This was referenced Sep 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
errors Issues and PRs related to JavaScript errors originated in Node.js core. fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants