Fix usage of environment in PAL #3140
Conversation
|
Oh, and this fixes #1664 😄 |
|
@janvorli Could you please take a look? I am travelling till mid next week... |
| // buffer anyway, so just stay in the critical section until then. | ||
| InternalEnterCriticalSection(pthrCurrent, &gcsEnvironment); | ||
| enteredCritSec = true; | ||
|
|
There was a problem hiding this comment.
It seems to me that instead of tracking the entering of critical section, it would be cleaner to move the string copying and critical section leaving here. This is the only code path that actually generates non-null value.
There was a problem hiding this comment.
Done, good idea.
|
@adityamandaleeka, thanks... if you haven't already done so, could you run the System.Runtime.Extensions tests from corefx with your changes? |
adityamandaleeka
force-pushed
the
env_pal
branch
3 times, most recently
from
82472d0 to
d475f00
Compare
|
LGTM |
|
Not sure what triggered that CI run today, but it seems to have failed due to infrastructure issues. It was fine before. @dotnet-bot retest this please |
|
@stephentoub I verified that all 543 of the tests in System.Runtime.Extensions.Tests passed with my coreclr changes. |
|
Thanks, @adityamandaleeka. |
|
|
||
| if (checkAlignmentSettings) | ||
| { | ||
| InternalFree(checkAlignmentSettings); |
There was a problem hiding this comment.
Is it safe to free the value returned by getenv()?
adityamandaleeka
force-pushed
the
env_pal
branch
2 times, most recently
from
395bb39 to
d694dfc
Compare
|
LGTM |
adityamandaleeka
force-pushed
the
env_pal
branch
from
9ecff63 to
f0bebe2
Compare
|
@dotnet-bot test this please |
|
@dotnet-bot test this please (requeuing this job per instructions from @mmitche) |
|
@dotnet-bot test OSX x64 Checked Build and Test |
adityamandaleeka
force-pushed
the
env_pal
branch
from
f0bebe2 to
f715d3b
Compare
adityamandaleeka
force-pushed
the
env_pal
branch
from
f715d3b to
3c74490
Compare
adityamandaleeka
force-pushed
the
env_pal
branch
from
48b4ccb to
d26de9e
Compare
Fix usage of environment in PAL
While investigating #1664, I noticed that the code we had for dealing with the environment was doing some dangerous things, and could be very problematic in certain cases (for instance, if user code PInvoked into a POSIX environment function).
This change makes it so we read the contents of the environment once on startup, copy them to our own internal representation, and operate on that internal representation from that point forward.
See this comment for more details.
@stephentoub @jkotas