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

fix: environment leak on Windows #1739

Merged
merged 1 commit into from
Oct 20, 2022
Merged

Conversation

awson
Copy link
Contributor

@awson awson commented Oct 16, 2022

This fixes an old todo, where spawning a child process on Windows modified the environment variables of the parent.

Fixes #1282

Copy link
Member

@gebner gebner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing this really old todo!

Please squash the commits!

Please remove the (#1282) from the commit message and PR title. Instead, please write Fixes #1282 in the PR message!

src/runtime/process.cpp Outdated Show resolved Hide resolved
src/runtime/process.cpp Outdated Show resolved Hide resolved
@awson awson changed the title fix: environment leak on Windows (#1282) fix: environment leak on Windows Oct 19, 2022
@gebner
Copy link
Member

gebner commented Oct 19, 2022

@awson Is there a reason you decided to stick with the statically-sized buffer instead of the dynamically-growing std::vector<char> version?

Is 0x8000 some magic limit on Windows? If so, we should document that!

@awson
Copy link
Contributor Author

awson commented Oct 20, 2022

@awson Is there a reason you decided to stick with the statically-sized buffer instead of the dynamically-growing std::vector<char> version?

Is 0x8000 some magic limit on Windows? If so, we should document that!

Exactly. MS documentation says "The total size of the environment block for a process may not exceed 32,767 characters".

@gebner
Copy link
Member

gebner commented Oct 20, 2022

Exactly. MS documentation says "The total size of the environment block for a process may not exceed 32,767 characters".

Good to know! Can you put that in a comment? Then the PR is good to go!

@awson
Copy link
Contributor Author

awson commented Oct 20, 2022

That said, my interpretation has always been that 32767 is the size of the block without the extra null char, but after reading MS docs here and there I see them using "total size" wording, hence we, perhaps, need to use 0x7fff here.

@gebner gebner enabled auto-merge (rebase) October 20, 2022 01:39
@gebner gebner merged commit d5063c8 into leanprover:master Oct 20, 2022
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.

Environment leaks between spawned processes (on Windows)
2 participants