src: make copies of startup environment variables by bnoordhuis · Pull Request #11051 · nodejs/node

bnoordhuis · 2017-01-28T12:36:19Z

Mutations of the environment can invalidate pointers to environment
variables, so make secure_getenv() copy them out instead of returning
pointers.

cc @sam-github

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

addaleax

LGTM with two suggestions

addaleax · 2017-01-29T05:50:36Z

src/node.cc

We may want to rename this if it no longer matches the secure_getenv(3) signature

node::SecureGetEnv()?

If you expose it in node_internal.h, I can drop 7c6a26e

Renamed to SafeGetenv() cuz less chars.

addaleax · 2017-01-29T05:52:28Z

src/node.cc

nit: text[0] == '1'

(text.length() == 1 && text[0] == '1')? Right now, it appears a value of ``10or100` would also be accepted, in violation of the docs. Though maybe that's a bug for another PR?

I'll update to text[0] == '1'. I wrote it like this to keep it as close as possible to the logic it replaces.

Though maybe that's a bug for another PR?

Yes, I would say so. FWIW, it doesn't bother me personally.

sam-github

This is reasonable to avoid unintended changes in the env.

sam-github · 2017-01-31T21:41:02Z

src/node.cc

node::SecureGetEnv()?

sam-github · 2017-01-31T21:45:08Z

src/node.cc

(text.length() == 1 && text[0] == '1')? Right now, it appears a value of ``10or100` would also be accepted, in violation of the docs. Though maybe that's a bug for another PR?

sam-github · 2017-01-31T21:45:51Z

src/node.cc

can no longer be nullptr

Good point, updated.

sam-github · 2017-01-31T21:47:36Z

src/node.cc

If you expose it in node_internal.h, I can drop 7c6a26e

gibfahn · 2017-02-06T23:11:19Z

ping @bnoordhuis

Mutations of the environment can invalidate pointers to environment variables, so make `secure_getenv()` copy them out instead of returning pointers.

bnoordhuis

Updated, PTAL. CI: https://ci.nodejs.org/job/node-test-pull-request/6256/

bnoordhuis · 2017-02-07T12:41:45Z

src/node.cc

Renamed to SafeGetenv() cuz less chars.

bnoordhuis · 2017-02-07T12:43:40Z

src/node.cc

I'll update to text[0] == '1'. I wrote it like this to keep it as close as possible to the logic it replaces.

Though maybe that's a bug for another PR?

Yes, I would say so. FWIW, it doesn't bother me personally.

bnoordhuis · 2017-02-07T12:44:36Z

src/node.cc

Good point, updated.

bnoordhuis · 2017-02-07T13:29:06Z

If you expose it in node_internal.h, I can drop 7c6a26e

@sam-github I'll let you do that in your PR, it would look incongruous (unnecessary) in this one.

Aside: if the use of std::string bothers anyone, I can replace it with a version that copies out to a plain char array (possibly with some mid-level template magic to infer array bounds - never do any work the compiler can do for you.)

jasnell · 2017-02-07T23:57:51Z

ping @sam-github

sam-github · 2017-02-08T14:08:43Z

Landed in a8734af

Mutations of the environment can invalidate pointers to environment variables, so make `secure_getenv()` copy them out instead of returning pointers. PR-URL: #11051 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

bnoordhuis · 2017-02-08T14:51:11Z

Zu schade, the commit log still mentions secure_getenv().

sam-github · 2017-02-08T20:21:26Z

src/node.cc

 #if defined(NODE_HAVE_I18N_SUPPORT)
    } else if (strncmp(arg, "--icu-data-dir=", 15) == 0) {
-      icu_data_dir = arg + 15;
+      icu_data_dir.assign(arg, 15);


When converting openssl_config to std::string, I discovered this copies the first 15 chars of arg into target, it needs to be icu_data_dir.assign(arg + 15); or something of the like. @srl295 Since tests pass, this means that this command line option has no test coverage.

Ai, obvious bug in retrospect. #11255

sam-github · 2017-02-08T20:24:11Z

Sorry, @bnoordhuis, I thought it would be helpful to land it since it was approved and had been waiting on me for several days, I won't do that again.

I found a problem while rebasing #11006 onto master, PTAL.

italoacasas · 2017-02-09T18:41:16Z

This is having conflicts when landing on v7.x-staging, any plan to backport this?

sam-github · 2017-02-09T20:28:15Z

Yes, I will create a backport branch for #11006, this, and #11255 since they all build on each other.

sam-github · 2017-02-09T20:33:14Z

@nodejs/ctc we need labels like backport-to-v#.#, IMO. For this PR, I would label it dont-land-on-v7.x to remove it from the branch-diff results for 7.x candidates, but then include the backport-to label to make clear that it is intended to land on 7.x, it just needs to be backported to hop over the semver-major PR that is blocking it (#10116, fwiw).

Mutations of the environment can invalidate pointers to environment variables, so make `secure_getenv()` copy them out instead of returning pointers. PR-URL: nodejs#11051 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

Mutations of the environment can invalidate pointers to environment variables, so make `secure_getenv()` copy them out instead of returning pointers. PR-URL: #11051 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

srl295

LGTM after the fact

Mutations of the environment can invalidate pointers to environment variables, so make `secure_getenv()` copy them out instead of returning pointers. PR-URL: #11051 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

Mutations of the environment can invalidate pointers to environment variables, so make `secure_getenv()` copy them out instead of returning pointers. PR-URL: nodejs#11051 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

jasnell · 2017-03-07T16:18:39Z

Needs a backport PR to land on v4 or v6

Mutations of the environment can invalidate pointers to environment variables, so make `secure_getenv()` copy them out instead of returning pointers. PR-URL: nodejs#11051 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

Mutations of the environment can invalidate pointers to environment variables, so make `secure_getenv()` copy them out instead of returning pointers. PR-URL: #11051 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

Mutations of the environment can invalidate pointers to environment variables, so make `secure_getenv()` copy them out instead of returning pointers. PR-URL: nodejs/node#11051 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

Mutations of the environment can invalidate pointers to environment variables, so make `secure_getenv()` copy them out instead of returning pointers. This is the part of nodejs#11051 that applies to nodejs@03e89b3

Mutations of the environment can invalidate pointers to environment variables, so make `secure_getenv()` copy them out instead of returning pointers. This is the part of nodejs#11051 that applies to be11fb4. This part wasn't backported to 6.x when nodejs#11051 was backported because the semver-minor introduction of NODE_REDIRECT_WARNINGS hadn't been backported yet. Now that the env var is backported, this last bit of nodejs#11051 is needed.

Mutations of the environment can invalidate pointers to environment variables, so make `secure_getenv()` copy them out instead of returning pointers. This is the part of #11051 that applies to be11fb4. This part wasn't backported to 6.x when #11051 was backported because the semver-minor introduction of NODE_REDIRECT_WARNINGS hadn't been backported yet. Now that the env var is backported, this last bit of #11051 is needed. PR-URL: #12677 Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. i18n-api Issues and PRs related to the i18n implementation. labels Jan 28, 2017

addaleax approved these changes Jan 29, 2017

View reviewed changes

jasnell approved these changes Jan 30, 2017

View reviewed changes

sam-github suggested changes Jan 31, 2017

View reviewed changes

gibfahn mentioned this pull request Feb 6, 2017

crypto: support OPENSSL_CONF again #11006

Merged

4 tasks

src: make copies of startup environment variables

be42f90

Mutations of the environment can invalidate pointers to environment variables, so make `secure_getenv()` copy them out instead of returning pointers.

bnoordhuis force-pushed the secure-getenv branch from 5c558d5 to be42f90 Compare February 7, 2017 13:23

bnoordhuis commented Feb 7, 2017

View reviewed changes

sam-github approved these changes Feb 8, 2017

View reviewed changes

sam-github closed this Feb 8, 2017

bnoordhuis deleted the secure-getenv branch February 8, 2017 14:51

sam-github reviewed Feb 8, 2017

View reviewed changes

sam-github added dont-land-on-v4.x labels Feb 9, 2017

sam-github removed dont-land-on-v4.x labels Feb 9, 2017

bnoordhuis mentioned this pull request Feb 13, 2017

src: fix recent --icu-data-dir= regression #11255

Merged

sam-github mentioned this pull request Feb 13, 2017

7.x backport: crypto: support OPENSSL_CONF again (and its dependencies) #11345

Closed

4 tasks

srl295 reviewed Feb 20, 2017

View reviewed changes

jasnell added dont-land-on-v4.x labels Mar 7, 2017

MylesBorins mentioned this pull request May 23, 2017

v6.11.0 proposal #13059

Merged

sam-github mentioned this pull request Oct 10, 2017

(v6.x backport) process: add --redirect-warnings command line argument #14480

Closed

4 tasks

sam-github mentioned this pull request Oct 11, 2017

(v6.x backport) src: allow CLI args in env with NODE_OPTIONS #12677

Closed

4 tasks

MylesBorins added backported-to-v6.x and removed lts-watch-v6.x labels Oct 16, 2017

Uh oh!

Conversation

bnoordhuis commented Jan 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sam-github left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gibfahn commented Feb 6, 2017

Uh oh!

bnoordhuis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bnoordhuis commented Feb 7, 2017

Uh oh!

jasnell commented Feb 7, 2017

Uh oh!

sam-github commented Feb 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bnoordhuis commented Feb 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sam-github commented Feb 8, 2017

Uh oh!

italoacasas commented Feb 9, 2017

Uh oh!

sam-github commented Feb 9, 2017

Uh oh!

sam-github commented Feb 9, 2017

Uh oh!

srl295 left a comment

Choose a reason for hiding this comment

Uh oh!

jasnell commented Mar 7, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

bnoordhuis commented Jan 28, 2017 •

edited

Loading

sam-github commented Feb 8, 2017 •

edited

Loading