Fix DataObject.GetData issues

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);
+    }
+}

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)

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);
+                }
             }
 
             /// <summary>
@@ -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;
             }

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;

src/System.Windows.Forms/tests/UnitTests/System.Windows.Forms.Tests.csproj

@@ -18,6 +18,7 @@
     <ProjectReference Include="..\..\..\System.Drawing.Design\src\System.Drawing.Design.Facade.csproj" />
     <ProjectReference Include="..\..\..\System.Windows.Forms.Primitives\src\System.Windows.Forms.Primitives.csproj" />
     <ProjectReference Include="..\..\..\Common\tests\InternalUtilitiesForTests\InternalUtilitiesForTests.csproj" />
+    <ProjectReference Include="..\..\..\System.Windows.Forms.Primitives\src\System.Windows.Forms.Primitives.csproj" />
   </ItemGroup>
 
   <ItemGroup>

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<InvalidEnumArgumentException>("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();
+        }
     }
 }