fix: upper case all env vars on Windows#161
Merged
ddneilson merged 1 commit intoOpenJobDescription:mainlinefrom Aug 7, 2024
Merged
fix: upper case all env vars on Windows#161ddneilson merged 1 commit intoOpenJobDescription:mainlinefrom
ddneilson merged 1 commit intoOpenJobDescription:mainlinefrom
Conversation
edwards-aws
force-pushed
the
edwards_fix_windows_casing
branch
from
8cc6aa7 to
73f99df
Compare
edwards-aws
changed the title
crowecawcaw
previously approved these changes
Aug 2, 2024
crowecawcaw
left a comment
crowecawcaw
left a comment
There was a problem hiding this comment.
Looks like the description of testing in the PR description is missing some text, but tests look thorough to me!
Contributor
Author
|
Thanks for pointing that out Stephen! I've updated the PR text! |
ddneilson
suggested changes
Aug 6, 2024
Contributor
ddneilson
left a comment
ddneilson
left a comment
There was a problem hiding this comment.
Thanks for the work, Cody. It looks like there's a linter issue that needs resolving, and a left-over debugging print statement. Otherwise, this looks great!
edwards-aws
force-pushed
the
edwards_fix_windows_casing
branch
2 times, most recently
from
36db590 to
b4fd005
Compare
Problem: We've observed on Windows systems that even though environment variables are case insensitive, the win32 API will allow duplicate environment variables with different casing. This causes issues as openjd reads in user environment variables via Python, which uppercases all environment variables it reads in on Windows. If there already existed an environment variable in the system that wasn't uppercased, say Path, and openjd reads in a variable it needs to update, say PATH. The win32 API will accept both. This leads to undefined behaviour when Windows accesses that environment variable Solution: Upper case all environment variables that come into openjd on Windows, whether it's through the win32 API or openjd. Signed-off-by: Cody Edwards <edwards@amazon.com>
edwards-aws
force-pushed
the
edwards_fix_windows_casing
branch
from
b4fd005 to
af3cb52
Compare
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| } | ||
| }""" | ||
|
|
||
| def _inject_value_to_user_dict(block: c_void_p) -> dict[str, str]: |
Check failure
Code scanning / CodeQL
Potentially uninitialized local variable
ddneilson
approved these changes
Aug 7, 2024
crowecawcaw
approved these changes
Aug 7, 2024
This was referenced Aug 12, 2024
Merged
Merged
Merged
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fixes: #157
What was the problem/requirement? (What/Why)
We've observed on Windows systems that even though environment variables are case insensitive, the win32 API will allow duplicate environment variables with different casing. This causes issues as openjd reads in user environment variables via Python, which uppercases all environment variables it reads in on Windows. If there already existed an environment variable in the system that wasn't uppercased, say Path, and openjd reads in a variable it needs to update, say PATH. The win32 API will accept both. This leads to undefined behaviour when Windows accesses that environment variable
What was the solution? (How)
Upper case all environment variables that come into openjd on Windows, whether it's through the win32 API or openjd.
What is the impact of this change?
Paths added by printing
openjd_env: key=valueshould override environment variables as expected, regardless of casing.How was this change tested?
I've added unit tests that ensure that the environment variables are all converted to upper case as expected and that variables are overwritten as expected regardless of casing.
Was this change documented?
N/A, but I've also added some dev documentation to make running the impersonation tests on Windows easier.
Is this a breaking change?
N/A
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.