test: add test to verify ErrnoException path #13958

danbev · 2017-06-28T11:28:57Z

This commit adds a test to verify that the path argument to
ErrnoException can contain UTF-8 characters.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

src

addaleax · 2017-06-28T11:36:04Z

I don’t think this is a good idea. Always interpreting it as Latin-1 instead of always interpreting it as UTF-8 isn’t going to help anyone; and at least on Unices, it’s somewhat reasonable to assume UTF-8 as the default, even if there is no guarantee that the path is actually UTF-8-encoded. (Not sure about Windows but I think for consistency there would have to be something (C runtime?) that transcodes to UTF-8 for us as well.)

I’d suggest dropping the FIXME(bnoordhuis) bit in the comment and maybe adding that there isn’t really any better option.

/cc @bnoordhuis

danbev · 2017-06-28T11:38:37Z

@addaleax Thanks, I was not sure about this and like you suggested perhaps dropping FIXME and commenting would be good here.

addaleax · 2017-06-28T12:53:54Z

@danbev It might also be good to have a test for this – if tests worked with your current patch, we don’t have one. Something like checking the error thrown by fs.stat[Sync]('nönexistent') might suffice.

danbev · 2017-06-28T14:13:27Z

Something like checking the error thrown by fs.stat[Sync]('nönexistent') might suffice.

I tried that and that succeeds as it looks like that would go through the UVException function which uses UTF-8. I'll look into how a test for this can be written.

jasnell · 2017-06-29T02:47:03Z

src/node.cc

@@ -877,8 +877,7 @@ Local<Value> ErrnoException(Isolate* isolate,

  Local<String> path_string;
  if (path != nullptr) {
-    // FIXME(bnoordhuis) It's questionable to interpret the file path as UTF-8.
-    path_string = String::NewFromUtf8(env->isolate(), path);
+    path_string = FIXED_ONE_BYTE_STRING(env->isolate(), path);


FWIW, there are edge cases where even this is going to be inadequate. Those are the same edge cases that led me to add the ability to pass Buffer instances to fs APIs... specifically, on posix systems, it's possible for a path string to contain multiple encodings... for instance, parent directory in ASCII, directory in Windows-1250, UTF-8 filename, etc. Unfortunately it's not theoretical either... actually had this happen.

May be worthwhile deprecating path_string and moving to a Buffer instance.

It's still an improvement of sorts. Any byte string is a valid Latin-1 string (although not per se a meaningful Latin-1 string) whereas conversion to UTF-8 is lossy - it's not bidirectional when the byte string contains invalid UTF-8 character sequences because those get replaced with U+FFFD.

@bnoordhuis I still think this change would be more confusing than helpful, given that Node defaults to UTF-8 in all places by now, encountering Latin-1 encoded paths is already pretty rare and won’t get more frequent over time.

I don't disagree and to be clear, I don't advocate making changes haphazardly, that wouldn't help.

Aside (and for background): node's use of UTF-8 for file paths is accidental, it wasn't a design choice. V8 originally only supported UTF-8, one-byte strings weren't added until much later.

addaleax · 2017-06-30T12:02:01Z

test/addons/errno-exception/test.js

+const err = binding.errno();
+assert.strictEqual(err.syscall, 'syscall');
+assert.strictEqual(err.errno, 10);
+assert.strictEqual(err.path, 'päth');


As a tiny suggestion, maybe check err.message as well :)

Oh, yes :) Thanks

danbev · 2017-08-09T06:30:18Z

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

danbev · 2017-08-09T08:43:31Z

test/arm failure looks unrelated

console output:

out/Release/cctest --gtest_output=tap:cctest.tap
[==========] Running 48 tests from 6 test cases.
[----------] Global test environment set-up.
[----------] 2 tests from Base64Test
[ RUN      ] Base64Test.Encode
[       OK ] Base64Test.Encode (0 ms)
[ RUN      ] Base64Test.Decode
[       OK ] Base64Test.Decode (1 ms)
[----------] 2 tests from Base64Test (1 ms total)

[----------] 2 tests from EnvironmentTest
[ RUN      ] EnvironmentTest.AtExitWithEnvironment
[       OK ] EnvironmentTest.AtExitWithEnvironment (31 ms)
[ RUN      ] EnvironmentTest.AtExitWithArgument
Received signal 11 SEGV_MAPERR 000007e966dc

==== C stack trace ===============================

[end of stack trace]

danbev · 2017-08-09T10:42:07Z

@addaleax Just wanted to ask if your approve still holds? I've rebased and reworded the commit message to reflect that this commit is now only adding a test and nothing else.

Regarding the failure arm failure I'm going to take a closer look at it now.

tniessen

The code itself LGTM.

gibfahn

LGTM modulo whitespace/comment nit.

gibfahn · 2017-08-12T23:50:08Z

test/addons/errno-exception/test.js

+const assert = require('assert');
+const binding = require(`./build/${common.buildType}/binding`);
+const err = binding.errno();
+assert.strictEqual(err.syscall, 'syscall');


Nit time (from the guide)...

'use strict'; const common = require('../../common'); // Verify that the path argument to node::ErrnoException() can contain UTF-8 // characters. const assert = require('assert'); const binding = require(`./build/${common.buildType}/binding`); const err = binding.errno(); assert.strictEqual(err.syscall, 'syscall'); assert.strictEqual(err.errno, 10); assert.strictEqual(err.path, 'päth');

Thanks, I'll update this.

This commit adds a test to verify that the path argument to ErrnoException can contain UTF-8 characters.

danbev · 2017-08-13T06:54:14Z

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

gibfahn

Perfect, thanks

This commit adds a test to verify that the path argument to ErrnoException can contain UTF-8 characters. PR-URL: nodejs#13958 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]>

danbev · 2017-08-14T05:32:36Z

Landed in 95c8df1

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Jun 28, 2017

addaleax added errors Issues and PRs related to JavaScript errors originated in Node.js core. semver-major PRs that contain breaking changes and should be released in the next major version. labels Jun 28, 2017

jasnell reviewed Jun 29, 2017

View reviewed changes

danbev force-pushed the error-fixed-one-byte-string branch from 0592a8a to 2fc1bb7 Compare June 30, 2017 07:44

addaleax approved these changes Jun 30, 2017

View reviewed changes

danbev force-pushed the error-fixed-one-byte-string branch from b83d30e to 8de6e76 Compare August 9, 2017 06:28

danbev changed the title ~~src: make path_string FIXED_ONE_BYTE_STRING~~ test: add test to verify ErrnoException path Aug 9, 2017

tniessen approved these changes Aug 9, 2017

View reviewed changes

gibfahn approved these changes Aug 12, 2017

View reviewed changes

danbev added 2 commits August 13, 2017 08:52

test: add test to verify ErrnoException path

b5f2c51

This commit adds a test to verify that the path argument to ErrnoException can contain UTF-8 characters.

squash: make test.js follow test style guidelines

32a3027

danbev force-pushed the error-fixed-one-byte-string branch from 2248040 to 32a3027 Compare August 13, 2017 06:53

gibfahn approved these changes Aug 13, 2017

View reviewed changes

danbev closed this Aug 14, 2017

danbev deleted the error-fixed-one-byte-string branch August 14, 2017 05:32

jasnell mentioned this pull request Oct 30, 2017

Proposal: 2017-10-31, Version 9.0.0 (Current) #15135

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: add test to verify ErrnoException path #13958

test: add test to verify ErrnoException path #13958

danbev commented Jun 28, 2017 •

edited

Loading

addaleax commented Jun 28, 2017

danbev commented Jun 28, 2017

addaleax commented Jun 28, 2017

danbev commented Jun 28, 2017

jasnell Jun 29, 2017

bnoordhuis Jun 29, 2017

addaleax Jun 29, 2017

bnoordhuis Jun 29, 2017

addaleax Jun 30, 2017

danbev Jun 30, 2017

danbev commented Aug 9, 2017

danbev commented Aug 9, 2017

danbev commented Aug 9, 2017

tniessen left a comment

gibfahn left a comment

gibfahn Aug 12, 2017

danbev Aug 13, 2017

danbev commented Aug 13, 2017

gibfahn left a comment

danbev commented Aug 14, 2017

test: add test to verify ErrnoException path #13958

test: add test to verify ErrnoException path #13958

Conversation

danbev commented Jun 28, 2017 • edited Loading

Checklist

Affected core subsystem(s)

addaleax commented Jun 28, 2017

danbev commented Jun 28, 2017

addaleax commented Jun 28, 2017

danbev commented Jun 28, 2017

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

Choose a reason for hiding this comment

danbev commented Aug 9, 2017

danbev commented Aug 9, 2017

danbev commented Aug 9, 2017

tniessen left a comment

Choose a reason for hiding this comment

gibfahn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danbev commented Aug 13, 2017

gibfahn left a comment

Choose a reason for hiding this comment

danbev commented Aug 14, 2017

danbev commented Jun 28, 2017 •

edited

Loading