From 813a5180d899697f96b7d30077edfe885d28776d Mon Sep 17 00:00:00 2001 From: Loni Tra Date: Mon, 29 Jan 2024 11:09:23 -0800 Subject: [PATCH 1/3] Fix 4555 --- .../src/System/Windows/Forms/OLE/Clipboard.cs | 17 ++++- .../System/Windows/Forms/OLE/DataObject.cs | 22 ++++++ .../System/Windows/Forms/ClipboardTests.cs | 69 +++++++++++++++++++ 3 files changed, 107 insertions(+), 1 deletion(-) 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 index 632339cde8b..81436cadcb4 100644 --- a/src/System.Windows.Forms/src/System/Windows/Forms/OLE/Clipboard.cs +++ b/src/System.Windows.Forms/src/System/Windows/Forms/OLE/Clipboard.cs @@ -41,7 +41,11 @@ public static unsafe void SetDataObject(object data, bool copy, int retryTimes, ArgumentOutOfRangeException.ThrowIfNegative(retryTimes); ArgumentOutOfRangeException.ThrowIfNegative(retryDelay); - using var dataObject = ComHelpers.GetComScope(data is IComDataObject ? data : new DataObject(data)); + // 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(wrappedData); HRESULT hr; int retry = retryTimes; @@ -120,6 +124,17 @@ public static unsafe void SetDataObject(object data, bool copy, int retryTimes, return null; } + if (dataObject is DataObject { IsWrappedForClipboard: true } wrappedData) + { + // 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.UnwrapInnerIDataObject() is { } innerData + ? innerData + : wrappedData; + } + + // 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); 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 index 972b0ebb900..a5f4b66c620 100644 --- a/src/System.Windows.Forms/src/System/Windows/Forms/OLE/DataObject.cs +++ b/src/System.Windows.Forms/src/System/Windows/Forms/OLE/DataObject.cs @@ -127,6 +127,28 @@ public DataObject(object data) internal DataObject(string format, bool autoConvert, object data) : this() => SetData(format, autoConvert, data); + /// + /// Flags that the original data was wrapped for clipboard purposes. + /// + internal bool IsWrappedForClipboard { get; set; } + + /// + /// Returns the inner data that the was created with if the original data implemented + /// . Otherwise, returns null. + /// This method should only be used if the was created for clipboard purposes. + /// + internal IDataObject? UnwrapInnerIDataObject() + { + if (!IsWrappedForClipboard) + { + throw new InvalidOperationException("This method should only be used for clipboard purposes."); + } + + return _innerData is DataStore or ComDataObjectAdapter + ? null + : _innerData; + } + /// /// Retrieves the data associated with the specified data format, using an automated conversion parameter to /// determine whether to convert the data to the format. 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 index 563a5313e0b..8aaf5a295c8 100644 --- a/src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/ClipboardTests.cs +++ b/src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/ClipboardTests.cs @@ -5,8 +5,10 @@ using System.ComponentModel; using System.Drawing; using System.Drawing.Imaging; +using System.Runtime.InteropServices; using System.Runtime.Serialization.Formatters.Binary; using Com = Windows.Win32.System.Com; +using ComTypes = System.Runtime.InteropServices.ComTypes; namespace System.Windows.Forms.Tests; @@ -535,4 +537,71 @@ public unsafe void ClipBoard_GetClipboard_ReturnsProxy() ((nint)proxyUnknown.Value).Should().NotBe((nint)realDataPointerUnknown.Value); ((nint)dataUnknown.Value).Should().Be((nint)realDataPointerUnknown.Value); } + + [WinFormsFact] + public void ClipBoard_Set_DoesNotWrapTwice() + { + string realDataObject = string.Empty; + Clipboard.SetDataObject(realDataObject); + IDataObject clipboardDataObject = Clipboard.GetDataObject(); + clipboardDataObject.Should().BeOfType(typeof(DataObject)); + ((DataObject)clipboardDataObject).IsWrappedForClipboard.Should().BeTrue(); + + Clipboard.SetDataObject(clipboardDataObject); + IDataObject clipboardDataObject2 = Clipboard.GetDataObject(); + clipboardDataObject2.Should().BeSameAs(clipboardDataObject); + } + + [WinFormsFact] + public void ClipBoard_GetSet_RoundTrip_ReturnsExpected() + { + CustomDataObject realDataObject = new(); + Clipboard.SetDataObject(realDataObject); + IDataObject clipboardDataObject = Clipboard.GetDataObject(); + clipboardDataObject.Should().BeSameAs(realDataObject); + clipboardDataObject.GetDataPresent("Foo").Should().BeTrue(); + clipboardDataObject.GetData("Foo").Should().Be("Bar"); + } + + 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(); + } } From 2132cdf20144050ffac7c592c1abbdc77738259e Mon Sep 17 00:00:00 2001 From: Loni Tra Date: Tue, 30 Jan 2024 13:12:23 -0800 Subject: [PATCH 2/3] Address feedback --- .../src/System/Windows/Forms/OLE/Clipboard.cs | 4 +--- .../src/System/Windows/Forms/OLE/DataObject.cs | 11 ++++------- 2 files changed, 5 insertions(+), 10 deletions(-) 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 index 81436cadcb4..53b492526f9 100644 --- a/src/System.Windows.Forms/src/System/Windows/Forms/OLE/Clipboard.cs +++ b/src/System.Windows.Forms/src/System/Windows/Forms/OLE/Clipboard.cs @@ -129,9 +129,7 @@ public static unsafe void SetDataObject(object data, bool copy, int retryTimes, // 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.UnwrapInnerIDataObject() is { } innerData - ? innerData - : wrappedData; + return wrappedData.TryUnwrapInnerIDataObject(); } // We did not place the data on the clipboard. Fall back to old behavior. 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 index a5f4b66c620..8c94af8c8dd 100644 --- a/src/System.Windows.Forms/src/System/Windows/Forms/OLE/DataObject.cs +++ b/src/System.Windows.Forms/src/System/Windows/Forms/OLE/DataObject.cs @@ -134,18 +134,15 @@ public DataObject(object data) /// /// Returns the inner data that the was created with if the original data implemented - /// . Otherwise, returns null. + /// . Otherwise, returns this. /// This method should only be used if the was created for clipboard purposes. /// - internal IDataObject? UnwrapInnerIDataObject() + internal IDataObject TryUnwrapInnerIDataObject() { - if (!IsWrappedForClipboard) - { - throw new InvalidOperationException("This method should only be used for clipboard purposes."); - } + Debug.Assert(IsWrappedForClipboard, "This method should only be used for clipboard purposes."); return _innerData is DataStore or ComDataObjectAdapter - ? null + ? this : _innerData; } From 9cceab04e3338886c9171a50427514351d0c5c05 Mon Sep 17 00:00:00 2001 From: Loni Tra Date: Tue, 30 Jan 2024 13:14:38 -0800 Subject: [PATCH 3/3] Change IsWrappedForClipboard to init only --- .../src/System/Windows/Forms/OLE/DataObject.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 index 8c94af8c8dd..83cdbb457a3 100644 --- a/src/System.Windows.Forms/src/System/Windows/Forms/OLE/DataObject.cs +++ b/src/System.Windows.Forms/src/System/Windows/Forms/OLE/DataObject.cs @@ -130,7 +130,7 @@ public DataObject(object data) /// /// Flags that the original data was wrapped for clipboard purposes. /// - internal bool IsWrappedForClipboard { get; set; } + internal bool IsWrappedForClipboard { get; init; } /// /// Returns the inner data that the was created with if the original data implemented