Reliability improvements for array marshalling

[jkoritzinsky]

Guard against byte length overflow. Use a local when marshalling by-ref arrays to avoid multithreading issues.

[AaronRobinsonMSFT]

@jkoritzinsky Can we get an example of the generated stub?

[jkoritzinsky]

From the By ref string array test:

namespace DllImportGenerator.IntegrationTests
{
    partial class NativeExportsNE
    {
        public partial class Arrays
        {
            public static partial void ReverseStrings(ref string[] strArray, out int numElements)
            {
                unsafe
                {
                    ushort **__strArray_gen_native = default;
                    numElements = default;
                    try
                    {
                        //
                        // Marshal
                        //
                        {
                            string[] strArray_local = strArray;
                            __strArray_gen_native = null;
                            if (strArray != null)
                            {
                                int strArray__bytelen = checked(sizeof(ushort *) * strArray_local.Length);
                                __strArray_gen_native = (ushort **)System.Runtime.InteropServices.Marshal.AllocCoTaskMem(strArray__bytelen);
                            }

                            if (strArray_local != null)
                                for (int __i = 0; __i < strArray_local.Length; ++__i)
                                {
                                    __strArray_gen_native[__i] = null;
                                    if (strArray_local[__i] != null)
                                    {
                                        __strArray_gen_native[__i] = (ushort *)System.Runtime.InteropServices.Marshal.StringToCoTaskMemUni(strArray_local[__i]);
                                    }
                                }
                        }

                        //
                        // Invoke
                        //
                        fixed (int *__numElements_gen_native = &numElements)
                        {
                            ReverseStrings__PInvoke__(&__strArray_gen_native, __numElements_gen_native);
                        }

                        //
                        // Unmarshal
                        //
                        {
                            string[] strArray_local;
                            if (__strArray_gen_native != null)
                            {
                                strArray_local = new string[checked((int)numElements)];
                                for (int __i = 0; __i < strArray_local.Length; ++__i)
                                {
                                    strArray_local[__i] = __strArray_gen_native[__i] == null ? null : new string ((char *)__strArray_gen_native[__i]);
                                }
                            }
                            else
                                strArray_local = null;
                            strArray = strArray_local;
                        }
                    }
                    finally
                    {
                        //
                        // Cleanup
                        //
                        if (strArray != null)
                            for (int __i = 0; __i < strArray.Length; ++__i)
                            {
                                System.Runtime.InteropServices.Marshal.FreeCoTaskMem((System.IntPtr)__strArray_gen_native[__i]);
                            }

                        System.Runtime.InteropServices.Marshal.FreeCoTaskMem((System.IntPtr)__strArray_gen_native);
                    }
                }
            }

            [System.Runtime.InteropServices.DllImportAttribute("NativeExportsNE", EntryPoint = "reverse_strings")]
            extern private static unsafe void ReverseStrings__PInvoke__(ushort ***strArray, int *numElements);
        }
    }
}

[jkotas]

                        //
                        // Marshal
                        //
                        __strArray_gen_native = null;

__strArray_gen_native = null is redundant.

[jkotas]

           string[] strArray_local = strArray;
                            if (strArray_local != null)

strArray_local has to be cached in a local before the buffer allocation. We need to allocate the buffer based on the same array that we will later use for the assignment.

[jkotas]

             __strArray_gen_native[__i] = null;
                                    if (strArray[__i] != null)
                                    {
                                        __strArray_gen_native[__i] = (ushort *)System.Runtime.InteropServices.Marshal.StringToCoTaskMemUni(strArray[__i]);
                                    }

Why can't this be just __strArray_gen_native[__i] = (ushort *)System.Runtime.InteropServices.Marshal.StringToCoTaskMemUni(strArray[__i]); ?

[jkotas]

strArray[__i] = __strArray_gen_native[__i] == null ? null : new string ((char *)__strArray_gen_native[__i]);

Should this be strArray_local[__i] = ... ?

[jkotas]

                        //
                        // Cleanup
                        //
                        if (strArray != null)
                            for (int __i = 0; __i < strArray.Length; ++__i)

strArray can be changed by the caller by accident at this point. This should use the cached strArray_local.Length that matches how much we actually allocated.

[jkoritzinsky]

Updated codegen:

namespace DllImportGenerator.IntegrationTests
{
    partial class NativeExportsNE
    {
        public partial class Arrays
        {
            public static partial void ReverseStrings(ref string[] strArray, out int numElements)
            {
                unsafe
                {
                    ushort **__strArray_gen_native = default;
                    numElements = default;
                    //
                    // Setup
                    //
                    string[] strArray_local = strArray;
                    try
                    {
                        //
                        // Marshal
                        //
                        __strArray_gen_native = null;
                        if (strArray != null)
                        {
                            int strArray__bytelen = checked(sizeof(ushort *) * strArray_local.Length);
                            __strArray_gen_native = (ushort **)System.Runtime.InteropServices.Marshal.AllocCoTaskMem(strArray__bytelen);
                        }

                        if (strArray_local != null)
                            for (int __i = 0; __i < strArray_local.Length; ++__i)
                            {
                                __strArray_gen_native[__i] = null;
                                if (strArray_local[__i] != null)
                                {
                                    __strArray_gen_native[__i] = (ushort *)System.Runtime.InteropServices.Marshal.StringToCoTaskMemUni(strArray_local[__i]);
                                }
                            }

                        //
                        // Invoke
                        //
                        fixed (int *__numElements_gen_native = &numElements)
                        {
                            ReverseStrings__PInvoke__(&__strArray_gen_native, __numElements_gen_native);
                        }

                        //
                        // Unmarshal
                        //
                        if (__strArray_gen_native != null)
                        {
                            strArray_local = new string[checked((int)numElements)];
                            for (int __i = 0; __i < strArray_local.Length; ++__i)
                            {
                                strArray_local[__i] = __strArray_gen_native[__i] == null ? null : new string ((char *)__strArray_gen_native[__i]);
                            }
                        }
                        else
                            strArray_local = null;
                        strArray = strArray_local;
                    }
                    finally
                    {
                        //
                        // Cleanup
                        //
                        if (strArray_local != null)
                            for (int __i = 0; __i < strArray_local.Length; ++__i)
                            {
                                System.Runtime.InteropServices.Marshal.FreeCoTaskMem((System.IntPtr)__strArray_gen_native[__i]);
                            }

                        System.Runtime.InteropServices.Marshal.FreeCoTaskMem((System.IntPtr)__strArray_gen_native);
                    }
                }
            }

            [System.Runtime.InteropServices.DllImportAttribute("NativeExportsNE", EntryPoint = "reverse_strings")]
            extern private static unsafe void ReverseStrings__PInvoke__(ushort ***strArray, int *numElements);
        }
    }
}

[AaronRobinsonMSFT]

I think all uses of strArray should be replaced with strArray_local. With that done we can collapse some of those if branches.

[jkoritzinsky]

I should be able to replace the rest of the usages of strArray, but merging the if blocks will be more difficult because they're generated in different places (one in the conditional stackalloc base class and one in the non-blittable array marshaller)

[jkotas]

I think you still need to have up-front Clear with this structure. If there is an exception thrown by the System.Runtime.InteropServices.Marshal.StringToCoTaskMemUni call, the finally block is going to use uninitialized memory.

The alternative that avoids up-front Clear would be to count how many slots got initialized and only free up those in the finally block.

[jkoritzinsky]

The up front clear is implemented in #379. If you want I can try porting it over to this PR, but I'd rather just get one of the PRs in, fix merge conflicts, and then get the other PR in.

[jkoritzinsky]

Does anyone have any more comments other than the up-front Clear?

[AaronRobinsonMSFT]

LGTM. My only other ask would be to add any relevant details to our design docs.