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..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 @@ -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,41 @@ 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); - - 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); + 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); + + 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); + + 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 +153,6 @@ private object GetDataFromHGLOBAL(string format, IntPtr hglobal) { data = ReadObjectFromHandle(hglobal, DataObject.RestrictDeserializationToSafeTypes(format)); } - - Kernel32.GlobalFree(hglobal); } return data; @@ -160,8 +174,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 +182,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 +194,10 @@ private object GetDataFromOleHGLOBAL(string format, out bool done) catch { } + finally + { + Ole32.ReleaseStgMedium(ref medium); + } } return data; } @@ -204,10 +220,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 +230,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 +243,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 the + // 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; 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 09a9e77a5f7..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 @@ -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,49 @@ public void DataObject_SetText_InvalidFormat_ThrowsInvalidEnumArgumentException( var dataObject = new DataObject(); 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) + { + 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 != (short)Interop.User32.CF.ENHMETAFILE || format.dwAspect != DVASPECT.DVASPECT_CONTENT || format.lindex != -1) + return (int)Interop.HRESULT.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(); + } } }