Defensive check for process.config.variables (v4-staging) #6114
Conversation
sneak
changed the title
mscdex
added
build
| @@ -19,7 +19,7 @@ const defaultSessionIdContext = getDefaultSessionIdContext(); | |||
| function getDefaultSessionIdContext() { | |||
| 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) { | |||
There was a problem hiding this comment.
nit: this needs to be line-wrapped but that can be done when the PR is landed.
There was a problem hiding this comment.
I generally wrap long lines myself but I decided to go with your suggestion from the previous PR:
There was a problem hiding this comment.
;-) yeah, I was being lazy. Sorry about that. As I said, the linting nit can be addressed when the PR is landed.
|
LGTM if CI is green. |
|
Why is this directly landing against release branches and not landing is master? |
|
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. |
|
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. |
|
@jasnell do you want to land this in the next v4.x? |
|
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. |
|
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:
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. |
|
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 |
MylesBorins
force-pushed
the
v4.x-staging
branch
from
6291de2
to
a8312f2
Compare
MylesBorins
force-pushed
the
v4.x-staging
branch
3 times, most recently
from
ed3d372
to
f14d9cf
Compare
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
|
Closing this in favor of #7551 |
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>
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>
Updated defensive fix (v2 PR of #6110)
Discussion here:
#3755 (comment)