Only unwrap given IDataObject

lonitra · lonitra · commit f629c98e363b · 2025-01-09T16:34:23.000-08:00
diff --git a/src/System.Private.Windows.Core/src/Windows/Win32/System/Com/ComHelpers.cs b/src/System.Private.Windows.Core/src/Windows/Win32/System/Com/ComHelpers.cs
@@ -132,7 +132,7 @@ internal static bool SupportsInterface<T>(object? @object) where T : unmanaged,
     /// <summary>
     ///  Attempts to unwrap a ComWrapper CCW as a particular managed object.
     /// </summary>
-    private static bool TryUnwrapComWrapperCCW<TWrapper>(
+    public static bool TryUnwrapComWrapperCCW<TWrapper>(
         IUnknown* unknown,
         [NotNullWhen(true)] out TWrapper? @interface) where TWrapper : class
     {
diff --git a/src/System.Windows.Forms/src/System/Windows/Forms/OLE/Clipboard.cs b/src/System.Windows.Forms/src/System/Windows/Forms/OLE/Clipboard.cs
@@ -40,15 +40,13 @@ public static unsafe void SetDataObject(object data, bool copy, int retryTimes,
         ArgumentOutOfRangeException.ThrowIfNegative(retryTimes);
         ArgumentOutOfRangeException.ThrowIfNegative(retryDelay);
 
-        // Always wrap the data in our DataObject since we know how to retrieve our DataObject from the proxy OleGetClipboard returns.
-        DataObject wrappedData = data is DataObject { IsWrappedForClipboard: true } alreadyWrapped
-            ? alreadyWrapped
-            : new DataObject(data) { IsWrappedForClipboard = true };
-        using var dataObject = ComHelpers.GetComScope<Com.IDataObject>(wrappedData);
+        // Always wrap the data if not already a DataObject. Mark whether the data is an IDataObject so we unwrap it properly on retrieval.
+        DataObject dataObject = data as DataObject ?? new DataObject(data) { IsOriginalNotIDataObject = data is not IDataObject };
+        using var iDataObject = ComHelpers.GetComScope<Com.IDataObject>(dataObject);
 
         HRESULT hr;
         int retry = retryTimes;
-        while ((hr = PInvoke.OleSetClipboard(dataObject)).Failed)
+        while ((hr = PInvoke.OleSetClipboard(iDataObject)).Failed)
         {
             if (--retry < 0)
             {
@@ -100,52 +98,24 @@ public static unsafe void SetDataObject(object data, bool copy, int retryTimes,
 
         // OleGetClipboard always returns a proxy. The proxy forwards all IDataObject method calls to the real data object,
         // without giving out the real data object. If the data placed on the clipboard is not one of our CCWs or the clipboard
-        // has been flushed, marshal will create a wrapper around the proxy for us to use. However, if the data placed on
+        // has been flushed, a wrapper around the proxy for us to use will be given. However, if the data placed on
         // the clipboard is one of our own and the clipboard has not been flushed, we need to retrieve the real data object
-        // pointer in order to retrieve the original managed object via ComWrappers. To do this, we must query for an
-        // interface that is not known to the proxy e.g. IComCallableWrapper. If we are able to query for IComCallableWrapper
-        // it means that the real data object is one of our CCWs and we've retrieved it successfully,
+        // pointer in order to retrieve the original managed object via ComWrappers if an IDataObject was set on the clipboard.
+        // To do this, we must query for an interface that is not known to the proxy e.g. IComCallableWrapper.
+        // If we are able to query for IComCallableWrapper it means that the real data object is one of our CCWs and we've retrieved it successfully,
         // otherwise it is not ours and we will use the wrapped proxy.
-        IUnknown* target = default;
         var realDataObject = proxyDataObject.TryQuery<IComCallableWrapper>(out hr);
-        if (hr.Succeeded)
-        {
-            target = realDataObject.AsUnknown;
-        }
-        else
-        {
-            target = proxyDataObject.AsUnknown;
-        }
-
-        if (!ComHelpers.TryGetObjectForIUnknown(target, out object? managedDataObject))
-        {
-            target->Release();
-            return null;
-        }
-
-        if (managedDataObject is not Com.IDataObject.Interface dataObject)
-        {
-            // We always wrap data set on the Clipboard in a DataObject, so if we do not have
-            // a IDataObject.Interface this means built-in com support is turned off and
-            // we have a proxy where there is no way to retrieve the original data object
-            // pointer from it likely because either the clipboard was flushed or the data on the
-            // clipboard is from another process. We need to mimic built-in com behavior and wrap the proxy ourselves.
-            // DataObject will ref count proxyDataObject properly to take ownership.
-            return new DataObject(proxyDataObject.Value);
-        }
 
-        if (dataObject is DataObject { IsWrappedForClipboard: true } wrappedData)
+        if (hr.Succeeded
+            && ComHelpers.TryUnwrapComWrapperCCW(realDataObject.AsUnknown, out DataObject? dataObject)
+            && !dataObject.IsOriginalNotIDataObject)
         {
-            // There is a DataObject on the clipboard that we placed there. If the real data object
-            // implements IDataObject, we want to unwrap it and return it. Otherwise return
-            // the DataObject as is.
-            return wrappedData.TryUnwrapInnerIDataObject();
+            // An IDataObject was given to us to place on the clipboard. We want to unwrap and return it instead of a proxy.
+            return dataObject.TryUnwrapInnerIDataObject();
         }
 
-        // We did not place the data on the clipboard. Fall back to old behavior.
-        return dataObject is IDataObject ido && !Marshal.IsComObject(dataObject)
-            ? ido
-            : new DataObject(dataObject);
+        // Original data given wasn't an IDataObject, give the proxy value back.
+        return new DataObject(proxyDataObject.Value);
     }
 
     /// <summary>
diff --git a/src/System.Windows.Forms/src/System/Windows/Forms/OLE/DataObject.cs b/src/System.Windows.Forms/src/System/Windows/Forms/OLE/DataObject.cs
@@ -73,9 +73,9 @@ public DataObject(object data)
     internal DataObject(string format, bool autoConvert, object data) : this() => SetData(format, autoConvert, data);
 
     /// <summary>
-    ///  Flags that the original data was wrapped for clipboard purposes.
+    ///  Flags that the original data was not a user passed <see cref="IDataObject"/>.
     /// </summary>
-    internal bool IsWrappedForClipboard { get; init; }
+    internal bool IsOriginalNotIDataObject { get; init; }
 
     /// <summary>
     ///  Returns the inner data that the <see cref="DataObject"/> was created with if the original data implemented
@@ -84,7 +84,7 @@ public DataObject(object data)
     /// </summary>
     internal IDataObject TryUnwrapInnerIDataObject()
     {
-        Debug.Assert(IsWrappedForClipboard, "This method should only be used for clipboard purposes.");
+        Debug.Assert(!IsOriginalNotIDataObject, "This method should only be used for clipboard purposes.");
         return _innerData.OriginalIDataObject is { } original ? original : this;
     }
 
diff --git a/src/System.Windows.Forms/tests/ComDisabledTests/ClipboardComTests.cs b/src/System.Windows.Forms/tests/ComDisabledTests/ClipboardComTests.cs
@@ -4,6 +4,8 @@
 #nullable enable
 
 using System.Drawing;
+using System.Runtime.InteropServices;
+using ComTypes = System.Runtime.InteropServices.ComTypes;
 
 namespace System.Windows.Forms.Tests;
 
@@ -28,4 +30,68 @@ public void Clipboard_SetData_CustomFormat_Color()
         Clipboard.ContainsData(format).Should().BeTrue();
         Clipboard.GetData(format).Should().Be(Color.Black);
     }
+
+    [WinFormsFact]
+    public void Clipboard_GetSet_IDataObject_RoundTrip_ReturnsExpected()
+    {
+        CustomDataObject realDataObject = new();
+        Clipboard.SetDataObject(realDataObject);
+
+        IDataObject clipboardDataObject = Clipboard.GetDataObject().Should().BeAssignableTo<IDataObject>().Subject;
+        clipboardDataObject.Should().BeSameAs(realDataObject);
+        clipboardDataObject.GetDataPresent("Foo").Should().BeTrue();
+        clipboardDataObject.GetData("Foo").Should().Be("Bar");
+    }
+
+    [WinFormsFact]
+    public void Clipboard_SetDataObject_DerivedDataObject_ReturnsExpected()
+    {
+        DerivedDataObject derived = new();
+        Clipboard.SetDataObject(derived);
+        Clipboard.GetDataObject().Should().BeSameAs(derived);
+    }
+
+    private class DerivedDataObject : DataObject { }
+
+    private class CustomDataObject : IDataObject, ComTypes.IDataObject
+    {
+        [DllImport("shell32.dll")]
+        public static extern int SHCreateStdEnumFmtEtc(uint cfmt, ComTypes.FORMATETC[] afmt, out ComTypes.IEnumFORMATETC ppenumFormatEtc);
+
+        int ComTypes.IDataObject.DAdvise(ref ComTypes.FORMATETC pFormatetc, ComTypes.ADVF advf, ComTypes.IAdviseSink adviseSink, out int connection) => throw new NotImplementedException();
+        void ComTypes.IDataObject.DUnadvise(int connection) => throw new NotImplementedException();
+        int ComTypes.IDataObject.EnumDAdvise(out ComTypes.IEnumSTATDATA enumAdvise) => throw new NotImplementedException();
+        ComTypes.IEnumFORMATETC ComTypes.IDataObject.EnumFormatEtc(ComTypes.DATADIR direction)
+        {
+            if (direction == ComTypes.DATADIR.DATADIR_GET)
+            {
+                // Create enumerator and return it
+                ComTypes.IEnumFORMATETC enumerator;
+                if (SHCreateStdEnumFmtEtc(0, [], out enumerator) == 0)
+                {
+                    return enumerator;
+                }
+            }
+
+            throw new NotImplementedException();
+        }
+
+        int ComTypes.IDataObject.GetCanonicalFormatEtc(ref ComTypes.FORMATETC formatIn, out ComTypes.FORMATETC formatOut) => throw new NotImplementedException();
+        object IDataObject.GetData(string format, bool autoConvert) => format == "Foo" ? "Bar" : null!;
+        object IDataObject.GetData(string format) => format == "Foo" ? "Bar" : null!;
+        object IDataObject.GetData(Type format) => null!;
+        void ComTypes.IDataObject.GetData(ref ComTypes.FORMATETC format, out ComTypes.STGMEDIUM medium) => throw new NotImplementedException();
+        void ComTypes.IDataObject.GetDataHere(ref ComTypes.FORMATETC format, ref ComTypes.STGMEDIUM medium) => throw new NotImplementedException();
+        bool IDataObject.GetDataPresent(string format, bool autoConvert) => format == "Foo";
+        bool IDataObject.GetDataPresent(string format) => format == "Foo";
+        bool IDataObject.GetDataPresent(Type format) => false;
+        string[] IDataObject.GetFormats(bool autoConvert) => ["Foo"];
+        string[] IDataObject.GetFormats() => ["Foo"];
+        int ComTypes.IDataObject.QueryGetData(ref ComTypes.FORMATETC format) => throw new NotImplementedException();
+        void IDataObject.SetData(string format, bool autoConvert, object? data) => throw new NotImplementedException();
+        void IDataObject.SetData(string format, object? data) => throw new NotImplementedException();
+        void IDataObject.SetData(Type format, object? data) => throw new NotImplementedException();
+        void IDataObject.SetData(object? data) => throw new NotImplementedException();
+        void ComTypes.IDataObject.SetData(ref ComTypes.FORMATETC formatIn, ref ComTypes.STGMEDIUM medium, bool release) => throw new NotImplementedException();
+    }
 }
diff --git a/src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/ClipboardTests.cs b/src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/ClipboardTests.cs
@@ -10,6 +10,7 @@
 using System.Runtime.InteropServices;
 using System.Runtime.Serialization.Formatters.Binary;
 using Com = Windows.Win32.System.Com;
+using Windows.Win32.System.Ole;
 using ComTypes = System.Runtime.InteropServices.ComTypes;
 
 namespace System.Windows.Forms.Tests;
@@ -520,20 +521,14 @@ public unsafe void Clipboard_GetClipboard_ReturnsProxy()
         ((nint)dataUnknown.Value).Should().Be((nint)realDataPointerUnknown.Value);
     }
 
+    private class DerivedDataObject : DataObject { }
+
     [WinFormsFact]
-    public void Clipboard_Set_DoesNotWrapTwice()
+    public void Clipboard_SetDataObject_DerivedDataObject_ReturnsExpected()
     {
-        string realDataObject = string.Empty;
-        Clipboard.SetDataObject(realDataObject);
-
-        IDataObject? clipboardDataObject = Clipboard.GetDataObject();
-        var dataObject = clipboardDataObject.Should().BeOfType<DataObject>().Which;
-        dataObject.IsWrappedForClipboard.Should().BeTrue();
-
-        Clipboard.SetDataObject(clipboardDataObject!);
-        IDataObject? clipboardDataObject2 = Clipboard.GetDataObject();
-        clipboardDataObject2.Should().NotBeNull();
-        clipboardDataObject2.Should().BeSameAs(clipboardDataObject);
+        DerivedDataObject derived = new();
+        Clipboard.SetDataObject(derived);
+        Clipboard.GetDataObject().Should().BeSameAs(derived);
     }
 
     [WinFormsFact]
@@ -589,4 +584,119 @@ ComTypes.IEnumFORMATETC ComTypes.IDataObject.EnumFormatEtc(ComTypes.DATADIR dire
         void IDataObject.SetData(object? data) => throw new NotImplementedException();
         void ComTypes.IDataObject.SetData(ref ComTypes.FORMATETC formatIn, ref ComTypes.STGMEDIUM medium, bool release) => throw new NotImplementedException();
     }
+
+    [DllImport("user32.dll")]
+    private static extern bool CloseClipboard();
+
+    [DllImport("user32.dll")]
+    private static extern bool OpenClipboard(HWND hWndNewOwner);
+
+    [DllImport("user32.dll")]
+    private static extern bool SetClipboardData(uint uFormat, HANDLE data);
+
+    [WinFormsFact]
+    public unsafe void Clipboard_RawClipboard_SetClipboardData_ReturnsExpected()
+    {
+        OpenClipboard(HWND.Null).Should().BeTrue();
+        string testString = "test";
+        SetClipboardData((uint)CLIPBOARD_FORMAT.CF_UNICODETEXT, (HANDLE)Marshal.StringToHGlobalUni(testString));
+        CloseClipboard().Should().BeTrue();
+
+        DataObject dataObject = Clipboard.GetDataObject().Should().BeOfType<DataObject>().Which;
+        dataObject.GetData(DataFormats.Text).Should().Be(testString);
+
+        Clipboard.ContainsText().Should().BeTrue();
+        Clipboard.ContainsData(DataFormats.Text).Should().BeTrue();
+        Clipboard.ContainsData(DataFormats.UnicodeText).Should().BeTrue();
+
+        Clipboard.GetText().Should().Be(testString);
+        Clipboard.GetText(TextDataFormat.Text).Should().Be(testString);
+        Clipboard.GetText(TextDataFormat.UnicodeText).Should().Be(testString);
+
+        Clipboard.GetData("System.String").Should().BeNull();
+    }
+
+    [WinFormsFact]
+    public void Clipboard_SetData_Text_Format_AllUpper()
+    {
+        // The fact that casing on input matters is likely incorrect, but behavior has been this way.
+        Clipboard.SetData("TEXT", "Hello, World!");
+        Clipboard.ContainsText().Should().BeTrue();
+        Clipboard.ContainsData("TEXT").Should().BeTrue();
+        Clipboard.ContainsData(DataFormats.Text).Should().BeTrue();
+        Clipboard.ContainsData(DataFormats.UnicodeText).Should().BeTrue();
+
+        IDataObject dataObject = Clipboard.GetDataObject().Should().BeAssignableTo<IDataObject>().Subject;
+        string[] formats = dataObject.GetFormats();
+        formats.Should().BeEquivalentTo(["System.String", "UnicodeText", "Text"]);
+
+        formats = dataObject.GetFormats(autoConvert: false);
+        formats.Should().BeEquivalentTo(["Text"]);
+
+        // CLIPBRD_E_BAD_DATA returned when trying to get clipboard data.
+        Clipboard.GetText().Should().BeEmpty();
+        Clipboard.GetText(TextDataFormat.Text).Should().BeEmpty();
+        Clipboard.GetText(TextDataFormat.UnicodeText).Should().BeEmpty();
+
+        Clipboard.GetData("System.String").Should().BeNull();
+        Clipboard.GetData("TEXT").Should().BeNull();
+    }
+
+    [WinFormsFact]
+    public void Clipboard_SetData_Text_Format_CanonicalCase()
+    {
+        string expected = "Hello, World!";
+        Clipboard.SetData("Text", expected);
+        Clipboard.ContainsText().Should().BeTrue();
+        Clipboard.ContainsData("TEXT").Should().BeTrue();
+        Clipboard.ContainsData(DataFormats.Text).Should().BeTrue();
+        Clipboard.ContainsData(DataFormats.UnicodeText).Should().BeTrue();
+
+        IDataObject dataObject = Clipboard.GetDataObject().Should().BeAssignableTo<IDataObject>().Subject;
+        string[] formats = dataObject.GetFormats();
+        formats.Should().BeEquivalentTo(["System.String", "UnicodeText", "Text"]);
+
+        formats = dataObject.GetFormats(autoConvert: false);
+        formats.Should().BeEquivalentTo(["System.String", "UnicodeText", "Text"]);
+
+        Clipboard.GetText().Should().Be(expected);
+        Clipboard.GetText(TextDataFormat.Text).Should().Be(expected);
+        Clipboard.GetText(TextDataFormat.UnicodeText).Should().Be(expected);
+
+        Clipboard.GetData("System.String").Should().Be(expected);
+
+        // Case sensitivity matters so we end up reading stream/object from HGLOBAL instead of string.
+        MemoryStream stream = Clipboard.GetData("TEXT").Should().BeOfType<MemoryStream>().Subject;
+        byte[] array = stream.ToArray();
+        array.Should().BeEquivalentTo("Hello, World!\0"u8.ToArray());
+    }
+
+    [WinFormsFact]
+    public void Clipboard_SetDataObject_Text()
+    {
+        string expected = "Hello, World!";
+        Clipboard.SetDataObject(expected);
+        Clipboard.ContainsText().Should().BeTrue();
+        Clipboard.ContainsData("TEXT").Should().BeTrue();
+        Clipboard.ContainsData(DataFormats.Text).Should().BeTrue();
+        Clipboard.ContainsData(DataFormats.UnicodeText).Should().BeTrue();
+
+        IDataObject dataObject = Clipboard.GetDataObject().Should().BeAssignableTo<IDataObject>().Subject;
+        string[] formats = dataObject.GetFormats();
+        formats.Should().BeEquivalentTo(["System.String", "UnicodeText", "Text"]);
+
+        formats = dataObject.GetFormats(autoConvert: false);
+        formats.Should().BeEquivalentTo(["System.String", "UnicodeText", "Text"]);
+
+        Clipboard.GetText().Should().Be(expected);
+        Clipboard.GetText(TextDataFormat.Text).Should().Be(expected);
+        Clipboard.GetText(TextDataFormat.UnicodeText).Should().Be(expected);
+
+        Clipboard.GetData("System.String").Should().Be(expected);
+
+        // Case sensitivity matters so we end up reading stream/object from HGLOBAL instead of string.
+        MemoryStream stream = Clipboard.GetData("TEXT").Should().BeOfType<MemoryStream>().Subject;
+        byte[] array = stream.ToArray();
+        array.Should().BeEquivalentTo("Hello, World!\0"u8.ToArray());
+    }
 }

Original file line number	Diff line number	Diff line change
`@@ -132,7 +132,7 @@ internal static bool SupportsInterface<T>(object? @object) where T : unmanaged,`
`132`	`132`	`/// <summary>`
`133`	`133`	`/// Attempts to unwrap a ComWrapper CCW as a particular managed object.`
`134`	`134`	`/// </summary>`
`135`		`- private static bool TryUnwrapComWrapperCCW<TWrapper>(`
	`135`	`+ public static bool TryUnwrapComWrapperCCW<TWrapper>(`
`136`	`136`	`IUnknown* unknown,`
`137`	`137`	`[NotNullWhen(true)] out TWrapper? @interface) where TWrapper : class`
`138`	`138`	`{`