MonoMod icon indicating copy to clipboard operation
MonoMod copied to clipboard

`CompileMethodHook` overrides Win32's `LastError` in .NET 6

Open punchready opened this issue 4 years ago • 13 comments

Description

It appears that on some systems, the CompileMethodHook overrides Win32's LastError somewhere in the execution of InvokeRealCompileMethod.

This is observed when initializing DetourHelper.Runtime which causes the jit hooks to be set up and using some system libraries which rely on the LastError, for example RandomAccess.ReadAtOffset (internally).

This occurs on two windows computers with .NET runtime 6.0.0 installed that i have access to, but curiously it works as expected on another computer i tested it on, and also on a windows VM on my computer, all with exactly the same binary used. CPUs of systems (all x64) with issue: i3-9100F, Ryzen 5 3600.

A quick check in binary search style between Monomod.RuntimeDetour versions suggests that the code works reliably fine in versions <= 21.11.1.1 and starts reliably breaking in versions >= 21.11.9.2, which is where support for .NET6 was enabled.

Example

using Microsoft.Win32.SafeHandles;
using MonoMod.RuntimeDetour;
using System;
using System.IO;
using System.Reflection;
using System.Runtime.InteropServices;
using System.Threading;

public sealed class Program
{
	[DllImport("kernel32.dll", CharSet = CharSet.Auto, SetLastError = true)]
	public static extern IntPtr CreateFile(
		[MarshalAs(UnmanagedType.LPTStr)] string filename,
		[MarshalAs(UnmanagedType.U4)] FileAccess access,
		[MarshalAs(UnmanagedType.U4)] FileShare share,
		IntPtr securityAttributes, // optional SECURITY_ATTRIBUTES struct or IntPtr.Zero
		[MarshalAs(UnmanagedType.U4)] FileMode creationDisposition,
		[MarshalAs(UnmanagedType.U4)] FileAttributes flagsAndAttributes,
		IntPtr templateFile);

	[DllImport("kernel32.dll", SetLastError = true)]
	[return: MarshalAs(UnmanagedType.Bool)]
	private static extern bool ReadFile(
		IntPtr hFile,
		[Out] byte[] lpBuffer,
		uint nNumberOfBytesToRead,
		out uint lpNumberOfBytesRead,
		[In] ref NativeOverlapped lpOverlapped);

	[DllImport("kernel32.dll", SetLastError = true)]
	[return: MarshalAs(UnmanagedType.Bool)]
	private static extern bool CloseHandle(IntPtr hObject);


	private const string PATH = "arbitrary_file.txt";
	private const int READ_SIZE = 1024; //anything that is greater than the file contents


	public static void Main(string[] args)
	{
		File.WriteAllText(PATH, "arbitrary line\ncrashing line"); //set up the test subject file

		_ = DetourHelper.Runtime; //commenting this out makes everything work fine

		DirectCall();
		RandomAccessCall();

		Console.ReadLine();
	}

	private static void DirectCall()
	{
		IntPtr handle = CreateFile(PATH, FileAccess.Read, FileShare.None, IntPtr.Zero, FileMode.Open, FileAttributes.Normal, IntPtr.Zero);

		//read READ_SIZE bytes at a time, two times (the first call will already read all of the file)
		int offset = 0;
		for (int i = 0; i < 2; i++)
		{
			//basically does what RandomAccess.ReadAtOffset does internally
			NativeOverlapped overlap = default;
			overlap.OffsetLow = offset;
			overlap.OffsetHigh = offset >> 32;

			byte[] buffer = new byte[READ_SIZE];
			bool result = ReadFile(handle, buffer, READ_SIZE, out uint bytesRead, ref overlap);

			int lePinvoke = Marshal.GetLastPInvokeError(); //method used by RandomAccess
			int leWin32 = Marshal.GetLastWin32Error();

			//expected: read 28 bytes and lastError = 0, then read 0 bytes and lastError = 38
			//works fine
			Console.WriteLine($"Direct: [Call {i}] result: {result}, bytes read: {bytesRead}, lastError: {lePinvoke}, {leWin32}");

			offset += READ_SIZE;
		}

		CloseHandle(handle);
	}

	private static void RandomAccessCall()
	{
		IntPtr handle = CreateFile(PATH, FileAccess.Read, FileShare.None, IntPtr.Zero, FileMode.Open, FileAttributes.Normal, IntPtr.Zero);
		SafeFileHandle sfHandle = new SafeFileHandle(handle, true);

		int offset = 0;
		for (int i = 0; i < 2; i++)
		{
			byte[] buffer = new byte[READ_SIZE];
			int bytesRead = -1;
			try
			{
				bytesRead = RandomAccess.Read(sfHandle, buffer, offset); //equivalent to RandomAccess.ReadAtOffset
			}
			catch (Exception ex)
			{
				Console.WriteLine($"RandomAccess: [Call {i}] error: {ex}");
			}

			int lePinvoke = Marshal.GetLastPInvokeError();
			int leWin32 = Marshal.GetLastWin32Error();
			//throws on the second call instead of returning 0
			Console.WriteLine($"RandomAccess: [Call {i}] bytes read: {bytesRead}, lastError: {lePinvoke}, {leWin32}");

			offset += READ_SIZE;
		}

		CloseHandle(handle); //this might cause another exception to be thrown if the second Read call fails, which is unrelated
	}
}

punchready avatar Mar 31 '22 18:03 punchready

Can I ask you to double check those versions? 21.11.11.01 isn't a tagged release.

nike4613 avatar Mar 31 '22 18:03 nike4613

Apologies, fixed!

punchready avatar Mar 31 '22 18:03 punchready

Can you reproduce this behavior on .NET 5?

nike4613 avatar Mar 31 '22 18:03 nike4613

If I'm not mistaken, System.IO.RandomAccess was only introduced in .NET 6. However the direct call method and StreamReader, which uses RandomAccess in .NET 6, both work fine in .NET 5 with 21.11.9.2

punchready avatar Mar 31 '22 19:03 punchready

Can you reproduce the GetLastError results with direct syscalls on .NET 5 though? I already know it has something to do with JIT hooks, as the only difference between the listed versions is whether or not they are enabled for .NET 6, but I'd like to know if it is also affected by some changes to the JIT in .NET 6.

nike4613 avatar Mar 31 '22 19:03 nike4613

The P/Invoke ReadFile works as expected in .NET 5 and 6 with all monomod versions, giving the correct LastError value, which leads me to believe that a change in one of the methods within RandomAccess are the cause. I can't further step into Marshal.GetLastPInvokeError, which is new in .NET 6, and unfortunately cannot execute Marshal.GetLastWin32Error for comparison in this place because of optimizations.

punchready avatar Mar 31 '22 19:03 punchready

Manually invoking GetLastError via P/Invoke after the issue occurs in RandomAccess also results in 0 being returned, but it returns the correct value without any hook active. And it also returns 38 correctly in the test case without RandomAccess.

punchready avatar Mar 31 '22 19:03 punchready

Does the incorrect GetLastError happen on every attempted read after a detour is installed, or just the first?

nike4613 avatar Mar 31 '22 19:03 nike4613

After subsequent calls all GetLastError methods correctly give 38, including the ones used within RandomAccess, I'm assuming it must have to do with JIT then.

Curiously, if i break at the return of FileStreamHelpers.GetLastWin32ErrorAndDisposeHandleIfInvalid and set the returned variable's value to 38, the subsequent GetLastError P/Invoke returns 38 too ... possibly because the variable is actually a direct reference to whatever register/memory location LastError is stored in?

punchready avatar Mar 31 '22 20:03 punchready

Alright, my working theory is that our JIT hook is somehow clearing the P/Invoke last error during its execution. It only happens the first time because the first time is the only time that a JIT compile is done between the native call and the GetLastError call. I'm not sure where the hook would be doing this, but it should be a fairly simple fix of just saving and reloading the last error value in the JIT hook.

nike4613 avatar Mar 31 '22 20:03 nike4613

Alright great! I'll try to be available in case more tests are required.

punchready avatar Mar 31 '22 20:03 punchready

There is a workaround to allow using system file api classes (such as File.ReadAllLines) again:

AppContext.SetSwitch("System.IO.UseNet5CompatFileStream", true);

Found in an unrelated thread at https://github.com/dotnet/runtime/issues/62851, but it does actually make normal file access classes work again, indicating that the issue might be specific to Marshal.GetLastPInvokeError, which is only present in the new .NET 6 file access code. However it seems to only work if executed before touching any file api class (i.e. before the File.WriteAllText call in the example).

There also seem to be a similar issues related to GetLastError in other parts of the system libraries, which I am now discovering one by one:

System.ArgumentException: 'overlapped' was not allocated by this ThreadPoolBoundHandle instance. (Parameter 'overlapped')
   at System.Threading.ThreadPoolBoundHandle.FreeNativeOverlapped(NativeOverlapped* overlapped)
   at System.Net.Sockets.SocketAsyncEventArgs.FreeNativeOverlapped(NativeOverlapped* overlapped)
   at System.Net.Sockets.SocketAsyncEventArgs.<>c.<.cctor>b__179_0(UInt32 errorCode, UInt32 numBytes, NativeOverlapped*nativeOverlapped)
   at System.Threading.ThreadPoolBoundHandleOverlapped.CompletionCallback(UInt32 errorCode, UInt32 numBytes, NativeOverlapped* nativeOverlapped)
   at System.Threading._IOCompletionCallback.PerformIOCompletionCallback(UInt32 errorCode, UInt32 numBytes, NativeOverlapped* pNativeOverlapped)

System.InvalidOperationException: 'overlapped' has already been freed.
   at System.Threading.ThreadPoolBoundHandleOverlapped.CompletionCallback(UInt32 errorCode, UInt32 numBytes, NativeOverlapped* nativeOverlapped)
   at System.Threading._IOCompletionCallback.PerformIOCompletionCallback(UInt32 errorCode, UInt32 numBytes, NativeOverlapped* pNativeOverlapped)

Both exceptions contain methods with an errorCode argument in the call stack, which leads me to believe that this is also simply caused by this parameter being now 0, causing the code to perform invalid operations.

punchready avatar Apr 01 '22 17:04 punchready

[no, i'm braindead, looking further]

bartico6 avatar Jun 11 '22 14:06 bartico6