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

os: don't use getCheckedFunction() in userInfo() #22599

Closed
wants to merge 1 commit into from

Conversation

danbev
Copy link
Contributor

@danbev danbev commented Aug 30, 2018

os.userInfo() takes an optional object as its first argument.
getCheckedFunction() adds a context object to the argument list
passed to the binding layer. The context object has the potential
to confuse the binding layer, particularly if an error occurs.
This commit makes userInfo() explicitly call into the binding
layer with two arguments.

We ran into this issue when running node on OpenShift where the user ID
will be assigned by OpenShift regardless what is specified in the
DockerFile. This leads to the command os.userInfo() to fail with the
following error:

sh-4.2$ node -p 'os.userInfo()'
node[99]: ../src/node_os.cc:364:void node::os::GetUserInfo(const
v8::FunctionCallbackInfo<v8::Value>&):
Assertion `(args.Length()) >= (2)' failed.
 1: 0x986d00 node::Abort() [node]
 2: 0x986df1  [node]
 3: 0x9f76ec  [node]
 4: 0xc07547  [node]
 5: 0xc08cad  [node]
 6: 0xc08d26 v8::internal::Builtin_HandleApiCall(int,
v8::internal::Object**, v8::internal::Isolate*) [node]
 7: 0x858ecf5c01d
Aborted

Using a command like whoami also fails:

sh-4.2$ id
uid=1000030000 gid=0(root) groups=0(root),1000030000

sh-4.2$ whoami
whoami: cannot find name for user ID 1000030000

The issue here is that we are not getting the libuv error as the check
is prior to that. Debugging this we can see the errno:

sh-4.2$ gdb --args node -p 'os.userInfo()'
(gdb) br node_os.cc:364
Breakpoint 1 at 0x9f74b8: node_os.cc:364. (2 locations)
(gdb) r
(gdb) print err
$1 = -2
(gdb)  print uv_err_name($1)
$3 = 0x1655163 "ENOENT"

This commit changes the args check to check that the length is one
instead of two.

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

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. os Issues and PRs related to the os subsystem. labels Aug 30, 2018
@danbev
Copy link
Contributor Author

danbev commented Aug 30, 2018

@cjihrig
Copy link
Contributor

cjihrig commented Aug 30, 2018

So, this is a valid fix, but I don't think it's the most correct one. Please take a look at cjihrig@00f21ee for an alternative fix. The issue is that os.userInfo() takes an optional object as its argument, and the JS layer adds a context object when calling the binding layer. I think it's better to always explicitly call the binding layer with two arguments.

@danbev
Copy link
Contributor Author

danbev commented Aug 30, 2018

@cjihrig Ah, that is much better, thanks!
Would you like to open a PR for that (and I'll close this one) or are you alright with me just using your commit?

@cjihrig
Copy link
Contributor

cjihrig commented Aug 30, 2018

Opened #22609

os.userInfo() takes an optional object as its first argument.
getCheckedFunction() adds a context object to the argument list
passed to the binding layer. The context object has the potential
to confuse the binding layer, particularly if an error occurs.
This commit makes userInfo() explicitly call into the binding
layer with two arguments.
@danbev danbev changed the title src: remove incorrect check from GetUserInfo os: don't use getCheckedFunction() in userInfo() Aug 31, 2018
@danbev danbev closed this Aug 31, 2018
@danbev danbev deleted the userInfo_issue branch August 31, 2018 04:18
cjihrig added a commit to cjihrig/node that referenced this pull request Sep 3, 2018
os.userInfo() takes an optional object as its first argument.
getCheckedFunction() adds a context object to the argument list
passed to the binding layer. The context object has the potential
to confuse the binding layer, particularly if an error occurs.
This commit makes userInfo() explicitly call into the binding
layer with two arguments.

PR-URL: nodejs#22609
Refs: nodejs#22599
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
targos pushed a commit that referenced this pull request Sep 3, 2018
os.userInfo() takes an optional object as its first argument.
getCheckedFunction() adds a context object to the argument list
passed to the binding layer. The context object has the potential
to confuse the binding layer, particularly if an error occurs.
This commit makes userInfo() explicitly call into the binding
layer with two arguments.

PR-URL: #22609
Refs: #22599
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
targos pushed a commit that referenced this pull request Sep 6, 2018
os.userInfo() takes an optional object as its first argument.
getCheckedFunction() adds a context object to the argument list
passed to the binding layer. The context object has the potential
to confuse the binding layer, particularly if an error occurs.
This commit makes userInfo() explicitly call into the binding
layer with two arguments.

PR-URL: #22609
Refs: #22599
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. os Issues and PRs related to the os subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants