mono / mono

Mono open source ECMA CLI, C# and .NET implementation.
https://www.mono-project.com
Other
11.13k stars 3.82k forks source link

Interpreter doesn't preserve last error on P/Invoke in MinGW builds #15541

Closed filipnavara closed 5 years ago

filipnavara commented 5 years ago

Steps to Reproduce

  1. Compile Mono with MinGW on Windows
  2. Run the following program with redirected standard output and MONO_ENV_OPTIONS=--interp:
using System;
using System.Runtime.InteropServices;

namespace HelloWorld
{
    class Program
    {
        [StructLayoutAttribute(LayoutKind.Sequential)]
        internal struct CONSOLE_SCREEN_BUFFER_INFO
        {
            internal COORD dwSize;
            internal COORD dwCursorPosition;
            internal short wAttributes;
            internal SMALL_RECT srWindow;
            internal COORD dwMaximumWindowSize;
        }

        [StructLayoutAttribute(LayoutKind.Sequential)]
        internal partial struct COORD
        {
            internal short X;
            internal short Y;
        }

        [StructLayoutAttribute(LayoutKind.Sequential)]
        internal partial struct SMALL_RECT
        {
            internal short Left;
            internal short Top;
            internal short Right;
            internal short Bottom;
        }

        internal partial class HandleTypes
        {
            internal const int STD_INPUT_HANDLE = -10;
            internal const int STD_OUTPUT_HANDLE = -11;
            internal const int STD_ERROR_HANDLE = -12;
        }

        [DllImport("kernel32.dll", SetLastError = true)]
        internal static extern bool GetConsoleScreenBufferInfo(IntPtr hConsoleOutput, out CONSOLE_SCREEN_BUFFER_INFO lpConsoleScreenBufferInfo);

        [DllImport("kernel32.dll")]
        internal static extern IntPtr GetStdHandle(int nStdHandle);

        static void Main(string[] args)
        {
            CONSOLE_SCREEN_BUFFER_INFO csbi;
            var outputHandle = GetStdHandle(HandleTypes.STD_OUTPUT_HANDLE);
            Console.WriteLine(GetConsoleScreenBufferInfo(outputHandle, out csbi) + " (expected false)");
            Console.WriteLine(Marshal.GetLastWin32Error() + " (expected 6)");
        }
    }
}

Current Behavior

Prints false and 0.

Expected Behavior

Prints false and 6.

On which platforms did you notice this

[ ] macOS [ ] Linux [x] Windows

Version Used:

Mono 6.5 e301847

filipnavara commented 5 years ago

The underlying error is that TLS access destroys the last error value before it is recorded on the return from P/Invoke call. The first place where it gets destroyed is

https://github.com/mono/mono/blob/112297ef5c076a2716c020ddb0bbf1e0a2da7703/mono/mini/interp/interp.c#L2003

There are likely others on the code path. A broad fix attempted in #15465 worked but it was rejected. A more targeted fix attempted in #15534 did NOT work which likely means that there are more places where the TLS access screws things up.

MSVC builds are not affected because they use __declspec(thread) TLS access instead of TlsGetValue API.

There is more than one code path in the interpreter that can hit this error:

  1. do_icall_wrapper, MINT_CALLI_NAT_FAST (as observed with the example above)
  2. ves_pinvoke_method
lateralusX commented 5 years ago

Fix for this problem should be in interpreter, it needs to preserve the last error just after the p/invoke, currently it runs MINT_MONO_LDPTR and an additional MINT_CALLI_NAT_FAST to read it before storing it using mono_marshal_set_last_error. This is done since it uses the wrapper generated by emit_native_wrapper_ilgen. In JIT that is fine since the lowering will be a call direct after p/invoke, but in interpreter it needs to run the above instructions through the interpreter loop and that will clobber last error value.

lateralusX commented 5 years ago

https://github.com/mono/mono/pull/15571 should take care of this issue.

lateralusX commented 5 years ago

Ran above tests on mingw 64-bit build mono runtime using interpreter and with the fix in #15571 I get:

False (expected false) 6 (expected 6)

So looks like it gets the expected behavior.

vargaz commented 5 years ago

There is a test in tests/last-error.cs.

lateralusX commented 5 years ago

Yes, I have run it locally with JIT+ Interpreter as well on both msvc as well as mingw. CI only uses msvc build, where it doesn't reproduce.

filipnavara commented 5 years ago

Fixed by #15571.