GC holes in some of the WIC/D2D code

[rickbrew]

I'm seeing several places like this, but here's one example: https://github.com/0xC0000054/PSFilterPdn/blob/5446a012e59118d56bffd3ac51dcf1f918ce1bef/src/PSFilterShim/PSApi/Imaging/Internal/WICBitmapSurface.cs#L44

    internal sealed unsafe class WICBitmapSurface<TPixel> : ImageSurface where TPixel : unmanaged, IPixelFormatInfo
    {
        private readonly ComPtr<IWICBitmap> bitmap;
        private readonly IWICFactory factory;

        public WICBitmapSurface(int width, int height, IWICFactory factory)
        {
            ArgumentNullException.ThrowIfNull(factory, nameof(factory));
            ThrowIfNotPositive(width, nameof(width));
            ThrowIfNotPositive(height, nameof(height));

            Width = width;
            Height = height;
            ChannelCount = TPixel.ChannelCount;
            BitsPerChannel = TPixel.BitsPerChannel;
            Format = TPixel.Format;
            SupportsTransparency = TPixel.SupportsTransparency;
            this.factory = factory;

            Guid wicPixelFormat = GetWICPixelFormat(Format);

            HRESULT hr = factory.Get()->CreateBitmap((uint)width,
                                                     (uint)height,
                                                     &wicPixelFormat,
                                                     WICBitmapCacheOnLoad,
                                                     bitmap.GetAddressOf()); // <-------------------------------
            WICException.ThrowIfFailed("Failed to create the WIC Bitmap.", hr);
            this.factory = factory;
        }

On the marked line, this is a GC hole. If the GC runs while CreateBitmap() is running, and if the WICBitmapSurface<TPixel> instance happens to get moved, then the native method will write to where this.bitmap was, which could be ... anything. The GC has no information to steer it away from doing this.

ComPtr<T>.GetAddressOf() should really be called DangerousGetAddressOf(). It uses Unsafe.AsPointer(ref this), which is only actually safe if this (the ComPtr<T>) is pinned, or otherwise unmovable (e.g. on the stack, inside a native memory alloc, ...).

You need to either pin bitmap, or just do what I do which is to bounce it through a stack local:

using ComPtr<IWICBitmap> spBitmap = default; // "sp" for "smart pointer". old school naming habit
HRESULT hr = factory.Get()->CreateBitmap(..., spBitmap.GetAddressOf());
WICException.ThrowIfFailed("Failed to create the WIC Bitmap.", hr);

bitmap.Attach(spBitmap.Detach()); // just moves the pointer, cheapest option but more verbose
OR
spBitmap.CopyTo(ref bitmap); // copy pointer and AddRef()
OR
bitmap = spBitmap.Get();  // implicit cast from T* to ComPtr<T>, which will AddRef()

[rickbrew]

It looks like this should also work:

fixed (IWICBitmap** ppBitmap = bitmap)
{
    HRESULT hr = factory.Get()->CreateBitmap(..., ppBitmap);
    ...
}

ComPtr<T> has a GetPinnableReference() method that returns a ref T*, which is what enables this.

[0xC0000054]

Thanks Rick. I missed that during my own code reviews. I am now auditing all of my ComPtr<T> code to address this.

[0xC0000054]

I use ComPtr<T>.Swap in a few places to transfer ownership from a local variable to a newly constructed class, see https://github.com/0xC0000054/PSFilterPdn/blob/5446a012e59118d56bffd3ac51dcf1f918ce1bef/src/PSFilterShim/PSApi/Rendering/Internal/DeviceBitmapBrush.cs#L25C13-L25C13 Since the runtime initializes the ComPtr<T> fields to 0/null when the class is created, calling Swap causes the ref ComPtr<T> constructor parameter to be set to null.