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

SetEnvironmentVariable does not work properly on Unix #9529

Closed
wfurt opened this issue Jan 10, 2018 · 5 comments
Closed

SetEnvironmentVariable does not work properly on Unix #9529

wfurt opened this issue Jan 10, 2018 · 5 comments

Comments

@wfurt
Copy link
Member

wfurt commented Jan 10, 2018

This is related to #4103 and dotnet/coreclr#3140.
In my case I use

Environment.SetEnvironmentVariable("all_proxy",` "http://10.121.122.110:3128");

and I expect HTTP Handler to honor it same way is if set before process start.
In this case, we do no fork child process so fix-up for process start is not applicable.

I understand that setenv() is not thread safe and that we choose not to actually set environment variables but to work on our copy instead.
We may perhaps re-consider and fix it.
There is similar pattern calling openssl where init function is not thread safe. To address that we simply added lock around the call to make it thread safe.
This would be needed only for Linux (PAL?) and I would think the setenv() call is not likely on performance critical path.

I also verified that when i call setnev() via [DllImport] works as expected.
It is workaround but it would be nice if the SetEnvironmentVariable just work.

@jkotas
Copy link
Member

jkotas commented Jan 10, 2018

If we "fix this", we would introduce a different set of problems: memory leaks and/or potential security issues on systems where setenv is not thread safe. https://github.com/dotnet/coreclr/issues/635#issuecomment-91495713 has my take on this.

I think it is fine to recommend to call setenv() via [DllImport] as workaround if you really need the libc environment changed, and you know what you are doing and understand the consequences.

@jkotas
Copy link
Member

jkotas commented Jan 10, 2018

http://www.club.cc.cmu.edu/~cmccabe/blog_the_setenv_fiasco.html

@stephentoub
Copy link
Member

Also, even if we serialize our own calls to setenv, that could still result in problems with libcurl concurrently calling getenv to read them.

And for ManagedHandler, it'll be using Environment.GetEnvironmentVariable rather than getenv and will "just work".

I don't think there's anything to be done here.

@parched
Copy link

parched commented Aug 14, 2022

Can this note be added to these docs? It would've saved me a fair bit of debugging time.

@danmoseley
Copy link
Member

@parched feel free to offer a PR to improve the doc (click edit button on doc page)

I'll close this as apparently that was originally intended

@danmoseley danmoseley closed this as not planned Won't fix, can't repro, duplicate, stale Aug 15, 2022
parched added a commit to parched/dotnet-api-docs that referenced this issue Aug 15, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Sep 14, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants