Migrate process errors from C++ to JS #19973

targos · 2018-04-12T12:24:46Z

commit 2b3b84822baa258fe67f17979b17fac523455612

lib: introduce internal/validators

Create a file to centralize argument validators that are used in
multiple internal modules.
Move validateInt32 and validateUint32 to this file.

commit aeb33c9b0d4be6693d7eda0810b4c3f4d213d743

process: migrate methods to throw errors with code

Migrate some methods from node.cc to JS in order to properly throw errors
with codes.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

BridgeAR

Just had a brief glance at it and it looks very good. I am going to have a closer look at it later on again to give a LG.

This does conflict with at least one other PR at the moment though that I expect to land earlier. Just as a heads up.

BridgeAR · 2018-04-12T12:37:27Z

src/node.cc

-  if (!args[1]->IsUint32() && !args[1]->IsString()) {
-    return env->ThrowTypeError("argument 2 must be a number or a string");
-  }
+  CHECK_EQ(args.Length(), 1);


Should this not be 2?

Right. That means we don't have any test for this function 😱

addaleax · 2018-04-12T13:46:21Z

lib/internal/errors.js

@@ -837,6 +837,7 @@ E('ERR_INVALID_SYNC_FORK_INPUT',
  TypeError);
 E('ERR_INVALID_THIS', 'Value of "this" must be of type %s', TypeError);
 E('ERR_INVALID_TUPLE', '%s must be an iterable %s tuple', TypeError);
+E('ERR_INVALID_UNIX_ID', 'Unix %s id does not exist: %s', Error);


ERR_INVALID_CREDENTIAL/ERR_UNKNOWN_CREDENTIAL? The error message is unambiguous, but it’s not really clear what’s meant by “Unix id” from just seeing the error code, imo

What about ERR_UNKNOWN_IDENTIFIER ?

And changing the message to "Group/User identifier does not exist: %s" ?

@targos I think that might be a bit generic, especially since “unknown identifier” is a term that has a different meaning in a programming language context

I’m fine with changing the error message to what you’re suggesting, but I’m also fine with it the way it is right now :)

ERR_INVALID_CREDENTIAL sounds better to me since you'd do man credentials to read the manual on this...or maybe ERR_INVALID_PRIVILEGES?

Oh, I changed it to ERR_INVALID_CREDENTIAL but forgot to push. Done now.

joyeecheung · 2018-04-12T13:08:02Z

lib/internal/process/methods.js

+    validateId(user, 'user');
+    validateId(extraGroup, 'extraGroup');
+    const result = _initgroups(user, extraGroup);
+    if (result === 1) {


This seems rather magical, can you put in a comment around these return values or use some kind of constants shared by both sides?

joyeecheung · 2018-04-12T13:15:52Z

src/node.cc

-        oct += c - '0';
-      }
-    }
+    int oct = args[0]->Uint32Value();;


Can you use the .As<Uint32>()->Value() cast here instead of the deprecated API?

joyeecheung · 2018-04-12T18:40:29Z

lib/internal/errors.js

@@ -837,6 +837,7 @@ E('ERR_INVALID_SYNC_FORK_INPUT',
  TypeError);
 E('ERR_INVALID_THIS', 'Value of "this" must be of type %s', TypeError);
 E('ERR_INVALID_TUPLE', '%s must be an iterable %s tuple', TypeError);
+E('ERR_INVALID_UNIX_ID', 'Unix %s id does not exist: %s', Error);


ERR_INVALID_CREDENTIAL sounds better to me since you'd do man credentials to read the manual on this...or maybe ERR_INVALID_PRIVILEGES?

joyeecheung · 2018-04-12T18:45:00Z

test/parallel/test-umask.js

+  assert.throws(() => {
+    process.umask(value);
+  }, {
+    code: 'ERR_INVALID_ARG_TYPE',


This error message reads somewhat odd to me. Shouldn't this be a ERR_INVALID_ARG_VALUE?

jasnell · 2018-04-14T19:08:08Z

@targos ... this needs a quick rebase if you would.

targos · 2018-04-15T10:06:11Z

Updated!

@joyeecheung I think I addressed all your comments.

BridgeAR

LGTM, just a few nits / suggestions.

BridgeAR · 2018-04-15T21:52:10Z

lib/internal/process/methods.js

+
+  function setgroups(groups) {
+    if (!Array.isArray(groups)) {
+      throw new ERR_INVALID_ARG_TYPE('groups', 'array', groups);


It should be Array

BridgeAR · 2018-04-15T21:54:14Z

lib/internal/process/methods.js

+  function umask(mask) {
+    if (typeof mask === 'undefined') {
+      return _umask(mask);
+    } else if (typeof mask === 'number') {


Nit: do not use the else in case it is not required due to the return.

BridgeAR · 2018-04-15T21:57:01Z

lib/internal/validators.js

@@ -0,0 +1,58 @@
+'use strict';
+


I like the consolidation of validation functions!

BridgeAR · 2018-04-15T21:58:39Z

lib/internal/validators.js

+      // 2 ** 31 === 2147483648
+      err = new ERR_OUT_OF_RANGE(name, '> -2147483649 && < 2147483648', value);
+    }
+    Error.captureStackTrace(err, validateInt32);


Suggestion: I think it is best to actually keep the internal structure visible in most cases. So I personally would not remove this frame. The same applies to the other validation functions.

I prefer not to change this in this PR.

BridgeAR · 2018-04-15T22:00:49Z

lib/internal/validators.js

+    } else if (!Number.isInteger(value)) {
+      err = new ERR_OUT_OF_RANGE(name, 'an integer', value);
+    } else {
+      const min = positive ? 1 : 0;


Suggestion: it is not much overhead but the default argument is slower than not having a default in all cases. The check itself could stay as it is, since it already tests for truthy values only.

I removed the default value

BridgeAR · 2018-04-15T22:04:14Z

lib/internal/validators.js

+      err = new ERR_OUT_OF_RANGE(name, 'an integer', value);
+    } else {
+      // 2 ** 31 === 2147483648
+      err = new ERR_OUT_OF_RANGE(name, '> -2147483649 && < 2147483648', value);


Just as a note: as far as I know, this function should actually check >= 0 && ... in all call cases. So we might want to rename the function / add a new one but this does not have to be in this PR.

BridgeAR · 2018-04-15T22:07:38Z

src/node.cc

-        oct += c - '0';
-      }
-    }
+    int oct = args[0].As<Uint32>()->Value();;


Redundant semicolon.

BridgeAR · 2018-04-15T22:11:41Z

test/parallel/test-process-chdir.js

+  message: /The "directory" argument must be of type string/
+};
+common.expectsError(function() { process.chdir({}); }, err);
+common.expectsError(function() { process.chdir(); }, err);


Please keep the third test but just change it to: process.chdir('x', 'y'); // Should not throw. That makes sure we have no regression in case the function is changed at any point in time.

Besides that it would also be good to keep assert.throws as it has a much nicer error output and is otherwise almost identical.

Please keep the third test but just change it to: process.chdir('x', 'y'); // Should not throw. That makes sure we have no regression in case the function is changed at any point in time.

I don't think we need this test. I removed the check because in other functions we never throw when too many arguments are passed.

Besides that it would also be good to keep assert.throws as it has a much nicer error output and is otherwise almost identical.

I know our opinions differ here, but I really like to be able to pass a RegExp. I don't have to put the exact string, so:

I don't need to make the test fail to know exactly what to put for the message.

I can use one object validator for multiple test cases (that's what I did here, as error the message is not exactly the same for those cases).

Message validator is shorter, so less likely to go over 80 chars and also less like to have to be updated in case we change something in the message format.

BridgeAR · 2018-04-15T22:12:53Z

test/parallel/test-umask.js

@@ -43,8 +43,17 @@ assert.strictEqual(old, process.umask());



Would you be so kind and rename this test file to test-process-umask.js?

targos · 2018-04-17T08:18:37Z

CI: https://ci.nodejs.org/job/node-test-pull-request/14345/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1368/

targos · 2018-04-18T15:22:40Z

@nodejs/tsc this needs more TSC reviews, please.

jasnell · 2018-04-18T18:19:00Z

I'm going to take this off the 10.0.0 milestone. It would need to land today and would need explicit @nodejs/tsc approval (not just no-objections) in order to make it in to 10.0.0 as I'm going to be cutting the release candidate build tomorrow.

Create a file to centralize argument validators that are used in multiple internal modules. Move validateInt32 and validateUint32 to this file.

Migrate some methods from node.cc to JS in order to properly throw errors with codes.

targos · 2018-04-19T08:24:06Z

Hopefully fixed the Windows errors by defining POSIX-specific methods only if they exist originally on the process object.

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

BridgeAR · 2018-04-23T09:32:24Z

lib/internal/process/methods.js

+                                        'must be an octal string');
+      }
+      const octal = Number.parseInt(mask, 8);
+      validateUint32(octal, 'mask');


Suggestion: This is actually only testing for <= 2 ** 32. All other checks are already done due to the RegExp above.
I personally would either remove the extra check or make the whole function stricter to only accept valid entries.
The reason validateUint32 is used for numbers is only about making sure no negative numbers got passed in (as far as I know).

Do you know what is the largest allowed value?

@targos In some fs methods we throw if the mode is larger than 0o777. It is not strictly documented wether we mask off or throw if the mode_t type of value passed to any API is larger than that though.

What happens in the system if the value is larger than 0o777 ?
For example if you pass 0o12345 ? is it truncated to 0o345 ?

Suggestion: add a TODO to investigate and land the PR. In fs there are a couple of validations that need improvements, especially having more consistent checks. I am also working on some of those.

@targos Yes it gets truncated/masked. http://man7.org/linux/man-pages/man2/umask.2.html

umask() sets the calling process's file mode creation mask (umask) to mask & 0777 (i.e., only the file permission bits of mask are used), and returns the previous value of the mask.

BridgeAR · 2018-04-26T17:45:05Z

Landed in e836128 and 2fd248f 🎉

I took the freedom to ignore my own comment.

Create a file to centralize argument validators that are used in multiple internal modules. Move validateInt32 and validateUint32 to this file. PR-URL: nodejs#19973 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com>

Migrate some methods from node.cc to JS in order to properly throw errors with codes. PR-URL: nodejs#19973 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com>

To ease future backports, create the process/methods file introduced in nodejs#19973. This commit just adds the JS functions that forward calls to C++ and does not change type checking.

To ease future backports, create the process/methods file introduced in #19973. This commit just adds the JS functions that forward calls to C++ and does not change type checking. PR-URL: #21172 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

targos added semver-major PRs that contain breaking changes and should be released in the next major version. errors Issues and PRs related to JavaScript errors originated in Node.js core. labels Apr 12, 2018

targos added this to the 10.0.0 milestone Apr 12, 2018

targos requested review from joyeecheung, BridgeAR and a team April 12, 2018 12:24

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Apr 12, 2018

targos added the process Issues and PRs related to the process subsystem. label Apr 12, 2018

BridgeAR reviewed Apr 12, 2018

View reviewed changes

addaleax reviewed Apr 12, 2018

View reviewed changes

joyeecheung reviewed Apr 12, 2018

View reviewed changes

BridgeAR self-requested a review April 13, 2018 19:17

targos force-pushed the error-codes-nodecc branch from be87ffb to 905e138 Compare April 15, 2018 09:19

targos force-pushed the error-codes-nodecc branch from 905e138 to d7bd31d Compare April 15, 2018 10:06

joyeecheung approved these changes Apr 15, 2018

View reviewed changes

BridgeAR approved these changes Apr 15, 2018

View reviewed changes

targos force-pushed the error-codes-nodecc branch from d7bd31d to 29f1182 Compare April 17, 2018 07:53

jasnell removed this from the 10.0.0 milestone Apr 18, 2018

jasnell approved these changes Apr 18, 2018

View reviewed changes

targos added 3 commits April 19, 2018 09:53

lib: introduce internal/validators

8a80bf7

Create a file to centralize argument validators that are used in multiple internal modules. Move validateInt32 and validateUint32 to this file.

process: migrate methods to throw errors with code

0ea5929

Migrate some methods from node.cc to JS in order to properly throw errors with codes.

fixup! fix Windows

6e43560

targos force-pushed the error-codes-nodecc branch from 29f1182 to 6e43560 Compare April 19, 2018 08:11

BridgeAR reviewed Apr 23, 2018

View reviewed changes

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 23, 2018

nodejs deleted a comment Apr 23, 2018

BridgeAR closed this Apr 26, 2018

targos deleted the error-codes-nodecc branch April 26, 2018 20:48

joyeecheung mentioned this pull request May 4, 2018

fs: too strict range check on 'mode' parameter #20498

Closed

MylesBorins mentioned this pull request May 22, 2018

fs: improve fchmod{Sync} validation #20588

Merged

2 tasks

targos mentioned this pull request Jun 5, 2018

[v10.x] lib: introduce internal/validators #21149

Closed

2 tasks

targos mentioned this pull request Jun 6, 2018

[v10.x] Backport pending pull requests #21172

Closed

gireeshpunathil mentioned this pull request Apr 10, 2020

process.setuid results in an abort #32750

Closed

		@@ -43,8 +43,17 @@ assert.strictEqual(old, process.umask());

Migrate process errors from C++ to JS #19973

Migrate process errors from C++ to JS #19973

Conversation

targos commented Apr 12, 2018 • edited Loading

Checklist

BridgeAR left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented Apr 14, 2018

targos commented Apr 15, 2018

BridgeAR left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

targos commented Apr 17, 2018

targos commented Apr 18, 2018

jasnell commented Apr 18, 2018

targos commented Apr 19, 2018

BridgeAR Apr 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joyeecheung Apr 23, 2018 • edited Loading

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

BridgeAR commented Apr 26, 2018

targos commented Apr 12, 2018 •

edited

Loading

BridgeAR Apr 23, 2018 •

edited

Loading

joyeecheung Apr 23, 2018 •

edited

Loading