Improve clipboard/OLE data handling and test coverage (#14257)

Refactor ReadUtf8StringFromHGLOBAL for safer null-termination and error handling. Prevent double-release of IStream in TryGetIStreamData. Return null instead of corrupted data when GetData fails. Add comprehensive unit tests and new test mocks for edge cases. Update NativeMethods.txt with CLIPBRD_E_CANT_OPEN. Fixes issues #11401 and #11402 in the WPF repo (which also apply to WinForms as the code is shared now).

Author: JeremyKuhne

src/System.Private.Windows.Core/src/NativeMethods.txt

@@ -9,6 +9,7 @@ CFSTR_FILENAMEA
 CFSTR_INDRAGLOOP
 CLIPBOARD_FORMAT
 CLIPBRD_E_BAD_DATA
+CLIPBRD_E_CANT_OPEN
 CloseEnhMetaFile
 CLR_*
 CoCreateInstance

src/System.Private.Windows.Core/src/System/Private/Windows/Ole/Composition.NativeToManagedAdapter.cs

@@ -226,7 +226,7 @@ private static unsafe string ReadStringFromHGLOBAL(HGLOBAL hglobal, bool unicode
             }
         }
 
-        private static unsafe string ReadUtf8StringFromHGLOBAL(HGLOBAL hglobal)
+        private static string ReadUtf8StringFromHGLOBAL(HGLOBAL hglobal)
         {
             void* buffer = PInvokeCore.GlobalLock(hglobal);
             if (buffer is null)
@@ -237,17 +237,30 @@ private static unsafe string ReadUtf8StringFromHGLOBAL(HGLOBAL hglobal)
             try
             {
                 int size = checked((int)PInvokeCore.GlobalSize(hglobal));
-                return size == 0
-                    ? throw new Win32Exception()
-                    : Encoding.UTF8.GetString((byte*)buffer, size - 1);
+                if (size == 0)
+                {
+                    throw new Win32Exception();
+                }
+
+                ReadOnlySpan<byte> bytes = new((byte*)buffer, size);
+
+                int nullIndex = bytes.IndexOf((byte)0);
+                if (nullIndex < 0)
+                {
+                    // Malformed, return empty string.
+                    return string.Empty;
+                }
+
+                bytes = bytes[..nullIndex];
+                return Encoding.UTF8.GetString(bytes);
             }
             finally
             {
                 PInvokeCore.GlobalUnlock(hglobal);
             }
         }
 
-        private static unsafe string[]? ReadFileListFromHDROP(HDROP hdrop)
+        private static string[]? ReadFileListFromHDROP(HDROP hdrop)
         {
             uint count = PInvokeCore.DragQueryFile(hdrop, iFile: 0xFFFFFFFF, lpszFile: null, cch: 0);
             if (count == 0)
@@ -256,7 +269,7 @@ private static unsafe string ReadUtf8StringFromHGLOBAL(HGLOBAL hglobal)
             }
 
             Span<char> fileName = stackalloc char[(int)PInvokeCore.MAX_PATH + 1];
-            string[] files = new string[count];
+            List<string> files = new((int)count);
 
             fixed (char* buffer = fileName)
             {
@@ -268,12 +281,11 @@ private static unsafe string ReadUtf8StringFromHGLOBAL(HGLOBAL hglobal)
                         continue;
                     }
 
-                    string s = fileName[..(int)charactersCopied].ToString();
-                    files[i] = s;
+                    files.Add(fileName[..(int)charactersCopied].ToString());
                 }
             }
 
-            return files;
+            return [.. files];
         }
 
         /// <summary>
@@ -365,10 +377,17 @@ private static bool TryGetHGLOBALData<T>(
             Debug.WriteLineIf(hr == HRESULT.COR_E_SERIALIZATION,
                 "COR_E_SERIALIZATION returned when trying to get clipboard data, for example, BinaryFormatter threw SerializationException.");
 
+            // If GetData failed, don't try to read from the medium - it may contain uninitialized data.
+            // (This can easily happen when the clipboard content changes between QueryGetData and GetData calls.)
+            if (hr.Failed)
+            {
+                return false;
+            }
+
             bool result = false;
             try
             {
-                if (medium.tymed == Com.TYMED.TYMED_HGLOBAL && !medium.hGlobal.IsNull && hr != HRESULT.COR_E_SERIALIZATION)
+                if (medium.tymed == Com.TYMED.TYMED_HGLOBAL && !medium.hGlobal.IsNull)
                 {
                     result = TryGetDataFromHGLOBAL(medium.hGlobal, in request, out data);
                 }
@@ -391,7 +410,7 @@ private static bool TryGetHGLOBALData<T>(
             return result;
         }
 
-        private static unsafe bool TryGetIStreamData<T>(
+        private static bool TryGetIStreamData<T>(
             Com.IDataObject* dataObject,
             ref readonly DataRequest request,
             [NotNullWhen(true)] out T? data)
@@ -420,8 +439,9 @@ private static unsafe bool TryGetIStreamData<T>(
                     return false;
                 }
 
-                using ComScope<Com.IStream> pStream = new((Com.IStream*)medium.hGlobal);
-                pStream.Value->Stat(out Com.STATSTG sstg, (uint)Com.STATFLAG.STATFLAG_DEFAULT);
+                // Don't wrap in ComScope - ReleaseStgMedium will release the stream.
+                Com.IStream* pStream = (Com.IStream*)medium.hGlobal;
+                pStream->Stat(out Com.STATSTG sstg, (uint)Com.STATFLAG.STATFLAG_DEFAULT);
 
                 hglobal = PInvokeCore.GlobalAlloc(GLOBAL_ALLOC_FLAGS.GMEM_MOVEABLE | GLOBAL_ALLOC_FLAGS.GMEM_ZEROINIT, (uint)sstg.cbSize);
 
@@ -433,7 +453,7 @@ private static unsafe bool TryGetIStreamData<T>(
                 }
 
                 void* ptr = PInvokeCore.GlobalLock(hglobal);
-                pStream.Value->Read((byte*)ptr, (uint)sstg.cbSize, null);
+                pStream->Read((byte*)ptr, (uint)sstg.cbSize, null);
                 PInvokeCore.GlobalUnlock(hglobal);
 
                 return TryGetDataFromHGLOBAL(hglobal, in request, out data);

src/System.Private.Windows.Core/tests/System.Private.Windows.Core.Tests/System/Private/Windows/Ole/FailingGetDataNativeDataObject.cs

@@ -0,0 +1,65 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+using Windows.Win32.Foundation;
+using Windows.Win32.System.Com;
+
+namespace System.Private.Windows.Ole;
+
+/// <summary>
+///  A native data object mock that returns success from QueryGetData but failure from GetData.
+///  This simulates a race condition where the clipboard changes between QueryGetData and GetData.
+/// </summary>
+internal unsafe class FailingGetDataNativeDataObject : NativeDataObjectMock
+{
+    private readonly ushort _format;
+    private readonly HRESULT _failureHResult;
+
+    public FailingGetDataNativeDataObject(ushort format, HRESULT failureHResult = default)
+    {
+        _format = format;
+
+        // Default to CLIPBRD_E_CANT_OPEN which can happen during clipboard contention.
+        _failureHResult = failureHResult == default ? HRESULT.CLIPBRD_E_CANT_OPEN : failureHResult;
+    }
+
+    public override HRESULT QueryGetData(FORMATETC* pformatetc)
+    {
+        if (pformatetc is null)
+        {
+            return HRESULT.DV_E_FORMATETC;
+        }
+
+        if (pformatetc->cfFormat != _format)
+        {
+            return HRESULT.DV_E_FORMATETC;
+        }
+
+        if (pformatetc->dwAspect != (uint)DVASPECT.DVASPECT_CONTENT)
+        {
+            return HRESULT.DV_E_DVASPECT;
+        }
+
+        if (pformatetc->lindex != -1)
+        {
+            return HRESULT.DV_E_LINDEX;
+        }
+
+        if (pformatetc->tymed != (uint)TYMED.TYMED_HGLOBAL)
+        {
+            return HRESULT.DV_E_TYMED;
+        }
+
+        // QueryGetData succeeds - the data appears to be available
+        return HRESULT.S_OK;
+    }
+
+    public override HRESULT GetData(FORMATETC* pformatetcIn, STGMEDIUM* pmedium)
+    {
+        // But GetData fails - simulating clipboard contention or the data being removed
+        // between QueryGetData and GetData calls
+        return _failureHResult;
+    }
+
+    protected override void Dispose(bool disposing) { }
+}

src/System.Private.Windows.Core/tests/System.Private.Windows.Core.Tests/System/Private/Windows/Ole/IStreamNativeDataObject.cs

@@ -0,0 +1,83 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+using Windows.Win32;
+using Windows.Win32.Foundation;
+using Windows.Win32.System.Com;
+
+namespace System.Private.Windows.Ole;
+
+/// <summary>
+///  A native data object mock that returns data via <see cref="TYMED.TYMED_ISTREAM"/>.
+/// </summary>
+internal unsafe class IStreamNativeDataObject : NativeDataObjectMock
+{
+    private readonly Stream _stream;
+    private readonly ushort _format;
+
+    public IStreamNativeDataObject(Stream stream, ushort format)
+    {
+        _stream = stream;
+        _format = format;
+    }
+
+    public override HRESULT QueryGetData(FORMATETC* pformatetc)
+    {
+        if (pformatetc is null)
+        {
+            return HRESULT.DV_E_FORMATETC;
+        }
+
+        if (pformatetc->cfFormat != _format)
+        {
+            return HRESULT.DV_E_FORMATETC;
+        }
+
+        if (pformatetc->dwAspect != (uint)DVASPECT.DVASPECT_CONTENT)
+        {
+            return HRESULT.DV_E_DVASPECT;
+        }
+
+        if (pformatetc->lindex != -1)
+        {
+            return HRESULT.DV_E_LINDEX;
+        }
+
+        if (pformatetc->tymed != (uint)TYMED.TYMED_ISTREAM)
+        {
+            return HRESULT.DV_E_TYMED;
+        }
+
+        return HRESULT.S_OK;
+    }
+
+    public override HRESULT GetData(FORMATETC* pformatetcIn, STGMEDIUM* pmedium)
+    {
+        HRESULT result = QueryGetData(pformatetcIn);
+        if (result.Failed)
+        {
+            return result;
+        }
+
+        if (pmedium is null)
+        {
+            return HRESULT.E_POINTER;
+        }
+
+        // Reset stream position for each GetData call
+        _stream.Position = 0;
+
+        // Create a ComManagedStream wrapper
+        ComManagedStream comStream = new(_stream);
+
+        // Return the IStream pointer in the STGMEDIUM
+        // Note: hGlobal is a union with pstm in STGMEDIUM
+        pmedium->hGlobal = (HGLOBAL)(nint)ComHelpers.GetComPointer<IStream>(comStream);
+        pmedium->tymed = TYMED.TYMED_ISTREAM;
+        pmedium->pUnkForRelease = null;
+
+        return HRESULT.S_OK;
+    }
+
+    protected override void Dispose(bool disposing) => _stream.Dispose();
+}