Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Accept environment variables with different cases on Windows. Fixes #607 #635

Merged
merged 1 commit into from
Jun 3, 2021

Conversation

fabioz
Copy link
Collaborator

@fabioz fabioz commented May 28, 2021

No description provided.

@int19h
Copy link
Contributor

int19h commented May 28, 2021

I don't think that's the right way to fix the problem. This particular snippet, and the corresponding test case, was there for microsoft/ptvsd#2055 - and the underlying issue was exactly that, that variables will overwrite each other regardless of name case. So if debugpy gets a mismatched dict, when merging that with pre-existing environment variables, which one wins is essentially random on older Python versions (where dicts are unordered). So the user would silently get the wrong PATH with no clear indication as to why. The normalization & check prevents that by making the issue explicit.

I believe this is consistent with what Windows does. It doesn't actually allow you to have two variables in the same environment that differ only by case when spawning a new process. So the client shouldn't be able to specify a dict with two variables that differ only by case - it's ambiguous which value wins in that case. The client, when it generates the dict from environment, should be similarly case-insensitive on Windows.

@fabioz
Copy link
Collaborator Author

fabioz commented May 28, 2021

Humm, in this particular issue (#607) the user has Path and PATH with the same contents actually, so, maybe we can just check for that particular case and accept it if both map to the same value (so, it wouldn't matter who wins as they're effectively the same).

@int19h
Copy link
Contributor

int19h commented May 28, 2021

That sounds reasonable.

@fabioz fabioz force-pushed the debugpy_607_duplicate_env_windows branch from 4cdf243 to 7774109 Compare May 31, 2021 10:16
@sonarcloud
Copy link

sonarcloud bot commented May 31, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@fabioz fabioz merged commit b07f372 into microsoft:main Jun 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants