This repository was archived by the owner on Mar 3, 2023. It is now read-only.
Fix getEnvFromShell() to correctly handle newlines in env vars #17628
Conversation
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
Fix atom#17627 by using awk to portably separate env vars with \0 instead of \n.
Contributor
|
Looks like you have linting errors @brettle. |
Contributor
|
@brettle Looks like the tests are failing in a consistent manner, and the failures definitely seem to be related to the changes in this PR. Let us know if you need any assistance with resolving the problems. |
Use printf("...%c...", 0) instead of printf("...\0...") to inject \0.
Contributor
Author
|
@thomasjo Thanks for the offer, but it looks like I've got it fixed. |
23 tasks
Contributor
|
Hey @brettle. Apologies for not looking into this on time. I know this PR has been here for a very long time, during that time Atom has seen a number of changes. Due to the changes, this PR has now become outdated. Is it possible for you to resolve the conflicts? |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of the Change
Fix #17627 by using awk to portably separate env vars with \0 instead of \n.
getEnvFromShell() was supposed to copy the user's environment as it exists in a new shell. However, it did that by running
envin that shell and using \n to split the output into var=value pairs. However, values can contain \n (e.g. exported bash functions) and such values will break the parsing. The fix is to have the var=value pairs separated by \0s. GNU'senv -0does that but won't work on Mac or BSD. Instead, we get the same result using a one line POSIX-compliantawkscript.Alternate Designs
I was going to just use
env -0until I realized that it was GNU-specific.Why Should This Be In Core?
It fixes a bug in core.
Benefits
It fixes and prevents issues where a subprocess is started with an incorrect environment due to multiline environment variables.
Possible Drawbacks
If
awkcorresponds to GNU'sawk, it will add AWKLIBPATH and AWKPATH to the environment (with default values) if they don't already exist.Verification Process
I modified the existing spec to include multiline values and confirmed it worked.
I also confirmed that multiline values set in .bashrc are actually showing up correctly in process.env after the change was made.
Applicable Issues
Issue #17627