Fix for #70007, fallback to old implementation if optimized way fails

[schuettecarsten]

Possible fix for #70007, fallback to old implementation if optimized way to query process name fails. This might happen if executed by a user that does not have the permission to call QueryFullProcessImageNameW.

[ghost]

Tagging subscribers to this area: @dotnet/area-system-diagnostics-process
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Possible fix for #70007, fallback to old implementation if optimized way to query process name fails. This might happen if executed by a user that does not have the permission to call QueryFullProcessImageNameW.

Author:	schuettecarsten
Assignees:	-
Labels:	area-System.Diagnostics.Process
Milestone:	-

[eerhardt]

Is there any way to write a test for this? If there is at least one Windows CI leg that runs as a non-admin user, we could have a test that finds the services.exe process ID (using native APIs or some other way so that Process.ProcessName isn't cached), and then calls

Process services = Process.GetProcessById(ServicesProcessId);
Assert.Equal("services", services.ProcessName);

[jaykrell]

The API to find services.exe etc. is the API used by the old code here -- it enumerates every process on the system and gives their process id, name, and more. So then you could turn around and feed that (just the process id) back into this code, have it fail the direct query, and get the exact same data again (modulo passage of time, different stack/heap addresses). Verifying that services.exe appears, or something?

[schuettecarsten]

Is there any way to write a test for this?

Where to place such a test project?

[schuettecarsten]

I simplified the changes. The new code will fallback to the old implementation if ProcessManager.GetProcessName returns null. That's all, and that should be enough to fix the initial issue. So, no need to throw an exception and catch it again.

[eerhardt]

Where to place such a test project?

@joperezr @ViktorHofer - do you know if we run any Windows CI legs as non-admin users? If we do, then the test could go in https://github.com/dotnet/runtime/tree/main/src/libraries/System.Diagnostics.Process/tests

[ViktorHofer]

Can't directly answer that but what I know is that we have the IsWindowsAndElevated platform detection property that would be used to condition tests:

runtime/src/libraries/Common/tests/TestUtilities/System/PlatformDetection.Windows.cs

Line 254 in 3d74b00

public static bool IsWindowsAndElevated

In your case you probably want an inversion: IsWindowsAndNotElevated?

[eerhardt]

@schuettecarsten - are you planning on adding a test for this?

[schuettecarsten]

@schuettecarsten - are you planning on adding a test for this?

@eerhardt, I was working on a test, but I do not think that I can make it in a reasonable time, first because of lots of other things to do at the moment, second because I did not yet try to compile the whole stuff myself for at least my local platform, just to be able to run the tests here. So, any documentation how to set this up would be helpful.

[eerhardt]

Check out https://github.com/dotnet/runtime/blob/main/docs/workflow/requirements/windows-requirements.md for setting up your machine. And then https://github.com/dotnet/runtime/tree/main/docs/workflow/building/libraries for building the "libraries" part of the repo.

[schuettecarsten]

@eerhardt - Is a test that relies on how it is executed in the build pipeline really a good idea? Of course we can analyze the process tree, look for the parents process name and check if it is services.exe. But how to figure out if it should be services.exe or can be something else?

So maybe the best solution is to go back to the bug that caused this fix and simply call WindowsServiceHelpers.IsWindowsService in a test, without checking the result itself. If the call works on all build pipelines, we are fine. Such a test already exists in Microsoft.Extensions.Hosting. And that this test never crashes on the build pipelines yet shows that there seems to be no pipeline that runs non-elevated.

I rebased my branch on latest changes in main branch, no changes in code.

[eerhardt]

Of course we can analyze the process tree, look for the parents process name and check if it is services.exe. But how to figure out if it should be services.exe or can be something else?

This bug isn't about the parent process. It just so happens that the parent process was the way that Hosting was getting the services.exe process - that was the parent process when the app is executed as a Windows Service.

My thinking for a test would be:

• On Windows only, get the services.exe process ID using a mechanism that doesn't cause the process to be loaded into .NET's System.Diagnostics.Process cache/information. (One way to do this is p/invoke into Windows APIs. There may be others that I'm not aware of.)
• Assert.Equal("services", Process.GetProcessById(servicesProcessId).ProcessName);

So maybe the best solution is to go back to the bug that caused this fix and simply call WindowsServiceHelpers.IsWindowsService in a test, without checking the result itself. If the call works on all build pipelines, we are fine. Such a test already exists in Microsoft.Extensions.Hosting. And that this test never crashes on the build pipelines yet shows that there seems to be no pipeline that runs non-elevated.

This is not an accurate assessment of the situation. Yes, there is a test in Microsoft.Extensions.Hosting that calls IsWindowsService. However, those tests are only ever executed outside of an actual Windows Service. The problem comes when IsWindowsService is called from a non-elevated process that IS running as a Windows Service. (Or more generally, a non-elevated process that was created from an elevated process.)

So maybe another way to test this is to spawn a non-elevated process from an elevated process, get the parent process, and check its name.

[schuettecarsten]

So maybe another way to test this is to spawn a non-elevated process from an elevated process, get the parent process, and check its name.

I think this is the best solution. The issue has nothing to do with services, but with requesting details of the parent process from a non-evevated process. I will try to build such a test within the next few days.

[schuettecarsten]

@eerhardt As a first step, I've created a simple console tool that reads the parent process name and writes it to the console. It uses the same code that IsWindowsService uses. I cannot make it compile as executable at the moment, the OutputType parameter is ignored or overwritten somewhere - any ideas how to fix this are welcome. Next, I have to create a test that starts this tool as an unelevated process and checks for the output.

[eerhardt]

I've created a simple console tool that reads the parent process name and writes it to the console. It uses the same code that IsWindowsService uses. I cannot make it compile as executable at the moment, the OutputType parameter is ignored or overwritten somewhere - any ideas how to fix this are welcome. Next, I have to create a test that starts this tool as an unelevated process and checks for the output.

We don't need to make a new executable to do this. We have the RemoteExecutor which we use in tests to create a new process, if we need to execute some code in a separate process from the unit test process.

Here's an example:

runtime/src/libraries/System.Diagnostics.Process/tests/ProcessTests.Unix.cs

Lines 240 to 251 in 475858d

	RemoteInvokeOptions options = new RemoteInvokeOptions();
	options.StartInfo.EnvironmentVariables["PATH"] = path;
	RemoteExecutor.Invoke(fileToOpen =>
	{
	using (var px = Process.Start(new ProcessStartInfo { UseShellExecute = true, FileName = fileToOpen }))
	{
	Assert.NotNull(px);
	px.WaitForExit();
	Assert.True(px.HasExited);
	Assert.Equal(42, px.ExitCode);
	}
	}, filename, options).Dispose();

[schuettecarsten]

Unfortunately, I am not able to provide a test for this in a reasonable time (which means in July or August). So, please take a look is this fix is valid for a merge in the current state, maybe without the half test code, as I think it is too important to fix this before net7.0 is released.

[jozkee]

@schuettecarsten did you try looking what's the error returned by QueryFullProcessImageName? Rather than doing a fallback, could it be possible that we are missing a permissions flag?

[jaykrell]

Other than not having to do with parent processes, this also has nothing to do with services. To write a test I suggest this, slightly icky but not terrible:
Copy the fallback code.
Modify it to find the pid of services.exe.
Assert there is at least one.
Pass that pid to the “framework” and assert it works.
Try it on “old” (7.0 prerelease) framework unelevated and see it fail.

Caveats:
Services.exe is a windows implementation detail, and there could be others that already work here. It is just a name, not special or reserved or anything.
Better would be to arrange to run a process, service or not, and verify a copy of the new code fails, fallback works, and framework works.

[jozkee]

@jaykrell @eerhardt can you please take a look at this snippet of a possible test for this scenario? Since @schuettecarsten won't be able to provide it, I can send a commit to this PR with the proper changes, assuming it looks good to you.

[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsWindowsAndNotElevated))]
public void NonElevatedUser_QueryProcessNameOfSystemProcess()
{
    const string Services = "services";

    string currentProcessUser = Helpers.GetProcessUserName(Process.GetCurrentProcess());
    Assert.NotNull(currentProcessUser);

    Process? systemOwnedServices = null;

    foreach (var p in Process.GetProcessesByName(Services))
    {
        // returns the username of the owner of the process or null if the username can't be queried.
        // for services.exe, this will be null.
        string? servicesUser = Helpers.GetProcessUserName(p); 

        // this isn't really verifying that services.exe is owned by SYSTEM, but we are sure it is not owned by the current user.
        if (servicesUser != currentProcessUser)
        {
            systemOwnedServices = p;
            break;
        }
    }

    Assert.NotNull(systemOwnedServices);
    Assert.Equal(Services, systemOwnedServices.ProcessName);

    systemOwnedServices = Process.GetProcessById(systemOwnedServices.Id);
    Assert.Equal(Services, systemOwnedServices.ProcessName);
}

[jozkee]

Rather than doing a fallback, could it be possible that we are missing a permissions flag?

FWIW, I looked at the native exception and OpenProcess returns Unauthorized Access, regardless of PROCESS_QUERY_LIMITED_INFORMATION. So, there's really no way to avoid regressing the scenario without falling back to NtQuerySystemInformation, AFAIK.

[eerhardt]

can you please take a look at this snippet of a possible test for this scenario?

@jozkee - can you try running that test without this fix? Does it fail with the same exception that we are getting in #70007?

Unhandled exception. System.InvalidOperationException: Process has exited, so the requested information is not available.
   at System.Diagnostics.Process.get_ProcessName()
   at ConsoleApp1.Program.Main(String[] args) 

My concern is that populating a Process instance for the services.exe process could cache the state in such a way that makes the test pass even without this fix. I would ideally think that querying directly into Windows APIs in the test for the services.exe process might be better than using .NET APIs.

[jozkee]

@eerhardt yes, the first assert uses a Process obtained by Process.GetProcessesByName("services.exe"), which does have a cached ProcessInfo. the 2nd assert, uses systemOwnedServices = Process.GetProcessById(systemOwnedServices.Id), which doesn't have the cache. Hence, the code without the fix does fail on the 2nd assert.

[eerhardt]

This LGTM - assuming the test you added failed previously with the same exception as the bug.

It would be nice if you could manually verify the Original Report in #70007 using WindowsServiceHelpers.IsWindowsService().

[eerhardt]

(nit) double blank lines

[jozkee]

It would be nice if you could manually verify the Original Report in #70007 using WindowsServiceHelpers.IsWindowsService().

It is fixed, I created the service as stated in #70007 (comment), took the new dll from .\artifacts\bin\testhost\net7.0-windows-Debug-x64\shared\Microsoft.NETCore.App\7.0.0 and place it on my C:\Program Files\dotnet\shared\Microsoft.NETCore.App\7.0.0-preview.5.22301.12 folder.

[jozkee]

CI issue is #45868.

[jaykrell]

8?

[jozkee]

that's TOKEN_QUERY. I didn't check what it exactly means since it was existing code, I just moved it here as I needed to reuse it.