FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Defensive check for process.config.variables (v4-staging) by sneak · Pull Request #6114 · nodejs/node · GitHub

/ node Public

Defensive check for process.config.variables (v4-staging) - #6114

Closed
sneak wants to merge 1 commit into
nodejs:v4.x-stagingfrom
sneak:sneak/20160408fipsdefensivefix
ConversationCommits (1)ChecksFiles changed
Closed

Defensive check for process.config.variables (v4-staging)#6114
sneak wants to merge 1 commit into
nodejs:v4.x-stagingfrom
sneak:sneak/20160408fipsdefensivefix

Conversation

sneak commented Apr 8, 2016

Copy link
Copy Markdown

Updated defensive fix (v2 PR of #6110)

Discussion here:

#3755 (comment)

sneak changed the title Defensive check for process.config.variables Defensive check for process.config.variables (v4-staging) Apr 8, 2016
sneak mentioned this pull request Apr 8, 2016
Closed
mscdex added build Issues and PRs related to build files or the CI. process Issues and PRs related to the process subsystem. v4.x labels Apr 8, 2016

jasnell commented Apr 8, 2016

Copy link
Copy Markdown
Member

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

jasnell reviewed Apr 8, 2016
View reviewed changes
Comment thread lib/_tls_wrap.js
var defaultText = process.argv.join(' ');
/* SSL_MAX_SID_CTX_LENGTH is 128 bits */
if (process.config.variables.openssl_fips) {
if (process.config && process.config.variables && process.config.variables.openssl_fips) {

jasnell Apr 8, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

nit: this needs to be line-wrapped but that can be done when the PR is landed.

sneak Apr 8, 2016

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I generally wrap long lines myself but I decided to go with your suggestion from the previous PR:

#6110 (comment)

jasnell Apr 8, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

;-) yeah, I was being lazy. Sorry about that. As I said, the linting nit can be addressed when the PR is landed.

jasnell commented Apr 8, 2016

Copy link
Copy Markdown
Member

LGTM if CI is green.
One minor linting nit.

Fishrock123 commented Apr 8, 2016

Copy link
Copy Markdown
Contributor

Why is this directly landing against release branches and not landing is master?

sneak commented Apr 8, 2016

Copy link
Copy Markdown
Author

I haven't tested against master. The problem occurs in latest 4.x and 5.x release. The lines for the fix aren't in the same file in master and I'm not sure why - I don't know your workflow.

jasnell commented Apr 8, 2016

Copy link
Copy Markdown
Member

The offending line of code does not exist in master. This is going to need a regression test that repro's the issue before it lands, which I'll be working on. In theory this shouldn't be happening so I need to investigate that a bit more.

MylesBorins commented Apr 8, 2016

Copy link
Copy Markdown
Contributor

@jasnell do you want to land this in the next v4.x?

jasnell commented Apr 9, 2016

Copy link
Copy Markdown
Member

Yes, we should. A quick investigation shows that there are existing modules that mutate process.config (which is problematic). This will need a quick regression test and a CI run plus a linting fix before it can land.

MylesBorins commented Apr 11, 2016

Copy link
Copy Markdown
Contributor

@sneak a new v4.x is going out tomorrow. If you can get through @jasnell 's check list we can likely include it

stefanmb commented Apr 11, 2016

Copy link
Copy Markdown
Contributor

Hi folks, I apologize about being late to this issue, I was away last week.

I agree that breaking existing apps is unacceptable, but I am not sure what we are proposing here is sufficient. While this PR fixes @sneak's problem, what about the reverse issue? If someone is using Node.js 4.x in FIPS mode and loads some code that replaces the process object as per #6115 (comment), then their app will also break. As I see it there are several possible approaches:

  1. Do nothing, process object is assumed to exist and not be clobbered.
  2. Use defensive programming, like this PR, which only fixes the problem for non-FIPS users.
  3. Process object should be frozen as per @Fishrock123's suggestion here Defensive fix for process.config.variables regression (v5) #6115 (comment).
  4. Introduce a new configuration object, that is frozen, and use that instead of process. However, this approach seems unlikely since it is only needed for 4.x and 5.x because the master does not fallback to md5 at all.

Just to make my position clear, I'm not opposed to landing this PR, as the original problem is real and it needs fixing, but we don't yet have a complete solution.

jasnell commented Apr 20, 2016

Copy link
Copy Markdown
Member

Quick update on this: I'm working on getting #6266 landed in master. Once that's landed, I plan to backport it to v5 and v4 and refactor the parts inside lib/* that depend directly on process.config to avoid this issue entirely.

MylesBorins force-pushed the v4.x-staging branch 3 times, most recently from ed3d372 to f14d9cf Compare June 28, 2016 22:49
jasnell added a commit to jasnell/node that referenced this pull request Jul 5, 2016
tls: use process.binding('config') to detect fips mode
5444436 
When the fips mode check was added sometime in v4 it caused a
regression in some edge cases (see nodejs#6114)
because `process.config` can be overwritten by userland modules.
This switches to using the backported process.binding('config') to
fix the regression.

Fixes: nodejs#6114
jasnell mentioned this pull request Jul 5, 2016
Closed
3 tasks

jasnell commented Jul 6, 2016

Copy link
Copy Markdown
Member

Closing this in favor of #7551

jasnell closed this Jul 6, 2016
MylesBorins pushed a commit that referenced this pull request Jul 11, 2016
tls: use process.binding('config') to detect fips mode
f5756d9 
When the fips mode check was added sometime in v4 it caused a
regression in some edge cases (see #6114)
because `process.config` can be overwritten by userland modules.
This switches to using the backported process.binding('config') to
fix the regression.

Fixes: #6114
MylesBorins pushed a commit that referenced this pull request Jul 11, 2016
tls: use process.binding('config') to detect fips mode
9ab72a9 
When the fips mode check was added sometime in v4 it caused a
regression in some edge cases (see #6114)
because `process.config` can be overwritten by userland modules.
This switches to using the backported process.binding('config') to
fix the regression.

Fixes: #6114
MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
tls: use process.binding('config') to detect fips mode
62fd6a5 
When the fips mode check was added sometime in v4 it caused a
regression in some edge cases (see #6114)
because `process.config` can be overwritten by userland modules.
This switches to using the backported process.binding('config') to
fix the regression.

Fixes: #6114
MylesBorins pushed a commit that referenced this pull request Jul 14, 2016
tls: use process.binding('config') to detect fips mode
65ede67 
When the fips mode check was added sometime in v4 it caused a
regression in some edge cases (see #6114)
because `process.config` can be overwritten by userland modules.
This switches to using the backported process.binding('config') to
fix the regression.

Fixes: #6114

PR-URL: #7551
Reviewed-By: Myles Borins <myles.borins@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 14, 2016
tls: use process.binding('config') to detect fips mode
06327e5 
When the fips mode check was added sometime in v4 it caused a
regression in some edge cases (see #6114)
because `process.config` can be overwritten by userland modules.
This switches to using the backported process.binding('config') to
fix the regression.

Fixes: #6114

PR-URL: #7551
Reviewed-By: Myles Borins <myles.borins@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

build Issues and PRs related to build files or the CI. process Issues and PRs related to the process subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

Back | FazBrowse Home | New Git URL