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

EnvironmentVariable operations with EnvironmentVariableTarget.User fails on Windows Nano Server #30566

Closed
ViktorHofer opened this issue Aug 11, 2019 · 15 comments · Fixed by #80099
Labels
area-System.Runtime bug disabled-test The test is disabled in source code against the issue os-windows-nano Nano Windows SKU
Milestone

Comments

@ViktorHofer
Copy link
Member

This seems to be a product bug because of an underlying windows api change.

cc @Anipik @stephentoub @danmosemsft

@ViktorHofer
Copy link
Member Author

Setting to 3.0 to make sure that the issue is triaged asap.

@ViktorHofer
Copy link
Member Author

Disabled the test in dotnet/corefx@a969488.

@Anipik
Copy link
Contributor

Anipik commented Aug 15, 2019

@ViktorHofer is any exception being thrown here ? do you have the failure\exception message ?

@ViktorHofer
Copy link
Member Author

No exception being thrown in source code. Just assert errors in the different tests that test the behavior.

@ericstj
Copy link
Member

ericstj commented Aug 27, 2019

@safern and I looked at this previously and were seeing that it's just storing stuff in the registry. I wonder if Nano removed the root key in the registry for user environment variables: https://github.com/dotnet/corefx/blob/954b1fdab8e98060cce3d0d88a34c3b0587a6acb/src/Common/src/CoreLib/System/Environment.Win32.cs#L51

@maryamariyan do you think you can take a look?

@danmoseley
Copy link
Member

This may have to move out of 3.0 now given there's no customer reports (?)

@ViktorHofer was it previously passing on a different version of Nano, and the upgrade broke it?

@ViktorHofer
Copy link
Member Author

was it previously passing on a different version of Nano, and the upgrade broke it?

Yes, the upgrade to Windows Nano Server 1809 broke this. I assume the number of customers that use .NET Core on Nano is limited.

@ViktorHofer
Copy link
Member Author

We should look at the api changes in Nano 1809 and/or talk with the Windows team to understand the issue better. This isn't 3.0 specific so I'm fine moving it out but it probably affects all .NET Core versions that support Nano and use that API.

@maryamariyan
Copy link
Member

maryamariyan commented Sep 4, 2019

I ran the test on Windows 10, Windows Nano Server 1809 and 1803.

The repro (failure for SetEnvironmentVariable) only happened in 1809.
The reason is HKEY_CURRENT_USER\Environment registry was not present.

After manually adding that in 1809, the test passes for Nano Server 1809 as well.

@danmoseley
Copy link
Member

Sounds like we could reasonably work around that in code then (eg create hte key or skip the operation or whatever)

@ericstj
Copy link
Member

ericstj commented Sep 5, 2019

Skipping the operation is what happens now: that's the bug. We should probably let the Windows folks know about this regression and confirm that removal of the key was intentional.

We could create the key, assuming we can get the ACLs right. On my machine it looks like this key has an explicit ACL (not inherited). We'd want to test what would happen when we create the key from elevated and non-elevated process (if such a thing exists in nano) to ensure we create the key with appropriate ACL.

We should test if we add the key, and create a new process, that the variable will be set in that process. Given Nano removed creation of the key, we need to confirm they didn't remove consumption of the key too.

@ViktorHofer
Copy link
Member Author

Creating the key ourselves is a bit scary, let's first talk with Windows.

@msftgits msftgits transferred this issue from dotnet/corefx Feb 1, 2020
@msftgits msftgits added this to the 5.0 milestone Feb 1, 2020
@maryamariyan maryamariyan added the untriaged New issue has not been triaged by the area owner label Feb 23, 2020
@joperezr joperezr removed the untriaged New issue has not been triaged by the area owner label Jul 7, 2020
@danmoseley danmoseley modified the milestones: 5.0.0, Future Aug 12, 2020
@danmoseley
Copy link
Member

Not blocking release of 5.0 product

@maryamariyan maryamariyan removed their assignment Feb 27, 2022
@MichalStrehovsky
Copy link
Member

I'm trying to bring up checked legs for the NativeAOT runtime and running into the same assert this is tracking (#79847). I don't believe it's NativeAOT specific because it only happens on Nano server. My theory is that we just don't have any test legs that would detect this with CoreCLR anymore.

I'm going to disable the entire System.Runtime.Extensions suite on the checked legs because I've already spent too much time on this 3.5 year old known bug. It would be nice if this got attention from the maintainers in 8.0.

@jkotas
Copy link
Member

jkotas commented Jan 2, 2023

My theory is that we just don't have any test legs that would detect this with CoreCLR anymore.

We do not have a leg that runs libraries tests on Windows Nano Server as admin with checked CoreLib. NativeAOT happens to have some coverage for this specific config by accident.

jkotas added a commit to jkotas/runtime that referenced this issue Jan 2, 2023
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jan 2, 2023
jkotas added a commit that referenced this issue Jan 3, 2023
* Ignore SendMessageTimeout failures on Windows Nano Server

* Fix tests

Fixes #30566
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jan 3, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Feb 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Runtime bug disabled-test The test is disabled in source code against the issue os-windows-nano Nano Windows SKU
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants