From 93cdd83612c8195c597c04ba5d2e1de2578da938 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20K=C3=A4s?= Date: Sun, 5 Apr 2020 16:26:38 +0200 Subject: [PATCH 1/5] Use ReleaseStgMedium to clean up shared resources --- .../Interop/Ole32/Interop.ReleaseStgMedium.cs | 20 ++++ .../Forms/DataObject.FormatEnumerator.cs | 4 - .../Windows/Forms/DataObject.OleConverter.cs | 91 +++++++++++-------- .../src/System/Windows/Forms/DataObject.cs | 2 - 4 files changed, 72 insertions(+), 45 deletions(-) create mode 100644 src/System.Windows.Forms.Primitives/src/Interop/Ole32/Interop.ReleaseStgMedium.cs diff --git a/src/System.Windows.Forms.Primitives/src/Interop/Ole32/Interop.ReleaseStgMedium.cs b/src/System.Windows.Forms.Primitives/src/Interop/Ole32/Interop.ReleaseStgMedium.cs new file mode 100644 index 00000000000..615a2c2b2ce --- /dev/null +++ b/src/System.Windows.Forms.Primitives/src/Interop/Ole32/Interop.ReleaseStgMedium.cs @@ -0,0 +1,20 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +using System; +using System.Runtime.InteropServices; +using System.Runtime.InteropServices.ComTypes; + +internal static partial class Interop +{ + internal static partial class Ole32 + { + // Even though the windows headers indicate that STGMEDIUM is input only this is not true, + // ReleaseStgMedium will clear the fields it released. It is important for classic marshaling + // to *NOT* mark the argument as input parameter because otherwise .NET will not transfer + // ownership of the COM pointers correctly. This defeats the purpose of calling this method. + [DllImport(Libraries.Ole32, ExactSpelling = true)] + public static extern void ReleaseStgMedium(ref STGMEDIUM pmedium); + } +} diff --git a/src/System.Windows.Forms/src/System/Windows/Forms/DataObject.FormatEnumerator.cs b/src/System.Windows.Forms/src/System/Windows/Forms/DataObject.FormatEnumerator.cs index c600e0df2e0..cda2b6950f7 100644 --- a/src/System.Windows.Forms/src/System/Windows/Forms/DataObject.FormatEnumerator.cs +++ b/src/System.Windows.Forms/src/System/Windows/Forms/DataObject.FormatEnumerator.cs @@ -94,10 +94,6 @@ public FormatEnumerator(IDataObject parent, string[] formats) { temp.tymed = TYMED.TYMED_GDI; } - else if (format.Equals(DataFormats.EnhancedMetafile)) - { - temp.tymed = TYMED.TYMED_ENHMF; - } else if (format.Equals(DataFormats.Text) || format.Equals(DataFormats.UnicodeText) || format.Equals(DataFormats.StringFormat) diff --git a/src/System.Windows.Forms/src/System/Windows/Forms/DataObject.OleConverter.cs b/src/System.Windows.Forms/src/System/Windows/Forms/DataObject.OleConverter.cs index 8a611f207e8..947a0cde6a4 100644 --- a/src/System.Windows.Forms/src/System/Windows/Forms/DataObject.OleConverter.cs +++ b/src/System.Windows.Forms/src/System/Windows/Forms/DataObject.OleConverter.cs @@ -60,8 +60,6 @@ private unsafe object GetDataFromOleIStream(string format) formatetc.lindex = -1; formatetc.tymed = TYMED.TYMED_ISTREAM; - medium.tymed = TYMED.TYMED_ISTREAM; - // Limit the # of exceptions we may throw below. if ((int)HRESULT.S_OK != QueryGetDataUnsafe(ref formatetc)) { @@ -77,23 +75,37 @@ private unsafe object GetDataFromOleIStream(string format) return null; } - if (medium.unionmember != IntPtr.Zero) + Ole32.IStream pStream = null; + IntPtr hglobal = IntPtr.Zero; + try { - Ole32.IStream pStream = (Ole32.IStream)Marshal.GetObjectForIUnknown(medium.unionmember); - Marshal.Release(medium.unionmember); - pStream.Stat(out Ole32.STATSTG sstg, Ole32.STATFLAG.STATFLAG_DEFAULT); + if (medium.tymed == TYMED.TYMED_ISTREAM && medium.unionmember != IntPtr.Zero) + { + pStream = (Ole32.IStream)Marshal.GetObjectForIUnknown(medium.unionmember); + pStream.Stat(out Ole32.STATSTG sstg, Ole32.STATFLAG.STATFLAG_DEFAULT); - IntPtr hglobal = Kernel32.GlobalAlloc( - Kernel32.GMEM.MOVEABLE | Kernel32.GMEM.DDESHARE | Kernel32.GMEM.ZEROINIT, - (uint)sstg.cbSize); - IntPtr ptr = Kernel32.GlobalLock(hglobal); - pStream.Read((byte*)ptr, (uint)sstg.cbSize, null); - Kernel32.GlobalUnlock(hglobal); + hglobal = Kernel32.GlobalAlloc( + Kernel32.GMEM.MOVEABLE | Kernel32.GMEM.DDESHARE | Kernel32.GMEM.ZEROINIT, + (uint)sstg.cbSize); + IntPtr ptr = Kernel32.GlobalLock(hglobal); + pStream.Read((byte*)ptr, (uint)sstg.cbSize, null); + Kernel32.GlobalUnlock(hglobal); - return GetDataFromHGLOBAL(format, hglobal); + return GetDataFromHGLOBAL(format, hglobal); + } + + return null; } + finally + { + if (hglobal != IntPtr.Zero) + Kernel32.GlobalFree(hglobal); + + if (pStream != null) + Marshal.ReleaseComObject(pStream); - return null; + Ole32.ReleaseStgMedium(ref medium); + } } /// @@ -137,8 +149,6 @@ private object GetDataFromHGLOBAL(string format, IntPtr hglobal) { data = ReadObjectFromHandle(hglobal, DataObject.RestrictDeserializationToSafeTypes(format)); } - - Kernel32.GlobalFree(hglobal); } return data; @@ -160,8 +170,6 @@ private object GetDataFromOleHGLOBAL(string format, out bool done) formatetc.lindex = -1; formatetc.tymed = TYMED.TYMED_HGLOBAL; - medium.tymed = TYMED.TYMED_HGLOBAL; - object data = null; if ((int)HRESULT.S_OK == QueryGetDataUnsafe(ref formatetc)) @@ -170,7 +178,7 @@ private object GetDataFromOleHGLOBAL(string format, out bool done) { innerData.GetData(ref formatetc, out medium); - if (medium.unionmember != IntPtr.Zero) + if (medium.tymed == TYMED.TYMED_HGLOBAL && medium.unionmember != IntPtr.Zero) { data = GetDataFromHGLOBAL(format, medium.unionmember); } @@ -182,6 +190,10 @@ private object GetDataFromOleHGLOBAL(string format, out bool done) catch { } + finally + { + Ole32.ReleaseStgMedium(ref medium); + } } return data; } @@ -204,10 +216,6 @@ private object GetDataFromOleOther(string format) { tymed = TYMED.TYMED_GDI; } - else if (format.Equals(DataFormats.EnhancedMetafile)) - { - tymed = TYMED.TYMED_ENHMF; - } if (tymed == (TYMED)0) { @@ -218,7 +226,6 @@ private object GetDataFromOleOther(string format) formatetc.dwAspect = DVASPECT.DVASPECT_CONTENT; formatetc.lindex = -1; formatetc.tymed = tymed; - medium.tymed = tymed; object data = null; if ((int)HRESULT.S_OK == QueryGetDataUnsafe(ref formatetc)) @@ -232,27 +239,33 @@ private object GetDataFromOleOther(string format) } } - if (medium.unionmember != IntPtr.Zero) + try { - if (format.Equals(DataFormats.Bitmap)) + if (medium.tymed == TYMED.TYMED_GDI && medium.unionmember != IntPtr.Zero) { - // as/urt 140870 -- GDI+ doesn't own this HBITMAP, but we can't - // delete it while the object is still around. So we have to do the really expensive - // thing of cloning the image so we can release the HBITMAP. - - // This bitmap is created by the com object which originally copied the bitmap to tbe - // clipboard. We call Add here, since DeleteObject calls Remove. - Image clipboardImage = Image.FromHbitmap(medium.unionmember); - if (clipboardImage != null) + if (format.Equals(DataFormats.Bitmap)) { - Image firstImage = clipboardImage; - clipboardImage = (Image)clipboardImage.Clone(); - Gdi32.DeleteObject(medium.unionmember); - firstImage.Dispose(); + // as/urt 140870 -- GDI+ doesn't own this HBITMAP, but we can't + // delete it while the object is still around. So we have to do the really expensive + // thing of cloning the image so we can release the HBITMAP. + + // This bitmap is created by the com object which originally copied the bitmap to tbe + // clipboard. We call Add here, since DeleteObject calls Remove. + Image clipboardImage = Image.FromHbitmap(medium.unionmember); + if (clipboardImage != null) + { + Image firstImage = clipboardImage; + clipboardImage = (Image)clipboardImage.Clone(); + firstImage.Dispose(); + } + data = clipboardImage; } - data = clipboardImage; } } + finally + { + Ole32.ReleaseStgMedium(ref medium); + } return data; } diff --git a/src/System.Windows.Forms/src/System/Windows/Forms/DataObject.cs b/src/System.Windows.Forms/src/System/Windows/Forms/DataObject.cs index 327723696c9..9cb3780f4c5 100644 --- a/src/System.Windows.Forms/src/System/Windows/Forms/DataObject.cs +++ b/src/System.Windows.Forms/src/System/Windows/Forms/DataObject.cs @@ -36,8 +36,6 @@ public partial class DataObject : IDataObject, IComDataObject new TYMED[] { TYMED.TYMED_HGLOBAL, TYMED.TYMED_ISTREAM, - TYMED.TYMED_ENHMF, - TYMED.TYMED_MFPICT, TYMED.TYMED_GDI}; private readonly IDataObject innerData = null; From a63e2bb74fc87c850b0bf0910a41716e1310f771 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20K=C3=A4s?= Date: Tue, 7 Apr 2020 20:07:42 +0200 Subject: [PATCH 2/5] PR feedback --- .../src/System/Windows/Forms/DataObject.OleConverter.cs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/System.Windows.Forms/src/System/Windows/Forms/DataObject.OleConverter.cs b/src/System.Windows.Forms/src/System/Windows/Forms/DataObject.OleConverter.cs index 947a0cde6a4..5c6f4022ae5 100644 --- a/src/System.Windows.Forms/src/System/Windows/Forms/DataObject.OleConverter.cs +++ b/src/System.Windows.Forms/src/System/Windows/Forms/DataObject.OleConverter.cs @@ -87,6 +87,10 @@ private unsafe object GetDataFromOleIStream(string format) hglobal = Kernel32.GlobalAlloc( Kernel32.GMEM.MOVEABLE | Kernel32.GMEM.DDESHARE | Kernel32.GMEM.ZEROINIT, (uint)sstg.cbSize); + // not throwing here because the other out of memory condition on GlobalAlloc + // happens inside innerData.GetData and gets turned into a null return value + if (hglobal == IntPtr.Zero) + return null; IntPtr ptr = Kernel32.GlobalLock(hglobal); pStream.Read((byte*)ptr, (uint)sstg.cbSize, null); Kernel32.GlobalUnlock(hglobal); @@ -249,7 +253,7 @@ private object GetDataFromOleOther(string format) // delete it while the object is still around. So we have to do the really expensive // thing of cloning the image so we can release the HBITMAP. - // This bitmap is created by the com object which originally copied the bitmap to tbe + // This bitmap is created by the com object which originally copied the bitmap to the // clipboard. We call Add here, since DeleteObject calls Remove. Image clipboardImage = Image.FromHbitmap(medium.unionmember); if (clipboardImage != null) From 85d11710438d45263e85dedf354d9e20742c588a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20K=C3=A4s?= Date: Tue, 7 Apr 2020 21:12:09 +0200 Subject: [PATCH 3/5] Unit test for bugfix --- .../System/Windows/Forms/DataObjectTests.cs | 49 +++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/DataObjectTests.cs b/src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/DataObjectTests.cs index 09a9e77a5f7..b7d118e1136 100644 --- a/src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/DataObjectTests.cs +++ b/src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/DataObjectTests.cs @@ -8,6 +8,8 @@ using System.Drawing; using System.IO; using System.Linq; +using System.Runtime.InteropServices; +using System.Runtime.InteropServices.ComTypes; using System.Runtime.Serialization; using Moq; using WinForms.Common.Tests; @@ -1558,5 +1560,52 @@ public void DataObject_SetText_InvalidFormat_ThrowsInvalidEnumArgumentException( var dataObject = new DataObject(); Assert.Throws("format", () => dataObject.SetText("text", format)); } + + private sealed class DataObjectIgnoringStorageMediumForEnhancedMetafile : System.Runtime.InteropServices.ComTypes.IDataObject + { + private const int DV_E_FORMATETC = unchecked((int)0x80040064); + private const int CF_ENHMETAFILE = 14; + + public void GetData(ref FORMATETC format, out STGMEDIUM medium) + { + Marshal.ThrowExceptionForHR(QueryGetData(ref format)); + + using var metafile = new Drawing.Imaging.Metafile("bitmaps/milkmateya01.emf"); + + medium = default; + medium.tymed = TYMED.TYMED_ENHMF; + medium.unionmember = metafile.GetHenhmetafile(); + } + + public int QueryGetData(ref FORMATETC format) + { + // do not check the requested storage medium, we always return a metafile handle, thats what Office does + + if (format.cfFormat != CF_ENHMETAFILE || format.dwAspect != DVASPECT.DVASPECT_CONTENT || format.lindex != -1) + return DV_E_FORMATETC; + + return 0; + } + + public IEnumFORMATETC EnumFormatEtc(DATADIR direction) => throw new NotImplementedException(); + public void GetDataHere(ref FORMATETC format, ref STGMEDIUM medium) => throw new NotImplementedException(); + public int GetCanonicalFormatEtc(ref FORMATETC formatIn, out FORMATETC formatOut) => throw new NotImplementedException(); + public void SetData(ref FORMATETC formatIn, ref STGMEDIUM medium, bool release) => throw new NotImplementedException(); + public int DAdvise(ref FORMATETC pFormatetc, ADVF advf, IAdviseSink adviseSink, out int connection) => throw new NotImplementedException(); + public void DUnadvise(int connection) => throw new NotImplementedException(); + public int EnumDAdvise(out IEnumSTATDATA enumAdvise) => throw new NotImplementedException(); + } + + [WinFormsFact] + public void DataObject_GetData_EnhancedMetafile_DoesNotTerminateProcess() + { + var data = new DataObject(new DataObjectIgnoringStorageMediumForEnhancedMetafile()); + + // Office ignores the storage medium in GetData(EnhancedMetafile) and always returns a handle, + // even when asked for a stream. This used to crash the process when DataObject interpreted the + // handle as a pointer to a COM IStream without checking the storage medium it retrieved. + + Assert.Null(data.GetData(DataFormats.EnhancedMetafile)); + } } } From 4ba7901c6f2e6dfbc5592f7bc58457a1a3554f0f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20K=C3=A4s?= Date: Mon, 20 Apr 2020 20:40:29 +0200 Subject: [PATCH 4/5] reference System.Windows.Forms.Primitives instead of duplicating interop constants --- .../tests/UnitTests/System.Windows.Forms.Tests.csproj | 1 + .../UnitTests/System/Windows/Forms/DataObjectTests.cs | 7 ++----- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/src/System.Windows.Forms/tests/UnitTests/System.Windows.Forms.Tests.csproj b/src/System.Windows.Forms/tests/UnitTests/System.Windows.Forms.Tests.csproj index 466f0ffcb29..4162eae72bf 100644 --- a/src/System.Windows.Forms/tests/UnitTests/System.Windows.Forms.Tests.csproj +++ b/src/System.Windows.Forms/tests/UnitTests/System.Windows.Forms.Tests.csproj @@ -18,6 +18,7 @@ + diff --git a/src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/DataObjectTests.cs b/src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/DataObjectTests.cs index b7d118e1136..6a4be2472e0 100644 --- a/src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/DataObjectTests.cs +++ b/src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/DataObjectTests.cs @@ -1563,9 +1563,6 @@ public void DataObject_SetText_InvalidFormat_ThrowsInvalidEnumArgumentException( private sealed class DataObjectIgnoringStorageMediumForEnhancedMetafile : System.Runtime.InteropServices.ComTypes.IDataObject { - private const int DV_E_FORMATETC = unchecked((int)0x80040064); - private const int CF_ENHMETAFILE = 14; - public void GetData(ref FORMATETC format, out STGMEDIUM medium) { Marshal.ThrowExceptionForHR(QueryGetData(ref format)); @@ -1581,8 +1578,8 @@ public int QueryGetData(ref FORMATETC format) { // do not check the requested storage medium, we always return a metafile handle, thats what Office does - if (format.cfFormat != CF_ENHMETAFILE || format.dwAspect != DVASPECT.DVASPECT_CONTENT || format.lindex != -1) - return DV_E_FORMATETC; + if (format.cfFormat != (short)Interop.User32.CF.ENHMETAFILE || format.dwAspect != DVASPECT.DVASPECT_CONTENT || format.lindex != -1) + return (int)Interop.HRESULT.DV_E_FORMATETC; return 0; } From c3d9df8259914c53fdf009c639ba7bd684010789 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20K=C3=A4s?= Date: Mon, 20 Apr 2020 20:41:20 +0200 Subject: [PATCH 5/5] move private nested class to end of containing class --- .../System/Windows/Forms/DataObjectTests.cs | 24 +++++++++---------- 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/DataObjectTests.cs b/src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/DataObjectTests.cs index 6a4be2472e0..d886fa7a47f 100644 --- a/src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/DataObjectTests.cs +++ b/src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/DataObjectTests.cs @@ -1561,6 +1561,18 @@ public void DataObject_SetText_InvalidFormat_ThrowsInvalidEnumArgumentException( Assert.Throws("format", () => dataObject.SetText("text", format)); } + [WinFormsFact] + public void DataObject_GetData_EnhancedMetafile_DoesNotTerminateProcess() + { + var data = new DataObject(new DataObjectIgnoringStorageMediumForEnhancedMetafile()); + + // Office ignores the storage medium in GetData(EnhancedMetafile) and always returns a handle, + // even when asked for a stream. This used to crash the process when DataObject interpreted the + // handle as a pointer to a COM IStream without checking the storage medium it retrieved. + + Assert.Null(data.GetData(DataFormats.EnhancedMetafile)); + } + private sealed class DataObjectIgnoringStorageMediumForEnhancedMetafile : System.Runtime.InteropServices.ComTypes.IDataObject { public void GetData(ref FORMATETC format, out STGMEDIUM medium) @@ -1592,17 +1604,5 @@ public int QueryGetData(ref FORMATETC format) public void DUnadvise(int connection) => throw new NotImplementedException(); public int EnumDAdvise(out IEnumSTATDATA enumAdvise) => throw new NotImplementedException(); } - - [WinFormsFact] - public void DataObject_GetData_EnhancedMetafile_DoesNotTerminateProcess() - { - var data = new DataObject(new DataObjectIgnoringStorageMediumForEnhancedMetafile()); - - // Office ignores the storage medium in GetData(EnhancedMetafile) and always returns a handle, - // even when asked for a stream. This used to crash the process when DataObject interpreted the - // handle as a pointer to a COM IStream without checking the storage medium it retrieved. - - Assert.Null(data.GetData(DataFormats.EnhancedMetafile)); - } } }