BeginUpdateResource requires custom handle handling

[zdavidsen]

Actual behavior

The handle returned by BeginUpdateResource must be closed by a call to EndUpdateResource instead of the normal CloseHandle call. The default destructor for SafeFileHandle calls CloseHandle, which throws an error because it's attempting to close a handle it was never meant to close. I worked around the issue by setting the handle as invalid after calling EndUpdateResource.

Expected behavior

The disposal of the HANDLE from BeginUpdateResource shouldn't throw an exception, which can't be traced because it's hidden in a generated Dispose call at the close of using block.

Repro steps

1. NativeMethods.txt content:

CreateTypeLib2
LoadTypeLibEx
CreateFile2
CreateFileMappingW
MapViewOfFileEx
UnmapViewOfFile
BeginUpdateResourceW
UpdateResourceW
EndUpdateResourceW

2. NativeMethods.json content (if present):

3. Any of your own code that should be shared?

Code using the various UpdateResource functions

        // And now it's time to take that shiny brand new type library, and shove it into a dll.

        var typeLibSize = new FileInfo(tempTypeLibPath).Length;

        // UpdateResource takes a ushort for the number of bytes to write, but the type lib size
        //  comes back as a long, so we need to check that the type lib will actually fit, and 
        //  abort if it doesn't fit
        if (typeLibSize > uint.MaxValue)
          throw new Exception("Type library is too big to embed as a resource");

        using var hTypeLib = PInvoke.CreateFile2(tempTypeLibPath, FILE_ACCESS_FLAGS.FILE_GENERIC_READ, 0, FILE_CREATION_DISPOSITION.OPEN_EXISTING, null);
        if (hTypeLib.IsInvalid)
          throw new Win32Exception();

        using var hMap = PInvoke.CreateFileMapping(hTypeLib, null, PAGE_PROTECTION_FLAGS.PAGE_READONLY, 0, 0, null);
        if (hMap.IsInvalid)
          throw new Win32Exception();

        var lpData = PInvoke.MapViewOfFileEx(hMap, FILE_MAP.FILE_MAP_READ, 0, 0, UIntPtr.Zero, null);
        if (lpData == null)
          throw new Win32Exception();

        try
        {
          // PInvoke wraps the returned handle in a SafeFileHandle, which is annoying because the
          //  handle from BeginUpdateResource needs to be closed by EndUpdateResource instead of
          //  the usual CloseHandle call.  So we need to make sure to do that manually and then
          //  mark the handle as invalid so that SafeFileHandle doesn't die trying to close it for us
          using var hUpdate = PInvoke.BeginUpdateResource(destDllPath, false);
          if (hUpdate.IsInvalid)
            throw new Win32Exception();

          try
          {
            // In order to properly embed the type library (and make sure we replace the existing one)
            //  we have to use a very specific 'type' and 'name' that are defined by convention.
            //  The 'type' is straightforward, but while the 'name' is typed as a LPWSTR, it can also
            //  take the result of the `MAKEINTRESOURCE(i)` macro, which essentially passes an integer
            //  constant instead of a pointer (i.e. `(char*)1` but more complicated).  This is very
            //  straightforward in C++, but the C# bindings generated by PInvoke take a normal string,
            //  which doesn't let us pass a specific "address" like we need to.  So we had to copy/
            //  define our own overload that lets us do all the unsafe things that the 
            //  API allows (and requires, in this case).
            if (!PInvoke.UpdateResource(hUpdate, "TYPELIB", 1, 0, lpData, (uint)typeLibSize))
            {
              // We don't care if this fails, we're already aborting anyway
              PInvoke.EndUpdateResource(hUpdate, true);
              throw new Win32Exception();
            }

            // This gave me a lot of trouble during development, returning a failed status but not actually
            //  reporting any errors. I finally narrowed it down to some leaked ITypeInfo objects up above,
            //  and after properly releasing those, this started working. Why those particular objects 
            //  matter is beyond me, I was leaking other ITypeInfo objects and ICreateTypeInfo objects
            //  in CopyTypeInfo with no issues, but I've cleaned those leaks up as well and things
            //  seem to be working just as well as the C++ implementation now.
            if (!PInvoke.EndUpdateResource(hUpdate, false))
              throw new Win32Exception();
          }
          finally
          {
            // Set the handle as closed/invalid so that SafeFileHandle doesn't try to close it for us
            //  with the wrong function and throw an exception.
            hUpdate.SetHandleAsInvalid();
          }
        }
        finally
        {
          PInvoke.UnmapViewOfFile(lpData);
        }
      }

UpdateResource overload

#pragma warning disable CS1591,CS1573,CS0465,CS0649,CS8019,CS1570,CS1584,CS1658,CS0436
namespace Windows.Win32
{
  using global::System;
  using global::System.Diagnostics;
  using global::System.Runtime.CompilerServices;
  using global::System.Runtime.InteropServices;
  using winmdroot = global::Windows.Win32;

  internal static partial class PInvoke
  {
    // Copied (and slightly modified) from the autogenerated pinvoke stuff
    // Needed an overload to handle the case where lpName comes from MAKEINTRESOURCE
    /// <inheritdoc cref="UpdateResource(winmdroot.Foundation.HANDLE, winmdroot.Foundation.PCWSTR, winmdroot.Foundation.PCWSTR, ushort, void*, uint)"/>
    internal static unsafe winmdroot.Foundation.BOOL UpdateResource(SafeHandle hUpdate, string lpType, short lpName, ushort wLanguage, void* lpData, uint cb)
    {
      bool hUpdateAddRef = false;
      try
      {
        // fixed (char* lpNameLocal = lpName)
        // {

        // the lpName transformation is patterned after the MAKEINTRESOURCE macro
        // #define MAKEINTRESOURCEW(i) ((LPWSTR)((ULONG_PTR)((WORD)(i))))
        // I manually evaluated all of the defines in the above expression to show all the base types
        // #define MAKEINTRESOURCEW(i) ((*wchar_t)((unsigned long long)((unsigned short)(i))))
        char* lpNameLocal = (char*)(ulong)lpName;
        fixed (char* lpTypeLocal = lpType)
        {
          winmdroot.Foundation.HANDLE hUpdateLocal;
          if (hUpdate is object)
          {
            hUpdate.DangerousAddRef(ref hUpdateAddRef);
            hUpdateLocal = (winmdroot.Foundation.HANDLE)hUpdate.DangerousGetHandle();
          }
          else
            hUpdateLocal = default(winmdroot.Foundation.HANDLE);
          winmdroot.Foundation.BOOL __result = PInvoke.UpdateResource(hUpdateLocal, lpTypeLocal, lpNameLocal, wLanguage, lpData, cb);
          return __result;
        }
        // }
      }
      finally
      {
        if (hUpdateAddRef)
          hUpdate.DangerousRelease();
      }
    }
  }
}

Context

• CsWin32 version: 0.1.619-beta
• Target Framework: netstandard2.0
• LangVersion: 8.0

[AArnott]

This will have to be fixed in the metadata. We should define a new typedef struct that's decorated with the proper release function and force BeginUpdateResource to return that. Then CsWin32 will generate a new SafeHandle type.