Inconsistent EPERM errno

[guybedford]

Version

All

Platform

All

Subsystem

process

What steps will reproduce the bug?

e.g. process.setgid(100) - returns EPERM with errno set to 1.

How often does it reproduce? Is there a required condition?

Always.

What is the expected behavior? Why is that the expected behavior?

There should likely be more consistenty wrt EPERM error numbers - UV_ error numbers are supposed to always be negative not positive. So the code should probably be -1 not 1.

What do you see instead?

The code reported is always 1.

// cc @jasnell

Additional information

No response

[jasnell]

@nodejs/tsc ... particularly interested in your thoughts on this. It's not clear at all what the correct value for the errno property is.

[juanarbol]

Is not that an "expected" behaviour?

static const gid_t gid_not_found = static_cast<gid_t>(-1);
// ...
  gid_t gid = gid_by_name(env->isolate(), args[0]);

  if (gid == gid_not_found) {
    // Tells JS to throw ERR_INVALID_CREDENTIAL
    args.GetReturnValue().Set(1);
  } else if (setgid(gid)) {
    env->ThrowErrnoException(errno, "setgid");
  } else {
    args.GetReturnValue().Set(0);
  }

Refs: https://github.com/nodejs/node/blob/main/src/node_credentials.cc#L302

[jasnell]

The issue is that it is inconsistent with the errno values we use elsewhere.

[jasnell]

For a good example, look at the test/parallel/test-dns-resolve-promises.js, which intentionally triggers an EPERM whose errno is -1 and not 1. Also look at test/parallel/test-fs- error-messages.js which very specifically tests that the errno property on an EPERM is equal to UV_EPERM

[juanarbol]

I mean, the setgid call is not handled by libuv, which uses UV__ERR¹ and sets errno to its negative.

By definition:

The <errno.h> header shall define the following macros which shall expand to integer constant expressions with type int, distinct positive values²

This can be solved just by flipping the sign... I mean, EPERM is 1... That's a common practice in kernel applications, consider this as an example

Logic seems fine.

setgid failed: Operation not permitted - 1
Process 185474 stopped
* thread #1, name = 'MainThread', stop reason = step over
    frame #0: 0x0000555557116fa3 node_g`node::credentials::SetGid(args=0x00007fffffffb0e0) at node_credentials.cc:305:5
   302      args.GetReturnValue().Set(1);
   303    } else if (setgid(gid)) {
   304      fprintf(stdout, "setgid failed: %s - %d\n", strerror(errno), errno);
-

This simple diff fixes the issue, but breaks a single test.

diff --git a/src/api/exceptions.cc b/src/api/exceptions.cc
index 871fe78de9..91549d78e7 100644
--- a/src/api/exceptions.cc
+++ b/src/api/exceptions.cc
@@ -56,7 +56,7 @@ Local<Value> ErrnoException(Isolate* isolate,
   Local<Object> obj = e.As<Object>();
   obj->Set(context,
            env->errno_string(),
-           Integer::New(isolate, errorno)).Check();
+           Integer::New(isolate, errorno > 0 ? -errorno : errorno)).Check();
   obj->Set(context, env->code_string(), estring).Check();

   if (path_string.IsEmpty() == false) {

=== debug test ===
Path: addons/errno-exception/test
node:assert:92
  throw new AssertionError(obj);
  ^

AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:

-10 !== 10

    at Object.<anonymous> (/home/juan/GitHub/node/test/addons/errno-exception/test.js:13:8)
    at Module._compile (node:internal/modules/cjs/loader:1691:14)
    at Object..js (node:internal/modules/cjs/loader:1823:10)
    at Module.load (node:internal/modules/cjs/loader:1426:32)
    at Module._load (node:internal/modules/cjs/loader:1249:12)
    at TracingChannel.traceSync (node:diagnostics_channel:322:14)
    at wrapModuleLoad (node:internal/modules/cjs/loader:235:24)
    at Module.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:152:5)
    at node:internal/main/run_main_module:33:47 {
  generatedMessage: true,
  code: 'ERR_ASSERTION',
  actual: -10,
  expected: 10,
  operator: 'strictEqual'
}

Footnotes

1. https://github.com/libuv/libuv/blob/b00c5d1a09c094020044e79e19f478a25b8e1431/include/uv/errno.h#L27 ↩

2. https://pubs.opengroup.org/onlinepubs/9699919799.2018edition/basedefs/errno.h.html ↩

[jasnell]

Unfortunately changing the value is like a semver-major change.

There's a few inconsistencies to handle... The errno property on SystemError (https://nodejs.org/docs/latest/api/errors.html#errorerrno) is specifically documented as a "negative number which corresponds to the error code defined in libuv Error handling", and yet we've obviously not been sticking to that definition and it's unclear if all errors that have an errno are expected to conform to SystemError or not... tho SystemError is the only error type in the documentation where errno is mentioned.

[juanarbol]

What about using uv_translate_sys_error¹ in the ErrnoException method? That will make it stick to the definition no matter what.

NVM, still a breaking change. Ok, I said nothing.

Footnotes

1. https://docs.libuv.org/en/v1.x/errors.html#c.uv_translate_sys_error ↩

[jasnell]

Yeah unfortunately I think we will need to go through and audit every place we assign the errno and make sure it's a negative value that aligns with the UV error codes.. and unfortunately that's going to be a semver-major change. We should try to update them all at once.