Defensive check for process.config.variables (v4-staging)#6114
Uh oh!
There was an error while loading. Please reload this page.
Conversation
sneak changed the title 
mscdex added build 
jasnell commented Apr 8, 2016
| vardefaultText=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.
jasnell commented Apr 8, 2016
LGTM if CI is green. |
Fishrock123 commented Apr 8, 2016
Why is this directly landing against release branches and not landing is master? |
sneak commented Apr 8, 2016
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
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
@jasnell do you want to land this in the next v4.x? |
jasnell commented Apr 9, 2016
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
stefanmb commented Apr 11, 2016
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. |
jasnell commented Apr 20, 2016
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 |
MylesBorinsforce-pushed the v4.x-staging branch 3 times, most recently from ed3d372 to f14d9cfCompareWhen 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 commented Jul 6, 2016
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 <[email protected]>
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 <[email protected]>
6 participants
Updated defensive fix (v2 PR of #6110)
Discussion here:
#3755 (comment)