Litany of bugs with ICustomMarshaler

[ststeiger]

Consider this pinvoke call

// int access(const char *pathname, int mode);
// https://linux.die.net/man/2/access
[System.Security.SuppressUnmanagedCodeSecurity]
[System.Runtime.InteropServices.DllImport(LIBC, CallingConvention = System.Runtime.InteropServices.CallingConvention.Cdecl, EntryPoint = "access", SetLastError = true)]

internal static extern int access([System.Runtime.InteropServices.MarshalAs(System.Runtime.InteropServices.UnmanagedType.CustomMarshaler, MarshalTypeRef = typeof(FileNameMarshaler))] string path, AccessModes mode);

Return Value
On success (all requested permissions granted), zero is returned. On error (at least one bit in mode asked for a permission that is denied, or some other error occurred), -1 is returned, and errno is set appropriately.

Here we use an ICustomMarshaler to marshal string to char*, because netstandard 2 doesn't support MarshalAs(System.Runtime.InteropServices.UnmanagedType.LPUTF8Str).
So inside the ICustomMarshaler, we allocate const char pathname as nonconst char pathname

        System.IntPtr System.Runtime.InteropServices.ICustomMarshaler.MarshalManagedToNative(object objManagedObj)
        {
            string managedObj = objManagedObj as string;
            
            if (managedObj == null)
                return System.IntPtr.Zero;
            
            // not null terminated
            byte[] strbuf = System.Text.Encoding.UTF8.GetBytes(managedObj);
            
            System.IntPtr buffer = System.Runtime.InteropServices.Marshal.AllocHGlobal(strbuf.Length + 1);
            System.Runtime.InteropServices.Marshal.Copy(strbuf, 0, buffer, strbuf.Length);

            // write the terminating null
            //Marshal.WriteByte(buffer + strbuf.Length, 0);

            long lngPosEnd = buffer.ToInt64() + strbuf.Length;
            System.IntPtr ptrPosEnd = new System.IntPtr(lngPosEnd);
            System.Runtime.InteropServices.Marshal.WriteByte(ptrPosEnd, 0);

            return buffer;
        } // End Function MarshalManagedToNative 

Which works fine, just until ICustomMarshaler cleans up the native data.

        void System.Runtime.InteropServices.ICustomMarshaler.CleanUpNativeData(System.IntPtr pNativeData)
        {
                System.Runtime.InteropServices.Marshal.FreeHGlobal(pNativeData);
        } // End Function CleanUpNativeData 

Now, the syscall succeds, I get a return value of -1, as it should; but the thing to notice here is that if I pass an invalid path to access, access returns -1, but GetLastWin32Error then returns 0 (SUCCESS...).
Which is really a pitty, because it's wrong, and it doesn't return 0 when I do the same in plain C.

Now after about an hour of debugging, I've found the culprit, and solved the problem:

        void System.Runtime.InteropServices.ICustomMarshaler.CleanUpNativeData(System.IntPtr pNativeData)
        {
            int lastError = System.Runtime.InteropServices.Marshal.GetLastWin32Error();
            System.Runtime.InteropServices.Marshal.FreeHGlobal(pNativeData);
            s_setLastWin32Error(lastError);
        } // End Function CleanUpNativeData 

The issue is, CleanUpNativeData calls FreeHGlobal, which calls a pinvoke with SetLastError, and as it should, FreeHGlobal succeeds, so GetLastWin32Error gets overwritten with 0...

So let's present the bugs:

public static System.Runtime.InteropServices.ICustomMarshaler GetInstance(string cookie)

Not part of the interface, but required ?

Then, if I have this class

public class Utf8CustomMarshaler
        : System.Runtime.InteropServices.ICustomMarshaler
{

        static Utf8CustomMarshaler _instance = new Utf8CustomMarshaler();
        
       // implementation here
        
        public static Utf8CustomMarshaler GetInstance(string cookie)
        {
            return _instance;
        } // End Function GetInstance 

}

It returns Not found GetInstance which returns System.Runtime.InteropServices.ICustomMarshaler .
Although Utf8CustomMarshaler implements ICustomMarshaler.
OK, fair enough, i get it, I returned Utf8CustomMarshaler instead of ICustomMarshaler.

But then, I do this:

    internal class FileNameMarshaler
        : Utf8CustomMarshaler
        // : libACL.Unix.FileNameMarshaler
{
        static FileNameMarshaler _instance = new FileNameMarshaler();
        
        
        public static System.Runtime.InteropServices.ICustomMarshaler GetInstance(string cookie)
        {
            return _instance;
        } // End Function GetInstance 
}

and the entire .NET runtime crashes, no exception thrown, you just have to figure that out yourselfs.

And then whatever calls CleanUpNativeData doesn't save and restore the return value ...
So errno is the errno-number of whatever pinvoke was last called in CleanUpNativeData that had SetLastError = true.
Probably MarshalNativeToManaged has similar issues, but I don't have a use case to test that.

Then I'm saying "OK, I'll just save and restore that value myselfs".
Well, bad luck, Marshal.SetLastWin32Error has protection level "internal".

So I have to use reflection to be able to invoke that method:

System.Type t = typeof(System.Runtime.InteropServices.Marshal);
System.Reflection.MethodInfo mi = t.GetMethod("SetLastWin32Error", System.Reflection.BindingFlags.Static | System.Reflection.BindingFlags.NonPublic);
// mi.Invoke(null, new object[] { (object)lastError });
s_setLastWin32Error = (System.Action<int>) mi.CreateDelegate(typeof(System.Action<int>));

And now, it finally works.
Calling a C function that takes or returns char* is basically a simple thing.
It shouldn't have taken the time it took...
I would probably still be searching the bug if JetBrains Rider wouldn't have an integrated decompiler...

And, the only reason I'm actually calling libc myselfs in the first place, by the way, is that Mono.Posix.NetStandard apparently is unable to return C#-strings (from readlink and others) without the trailing \0, which makes me wanna vomit.

Oh, and, the only reason why Mono.Unix.Native.FileNameMarshaler gets the errorcode right, is, that by haphazard, Mono.Unix.Native.Stdlib.free doesn't call the free-pinvoke with SetLastError=true, which is shady, too.

If free fails, I never have a chance to get the error message...

Also, if public static System.Runtime.InteropServices.ICustomMarshaler GetInstance(string cookie)
was part of the interface, I probably wouldn't have made the mistake with ICustomMarshaler vs. Utf8CustomMarshaler.

Also, the docs should get updated with the important information that you need to backup the value of GetLastWin32Error.

Also, I think I once not-so-unrecently had issues with System.Diagnostics.Process.ProcessName on Linux because of trailing \0's.
After these findings, I think it might be because Process uses Mono.Posix readlink on /proc//exe which returns the process name with a trailing \0.

[MichalStrehovsky]

Also, if public static System.Runtime.InteropServices.ICustomMarshaler GetInstance(string cookie)
was part of the interface

It cannot be part of the interface because the method must be static - this is how the runtime gets an instance in the first place. Interface methods can only be invoked once there is an instance.

because netstandard 2 doesn't support MarshalAs(System.Runtime.InteropServices.UnmanagedType.LPUTF8Str).

LPUTF8Str is just an enum constant. I think it should be possible to whack a MarshalAs((UnmanagedType)48) and it's going to work. The reason why it's not in netstandard 2.0 is because .NET Framework doesn't support it, but I assume you're running this p/invoke only on some unix-y operating system where .NET Framework will never be in play.

Hopefully once p/invoke source generation comes into play, nobody will need to rely on ICustomMarshaller anymore.

Cc @AaronRobinsonMSFT. Some of this might be in scope for .NET 5 (depending on how LastError is handled in source generators).

[AaronRobinsonMSFT]

Not part of the interface, but required ?

This is documented here. There are rough edges with the ICustomMarshaler API. It is for advanced scenarios so we have historically simply documented it as much as possible rather than optimizing the API for ease of use. As @MichalStrehovsky mentions this would involve more abstractions since the function must be static. An alternative approach would be to dictate the implementing type must have a constructor that takes a string.

unable to return C#-strings (from readlink and others) without the trailing \0

This is needed because of the native convention of \0 terminated strings. Not all APIs provide a length and since C defines a string as having a \0 terminator (section 7.1.1 from C standard) that is what is followed here. Basically it isn't a string unless it has the \0 terminator.

A string is a contiguous sequence of characters terminated by and including the first null character.
The term multibyte string is sometimes used instead to emphasize special processing given to
multibyte characters contained in the string or to avoid confusion with a wide string. A pointer to
a string is a pointer to its initial (lowest addressed) character. The length of a string is the number
of bytes preceding the null character and the value of a string is the sequence of the values of the
contained characters, in order.

Out of curiosity are these issues coming from using the mono runtime or coreclr? Either way we need to investigate this error code behavior. As @MichalStrehovsky mentions we are starting to look at source generation of P/Invoke interop which would make investigating these issues much easier.

[AaronRobinsonMSFT]

Given the OP indicates the following is failing in their scenario (doesn't repro in coreclr) and given the multiple references to mono, I am going to reassign this to interop-mono.

But then, I do this:

    internal class FileNameMarshaler
        : Utf8CustomMarshaler
        // : libACL.Unix.FileNameMarshaler
{
        static FileNameMarshaler _instance = new FileNameMarshaler();
        
        
        public static System.Runtime.InteropServices.ICustomMarshaler GetInstance(string cookie)
        {
            return _instance;
        } // End Function GetInstance 
}

and the entire .NET runtime crashes, no exception thrown, you just have to figure that out yourselfs.

[ststeiger]

I think to reproduce, you need to return Utf8CustomMarshaler instead of System.Runtime.InteropServices.ICustomMarshaler in FileNameMarshaler, while Utf8CustomMarshaler returns ICustomMarshaler (or vice-versa).

Definitely was .NET Core and not mono.

[jkoritzinsky]

For clarity, I'm going to summarize the issues encountered here:

1. GetInstance is not part of the interface contract.

• There's not much we can do here as it has to be static.

2. GetInstance must return ICustomMarshaler instead of a derived type.

• In combination with issue 1, we should update the docs to document this.
• We can consider adding support for a GetInstance method that returns the same type it is on depending on cost.

3. CoreCLR crashes if the GetInstance method returns an instance of a different ICustomMarshaler-derived type.

• This should be investigated as an issue. I'll take a look.

4. The Win32 error code gets overridden when calling FreeHGlobal.

• This is expected behavior.
• We can update the docs warning about this.

5. There is no mechanism without reflection to set the "last Win32 error code"

• This was fixed in Add Get/SetLastPInvokeError and Get/SetLastSystemError APIs #51505