PROCESS_BASIC_INFORMATION incorrectly generated?

mitchcapper commented 1 year ago

Actual behavior

Declaring an instance of the struct PROCESS_BASIC_INFORMATION and passing it to NtQueryInformationProcess results in either a read/write memory error or incorrect values ending up in the struct. The issue seems to be the PEB[] PebBaseAddress declaration.

While the struct has the correct sizeof compared to when it is read from memory even the unrelated struct fields (like UniqueProcessId) have the wrong values when passed to ntqueryinformationprocess. I was confused that VS internal seemed to use the call without issue, but then realized they are actually declaring their own instance of the class: https://github.com/microsoft/vs-extension-testing/blob/b78ac2a63a471cc0a56b45af0a1d7b2473288202/src/Microsoft.VisualStudio.Extensibility.Testing.Xunit.Shared/Harness/VisualStudioInstance.cs#L273-L281

Here is the generated version

internal partial struct PROCESS_BASIC_INFORMATION
        {
            internal winmdroot.Foundation.NTSTATUS ExitStatus;
            internal winmdroot.System.Threading.PEB[] PebBaseAddress;
            internal nuint AffinityMask;
            internal int BasePriority;
            internal nuint UniqueProcessId;
            internal nuint InheritedFromUniqueProcessId;
        }

changing winmdroot.System.Threading.PEB[] to IntPtr fixes it.

Expected behavior

Calling NtQueryInformationProcess with it to not throw an exception and have the right data values.

Repro steps

NativeMethods.txt content:
```
NtQueryInformationProcess
```

Any of your own code that should be shared?

nuint nreturnLength = 0;
PROCESS_BASIC_INFORMATION pbi = default;
PInvoke.NtQueryInformationProcess(handle, PROCESSINFOCLASS.ProcessBasicInformation, &pbi, (uint)Marshal.SizeOf<PROCESS_BASIC_INFORMATION>(), ref returnLength);

Context

CsWin32 version: [e.g. 0.4.422-beta]
Win32Metadata version (if explicitly set by project):
Target Framework: [e.g. netstandard2.0]
LangVersion (if explicitly set by project): [e.g. 9]

AArnott commented 1 year ago

Thanks for this report. But I'm struggling to get your example to compile. You take a pointer to PROCESS_BASIC_INFORMATION but C# won't let you do that because it is a managed type (due to the array field). If I change NativeMethods.json to disallow marshaling, that allows the pointer you declared to work, but that also converts the array to a pointer, which would resolve your problem.

AArnott commented 1 year ago

Oh, I guess that CS8500 is just a warning. But it is an apt warning. It's telling you this won't work. :) You can't take a pointer to a managed type and expect it to behave. So the workaround would be setting allowMarshaling to false in the NativeMethods.json file as I mentioned above. Or I think you can use Marshal.StructureToPtr to get .NET to do its marshaling thing on the array first so you can then pass your pointer into NtQueryInformationProcess.

mitchcapper commented 1 year ago

Sorry it was a quick example from the vs code but below is a complete one. Although it sounds like you did have it working. NativeMethods.txt:

NtQueryInformationProcess
OpenProcess
PROCESS_BASIC_INFORMATION

Program.cs:

using System;
using Microsoft.Win32.SafeHandles;
using Windows.Win32;
using Windows.Win32.System.Threading;
using Marshal = System.Runtime.InteropServices.Marshal;
namespace NtQueryInformationProcess {
    internal class Program {
        unsafe static void Main(string[] args) {
            uint processId = 29120;
            var handle = PInvoke.OpenProcess_SafeHandle(PROCESS_ACCESS_RIGHTS.PROCESS_QUERY_INFORMATION | PROCESS_ACCESS_RIGHTS.PROCESS_VM_READ, false, processId);
            TEST_PROCESS_BASIC_INFORMATION pbi=default;
            PROCESS_BASIC_INFORMATION pbiOfficial=default;
            PROCESS_BASIC_INFORMATION *pbiOfficial2;
            TEST_PROCESS_BASIC_INFORMATION* pbi2;

            TryFetch(processId, handle, &pbi, (it) => it->UniqueProcessId, Marshal.SizeOf(pbi));
            var alloced = Marshal.AllocHGlobal(Marshal.SizeOf(pbiOfficial));
            var alloced2 = Marshal.AllocHGlobal(Marshal.SizeOf(pbiOfficial));
            pbiOfficial2 =(PROCESS_BASIC_INFORMATION *) alloced;
            pbi2 = (TEST_PROCESS_BASIC_INFORMATION*)alloced;
            TryFetch(processId, handle, pbi2, (it) => it->UniqueProcessId, Marshal.SizeOf(pbiOfficial));
            TryFetch(processId, handle, (PROCESS_BASIC_INFORMATION*)alloced2, (it) => it->UniqueProcessId, Marshal.SizeOf(pbiOfficial));
            var pbiOfficial3 = (TEST_PROCESS_BASIC_INFORMATION)Marshal.PtrToStructure(alloced2,typeof(TEST_PROCESS_BASIC_INFORMATION));
            Console.WriteLine($"Manual PtrToStructure: {pbiOfficial3.UniqueProcessId}");
            TryFetch(processId, handle, pbiOfficial2, (it) => it->UniqueProcessId, Marshal.SizeOf(pbiOfficial));
            TryFetch(processId, handle, &pbiOfficial, (it) => it->UniqueProcessId, Marshal.SizeOf(pbiOfficial));
        }
        unsafe private delegate nuint GetUniqueId<T>(T*p);
        unsafe private static void TryFetch<T>(uint pid, SafeFileHandle procHandle, T * pbi, GetUniqueId<T> UniquidId, int pbiSize){
            var returnLength = 0U;
            IntPtr pbiAddr1 = (IntPtr)pbi;
            var res = PInvoke.NtQueryInformationProcess(procHandle,PROCESSINFOCLASS.ProcessBasicInformation,pbi, (uint)pbiSize, ref returnLength);
            Console.WriteLine($"procHandleAddy: {procHandle.DangerousGetHandle()} pbiAddr: {pbiAddr1} size: {pbiSize} actual pid: {pid} UniqueProcessId: {UniquidId(pbi)} status: {res.SeverityCode}  last pInvokeError: {Marshal.GetLastPInvokeError()} Last win32 error: {Marshal.GetLastWin32Error()} Last sys error: {Marshal.GetLastSystemError()}");
        }
        [System.Runtime.InteropServices.StructLayout(System.Runtime.InteropServices.LayoutKind.Sequential)]
        internal struct TEST_PROCESS_BASIC_INFORMATION {
            internal Windows.Win32.Foundation.NTSTATUS ExitStatus;
            internal IntPtr PebBaseAddress;
            internal nuint AffinityMask;
            internal int BasePriority;
            internal nuint UniqueProcessId;
            internal nuint InheritedFromUniqueProcessId;
        }
    }
}

So the win32 generated code does convert the pointer to a PEB structure to the PEB[] array which cannot, as you mentioned, properly be used. I am not sure Marshal.StructureToPtr would actually have a use? It requires you to have already allocated the unmanaged memory it just copies the managed data to the unmanaged memory but NtQueryInformationProcess doesn't read any data from the memory it is writing the data to it. We couldn't use Marshal.PtrToStructure afterwards either I believe, as the array would still cause the alignment to be incorrect.

I am sure allowMarshaling to false would resolve but that would disable marshaling for everything. It would still seem this is a bug? As far as I know this structure is only used by QueryInformationProcess, and the structure generated essentially can't ever be used. There is also the fact that while PebBaseAddress is a pointer to a PEB structure, its a pointer in the other processes memory. I believe that means our process could never read it directly (in terms of the marshaled structure generated) unless we were calling it on ourselves. So in this sort of situation doesn't an IntPtr or a nuint make more sense?

I realize this is auto generated code but I can't imagine being the only one who would add PROCESS_BASIC_INFORMATION to the NativeMethods.txt and expect to get something usable back. Maybe a blacklist of structures that are invalid to marshal so if a user tried best they would get would be a pointer or it would simply refuse to generate it at all.

AArnott commented 1 year ago

Thanks for elaborating. I haven't used this API myself, and it's helpful to me that you're explaining how this API is used.

I did a little more digging and the PebBaseAddress is an array instead of a pointer because PEB is a managed type. It's managed because one of its fields is PPS_POST_PROCESS_INIT_ROUTINE PostProcessInitRoutine, and PPS_POST_PROCESS_INIT_ROUTINE is a delegate (a managed type).

We do have the capability in CsWin32 to force certain type hierarchies to be emitted as blittable (non-marshaled). Given what you're saying, it sounds like PROCESS_BASIC_INFORMATION is perhaps one of those.

mitchcapper commented 1 year ago

I believe PROCESS_BASIC_INFORMATION isn't used at any other time than for NtQueryInformationProcess (backed up that MS defines the struct on the function page and not as its own type + a search of other code shows similar exclusive use).

While you could call NtQueryInformationProcess on yourself (say to get the parent process id) even doing so I don't think there is any way you could used the PROCESS_BASIC_INFORMATION as declared. You have access to all the memory but, as mentioned, can't martial that from a ptr into the managed type (as it is both a value type for an existing struct and Marshal.PtrToStructure(alloced,typeof(PROCESS_BASIC_INFORMATION)) won't work as the sizing doesn't match.

Aside from getting the parent process ID the other main use for NtQueryInformationProcess is likely to find the PEB address of a remote process to be able to access some of the PEB fields (cwd/command line). For remote processes the marshaling won't make sense (as it won't be accessible either and would still size wrong).

If PROCESS_BASIC_INFORMATION.PEB[] isn't accessible/usable in any situation, and will fault if used in the intended situations forcing it to be blittable would seem like the way to go.

AArnott commented 1 year ago

Agreed. I actually spent a good part of yesterday fixing this in CsWin32. Rather than special case this particular struct, I was looking for the systemic problem so that similar structs that may have the same problem can be fixed at the same time. I found several bugs along the way, and fixing it isn't trivial. However I have a set of changes that I'm feeling good about and am running our exhaustive generation tests on it now to find any remaining issues. I hope to have a PR later today.

microsoft / CsWin32