Last PInvoke error in LibraryImport may not be preserved after runtime internal calls

[k15tfu]

Hi!

It turned out that sometimes .NET 7 managed app may behave differently while under profiling. For example, as far as I can see File.Exists() uses GetFileAttributesExW() as follows:

internal static unsafe partial class Kernel32
{
    [System.CodeDom.Compiler.GeneratedCodeAttribute("Microsoft.Interop.LibraryImportGenerator", "7.0.7.1805")]
    private static partial bool GetFileAttributesExPrivate(string name, global::Interop.Kernel32.GET_FILEEX_INFO_LEVELS fileInfoLevel, ref global::Interop.Kernel32.WIN32_FILE_ATTRIBUTE_DATA lpFileInformation)
    {
        int __lastError;
        bool __retVal;
        int __retVal_native;
        // Pin - Pin data in preparation for calling the P/Invoke.
        fixed (void* __name_native = &global::System.Runtime.InteropServices.Marshalling.Utf16StringMarshaller.GetPinnableReference(name))
        fixed (global::Interop.Kernel32.WIN32_FILE_ATTRIBUTE_DATA* __lpFileInformation_native = &lpFileInformation)
        {
            System.Runtime.InteropServices.Marshal.SetLastSystemError(0);
            __retVal_native = __PInvoke((ushort*)__name_native, fileInfoLevel, __lpFileInformation_native);
            __lastError = System.Runtime.InteropServices.Marshal.GetLastSystemError();  <-- here
        }

        // Unmarshal - Convert native data to managed data.
        __retVal = __retVal_native != 0;
        System.Runtime.InteropServices.Marshal.SetLastPInvokeError(__lastError);
        return __retVal;
        // Local P/Invoke
        [System.Runtime.InteropServices.DllImportAttribute("kernel32.dll", EntryPoint = "GetFileAttributesExW", ExactSpelling = true)]
        static extern unsafe int __PInvoke(ushort* name, global::Interop.Kernel32.GET_FILEEX_INFO_LEVELS fileInfoLevel, global::Interop.Kernel32.WIN32_FILE_ATTRIBUTE_DATA* lpFileInformation);
    }
}

In this case File.Exists() returns true for files that do not exist when profiler's ELT hooks are used, here is demo app:

using System.Runtime.InteropServices;

namespace file_exists_csharp
{
    internal class Program
    {

        static void Main(string[] args)
        {
            if (File.Exists("file-that-should-not-exist.txt"))
                throw new Exception("File exists");
        }
    }
}

Profiler Enter callback resets the last error code when Marshal.GetLastSystemError() is being called:

public static int GetLastSystemError()
{
    return Interop.Kernel32.GetLastError();
}

These callbacks are set by SetJitHelperFunction() and there are other JIT helper functions (e.g. CORINFO_HELP_NEWARR_1_OBJ) that probably won't be called before Marshal.GetLastSystemError() in the generated code, and also there are BEGIN_PRESERVE_LAST_ERROR / END_PRESERVE_LAST_ERROR macros that are used in internal JIT helpers etc.

I have also looked into Thread::InjectActivation() which uses PAL_InjectActivation() and inject_activation_handler() on Linux/macOS, and it preserves errno before calling HandleSuspensionForInterruptedThread(), but it is not done for CheckActivationSafePoint() which uses GetThreadNULLOk() and e.g. on Linux ARM32 it internally calls __tls_get_addr(), do we need to wrap it as well?

Same also applies for profiler based IL instrumentation (code coverage).

Linked issues: #10727, #75828, #77364.

[ghost]

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

Issue Details

---

Hi!

It turned out that sometimes .NET 7 managed app may behave differently while under profiling. For example, as far as I can see File.Exists() uses GetFileAttributesExW() as follows:

internal static unsafe partial class Kernel32
{
    [System.CodeDom.Compiler.GeneratedCodeAttribute("Microsoft.Interop.LibraryImportGenerator", "7.0.7.1805")]
    private static partial bool GetFileAttributesExPrivate(string name, global::Interop.Kernel32.GET_FILEEX_INFO_LEVELS fileInfoLevel, ref global::Interop.Kernel32.WIN32_FILE_ATTRIBUTE_DATA lpFileInformation)
    {
        int __lastError;
        bool __retVal;
        int __retVal_native;
        // Pin - Pin data in preparation for calling the P/Invoke.
        fixed (void* __name_native = &global::System.Runtime.InteropServices.Marshalling.Utf16StringMarshaller.GetPinnableReference(name))
        fixed (global::Interop.Kernel32.WIN32_FILE_ATTRIBUTE_DATA* __lpFileInformation_native = &lpFileInformation)
        {
            System.Runtime.InteropServices.Marshal.SetLastSystemError(0);
            __retVal_native = __PInvoke((ushort*)__name_native, fileInfoLevel, __lpFileInformation_native);
            __lastError = System.Runtime.InteropServices.Marshal.GetLastSystemError();  <-- here
        }

        // Unmarshal - Convert native data to managed data.
        __retVal = __retVal_native != 0;
        System.Runtime.InteropServices.Marshal.SetLastPInvokeError(__lastError);
        return __retVal;
        // Local P/Invoke
        [System.Runtime.InteropServices.DllImportAttribute("kernel32.dll", EntryPoint = "GetFileAttributesExW", ExactSpelling = true)]
        static extern unsafe int __PInvoke(ushort* name, global::Interop.Kernel32.GET_FILEEX_INFO_LEVELS fileInfoLevel, global::Interop.Kernel32.WIN32_FILE_ATTRIBUTE_DATA* lpFileInformation);
    }
}

In this case File.Exists() returns true for files that do not exist when profiler's ELT hooks are used, here is demo app:

using System.Runtime.InteropServices;

namespace file_exists_csharp
{
    internal class Program
    {

        static void Main(string[] args)
        {
            if (File.Exists("file-that-should-not-exist.txt"))
                throw new Exception("File exists");
        }
    }
}

Profiler Enter callback resets the last error code when Marshal.GetLastSystemError() is being called:

public static int GetLastSystemError()
{
    return Interop.Kernel32.GetLastError();
}

These callbacks are set by SetJitHelperFunction() and there are other JIT helper functions (e.g. CORINFO_HELP_NEWARR_1_OBJ) that probably won't be called before Marshal.GetLastSystemError() in the generated code, and also there are BEGIN_PRESERVE_LAST_ERROR / END_PRESERVE_LAST_ERROR macros that are used in internal JIT helpers etc.

I have also looked into Thread::InjectActivation() which uses PAL_InjectActivation() and inject_activation_handler() on Linux/macOS, and it preserves errno before calling HandleSuspensionForInterruptedThread(), but it is not done for CheckActivationSafePoint() which uses GetThreadNULLOk() and e.g. on Linux ARM32 it internally calls __tls_get_addr(), do we need to wrap it as well?

Linked issues: #10727, #75828, #77364.

Author:	k15tfu
Assignees:	-
Labels:	area-System.Runtime.InteropServices, untriaged
Milestone:	-

[ww898]

I would be great if the future fix will be backported to 7.0.x. Because huge impact for all profilers.

[k15tfu]

Friendly ping.

[jkoritzinsky]

I believe it is up to profilers to ensure that they don't change any state that can be unintentionally observed by managed code.

I'll send this to the profiler team to see what they think.

[ghost]

Tagging subscribers to this area: @tommcdon
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Hi!

It turned out that sometimes .NET 7 managed app may behave differently while under profiling. For example, as far as I can see File.Exists() uses GetFileAttributesExW() as follows:

internal static unsafe partial class Kernel32
{
    [System.CodeDom.Compiler.GeneratedCodeAttribute("Microsoft.Interop.LibraryImportGenerator", "7.0.7.1805")]
    private static partial bool GetFileAttributesExPrivate(string name, global::Interop.Kernel32.GET_FILEEX_INFO_LEVELS fileInfoLevel, ref global::Interop.Kernel32.WIN32_FILE_ATTRIBUTE_DATA lpFileInformation)
    {
        int __lastError;
        bool __retVal;
        int __retVal_native;
        // Pin - Pin data in preparation for calling the P/Invoke.
        fixed (void* __name_native = &global::System.Runtime.InteropServices.Marshalling.Utf16StringMarshaller.GetPinnableReference(name))
        fixed (global::Interop.Kernel32.WIN32_FILE_ATTRIBUTE_DATA* __lpFileInformation_native = &lpFileInformation)
        {
            System.Runtime.InteropServices.Marshal.SetLastSystemError(0);
            __retVal_native = __PInvoke((ushort*)__name_native, fileInfoLevel, __lpFileInformation_native);
            __lastError = System.Runtime.InteropServices.Marshal.GetLastSystemError();  <-- here
        }

        // Unmarshal - Convert native data to managed data.
        __retVal = __retVal_native != 0;
        System.Runtime.InteropServices.Marshal.SetLastPInvokeError(__lastError);
        return __retVal;
        // Local P/Invoke
        [System.Runtime.InteropServices.DllImportAttribute("kernel32.dll", EntryPoint = "GetFileAttributesExW", ExactSpelling = true)]
        static extern unsafe int __PInvoke(ushort* name, global::Interop.Kernel32.GET_FILEEX_INFO_LEVELS fileInfoLevel, global::Interop.Kernel32.WIN32_FILE_ATTRIBUTE_DATA* lpFileInformation);
    }
}

In this case File.Exists() returns true for files that do not exist when profiler's ELT hooks are used, here is demo app:

using System.Runtime.InteropServices;

namespace file_exists_csharp
{
    internal class Program
    {

        static void Main(string[] args)
        {
            if (File.Exists("file-that-should-not-exist.txt"))
                throw new Exception("File exists");
        }
    }
}

Profiler Enter callback resets the last error code when Marshal.GetLastSystemError() is being called:

public static int GetLastSystemError()
{
    return Interop.Kernel32.GetLastError();
}

These callbacks are set by SetJitHelperFunction() and there are other JIT helper functions (e.g. CORINFO_HELP_NEWARR_1_OBJ) that probably won't be called before Marshal.GetLastSystemError() in the generated code, and also there are BEGIN_PRESERVE_LAST_ERROR / END_PRESERVE_LAST_ERROR macros that are used in internal JIT helpers etc.

I have also looked into Thread::InjectActivation() which uses PAL_InjectActivation() and inject_activation_handler() on Linux/macOS, and it preserves errno before calling HandleSuspensionForInterruptedThread(), but it is not done for CheckActivationSafePoint() which uses GetThreadNULLOk() and e.g. on Linux ARM32 it internally calls __tls_get_addr(), do we need to wrap it as well?

Same also applies for profiler based IL instrumentation (code coverage).

Linked issues: #10727, #75828, #77364.

Author:	k15tfu
Assignees:	-
Labels:	area-System.Runtime.InteropServices, area-Diagnostics-coreclr
Milestone:	-

[tommcdon]

We are past platform shutdown for .NET 8 and are driving to zero bugs. Since this looks like a pre-existing issue and seems to be a non-trivial amount of work, moving to .NET 9