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

src: call linux getauxval(AT_SECURE) in SafeGetenv #12548

Closed
wants to merge 3 commits into from

Conversation

@danbev
Copy link
Member

commented Apr 20, 2017

This commit attempts to fix the following TODO:

// TODO(bnoordhuis) Should perhaps also check whether getauxval(AT_SECURE) is non-zero on Linux.
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

src

@danbev

This comment has been minimized.

Copy link
Member Author

commented Apr 20, 2017

@cjihrig
Copy link
Contributor

left a comment

LGTM if the CI is happy... but it appears not to be so far.

@bnoordhuis

This comment has been minimized.

Copy link
Member

commented Apr 20, 2017

I'm afraid you can't use plain getauxval() because it is a fairly recent addition to glibc (>=2.16) and we support (much) older versions.

You can get the value of AT_SECURE by rooting through the auxiliary vector that the kernel puts after the programs arguments and the environment but that is complicated by the relocating of argv and environ that uv_setup_args() does right after startup.

Perhaps the best option is to do it as early as possible, in main() in src/node_main.cc. Doing it in Start() won't work because it could be an embedder-provided argv vector.

EDIT: I owe you an apology for the comment. I was using "call getauxval(AT_SECURE)" as a shorthand for "call our lovingly handcrafted homegrown getauxval(AT_SECURE) reimplementation that libc does not provide" but there is no way you could have guessed that from looking at it.

@Fishrock123

This comment has been minimized.

Copy link
Member

commented Apr 20, 2017

Maybe we can use this if node is compiled with a version that supports it? idk what our guidelines would be for #if statements like that. (I think that is possible to begin with...)

@danbev

This comment has been minimized.

Copy link
Member Author

commented Apr 21, 2017

Maybe we can use this if node is compiled with a version that supports it? idk what our guidelines would be for #if statements like that. (I think that is possible to begin with...)

I'll try this and see if I can get that right and have it pass the CI tests. Let me know if this is what you hade in mind.

@danbev

This comment has been minimized.

Copy link
Member Author

commented Apr 21, 2017

@danbev

This comment has been minimized.

Copy link
Member Author

commented Apr 21, 2017

I was using "call getauxval(AT_SECURE)" as a shorthand for "call our lovingly handcrafted homegrown getauxval(AT_SECURE)

Just trying to see if I understand what should happen here. We need to retrieve the auxiliary vector information in node_main, and store the key value pairs so that when the node version of getauxval is called the value can be returned?

@bnoordhuis

This comment has been minimized.

Copy link
Member

commented Apr 21, 2017

idk what our guidelines would be for #if statements like that.

Not allowed, but dynamic detection with dlsym() is.

Rationale: a binary compiled on a system with getauxval() would be unusable on a system without.

We need to retrieve the auxiliary vector information in node_main, and store the key value pairs so that when the node version of getauxval is called the value can be returned?

Correct. You don't need to copy out the aux vector (libuv doesn't touch it) but neither would it hurt.

@danbev danbev force-pushed the danbev:getauxval_at_secure branch Apr 24, 2017

@danbev

This comment has been minimized.

Copy link
Member Author

commented Apr 24, 2017

Correct. You don't need to copy out the aux vector (libuv doesn't touch it) but neither would it hurt.

Thanks for the verifying. I've made an attempt at this now. It lacks any form of tests but if this looks like I'm on the right path I'll take a look at adding something. I've just manually tested using the following steps for now:

$ setcap cap_net_raw+ep out/Release/node
$ NODE_PENDING_DEPRECATION="1" out/Release/node -p "process.binding('config').pendingDeprecation"
true
$ useradd test
$ su test
$ NODE_PENDING_DEPRECATION="1" out/Release/node -p "process.binding('config').pendingDeprecation"
undefined
@danbev

This comment has been minimized.

Copy link
Member Author

commented Apr 24, 2017

@danbev danbev force-pushed the danbev:getauxval_at_secure branch May 5, 2017

@danbev

This comment has been minimized.

Copy link
Member Author

commented May 6, 2017

@danbev danbev force-pushed the danbev:getauxval_at_secure branch May 17, 2017

@danbev

This comment has been minimized.

Copy link
Member Author

commented May 17, 2017

Rebased CI: https://ci.nodejs.org/job/node-test-pull-request/8117/
I want to trigger the failures again to try to figure out the issues.

@danbev danbev force-pushed the danbev:getauxval_at_secure branch May 18, 2017

@danbev

This comment has been minimized.

Copy link
Member Author

commented May 18, 2017

test/arm seems to have completed successfully

https://ci.nodejs.org/job/node-test-commit-arm/nodes=armv7-ubuntu1404/9741/console:

Run condition [Always] enabling perform for step [[Execute a set of scripts, Execute a set of scripts]]
TAP Reports Processing: START
Looking for TAP results report in workspace using pattern: *.tap
Saving reports...
Processing '/var/lib/jenkins/jobs/node-test-commit-arm/configurations/axis-nodes/armv7-ubuntu1404/builds/9741/tap-master-files/cctest.tap'
Parsing TAP test result [/var/lib/jenkins/jobs/node-test-commit-arm/configurations/axis-nodes/armv7-ubuntu1404/builds/9741/tap-master-files/cctest.tap].
Processing '/var/lib/jenkins/jobs/node-test-commit-arm/configurations/axis-nodes/armv7-ubuntu1404/builds/9741/tap-master-files/test.tap'
Parsing TAP test result [/var/lib/jenkins/jobs/node-test-commit-arm/configurations/axis-nodes/armv7-ubuntu1404/builds/9741/tap-master-files/test.tap].
TAP Reports Processing: FINISH
Checking ^not ok
Notifying upstream projects of job completion
Finished: SUCCESS
@danbev

This comment has been minimized.

Copy link
Member Author

commented May 18, 2017

@bnoordhuis Could you take another look at this when you get a chance?

src/node.cc Outdated
#if defined(__linux__)
AuxVector::AuxVector() {
char** envp = environ;
while (*envp++ != NULL) {}

This comment has been minimized.

Copy link
@bnoordhuis

bnoordhuis May 18, 2017

Member

nullptr

This comment has been minimized.

Copy link
@danbev

danbev May 18, 2017

Author Member

I'll fix this.

src/node.cc Outdated
Elf_auxv_t* auxv = reinterpret_cast<Elf_auxv_t*>(envp);
for (; auxv->a_type != AT_NULL; auxv++) {
auxv_map_[auxv->a_type] = auxv->a_un.a_val;
}

This comment has been minimized.

Copy link
@bnoordhuis

bnoordhuis May 18, 2017

Member

You should skip AT_IGNORE entries here. That said, I'd make this the simplest possible implementation and not bother with a map since we're only interested in AT_SECURE. E.g. bool linux_at_secure; in node.cc in and set it in node_main.cc if AT_SECURE is present (with an extern bool linux_at_secure; declaration to make it link.)

I'd be okay with unconditionally defining the bool to save on #ifdef __linux__ ugliness.

This comment has been minimized.

Copy link
@danbev

danbev May 18, 2017

Author Member

That sounds much better. I'll give that a go and see what you think.

src/node.h Outdated
void operator=(AuxVector const&);
std::map<std::size_t, std::size_t> auxv_map_;
};
#endif

This comment has been minimized.

Copy link
@bnoordhuis

bnoordhuis May 18, 2017

Member

This should not go into node.h, it's a public header. node_internals.h is okay, though.

This comment has been minimized.

Copy link
@danbev

danbev May 18, 2017

Author Member

I'll move it. Thanks

src: add linux getauxval(AT_SECURE) in SafeGetenv
This commit attempts to fix the following TODO:
// TODO(bnoordhuis) Should perhaps also check whether
getauxval(AT_SECURE) is non-zero on Linux.

This can be manually tested at the moment using the following steps:

$ setcap cap_net_raw+ep out/Release/node
$ NODE_PENDING_DEPRECATION="1" out/Release/node -p
"process.binding('config').pendingDeprecation"
true
$ useradd test
$ su test
$ NODE_PENDING_DEPRECATION="1" out/Release/node -p
"process.binding('config').pendingDeprecation"
undefined

@danbev danbev force-pushed the danbev:getauxval_at_secure branch to a7c7bea May 18, 2017

@danbev

This comment has been minimized.

Copy link
Member Author

commented May 22, 2017

@bnoordhuis I've made another attempt on this and tried to follow your suggestions. Could you take another look and see if this was what you had in mind? Thanks

@bnoordhuis
Copy link
Member

left a comment

LGTM modulo nit and question. Nice work.

#else
#define Elf_auxv_t Elf32_auxv_t
#endif // __LP64__
extern char **environ;

This comment has been minimized.

Copy link
@bnoordhuis

bnoordhuis May 24, 2017

Member

Style nit: extern char** environ;

This comment has been minimized.

Copy link
@danbev

danbev May 24, 2017

Author Member

I'll fix this.

@@ -71,7 +71,33 @@ int wmain(int argc, wchar_t *wargv[]) {
}
#else
// UNIX
#ifdef __linux__
#include <elf.h>
#include <inttypes.h>

This comment has been minimized.

Copy link
@bnoordhuis

bnoordhuis May 24, 2017

Member

What do you need inttypes.h for?

@danbev

This comment has been minimized.

Copy link
Member Author

commented May 24, 2017

danbev added a commit to danbev/node that referenced this pull request May 25, 2017
src: add linux getauxval(AT_SECURE) in SafeGetenv
This commit attempts to fix the following TODO:
// TODO(bnoordhuis) Should perhaps also check whether
getauxval(AT_SECURE) is non-zero on Linux.

This can be manually tested at the moment using the following steps:

$ setcap cap_net_raw+ep out/Release/node
$ NODE_PENDING_DEPRECATION="1" out/Release/node -p
"process.binding('config').pendingDeprecation"
true
$ useradd test
$ su test
$ NODE_PENDING_DEPRECATION="1" out/Release/node -p
"process.binding('config').pendingDeprecation"
undefined

PR-URL: nodejs#12548
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@danbev

This comment has been minimized.

Copy link
Member Author

commented May 25, 2017

Landed in 6caf1b0

@danbev danbev closed this May 25, 2017

@danbev danbev deleted the danbev:getauxval_at_secure branch May 25, 2017

jasnell added a commit that referenced this pull request May 25, 2017
src: add linux getauxval(AT_SECURE) in SafeGetenv
This commit attempts to fix the following TODO:
// TODO(bnoordhuis) Should perhaps also check whether
getauxval(AT_SECURE) is non-zero on Linux.

This can be manually tested at the moment using the following steps:

$ setcap cap_net_raw+ep out/Release/node
$ NODE_PENDING_DEPRECATION="1" out/Release/node -p
"process.binding('config').pendingDeprecation"
true
$ useradd test
$ su test
$ NODE_PENDING_DEPRECATION="1" out/Release/node -p
"process.binding('config').pendingDeprecation"
undefined

PR-URL: #12548
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@danbev danbev referenced this pull request May 26, 2017
2 of 2 tasks complete
jasnell added a commit that referenced this pull request May 28, 2017
src: add linux getauxval(AT_SECURE) in SafeGetenv
This commit attempts to fix the following TODO:
// TODO(bnoordhuis) Should perhaps also check whether
getauxval(AT_SECURE) is non-zero on Linux.

This can be manually tested at the moment using the following steps:

$ setcap cap_net_raw+ep out/Release/node
$ NODE_PENDING_DEPRECATION="1" out/Release/node -p
"process.binding('config').pendingDeprecation"
true
$ useradd test
$ su test
$ NODE_PENDING_DEPRECATION="1" out/Release/node -p
"process.binding('config').pendingDeprecation"
undefined

PR-URL: #12548
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@jasnell jasnell referenced this pull request May 28, 2017
@gibfahn gibfahn referenced this pull request Jun 15, 2017
2 of 3 tasks complete
@MylesBorins

This comment has been minimized.

Copy link
Member

commented Jul 17, 2017

This does not land cleanly in LTS. Please feel free to manually backport. Please also feel free to replace the backport request label with do-not-land if it shouldn't land

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.