From df5df6ce55ada273fdf736789d92b2739e55df2f Mon Sep 17 00:00:00 2001 From: Eric Erhardt Date: Mon, 21 Jun 2021 18:38:54 -0500 Subject: [PATCH 01/11] First round of converting System.Drawing.Common to COMWrappers Using COM Wrappers makes the library trim compatible. --- .../Interop/Windows/Ole32/Interop.IStream.cs | 51 ++++ .../src/System.Drawing.Common.csproj | 7 + .../src/System/Drawing/DrawingComWrappers.cs | 279 ++++++++++++++++++ .../Drawing/Icon.Windows.COMWrappers.cs | 80 +++++ .../Drawing/Icon.Windows.NoCOMWrappers.cs | 115 ++++++++ .../src/System/Drawing/Icon.Windows.cs | 101 ------- .../src/System/Drawing/Internal/GPStream.cs | 49 ++- .../System.Drawing.Common/tests/IconTests.cs | 13 +- 8 files changed, 590 insertions(+), 105 deletions(-) create mode 100644 src/libraries/System.Drawing.Common/src/System/Drawing/DrawingComWrappers.cs create mode 100644 src/libraries/System.Drawing.Common/src/System/Drawing/Icon.Windows.COMWrappers.cs create mode 100644 src/libraries/System.Drawing.Common/src/System/Drawing/Icon.Windows.NoCOMWrappers.cs diff --git a/src/libraries/Common/src/Interop/Windows/Ole32/Interop.IStream.cs b/src/libraries/Common/src/Interop/Windows/Ole32/Interop.IStream.cs index 1b78ce6f4745a6..0e8c269be1dad9 100644 --- a/src/libraries/Common/src/Interop/Windows/Ole32/Interop.IStream.cs +++ b/src/libraries/Common/src/Interop/Windows/Ole32/Interop.IStream.cs @@ -1,6 +1,7 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +using System; using System.IO; using System.Runtime.InteropServices; @@ -62,5 +63,55 @@ void Stat( IStream Clone(); } + + // This needs to be duplicated from IStream above because ComWrappers doesn't support re-using + // ComImport interfaces. The above interface can be deleted once all usages of the built-in COM support + // are converted to use ComWrappers. + internal interface IStreamComWrapper + { + static readonly Guid Guid = new Guid(0x0000000C, 0x0000, 0x0000, 0xC0, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x46); + + // pcbRead is optional so it must be a pointer + unsafe void Read(byte* pv, uint cb, uint* pcbRead); + + // pcbWritten is optional so it must be a pointer + unsafe void Write(byte* pv, uint cb, uint* pcbWritten); + + // SeekOrgin matches the native values, plibNewPosition is optional + unsafe void Seek(long dlibMove, SeekOrigin dwOrigin, ulong* plibNewPosition); + + void SetSize(ulong libNewSize); + + // pcbRead and pcbWritten are optional + unsafe void CopyTo( + IStreamComWrapper pstm, + ulong cb, + ulong* pcbRead, + ulong* pcbWritten); + + void Commit(uint grfCommitFlags); + + void Revert(); + + // Using PreserveSig to allow explicitly returning the HRESULT for "not supported". + + [PreserveSig] + HRESULT LockRegion( + ulong libOffset, + ulong cb, + uint dwLockType); + + [PreserveSig] + HRESULT UnlockRegion( + ulong libOffset, + ulong cb, + uint dwLockType); + + void Stat( + out STATSTG pstatstg, + STATFLAG grfStatFlag); + + IStreamComWrapper Clone(); + } } } diff --git a/src/libraries/System.Drawing.Common/src/System.Drawing.Common.csproj b/src/libraries/System.Drawing.Common/src/System.Drawing.Common.csproj index 092ea6af165e91..b02d55a46c6a2b 100644 --- a/src/libraries/System.Drawing.Common/src/System.Drawing.Common.csproj +++ b/src/libraries/System.Drawing.Common/src/System.Drawing.Common.csproj @@ -317,6 +317,13 @@ + + + + + + + diff --git a/src/libraries/System.Drawing.Common/src/System/Drawing/DrawingComWrappers.cs b/src/libraries/System.Drawing.Common/src/System/Drawing/DrawingComWrappers.cs new file mode 100644 index 00000000000000..6e1f325d5b7a4d --- /dev/null +++ b/src/libraries/System.Drawing.Common/src/System/Drawing/DrawingComWrappers.cs @@ -0,0 +1,279 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Collections; +using System.Diagnostics; +using System.IO; +using System.Runtime.CompilerServices; +using System.Runtime.InteropServices; + +namespace System.Drawing +{ + /// + /// The ComWrappers implementation for System.Drawing.Common's COM interop usages. + /// + /// Supports IStream and IPicture COM interfaces. + /// + internal unsafe class DrawingComWrappers : ComWrappers + { + private static readonly ComInterfaceEntry* s_wrapperEntry = InitializeComInterfaceEntry(); + private static readonly Lazy s_instance = new Lazy(() => new DrawingComWrappers()); + + private DrawingComWrappers() { } + + internal static DrawingComWrappers Instance => s_instance.Value; + + internal static void CheckStatus(int result) + { + if (result != 0) + { + throw new ExternalException() { HResult = result }; + } + } + + private static ComInterfaceEntry* InitializeComInterfaceEntry() + { + GetIUnknownImpl(out IntPtr fpQueryInteface, out IntPtr fpAddRef, out IntPtr fpRelease); + + IStreamVtbl iStreamVtbl = IStreamVtbl.Create(fpQueryInteface, fpAddRef, fpRelease); + + IntPtr iStreamVtblRaw = RuntimeHelpers.AllocateTypeAssociatedMemory(typeof(IStreamVtbl), sizeof(IStreamVtbl)); + Marshal.StructureToPtr(iStreamVtbl, iStreamVtblRaw, false); + + ComInterfaceEntry* wrapperEntry = (ComInterfaceEntry*)RuntimeHelpers.AllocateTypeAssociatedMemory(typeof(IStreamVtbl), sizeof(ComInterfaceEntry)); + wrapperEntry->IID = Interop.Ole32.IStreamComWrapper.Guid; + wrapperEntry->Vtable = iStreamVtblRaw; + return wrapperEntry; + } + + protected override unsafe ComInterfaceEntry* ComputeVtables(object obj, CreateComInterfaceFlags flags, out int count) + { + Debug.Assert(obj is Interop.Ole32.IStreamComWrapper); + Debug.Assert(s_wrapperEntry != null); + + // Always return the same table mappings. + count = 1; + return s_wrapperEntry; + } + + protected override object CreateObject(IntPtr externalComObject, CreateObjectFlags flags) + { + Debug.Assert(flags == CreateObjectFlags.None); + + Guid pictureGuid = IPicture.Guid; + int hr = Marshal.QueryInterface(externalComObject, ref pictureGuid, out IntPtr comObject); + if (hr == 0) + { + return new PictureWrapper(comObject); + } + + throw new NotImplementedException(); + } + + protected override void ReleaseObjects(IEnumerable objects) + { + throw new NotImplementedException(); + } + + internal struct IUnknownVtbl + { + public IntPtr QueryInterface; + public IntPtr AddRef; + public IntPtr Release; + } + + internal struct IStreamVtbl + { + public IUnknownVtbl IUnknownImpl; + public IntPtr Read; + public IntPtr Write; + public IntPtr Seek; + public IntPtr SetSize; + public IntPtr CopyTo; + public IntPtr Commit; + public IntPtr Revert; + public IntPtr LockRegion; + public IntPtr UnlockRegion; + public IntPtr Stat; + public IntPtr Clone; + + private delegate void _Read(IntPtr thisPtr, byte* pv, uint cb, uint* pcbRead); + private delegate void _Write(IntPtr thisPtr, byte* pv, uint cb, uint* pcbWritten); + private delegate void _Seek(IntPtr thisPtr, long dlibMove, SeekOrigin dwOrigin, ulong* plibNewPosition); + private delegate void _SetSize(IntPtr thisPtr, ulong libNewSize); + private delegate void _CopyTo(IntPtr thisPtr, IntPtr pstm, ulong cb, ulong* pcbRead, ulong* pcbWritten); + private delegate void _Commit(IntPtr thisPtr, uint grfCommitFlags); + private delegate void _Revert(IntPtr thisPtr); + private delegate Interop.HRESULT _LockRegion(IntPtr thisPtr, ulong libOffset, ulong cb, uint dwLockType); + private delegate Interop.HRESULT _UnlockRegion(IntPtr thisPtr, ulong libOffset, ulong cb, uint dwLockType); + private delegate void _Stat(IntPtr thisPtr, out Interop.Ole32.STATSTG pstatstg, Interop.Ole32.STATFLAG grfStatFlag); + private delegate IntPtr _Clone(IntPtr thisPtr); + + private static _Read s_Read = new _Read(ReadImplementation); + private static _Write s_Write = new _Write(WriteImplementation); + private static _Seek s_Seek = new _Seek(SeekImplementation); + private static _SetSize s_SetSize = new _SetSize(SetSizeImplementation); + private static _CopyTo s_CopyTo = new _CopyTo(CopyToImplementation); + private static _Commit s_Commit = new _Commit(CommitImplementation); + private static _Revert s_Revert = new _Revert(RevertImplementation); + private static _LockRegion s_LockRegion = new _LockRegion(LockRegionImplementation); + private static _UnlockRegion s_UnlockRegion = new _UnlockRegion(UnlockRegionImplementation); + private static _Stat s_Stat = new _Stat(StatImplementation); + private static _Clone s_Clone = new _Clone(CloneImplementation); + + public static IStreamVtbl Create(IntPtr fpQueryInteface, IntPtr fpAddRef, IntPtr fpRelease) + { + return new IStreamVtbl() + { + IUnknownImpl = new IUnknownVtbl() + { + QueryInterface = fpQueryInteface, + AddRef = fpAddRef, + Release = fpRelease + }, + Read = Marshal.GetFunctionPointerForDelegate(s_Read), + Write = Marshal.GetFunctionPointerForDelegate(s_Write), + Seek = Marshal.GetFunctionPointerForDelegate(s_Seek), + SetSize = Marshal.GetFunctionPointerForDelegate(s_SetSize), + CopyTo = Marshal.GetFunctionPointerForDelegate(s_CopyTo), + Commit = Marshal.GetFunctionPointerForDelegate(s_Commit), + Revert = Marshal.GetFunctionPointerForDelegate(s_Revert), + LockRegion = Marshal.GetFunctionPointerForDelegate(s_LockRegion), + UnlockRegion = Marshal.GetFunctionPointerForDelegate(s_UnlockRegion), + Stat = Marshal.GetFunctionPointerForDelegate(s_Stat), + Clone = Marshal.GetFunctionPointerForDelegate(s_Clone), + }; + } + + private static void ReadImplementation(IntPtr thisPtr, byte* pv, uint cb, uint* pcbRead) + { + Interop.Ole32.IStreamComWrapper inst = ComInterfaceDispatch.GetInstance((ComInterfaceDispatch*)thisPtr); + inst.Read(pv, cb, pcbRead); + } + + private static void WriteImplementation(IntPtr thisPtr, byte* pv, uint cb, uint* pcbWritten) + { + Interop.Ole32.IStreamComWrapper inst = ComInterfaceDispatch.GetInstance((ComInterfaceDispatch*)thisPtr); + inst.Write(pv, cb, pcbWritten); + } + + private static void SeekImplementation(IntPtr thisPtr, long dlibMove, SeekOrigin dwOrigin, ulong* plibNewPosition) + { + Interop.Ole32.IStreamComWrapper inst = ComInterfaceDispatch.GetInstance((ComInterfaceDispatch*)thisPtr); + inst.Seek(dlibMove, dwOrigin, plibNewPosition); + } + + private static void SetSizeImplementation(IntPtr thisPtr, ulong libNewSize) + { + Interop.Ole32.IStreamComWrapper inst = ComInterfaceDispatch.GetInstance((ComInterfaceDispatch*)thisPtr); + inst.SetSize(libNewSize); + } + + private static void CopyToImplementation(IntPtr thisPtr, IntPtr pstm, ulong cb, ulong* pcbRead, ulong* pcbWritten) + { + Interop.Ole32.IStreamComWrapper inst = ComInterfaceDispatch.GetInstance((ComInterfaceDispatch*)thisPtr); + Interop.Ole32.IStreamComWrapper pstmStream = ComInterfaceDispatch.GetInstance((ComInterfaceDispatch*)pstm); + + inst.CopyTo(pstmStream, cb, pcbRead, pcbWritten); + } + + private static void CommitImplementation(IntPtr thisPtr, uint grfCommitFlags) + { + Interop.Ole32.IStreamComWrapper inst = ComInterfaceDispatch.GetInstance((ComInterfaceDispatch*)thisPtr); + inst.Commit(grfCommitFlags); + } + + private static void RevertImplementation(IntPtr thisPtr) + { + Interop.Ole32.IStreamComWrapper inst = ComInterfaceDispatch.GetInstance((ComInterfaceDispatch*)thisPtr); + inst.Revert(); + } + + private static Interop.HRESULT LockRegionImplementation(IntPtr thisPtr, ulong libOffset, ulong cb, uint dwLockType) + { + Interop.Ole32.IStreamComWrapper inst = ComInterfaceDispatch.GetInstance((ComInterfaceDispatch*)thisPtr); + return inst.LockRegion(libOffset, cb, dwLockType); + } + + private static Interop.HRESULT UnlockRegionImplementation(IntPtr thisPtr, ulong libOffset, ulong cb, uint dwLockType) + { + Interop.Ole32.IStreamComWrapper inst = ComInterfaceDispatch.GetInstance((ComInterfaceDispatch*)thisPtr); + return inst.UnlockRegion(libOffset, cb, dwLockType); + } + + private static void StatImplementation(IntPtr thisPtr, out Interop.Ole32.STATSTG pstatstg, Interop.Ole32.STATFLAG grfStatFlag) + { + Interop.Ole32.IStreamComWrapper inst = ComInterfaceDispatch.GetInstance((ComInterfaceDispatch*)thisPtr); + inst.Stat(out pstatstg, grfStatFlag); + } + + private static IntPtr CloneImplementation(IntPtr thisPtr) + { + Interop.Ole32.IStreamComWrapper inst = ComInterfaceDispatch.GetInstance((ComInterfaceDispatch*)thisPtr); + + return Instance.GetOrCreateComInterfaceForObject(inst.Clone(), CreateComInterfaceFlags.None); + } + } + +#pragma warning disable CS0649 // fields are never assigned to + internal struct IPictureVtbl + { + public IUnknownVtbl IUnknownImpl; + public IntPtr GetHandle; + public IntPtr GetHPal; + public IntPtr GetPictureType; + public IntPtr GetWidth; + public IntPtr GetHeight; + public IntPtr Render; + public IntPtr SetHPal; + public IntPtr GetCurDC; + public IntPtr SelectPicture; + public IntPtr GetKeepOriginalFormat; + public IntPtr SetKeepOriginalFormat; + public IntPtr PictureChanged; + public _SaveAsFile SaveAsFile; + public IntPtr GetAttributes; + public IntPtr SetHdc; + + public delegate int _SaveAsFile(IntPtr thisPtr, IntPtr pstm, int fSaveMemCopy, out int pcbSize); + } + + internal struct VtblPtr + { + public IntPtr Vtbl; + } +#pragma warning restore CS0649 + + internal interface IPicture + { + static readonly Guid Guid = new Guid(0x7BF80980, 0xBF32, 0x101A, 0x8B, 0xBB, 0, 0xAA, 0x00, 0x30, 0x0C, 0xAB); + + // NOTE: Only SaveAsFile is invoked. The other methods on IPicture are not necessary + + int SaveAsFile(IntPtr pstm, int fSaveMemCopy, out int pcbSize); + } + + private class PictureWrapper : IPicture + { + private readonly IntPtr _wrappedInstance; + private readonly IPictureVtbl _vtable; + + public PictureWrapper(IntPtr wrappedInstance) + { + _wrappedInstance = wrappedInstance; + + VtblPtr inst = Marshal.PtrToStructure(_wrappedInstance); + _vtable = Marshal.PtrToStructure(inst.Vtbl); + } + + public int SaveAsFile(IntPtr pstm, int fSaveMemCopy, out int pcbSize) + { + // Get the IStream implementation, since the ComWrappers runtime returns a pointer to the IUnknown interface implementation + Guid streamGuid = Interop.Ole32.IStreamComWrapper.Guid; + CheckStatus(Marshal.QueryInterface(pstm, ref streamGuid, out IntPtr pstmImpl)); + + return _vtable.SaveAsFile(_wrappedInstance, pstmImpl, fSaveMemCopy, out pcbSize); + } + } + } +} diff --git a/src/libraries/System.Drawing.Common/src/System/Drawing/Icon.Windows.COMWrappers.cs b/src/libraries/System.Drawing.Common/src/System/Drawing/Icon.Windows.COMWrappers.cs new file mode 100644 index 00000000000000..706e9b71feb660 --- /dev/null +++ b/src/libraries/System.Drawing.Common/src/System/Drawing/Icon.Windows.COMWrappers.cs @@ -0,0 +1,80 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Drawing.Internal; +using System.IO; +using System.Runtime.InteropServices; +using System.Runtime.Serialization; + +namespace System.Drawing +{ + public sealed partial class Icon : MarshalByRefObject, ICloneable, IDisposable, ISerializable + { + public unsafe void Save(Stream outputStream) + { + if (_iconData != null) + { + outputStream.Write(_iconData, 0, _iconData.Length); + } + else + { + if (outputStream == null) + throw new ArgumentNullException(nameof(outputStream)); + + // Ideally, we would pick apart the icon using + // GetIconInfo, and then pull the individual bitmaps out, + // converting them to DIBS and saving them into the file. + // But, in the interest of simplicity, we just call to + // OLE to do it for us. + PICTDESC pictdesc = PICTDESC.CreateIconPICTDESC(Handle); + Guid g = DrawingComWrappers.IPicture.Guid; + IntPtr lpPicture; + DrawingComWrappers.CheckStatus(OleCreatePictureIndirect(&pictdesc, &g, false, &lpPicture)); + + IntPtr streamPtr = IntPtr.Zero; + try + { + DrawingComWrappers.IPicture picture = (DrawingComWrappers.IPicture)DrawingComWrappers.Instance.GetOrCreateObjectForComInstance(lpPicture, CreateObjectFlags.None); + + var gpStream = new GPStream(outputStream, makeSeekable: false); + streamPtr = DrawingComWrappers.Instance.GetOrCreateComInterfaceForObject(gpStream, CreateComInterfaceFlags.None); + + DrawingComWrappers.CheckStatus(picture.SaveAsFile(streamPtr, -1, out _)); + } + finally + { + if (streamPtr != IntPtr.Zero) + { + Marshal.Release(streamPtr); + } + + if (lpPicture != IntPtr.Zero) + { + Marshal.Release(lpPicture); + } + } + } + } + + [DllImport(Interop.Libraries.Oleaut32)] + private static unsafe extern int OleCreatePictureIndirect(PICTDESC* pictdesc, Guid* refiid, bool fOwn, IntPtr* lplpvObj); + + [StructLayout(LayoutKind.Sequential)] + private readonly struct PICTDESC + { + public readonly int SizeOfStruct; + public readonly int PicType; + public readonly IntPtr Icon; + + private PICTDESC(int sizeOfStruct, int picType, IntPtr hicon) + { + SizeOfStruct = sizeOfStruct; + PicType = picType; + Icon = hicon; + } + + public static PICTDESC CreateIconPICTDESC(IntPtr hicon) => + new PICTDESC(8 + IntPtr.Size, Ole.PICTYPE_ICON, hicon); + } + } +} diff --git a/src/libraries/System.Drawing.Common/src/System/Drawing/Icon.Windows.NoCOMWrappers.cs b/src/libraries/System.Drawing.Common/src/System/Drawing/Icon.Windows.NoCOMWrappers.cs new file mode 100644 index 00000000000000..34a5d19a0d9c83 --- /dev/null +++ b/src/libraries/System.Drawing.Common/src/System/Drawing/Icon.Windows.NoCOMWrappers.cs @@ -0,0 +1,115 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Diagnostics; +using System.Drawing.Internal; +using System.IO; +using System.Runtime.InteropServices; +using System.Runtime.Serialization; + +namespace System.Drawing +{ + public sealed partial class Icon : MarshalByRefObject, ICloneable, IDisposable, ISerializable + { + public void Save(Stream outputStream) + { + if (_iconData != null) + { + outputStream.Write(_iconData, 0, _iconData.Length); + } + else + { + // Ideally, we would pick apart the icon using + // GetIconInfo, and then pull the individual bitmaps out, + // converting them to DIBS and saving them into the file. + // But, in the interest of simplicity, we just call to + // OLE to do it for us. + PICTDESC pictdesc = PICTDESC.CreateIconPICTDESC(Handle); + Guid g = typeof(IPicture).GUID; + IPicture picture = OleCreatePictureIndirect(pictdesc, ref g, false); + + if (picture != null) + { + try + { + if (outputStream == null) + throw new ArgumentNullException(nameof(outputStream)); + + picture.SaveAsFile(new GPStream(outputStream, makeSeekable: false), -1, out int temp); + } + finally + { + Debug.Assert(RuntimeInformation.IsOSPlatform(OSPlatform.Windows)); + Marshal.ReleaseComObject(picture); + } + } + } + } + + [DllImport(Interop.Libraries.Oleaut32, PreserveSig = false)] + internal static extern IPicture OleCreatePictureIndirect(PICTDESC pictdesc, [In]ref Guid refiid, bool fOwn); + + [ComImport] + [Guid("7BF80980-BF32-101A-8BBB-00AA00300CAB")] + [InterfaceType(ComInterfaceType.InterfaceIsIUnknown)] + internal interface IPicture + { + IntPtr GetHandle(); + + IntPtr GetHPal(); + + [return: MarshalAs(UnmanagedType.I2)] + short GetPictureType(); + + int GetWidth(); + + int GetHeight(); + + void Render(); + + void SetHPal([In] IntPtr phpal); + + IntPtr GetCurDC(); + + void SelectPicture([In] IntPtr hdcIn, + [Out, MarshalAs(UnmanagedType.LPArray)] int[] phdcOut, + [Out, MarshalAs(UnmanagedType.LPArray)] int[] phbmpOut); + + [return: MarshalAs(UnmanagedType.Bool)] + bool GetKeepOriginalFormat(); + + void SetKeepOriginalFormat([In, MarshalAs(UnmanagedType.Bool)] bool pfkeep); + + void PictureChanged(); + + [PreserveSig] + int SaveAsFile([In, MarshalAs(UnmanagedType.Interface)] Interop.Ole32.IStream pstm, + [In] int fSaveMemCopy, + [Out] out int pcbSize); + + int GetAttributes(); + + void SetHdc([In] IntPtr hdc); + } + + [StructLayout(LayoutKind.Sequential)] + internal sealed class PICTDESC + { + internal int cbSizeOfStruct; + public int picType; + internal IntPtr union1; + internal int union2; + internal int union3; + + public static PICTDESC CreateIconPICTDESC(IntPtr hicon) + { + return new PICTDESC() + { + cbSizeOfStruct = 12, + picType = Ole.PICTYPE_ICON, + union1 = hicon + }; + } + } + } +} diff --git a/src/libraries/System.Drawing.Common/src/System/Drawing/Icon.Windows.cs b/src/libraries/System.Drawing.Common/src/System/Drawing/Icon.Windows.cs index 6bd26a336c1bff..4acd6376e4c437 100644 --- a/src/libraries/System.Drawing.Common/src/System/Drawing/Icon.Windows.cs +++ b/src/libraries/System.Drawing.Common/src/System/Drawing/Icon.Windows.cs @@ -637,41 +637,6 @@ private unsafe void Initialize(int width, int height) } } - public void Save(Stream outputStream) - { - if (_iconData != null) - { - outputStream.Write(_iconData, 0, _iconData.Length); - } - else - { - // Ideally, we would pick apart the icon using - // GetIconInfo, and then pull the individual bitmaps out, - // converting them to DIBS and saving them into the file. - // But, in the interest of simplicity, we just call to - // OLE to do it for us. - PICTDESC pictdesc = PICTDESC.CreateIconPICTDESC(Handle); - Guid g = typeof(IPicture).GUID; - IPicture picture = OleCreatePictureIndirect(pictdesc, ref g, false); - - if (picture != null) - { - try - { - if (outputStream == null) - throw new ArgumentNullException(nameof(outputStream)); - - picture.SaveAsFile(new GPStream(outputStream, makeSeekable: false), -1, out int temp); - } - finally - { - Debug.Assert(RuntimeInformation.IsOSPlatform(OSPlatform.Windows)); - Marshal.ReleaseComObject(picture); - } - } - } - } - private unsafe void CopyBitmapData(BitmapData sourceData, BitmapData targetData) { byte* srcPtr = (byte*)sourceData.Scan0; @@ -905,75 +870,9 @@ private bool HasPngSignature() public override string ToString() => SR.toStringIcon; - [DllImport(Interop.Libraries.Oleaut32, PreserveSig = false)] - internal static extern IPicture OleCreatePictureIndirect(PICTDESC pictdesc, [In]ref Guid refiid, bool fOwn); - - [ComImport] - [Guid("7BF80980-BF32-101A-8BBB-00AA00300CAB")] - [InterfaceType(ComInterfaceType.InterfaceIsIUnknown)] - internal interface IPicture - { - IntPtr GetHandle(); - - IntPtr GetHPal(); - - [return: MarshalAs(UnmanagedType.I2)] - short GetPictureType(); - - int GetWidth(); - - int GetHeight(); - - void Render(); - - void SetHPal([In] IntPtr phpal); - - IntPtr GetCurDC(); - - void SelectPicture([In] IntPtr hdcIn, - [Out, MarshalAs(UnmanagedType.LPArray)] int[] phdcOut, - [Out, MarshalAs(UnmanagedType.LPArray)] int[] phbmpOut); - - [return: MarshalAs(UnmanagedType.Bool)] - bool GetKeepOriginalFormat(); - - void SetKeepOriginalFormat([In, MarshalAs(UnmanagedType.Bool)] bool pfkeep); - - void PictureChanged(); - - [PreserveSig] - int SaveAsFile([In, MarshalAs(UnmanagedType.Interface)] Interop.Ole32.IStream pstm, - [In] int fSaveMemCopy, - [Out] out int pcbSize); - - int GetAttributes(); - - void SetHdc([In] IntPtr hdc); - } - internal static class Ole { public const int PICTYPE_ICON = 3; } - - [StructLayout(LayoutKind.Sequential)] - internal sealed class PICTDESC - { - internal int cbSizeOfStruct; - public int picType; - internal IntPtr union1; - internal int union2; - internal int union3; - - public static PICTDESC CreateIconPICTDESC(IntPtr hicon) - { - return new PICTDESC() - { - cbSizeOfStruct = 12, - picType = Ole.PICTYPE_ICON, - union1 = hicon - }; - } - } } } diff --git a/src/libraries/System.Drawing.Common/src/System/Drawing/Internal/GPStream.cs b/src/libraries/System.Drawing.Common/src/System/Drawing/Internal/GPStream.cs index 6040cd85c982c6..4b505101a44dea 100644 --- a/src/libraries/System.Drawing.Common/src/System/Drawing/Internal/GPStream.cs +++ b/src/libraries/System.Drawing.Common/src/System/Drawing/Internal/GPStream.cs @@ -3,11 +3,10 @@ using System.Buffers; using System.IO; -using System.Runtime.InteropServices; namespace System.Drawing.Internal { - internal sealed class GPStream : Interop.Ole32.IStream + internal sealed class GPStream : Interop.Ole32.IStream, Interop.Ole32.IStreamComWrapper { private readonly Stream _dataStream; @@ -51,6 +50,15 @@ public Interop.Ole32.IStream Clone() }; } + Interop.Ole32.IStreamComWrapper Interop.Ole32.IStreamComWrapper.Clone() + { + // The cloned object should have the same current "position" + return new GPStream(_dataStream) + { + _virtualPosition = _virtualPosition + }; + } + public void Commit(uint grfCommitFlags) { _dataStream.Flush(); @@ -96,6 +104,43 @@ public unsafe void CopyTo(Interop.Ole32.IStream pstm, ulong cb, ulong* pcbRead, *pcbWritten = totalWritten; } + unsafe void Interop.Ole32.IStreamComWrapper.CopyTo(Interop.Ole32.IStreamComWrapper pstm, ulong cb, ulong* pcbRead, ulong* pcbWritten) + { + byte[] buffer = ArrayPool.Shared.Rent(4096); + + ulong remaining = cb; + ulong totalWritten = 0; + ulong totalRead = 0; + + fixed (byte* b = buffer) + { + while (remaining > 0) + { + uint read = remaining < (ulong)buffer.Length ? (uint)remaining : (uint)buffer.Length; + Read(b, read, &read); + remaining -= read; + totalRead += read; + + if (read == 0) + { + break; + } + + uint written; + pstm.Write(b, read, &written); + totalWritten += written; + } + } + + ArrayPool.Shared.Return(buffer); + + if (pcbRead != null) + *pcbRead = totalRead; + + if (pcbWritten != null) + *pcbWritten = totalWritten; + } + public unsafe void Read(byte* pv, uint cb, uint* pcbRead) { ActualizeVirtualPosition(); diff --git a/src/libraries/System.Drawing.Common/tests/IconTests.cs b/src/libraries/System.Drawing.Common/tests/IconTests.cs index 2ad23d5f18d5fd..108e277aee25bd 100644 --- a/src/libraries/System.Drawing.Common/tests/IconTests.cs +++ b/src/libraries/System.Drawing.Common/tests/IconTests.cs @@ -528,7 +528,7 @@ public void Save_ClosedOutputStreamIconData_ThrowsException() [ActiveIssue("https://github.com/dotnet/runtime/issues/22221", TestPlatforms.AnyUnix)] [ActiveIssue("https://github.com/dotnet/runtime/issues/47759", TestPlatforms.Windows, TargetFrameworkMonikers.Netcoreapp, TestRuntimes.Mono)] [ConditionalFact(Helpers.IsDrawingSupported)] - public void Save_ClosedOutputStreamNoIconData_DoesNothing() + public void Save_ClosedOutputStreamNoIconData() { using (var source = new Icon(Helpers.GetTestBitmapPath("48x48_multiple_entries_4bit.ico"))) using (var icon = Icon.FromHandle(source.Handle)) @@ -536,7 +536,16 @@ public void Save_ClosedOutputStreamNoIconData_DoesNothing() var stream = new MemoryStream(); stream.Close(); - icon.Save(stream); + if (PlatformDetection.IsNetFramework) + { + // The ObjectDisposedException is ignored in previous .NET versions, + // so the following does nothing. + icon.Save(stream); + } + else + { + Assert.Throws(() => icon.Save(stream)); + } } } From b809c88ba4352ed4c50b1513758c64a31f7bab97 Mon Sep 17 00:00:00 2001 From: Eric Erhardt Date: Wed, 23 Jun 2021 13:00:05 -0500 Subject: [PATCH 02/11] Add Trimming Test for Icon.Save --- .../src/ILLink/ILLink.Suppressions.xml | 6 ---- .../tests/TrimmingTests/IconSave.cs | 28 +++++++++++++++++++ .../System.Drawing.Common.TrimmingTests.proj | 13 +++++++++ 3 files changed, 41 insertions(+), 6 deletions(-) create mode 100644 src/libraries/System.Drawing.Common/tests/TrimmingTests/IconSave.cs create mode 100644 src/libraries/System.Drawing.Common/tests/TrimmingTests/System.Drawing.Common.TrimmingTests.proj diff --git a/src/libraries/System.Drawing.Common/src/ILLink/ILLink.Suppressions.xml b/src/libraries/System.Drawing.Common/src/ILLink/ILLink.Suppressions.xml index d5734aab1a2a96..729abfdeeba54f 100644 --- a/src/libraries/System.Drawing.Common/src/ILLink/ILLink.Suppressions.xml +++ b/src/libraries/System.Drawing.Common/src/ILLink/ILLink.Suppressions.xml @@ -1,12 +1,6 @@  - - ILLink - IL2050 - member - M:System.Drawing.Icon.OleCreatePictureIndirect(System.Drawing.Icon.PICTDESC,System.Guid@,System.Boolean) - ILLink IL2050 diff --git a/src/libraries/System.Drawing.Common/tests/TrimmingTests/IconSave.cs b/src/libraries/System.Drawing.Common/tests/TrimmingTests/IconSave.cs new file mode 100644 index 00000000000000..0e15a645c5135e --- /dev/null +++ b/src/libraries/System.Drawing.Common/tests/TrimmingTests/IconSave.cs @@ -0,0 +1,28 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Drawing; +using System.IO; + +/// +/// Tests that Icon.Save works when the Icon is loaded from an IntPtr. +/// This causes COM to be used to save the Icon. +/// +class Program +{ + static int Main(string[] args) + { + Icon i = SystemIcons.WinLogo; + using MemoryStream stream = new(); + + i.Save(stream); + + // ensure something was written to the stream + if (stream.Position == 0) + { + return -1; + } + + return 100; + } +} diff --git a/src/libraries/System.Drawing.Common/tests/TrimmingTests/System.Drawing.Common.TrimmingTests.proj b/src/libraries/System.Drawing.Common/tests/TrimmingTests/System.Drawing.Common.TrimmingTests.proj new file mode 100644 index 00000000000000..17a726b45d63d7 --- /dev/null +++ b/src/libraries/System.Drawing.Common/tests/TrimmingTests/System.Drawing.Common.TrimmingTests.proj @@ -0,0 +1,13 @@ + + + + + System.Drawing.Common + + + + + + + + From dcf3cfd187d51ff87037a9bde3401f186f01b9ca Mon Sep 17 00:00:00 2001 From: Eric Erhardt Date: Wed, 16 Jun 2021 22:14:42 -0500 Subject: [PATCH 03/11] Add support for OS specific trimming tests --- eng/testing/linker/project.csproj.template | 2 +- eng/testing/linker/trimmingTests.targets | 14 ++++++++------ .../System.Drawing.Common.TrimmingTests.proj | 4 +++- 3 files changed, 12 insertions(+), 8 deletions(-) diff --git a/eng/testing/linker/project.csproj.template b/eng/testing/linker/project.csproj.template index 7f8ccaba794415..38c6d68f901ab4 100644 --- a/eng/testing/linker/project.csproj.template +++ b/eng/testing/linker/project.csproj.template @@ -8,7 +8,7 @@ {WasmAppBuilderTasksAssemblyPath} {MicrosoftNetCoreAppRuntimePackRidDir} {RepositoryEngineeringDir} - {NetCoreAppCurrent} + {TestTargetFramework} {RuntimeIdentifier} {UseMonoRuntime} {TargetingPackDir} diff --git a/eng/testing/linker/trimmingTests.targets b/eng/testing/linker/trimmingTests.targets index f43dbee3b4e1f2..6f084b6ddb41ef 100644 --- a/eng/testing/linker/trimmingTests.targets +++ b/eng/testing/linker/trimmingTests.targets @@ -25,14 +25,16 @@ $([MSBuild]::NormalizeDirectory('$(TrimmingTestProjectsDir)', '$(MSBuildProjectName)', '%(Filename)', '$(PackageRID)')) $(PackageRID) + $(NetCoreAppCurrent) + $(NetCoreAppCurrent)-%(TestConsoleAppSourceFiles.TargetOS) - + %(ProjectDir)project.csproj - $([MSBuild]::NormalizePath('%(ProjectDir)', 'bin', '$(Configuration)', '$(NetCoreAppCurrent)', '%(TestRuntimeIdentifier)', 'publish', 'project')) - $([MSBuild]::NormalizePath('%(ProjectDir)', 'bin', '$(Configuration)', '$(NetCoreAppCurrent)', '%(TestRuntimeIdentifier)', 'AppBundle', 'run-v8.sh')) - $([MSBuild]::NormalizeDirectory('%(ProjectDir)', 'bin', '$(Configuration)', '$(NetCoreAppCurrent)', '%(TestRuntimeIdentifier)', 'publish')) - $([MSBuild]::NormalizeDirectory('%(ProjectDir)', 'bin', '$(Configuration)', '$(NetCoreAppCurrent)', '%(TestRuntimeIdentifier)', 'AppBundle')) + $([MSBuild]::NormalizePath('%(ProjectDir)', 'bin', '$(Configuration)', '%(TestTargetFramework)', '%(TestRuntimeIdentifier)', 'publish', 'project')) + $([MSBuild]::NormalizePath('%(ProjectDir)', 'bin', '$(Configuration)', '%(TestTargetFramework)', '%(TestRuntimeIdentifier)', 'AppBundle', 'run-v8.sh')) + $([MSBuild]::NormalizeDirectory('%(ProjectDir)', 'bin', '$(Configuration)', '%(TestTargetFramework)', '%(TestRuntimeIdentifier)', 'publish')) + $([MSBuild]::NormalizeDirectory('%(ProjectDir)', 'bin', '$(Configuration)', '%(TestTargetFramework)', '%(TestRuntimeIdentifier)', 'AppBundle')) @@ -70,7 +72,7 @@ + From c3d76ba59fe15aaa59c56d4dac2280cb712aa215 Mon Sep 17 00:00:00 2001 From: Eric Erhardt Date: Wed, 23 Jun 2021 18:51:18 -0500 Subject: [PATCH 04/11] Respond to PR feedback * Use function pointers instead of delegates * Rename Guid to IID * Better interop to closely match the native side * Release any COM pointer that was QueryInterface * Use pointers instead of Marshal.PtrToStructure/StructureToPtr --- .../src/Interop/Windows/Interop.HRESULT.cs | 1 + .../Interop/Windows/Ole32/Interop.IStream.cs | 2 +- .../src/System/Drawing/DrawingComWrappers.cs | 182 +++++++++--------- .../Drawing/Icon.Windows.COMWrappers.cs | 15 +- 4 files changed, 104 insertions(+), 96 deletions(-) diff --git a/src/libraries/Common/src/Interop/Windows/Interop.HRESULT.cs b/src/libraries/Common/src/Interop/Windows/Interop.HRESULT.cs index be0cff44adb5e0..b32676c03a77a0 100644 --- a/src/libraries/Common/src/Interop/Windows/Interop.HRESULT.cs +++ b/src/libraries/Common/src/Interop/Windows/Interop.HRESULT.cs @@ -13,6 +13,7 @@ internal enum HRESULT : int E_FAIL = unchecked((int)0x80004005), E_UNEXPECTED = unchecked((int)0x8000FFFF), STG_E_INVALIDFUNCTION = unchecked((int)0x80030001L), + STG_E_INVALIDPOINTER = unchecked((int)0x80030009), STG_E_INVALIDPARAMETER = unchecked((int)0x80030057), STG_E_INVALIDFLAG = unchecked((int)0x800300FF), E_ACCESSDENIED = unchecked((int)0x80070005), diff --git a/src/libraries/Common/src/Interop/Windows/Ole32/Interop.IStream.cs b/src/libraries/Common/src/Interop/Windows/Ole32/Interop.IStream.cs index 0e8c269be1dad9..0047ae4ec225f2 100644 --- a/src/libraries/Common/src/Interop/Windows/Ole32/Interop.IStream.cs +++ b/src/libraries/Common/src/Interop/Windows/Ole32/Interop.IStream.cs @@ -69,7 +69,7 @@ void Stat( // are converted to use ComWrappers. internal interface IStreamComWrapper { - static readonly Guid Guid = new Guid(0x0000000C, 0x0000, 0x0000, 0xC0, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x46); + static readonly Guid IID = new Guid(0x0000000C, 0x0000, 0x0000, 0xC0, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x46); // pcbRead is optional so it must be a pointer unsafe void Read(byte* pv, uint cb, uint* pcbRead); diff --git a/src/libraries/System.Drawing.Common/src/System/Drawing/DrawingComWrappers.cs b/src/libraries/System.Drawing.Common/src/System/Drawing/DrawingComWrappers.cs index 6e1f325d5b7a4d..d44beb9b227403 100644 --- a/src/libraries/System.Drawing.Common/src/System/Drawing/DrawingComWrappers.cs +++ b/src/libraries/System.Drawing.Common/src/System/Drawing/DrawingComWrappers.cs @@ -17,12 +17,10 @@ namespace System.Drawing internal unsafe class DrawingComWrappers : ComWrappers { private static readonly ComInterfaceEntry* s_wrapperEntry = InitializeComInterfaceEntry(); - private static readonly Lazy s_instance = new Lazy(() => new DrawingComWrappers()); + internal static DrawingComWrappers Instance { get; } = new DrawingComWrappers(); private DrawingComWrappers() { } - internal static DrawingComWrappers Instance => s_instance.Value; - internal static void CheckStatus(int result) { if (result != 0) @@ -35,13 +33,11 @@ internal static void CheckStatus(int result) { GetIUnknownImpl(out IntPtr fpQueryInteface, out IntPtr fpAddRef, out IntPtr fpRelease); - IStreamVtbl iStreamVtbl = IStreamVtbl.Create(fpQueryInteface, fpAddRef, fpRelease); - IntPtr iStreamVtblRaw = RuntimeHelpers.AllocateTypeAssociatedMemory(typeof(IStreamVtbl), sizeof(IStreamVtbl)); - Marshal.StructureToPtr(iStreamVtbl, iStreamVtblRaw, false); + IStreamVtbl.Fill((IStreamVtbl*)iStreamVtblRaw, fpQueryInteface, fpAddRef, fpRelease); ComInterfaceEntry* wrapperEntry = (ComInterfaceEntry*)RuntimeHelpers.AllocateTypeAssociatedMemory(typeof(IStreamVtbl), sizeof(ComInterfaceEntry)); - wrapperEntry->IID = Interop.Ole32.IStreamComWrapper.Guid; + wrapperEntry->IID = Interop.Ole32.IStreamComWrapper.IID; wrapperEntry->Vtable = iStreamVtblRaw; return wrapperEntry; } @@ -60,8 +56,8 @@ protected override object CreateObject(IntPtr externalComObject, CreateObjectFla { Debug.Assert(flags == CreateObjectFlags.None); - Guid pictureGuid = IPicture.Guid; - int hr = Marshal.QueryInterface(externalComObject, ref pictureGuid, out IntPtr comObject); + Guid pictureIID = IPicture.IID; + int hr = Marshal.QueryInterface(externalComObject, ref pictureIID, out IntPtr comObject); if (hr == 0) { return new PictureWrapper(comObject); @@ -82,136 +78,134 @@ internal struct IUnknownVtbl public IntPtr Release; } - internal struct IStreamVtbl + internal unsafe struct IStreamVtbl { public IUnknownVtbl IUnknownImpl; - public IntPtr Read; - public IntPtr Write; - public IntPtr Seek; - public IntPtr SetSize; - public IntPtr CopyTo; - public IntPtr Commit; - public IntPtr Revert; - public IntPtr LockRegion; - public IntPtr UnlockRegion; - public IntPtr Stat; - public IntPtr Clone; - - private delegate void _Read(IntPtr thisPtr, byte* pv, uint cb, uint* pcbRead); - private delegate void _Write(IntPtr thisPtr, byte* pv, uint cb, uint* pcbWritten); - private delegate void _Seek(IntPtr thisPtr, long dlibMove, SeekOrigin dwOrigin, ulong* plibNewPosition); - private delegate void _SetSize(IntPtr thisPtr, ulong libNewSize); - private delegate void _CopyTo(IntPtr thisPtr, IntPtr pstm, ulong cb, ulong* pcbRead, ulong* pcbWritten); - private delegate void _Commit(IntPtr thisPtr, uint grfCommitFlags); - private delegate void _Revert(IntPtr thisPtr); - private delegate Interop.HRESULT _LockRegion(IntPtr thisPtr, ulong libOffset, ulong cb, uint dwLockType); - private delegate Interop.HRESULT _UnlockRegion(IntPtr thisPtr, ulong libOffset, ulong cb, uint dwLockType); - private delegate void _Stat(IntPtr thisPtr, out Interop.Ole32.STATSTG pstatstg, Interop.Ole32.STATFLAG grfStatFlag); - private delegate IntPtr _Clone(IntPtr thisPtr); - - private static _Read s_Read = new _Read(ReadImplementation); - private static _Write s_Write = new _Write(WriteImplementation); - private static _Seek s_Seek = new _Seek(SeekImplementation); - private static _SetSize s_SetSize = new _SetSize(SetSizeImplementation); - private static _CopyTo s_CopyTo = new _CopyTo(CopyToImplementation); - private static _Commit s_Commit = new _Commit(CommitImplementation); - private static _Revert s_Revert = new _Revert(RevertImplementation); - private static _LockRegion s_LockRegion = new _LockRegion(LockRegionImplementation); - private static _UnlockRegion s_UnlockRegion = new _UnlockRegion(UnlockRegionImplementation); - private static _Stat s_Stat = new _Stat(StatImplementation); - private static _Clone s_Clone = new _Clone(CloneImplementation); - - public static IStreamVtbl Create(IntPtr fpQueryInteface, IntPtr fpAddRef, IntPtr fpRelease) + public delegate* unmanaged Read; + public delegate* unmanaged Write; + public delegate* unmanaged Seek; + public delegate* unmanaged SetSize; + public delegate* unmanaged CopyTo; + public delegate* unmanaged Commit; + public delegate* unmanaged Revert; + public delegate* unmanaged LockRegion; + public delegate* unmanaged UnlockRegion; + public delegate* unmanaged Stat; + public delegate* unmanaged Clone; + + public static void Fill(IStreamVtbl* vtable, IntPtr fpQueryInteface, IntPtr fpAddRef, IntPtr fpRelease) { - return new IStreamVtbl() + vtable->IUnknownImpl = new IUnknownVtbl() { - IUnknownImpl = new IUnknownVtbl() - { - QueryInterface = fpQueryInteface, - AddRef = fpAddRef, - Release = fpRelease - }, - Read = Marshal.GetFunctionPointerForDelegate(s_Read), - Write = Marshal.GetFunctionPointerForDelegate(s_Write), - Seek = Marshal.GetFunctionPointerForDelegate(s_Seek), - SetSize = Marshal.GetFunctionPointerForDelegate(s_SetSize), - CopyTo = Marshal.GetFunctionPointerForDelegate(s_CopyTo), - Commit = Marshal.GetFunctionPointerForDelegate(s_Commit), - Revert = Marshal.GetFunctionPointerForDelegate(s_Revert), - LockRegion = Marshal.GetFunctionPointerForDelegate(s_LockRegion), - UnlockRegion = Marshal.GetFunctionPointerForDelegate(s_UnlockRegion), - Stat = Marshal.GetFunctionPointerForDelegate(s_Stat), - Clone = Marshal.GetFunctionPointerForDelegate(s_Clone), + QueryInterface = fpQueryInteface, + AddRef = fpAddRef, + Release = fpRelease }; + vtable->Read = &ReadImplementation; + vtable->Write = &WriteImplementation; + vtable->Seek = &SeekImplementation; + vtable->SetSize = &SetSizeImplementation; + vtable->CopyTo = &CopyToImplementation; + vtable->Commit = &CommitImplementation; + vtable->Revert = &RevertImplementation; + vtable->LockRegion = &LockRegionImplementation; + vtable->UnlockRegion = &UnlockRegionImplementation; + vtable->Stat = &StatImplementation; + vtable->Clone = &CloneImplementation; } - private static void ReadImplementation(IntPtr thisPtr, byte* pv, uint cb, uint* pcbRead) + [UnmanagedCallersOnly] + private static Interop.HRESULT ReadImplementation(IntPtr thisPtr, byte* pv, uint cb, uint* pcbRead) { Interop.Ole32.IStreamComWrapper inst = ComInterfaceDispatch.GetInstance((ComInterfaceDispatch*)thisPtr); inst.Read(pv, cb, pcbRead); + return Interop.HRESULT.S_OK; } - private static void WriteImplementation(IntPtr thisPtr, byte* pv, uint cb, uint* pcbWritten) + [UnmanagedCallersOnly] + private static Interop.HRESULT WriteImplementation(IntPtr thisPtr, byte* pv, uint cb, uint* pcbWritten) { Interop.Ole32.IStreamComWrapper inst = ComInterfaceDispatch.GetInstance((ComInterfaceDispatch*)thisPtr); inst.Write(pv, cb, pcbWritten); + return Interop.HRESULT.S_OK; } - private static void SeekImplementation(IntPtr thisPtr, long dlibMove, SeekOrigin dwOrigin, ulong* plibNewPosition) + [UnmanagedCallersOnly] + private static Interop.HRESULT SeekImplementation(IntPtr thisPtr, long dlibMove, SeekOrigin dwOrigin, ulong* plibNewPosition) { Interop.Ole32.IStreamComWrapper inst = ComInterfaceDispatch.GetInstance((ComInterfaceDispatch*)thisPtr); inst.Seek(dlibMove, dwOrigin, plibNewPosition); + return Interop.HRESULT.S_OK; } - private static void SetSizeImplementation(IntPtr thisPtr, ulong libNewSize) + [UnmanagedCallersOnly] + private static Interop.HRESULT SetSizeImplementation(IntPtr thisPtr, ulong libNewSize) { Interop.Ole32.IStreamComWrapper inst = ComInterfaceDispatch.GetInstance((ComInterfaceDispatch*)thisPtr); inst.SetSize(libNewSize); + return Interop.HRESULT.S_OK; } - private static void CopyToImplementation(IntPtr thisPtr, IntPtr pstm, ulong cb, ulong* pcbRead, ulong* pcbWritten) + [UnmanagedCallersOnly] + private static Interop.HRESULT CopyToImplementation(IntPtr thisPtr, IntPtr pstm, ulong cb, ulong* pcbRead, ulong* pcbWritten) { Interop.Ole32.IStreamComWrapper inst = ComInterfaceDispatch.GetInstance((ComInterfaceDispatch*)thisPtr); Interop.Ole32.IStreamComWrapper pstmStream = ComInterfaceDispatch.GetInstance((ComInterfaceDispatch*)pstm); inst.CopyTo(pstmStream, cb, pcbRead, pcbWritten); + return Interop.HRESULT.S_OK; } - private static void CommitImplementation(IntPtr thisPtr, uint grfCommitFlags) + [UnmanagedCallersOnly] + private static Interop.HRESULT CommitImplementation(IntPtr thisPtr, uint grfCommitFlags) { Interop.Ole32.IStreamComWrapper inst = ComInterfaceDispatch.GetInstance((ComInterfaceDispatch*)thisPtr); inst.Commit(grfCommitFlags); + return Interop.HRESULT.S_OK; } - private static void RevertImplementation(IntPtr thisPtr) + [UnmanagedCallersOnly] + private static Interop.HRESULT RevertImplementation(IntPtr thisPtr) { Interop.Ole32.IStreamComWrapper inst = ComInterfaceDispatch.GetInstance((ComInterfaceDispatch*)thisPtr); inst.Revert(); + return Interop.HRESULT.S_OK; } + [UnmanagedCallersOnly] private static Interop.HRESULT LockRegionImplementation(IntPtr thisPtr, ulong libOffset, ulong cb, uint dwLockType) { Interop.Ole32.IStreamComWrapper inst = ComInterfaceDispatch.GetInstance((ComInterfaceDispatch*)thisPtr); return inst.LockRegion(libOffset, cb, dwLockType); } + [UnmanagedCallersOnly] private static Interop.HRESULT UnlockRegionImplementation(IntPtr thisPtr, ulong libOffset, ulong cb, uint dwLockType) { Interop.Ole32.IStreamComWrapper inst = ComInterfaceDispatch.GetInstance((ComInterfaceDispatch*)thisPtr); return inst.UnlockRegion(libOffset, cb, dwLockType); } - private static void StatImplementation(IntPtr thisPtr, out Interop.Ole32.STATSTG pstatstg, Interop.Ole32.STATFLAG grfStatFlag) + [UnmanagedCallersOnly] + private static Interop.HRESULT StatImplementation(IntPtr thisPtr, out Interop.Ole32.STATSTG pstatstg, Interop.Ole32.STATFLAG grfStatFlag) { Interop.Ole32.IStreamComWrapper inst = ComInterfaceDispatch.GetInstance((ComInterfaceDispatch*)thisPtr); inst.Stat(out pstatstg, grfStatFlag); + return Interop.HRESULT.S_OK; } - private static IntPtr CloneImplementation(IntPtr thisPtr) + [UnmanagedCallersOnly] + private static Interop.HRESULT CloneImplementation(IntPtr thisPtr, IntPtr* ppstm) { + if (ppstm == null) + { + return Interop.HRESULT.STG_E_INVALIDPOINTER; + } + Interop.Ole32.IStreamComWrapper inst = ComInterfaceDispatch.GetInstance((ComInterfaceDispatch*)thisPtr); - return Instance.GetOrCreateComInterfaceForObject(inst.Clone(), CreateComInterfaceFlags.None); + *ppstm = Instance.GetOrCreateComInterfaceForObject(inst.Clone(), CreateComInterfaceFlags.None); + return Interop.HRESULT.S_OK; } } @@ -231,11 +225,9 @@ internal struct IPictureVtbl public IntPtr GetKeepOriginalFormat; public IntPtr SetKeepOriginalFormat; public IntPtr PictureChanged; - public _SaveAsFile SaveAsFile; + public delegate* unmanaged SaveAsFile; public IntPtr GetAttributes; public IntPtr SetHdc; - - public delegate int _SaveAsFile(IntPtr thisPtr, IntPtr pstm, int fSaveMemCopy, out int pcbSize); } internal struct VtblPtr @@ -244,35 +236,47 @@ internal struct VtblPtr } #pragma warning restore CS0649 - internal interface IPicture + internal interface IPicture : IDisposable { - static readonly Guid Guid = new Guid(0x7BF80980, 0xBF32, 0x101A, 0x8B, 0xBB, 0, 0xAA, 0x00, 0x30, 0x0C, 0xAB); + static readonly Guid IID = new Guid(0x7BF80980, 0xBF32, 0x101A, 0x8B, 0xBB, 0, 0xAA, 0x00, 0x30, 0x0C, 0xAB); // NOTE: Only SaveAsFile is invoked. The other methods on IPicture are not necessary - int SaveAsFile(IntPtr pstm, int fSaveMemCopy, out int pcbSize); + int SaveAsFile(IntPtr pstm, int fSaveMemCopy, int* pcbSize); } - private class PictureWrapper : IPicture + private unsafe class PictureWrapper : IPicture { private readonly IntPtr _wrappedInstance; - private readonly IPictureVtbl _vtable; + private readonly IPictureVtbl* _vtable; public PictureWrapper(IntPtr wrappedInstance) { _wrappedInstance = wrappedInstance; - VtblPtr inst = Marshal.PtrToStructure(_wrappedInstance); - _vtable = Marshal.PtrToStructure(inst.Vtbl); + VtblPtr* inst = (VtblPtr*)_wrappedInstance; + _vtable = (IPictureVtbl*)inst->Vtbl; } - public int SaveAsFile(IntPtr pstm, int fSaveMemCopy, out int pcbSize) + public void Dispose() + { + Marshal.Release(_wrappedInstance); + } + + public int SaveAsFile(IntPtr pstm, int fSaveMemCopy, int* pcbSize) { // Get the IStream implementation, since the ComWrappers runtime returns a pointer to the IUnknown interface implementation - Guid streamGuid = Interop.Ole32.IStreamComWrapper.Guid; - CheckStatus(Marshal.QueryInterface(pstm, ref streamGuid, out IntPtr pstmImpl)); + Guid streamIID = Interop.Ole32.IStreamComWrapper.IID; + CheckStatus(Marshal.QueryInterface(pstm, ref streamIID, out IntPtr pstmImpl)); - return _vtable.SaveAsFile(_wrappedInstance, pstmImpl, fSaveMemCopy, out pcbSize); + try + { + return _vtable->SaveAsFile(_wrappedInstance, pstmImpl, fSaveMemCopy, pcbSize); + } + finally + { + Marshal.Release(pstmImpl); + } } } } diff --git a/src/libraries/System.Drawing.Common/src/System/Drawing/Icon.Windows.COMWrappers.cs b/src/libraries/System.Drawing.Common/src/System/Drawing/Icon.Windows.COMWrappers.cs index 706e9b71feb660..4935f004cb1d3d 100644 --- a/src/libraries/System.Drawing.Common/src/System/Drawing/Icon.Windows.COMWrappers.cs +++ b/src/libraries/System.Drawing.Common/src/System/Drawing/Icon.Windows.COMWrappers.cs @@ -1,6 +1,7 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +using System.Diagnostics; using System.Drawing.Internal; using System.IO; using System.Runtime.InteropServices; @@ -27,29 +28,31 @@ public unsafe void Save(Stream outputStream) // But, in the interest of simplicity, we just call to // OLE to do it for us. PICTDESC pictdesc = PICTDESC.CreateIconPICTDESC(Handle); - Guid g = DrawingComWrappers.IPicture.Guid; + Guid iid = DrawingComWrappers.IPicture.IID; IntPtr lpPicture; - DrawingComWrappers.CheckStatus(OleCreatePictureIndirect(&pictdesc, &g, false, &lpPicture)); + DrawingComWrappers.CheckStatus(OleCreatePictureIndirect(&pictdesc, &iid, fOwn: 0, &lpPicture)); IntPtr streamPtr = IntPtr.Zero; try { - DrawingComWrappers.IPicture picture = (DrawingComWrappers.IPicture)DrawingComWrappers.Instance.GetOrCreateObjectForComInstance(lpPicture, CreateObjectFlags.None); + using DrawingComWrappers.IPicture picture = (DrawingComWrappers.IPicture)DrawingComWrappers.Instance.GetOrCreateObjectForComInstance(lpPicture, CreateObjectFlags.None); var gpStream = new GPStream(outputStream, makeSeekable: false); streamPtr = DrawingComWrappers.Instance.GetOrCreateComInterfaceForObject(gpStream, CreateComInterfaceFlags.None); - DrawingComWrappers.CheckStatus(picture.SaveAsFile(streamPtr, -1, out _)); + DrawingComWrappers.CheckStatus(picture.SaveAsFile(streamPtr, -1, null)); } finally { if (streamPtr != IntPtr.Zero) { - Marshal.Release(streamPtr); + int count = Marshal.Release(streamPtr); + Debug.Assert(count == 0); } if (lpPicture != IntPtr.Zero) { + // don't assert the count went to 0 here because some other thread could be using the same lpPicture Marshal.Release(lpPicture); } } @@ -57,7 +60,7 @@ public unsafe void Save(Stream outputStream) } [DllImport(Interop.Libraries.Oleaut32)] - private static unsafe extern int OleCreatePictureIndirect(PICTDESC* pictdesc, Guid* refiid, bool fOwn, IntPtr* lplpvObj); + private static unsafe extern int OleCreatePictureIndirect(PICTDESC* pictdesc, Guid* refiid, int fOwn, IntPtr* lplpvObj); [StructLayout(LayoutKind.Sequential)] private readonly struct PICTDESC From e6cd2137a86cea8022e536b3971a0ddcab838d89 Mon Sep 17 00:00:00 2001 From: Eric Erhardt Date: Thu, 24 Jun 2021 11:14:20 -0500 Subject: [PATCH 05/11] PR feedback --- .../Interop/Windows/Ole32/Interop.IStream.cs | 4 -- .../src/System/Drawing/DrawingComWrappers.cs | 41 +++---------------- .../Drawing/Icon.Windows.COMWrappers.cs | 6 +-- 3 files changed, 9 insertions(+), 42 deletions(-) diff --git a/src/libraries/Common/src/Interop/Windows/Ole32/Interop.IStream.cs b/src/libraries/Common/src/Interop/Windows/Ole32/Interop.IStream.cs index 0047ae4ec225f2..52a0dbf9e3d083 100644 --- a/src/libraries/Common/src/Interop/Windows/Ole32/Interop.IStream.cs +++ b/src/libraries/Common/src/Interop/Windows/Ole32/Interop.IStream.cs @@ -93,15 +93,11 @@ unsafe void CopyTo( void Revert(); - // Using PreserveSig to allow explicitly returning the HRESULT for "not supported". - - [PreserveSig] HRESULT LockRegion( ulong libOffset, ulong cb, uint dwLockType); - [PreserveSig] HRESULT UnlockRegion( ulong libOffset, ulong cb, diff --git a/src/libraries/System.Drawing.Common/src/System/Drawing/DrawingComWrappers.cs b/src/libraries/System.Drawing.Common/src/System/Drawing/DrawingComWrappers.cs index d44beb9b227403..e14568bad2022d 100644 --- a/src/libraries/System.Drawing.Common/src/System/Drawing/DrawingComWrappers.cs +++ b/src/libraries/System.Drawing.Common/src/System/Drawing/DrawingComWrappers.cs @@ -16,6 +16,7 @@ namespace System.Drawing /// internal unsafe class DrawingComWrappers : ComWrappers { + private const int OK = 0; private static readonly ComInterfaceEntry* s_wrapperEntry = InitializeComInterfaceEntry(); internal static DrawingComWrappers Instance { get; } = new DrawingComWrappers(); @@ -23,7 +24,7 @@ private DrawingComWrappers() { } internal static void CheckStatus(int result) { - if (result != 0) + if (result != OK) { throw new ExternalException() { HResult = result }; } @@ -209,33 +210,6 @@ private static Interop.HRESULT CloneImplementation(IntPtr thisPtr, IntPtr* ppstm } } -#pragma warning disable CS0649 // fields are never assigned to - internal struct IPictureVtbl - { - public IUnknownVtbl IUnknownImpl; - public IntPtr GetHandle; - public IntPtr GetHPal; - public IntPtr GetPictureType; - public IntPtr GetWidth; - public IntPtr GetHeight; - public IntPtr Render; - public IntPtr SetHPal; - public IntPtr GetCurDC; - public IntPtr SelectPicture; - public IntPtr GetKeepOriginalFormat; - public IntPtr SetKeepOriginalFormat; - public IntPtr PictureChanged; - public delegate* unmanaged SaveAsFile; - public IntPtr GetAttributes; - public IntPtr SetHdc; - } - - internal struct VtblPtr - { - public IntPtr Vtbl; - } -#pragma warning restore CS0649 - internal interface IPicture : IDisposable { static readonly Guid IID = new Guid(0x7BF80980, 0xBF32, 0x101A, 0x8B, 0xBB, 0, 0xAA, 0x00, 0x30, 0x0C, 0xAB); @@ -245,17 +219,13 @@ internal interface IPicture : IDisposable int SaveAsFile(IntPtr pstm, int fSaveMemCopy, int* pcbSize); } - private unsafe class PictureWrapper : IPicture + private class PictureWrapper : IPicture { private readonly IntPtr _wrappedInstance; - private readonly IPictureVtbl* _vtable; public PictureWrapper(IntPtr wrappedInstance) { _wrappedInstance = wrappedInstance; - - VtblPtr* inst = (VtblPtr*)_wrappedInstance; - _vtable = (IPictureVtbl*)inst->Vtbl; } public void Dispose() @@ -263,7 +233,7 @@ public void Dispose() Marshal.Release(_wrappedInstance); } - public int SaveAsFile(IntPtr pstm, int fSaveMemCopy, int* pcbSize) + public unsafe int SaveAsFile(IntPtr pstm, int fSaveMemCopy, int* pcbSize) { // Get the IStream implementation, since the ComWrappers runtime returns a pointer to the IUnknown interface implementation Guid streamIID = Interop.Ole32.IStreamComWrapper.IID; @@ -271,7 +241,8 @@ public int SaveAsFile(IntPtr pstm, int fSaveMemCopy, int* pcbSize) try { - return _vtable->SaveAsFile(_wrappedInstance, pstmImpl, fSaveMemCopy, pcbSize); + return ((delegate* unmanaged)(*(*(void***)_wrappedInstance + 15 /* IPicture.SaveAsFile slot */))) + (_wrappedInstance, pstmImpl, fSaveMemCopy, pcbSize); } finally { diff --git a/src/libraries/System.Drawing.Common/src/System/Drawing/Icon.Windows.COMWrappers.cs b/src/libraries/System.Drawing.Common/src/System/Drawing/Icon.Windows.COMWrappers.cs index 4935f004cb1d3d..47de3032558581 100644 --- a/src/libraries/System.Drawing.Common/src/System/Drawing/Icon.Windows.COMWrappers.cs +++ b/src/libraries/System.Drawing.Common/src/System/Drawing/Icon.Windows.COMWrappers.cs @@ -69,15 +69,15 @@ private readonly struct PICTDESC public readonly int PicType; public readonly IntPtr Icon; - private PICTDESC(int sizeOfStruct, int picType, IntPtr hicon) + private unsafe PICTDESC(int picType, IntPtr hicon) { - SizeOfStruct = sizeOfStruct; + SizeOfStruct = sizeof(PICTDESC); PicType = picType; Icon = hicon; } public static PICTDESC CreateIconPICTDESC(IntPtr hicon) => - new PICTDESC(8 + IntPtr.Size, Ole.PICTYPE_ICON, hicon); + new PICTDESC(Ole.PICTYPE_ICON, hicon); } } } From 84559f68a265037e3680ef4706e71366d867b79e Mon Sep 17 00:00:00 2001 From: Eric Erhardt Date: Thu, 24 Jun 2021 11:39:43 -0500 Subject: [PATCH 06/11] Remove duplicated IStreamComWrapper. --- .../Interop/Windows/Ole32/Interop.IStream.cs | 46 ------------------ .../src/System/Drawing/DrawingComWrappers.cs | 31 ++++++------ .../src/System/Drawing/Internal/GPStream.cs | 48 +------------------ 3 files changed, 17 insertions(+), 108 deletions(-) diff --git a/src/libraries/Common/src/Interop/Windows/Ole32/Interop.IStream.cs b/src/libraries/Common/src/Interop/Windows/Ole32/Interop.IStream.cs index 52a0dbf9e3d083..2eda49a3774eb4 100644 --- a/src/libraries/Common/src/Interop/Windows/Ole32/Interop.IStream.cs +++ b/src/libraries/Common/src/Interop/Windows/Ole32/Interop.IStream.cs @@ -63,51 +63,5 @@ void Stat( IStream Clone(); } - - // This needs to be duplicated from IStream above because ComWrappers doesn't support re-using - // ComImport interfaces. The above interface can be deleted once all usages of the built-in COM support - // are converted to use ComWrappers. - internal interface IStreamComWrapper - { - static readonly Guid IID = new Guid(0x0000000C, 0x0000, 0x0000, 0xC0, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x46); - - // pcbRead is optional so it must be a pointer - unsafe void Read(byte* pv, uint cb, uint* pcbRead); - - // pcbWritten is optional so it must be a pointer - unsafe void Write(byte* pv, uint cb, uint* pcbWritten); - - // SeekOrgin matches the native values, plibNewPosition is optional - unsafe void Seek(long dlibMove, SeekOrigin dwOrigin, ulong* plibNewPosition); - - void SetSize(ulong libNewSize); - - // pcbRead and pcbWritten are optional - unsafe void CopyTo( - IStreamComWrapper pstm, - ulong cb, - ulong* pcbRead, - ulong* pcbWritten); - - void Commit(uint grfCommitFlags); - - void Revert(); - - HRESULT LockRegion( - ulong libOffset, - ulong cb, - uint dwLockType); - - HRESULT UnlockRegion( - ulong libOffset, - ulong cb, - uint dwLockType); - - void Stat( - out STATSTG pstatstg, - STATFLAG grfStatFlag); - - IStreamComWrapper Clone(); - } } } diff --git a/src/libraries/System.Drawing.Common/src/System/Drawing/DrawingComWrappers.cs b/src/libraries/System.Drawing.Common/src/System/Drawing/DrawingComWrappers.cs index e14568bad2022d..76995f4e3c5bae 100644 --- a/src/libraries/System.Drawing.Common/src/System/Drawing/DrawingComWrappers.cs +++ b/src/libraries/System.Drawing.Common/src/System/Drawing/DrawingComWrappers.cs @@ -17,6 +17,7 @@ namespace System.Drawing internal unsafe class DrawingComWrappers : ComWrappers { private const int OK = 0; + private static readonly Guid IStreamIID = new Guid(0x0000000C, 0x0000, 0x0000, 0xC0, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x46); private static readonly ComInterfaceEntry* s_wrapperEntry = InitializeComInterfaceEntry(); internal static DrawingComWrappers Instance { get; } = new DrawingComWrappers(); @@ -38,14 +39,14 @@ internal static void CheckStatus(int result) IStreamVtbl.Fill((IStreamVtbl*)iStreamVtblRaw, fpQueryInteface, fpAddRef, fpRelease); ComInterfaceEntry* wrapperEntry = (ComInterfaceEntry*)RuntimeHelpers.AllocateTypeAssociatedMemory(typeof(IStreamVtbl), sizeof(ComInterfaceEntry)); - wrapperEntry->IID = Interop.Ole32.IStreamComWrapper.IID; + wrapperEntry->IID = IStreamIID; wrapperEntry->Vtable = iStreamVtblRaw; return wrapperEntry; } protected override unsafe ComInterfaceEntry* ComputeVtables(object obj, CreateComInterfaceFlags flags, out int count) { - Debug.Assert(obj is Interop.Ole32.IStreamComWrapper); + Debug.Assert(obj is Interop.Ole32.IStream); Debug.Assert(s_wrapperEntry != null); // Always return the same table mappings. @@ -118,7 +119,7 @@ public static void Fill(IStreamVtbl* vtable, IntPtr fpQueryInteface, IntPtr fpAd [UnmanagedCallersOnly] private static Interop.HRESULT ReadImplementation(IntPtr thisPtr, byte* pv, uint cb, uint* pcbRead) { - Interop.Ole32.IStreamComWrapper inst = ComInterfaceDispatch.GetInstance((ComInterfaceDispatch*)thisPtr); + Interop.Ole32.IStream inst = ComInterfaceDispatch.GetInstance((ComInterfaceDispatch*)thisPtr); inst.Read(pv, cb, pcbRead); return Interop.HRESULT.S_OK; } @@ -126,7 +127,7 @@ private static Interop.HRESULT ReadImplementation(IntPtr thisPtr, byte* pv, uint [UnmanagedCallersOnly] private static Interop.HRESULT WriteImplementation(IntPtr thisPtr, byte* pv, uint cb, uint* pcbWritten) { - Interop.Ole32.IStreamComWrapper inst = ComInterfaceDispatch.GetInstance((ComInterfaceDispatch*)thisPtr); + Interop.Ole32.IStream inst = ComInterfaceDispatch.GetInstance((ComInterfaceDispatch*)thisPtr); inst.Write(pv, cb, pcbWritten); return Interop.HRESULT.S_OK; } @@ -134,7 +135,7 @@ private static Interop.HRESULT WriteImplementation(IntPtr thisPtr, byte* pv, uin [UnmanagedCallersOnly] private static Interop.HRESULT SeekImplementation(IntPtr thisPtr, long dlibMove, SeekOrigin dwOrigin, ulong* plibNewPosition) { - Interop.Ole32.IStreamComWrapper inst = ComInterfaceDispatch.GetInstance((ComInterfaceDispatch*)thisPtr); + Interop.Ole32.IStream inst = ComInterfaceDispatch.GetInstance((ComInterfaceDispatch*)thisPtr); inst.Seek(dlibMove, dwOrigin, plibNewPosition); return Interop.HRESULT.S_OK; } @@ -142,7 +143,7 @@ private static Interop.HRESULT SeekImplementation(IntPtr thisPtr, long dlibMove, [UnmanagedCallersOnly] private static Interop.HRESULT SetSizeImplementation(IntPtr thisPtr, ulong libNewSize) { - Interop.Ole32.IStreamComWrapper inst = ComInterfaceDispatch.GetInstance((ComInterfaceDispatch*)thisPtr); + Interop.Ole32.IStream inst = ComInterfaceDispatch.GetInstance((ComInterfaceDispatch*)thisPtr); inst.SetSize(libNewSize); return Interop.HRESULT.S_OK; } @@ -150,8 +151,8 @@ private static Interop.HRESULT SetSizeImplementation(IntPtr thisPtr, ulong libNe [UnmanagedCallersOnly] private static Interop.HRESULT CopyToImplementation(IntPtr thisPtr, IntPtr pstm, ulong cb, ulong* pcbRead, ulong* pcbWritten) { - Interop.Ole32.IStreamComWrapper inst = ComInterfaceDispatch.GetInstance((ComInterfaceDispatch*)thisPtr); - Interop.Ole32.IStreamComWrapper pstmStream = ComInterfaceDispatch.GetInstance((ComInterfaceDispatch*)pstm); + Interop.Ole32.IStream inst = ComInterfaceDispatch.GetInstance((ComInterfaceDispatch*)thisPtr); + Interop.Ole32.IStream pstmStream = ComInterfaceDispatch.GetInstance((ComInterfaceDispatch*)pstm); inst.CopyTo(pstmStream, cb, pcbRead, pcbWritten); return Interop.HRESULT.S_OK; @@ -160,7 +161,7 @@ private static Interop.HRESULT CopyToImplementation(IntPtr thisPtr, IntPtr pstm, [UnmanagedCallersOnly] private static Interop.HRESULT CommitImplementation(IntPtr thisPtr, uint grfCommitFlags) { - Interop.Ole32.IStreamComWrapper inst = ComInterfaceDispatch.GetInstance((ComInterfaceDispatch*)thisPtr); + Interop.Ole32.IStream inst = ComInterfaceDispatch.GetInstance((ComInterfaceDispatch*)thisPtr); inst.Commit(grfCommitFlags); return Interop.HRESULT.S_OK; } @@ -168,7 +169,7 @@ private static Interop.HRESULT CommitImplementation(IntPtr thisPtr, uint grfComm [UnmanagedCallersOnly] private static Interop.HRESULT RevertImplementation(IntPtr thisPtr) { - Interop.Ole32.IStreamComWrapper inst = ComInterfaceDispatch.GetInstance((ComInterfaceDispatch*)thisPtr); + Interop.Ole32.IStream inst = ComInterfaceDispatch.GetInstance((ComInterfaceDispatch*)thisPtr); inst.Revert(); return Interop.HRESULT.S_OK; } @@ -176,21 +177,21 @@ private static Interop.HRESULT RevertImplementation(IntPtr thisPtr) [UnmanagedCallersOnly] private static Interop.HRESULT LockRegionImplementation(IntPtr thisPtr, ulong libOffset, ulong cb, uint dwLockType) { - Interop.Ole32.IStreamComWrapper inst = ComInterfaceDispatch.GetInstance((ComInterfaceDispatch*)thisPtr); + Interop.Ole32.IStream inst = ComInterfaceDispatch.GetInstance((ComInterfaceDispatch*)thisPtr); return inst.LockRegion(libOffset, cb, dwLockType); } [UnmanagedCallersOnly] private static Interop.HRESULT UnlockRegionImplementation(IntPtr thisPtr, ulong libOffset, ulong cb, uint dwLockType) { - Interop.Ole32.IStreamComWrapper inst = ComInterfaceDispatch.GetInstance((ComInterfaceDispatch*)thisPtr); + Interop.Ole32.IStream inst = ComInterfaceDispatch.GetInstance((ComInterfaceDispatch*)thisPtr); return inst.UnlockRegion(libOffset, cb, dwLockType); } [UnmanagedCallersOnly] private static Interop.HRESULT StatImplementation(IntPtr thisPtr, out Interop.Ole32.STATSTG pstatstg, Interop.Ole32.STATFLAG grfStatFlag) { - Interop.Ole32.IStreamComWrapper inst = ComInterfaceDispatch.GetInstance((ComInterfaceDispatch*)thisPtr); + Interop.Ole32.IStream inst = ComInterfaceDispatch.GetInstance((ComInterfaceDispatch*)thisPtr); inst.Stat(out pstatstg, grfStatFlag); return Interop.HRESULT.S_OK; } @@ -203,7 +204,7 @@ private static Interop.HRESULT CloneImplementation(IntPtr thisPtr, IntPtr* ppstm return Interop.HRESULT.STG_E_INVALIDPOINTER; } - Interop.Ole32.IStreamComWrapper inst = ComInterfaceDispatch.GetInstance((ComInterfaceDispatch*)thisPtr); + Interop.Ole32.IStream inst = ComInterfaceDispatch.GetInstance((ComInterfaceDispatch*)thisPtr); *ppstm = Instance.GetOrCreateComInterfaceForObject(inst.Clone(), CreateComInterfaceFlags.None); return Interop.HRESULT.S_OK; @@ -236,7 +237,7 @@ public void Dispose() public unsafe int SaveAsFile(IntPtr pstm, int fSaveMemCopy, int* pcbSize) { // Get the IStream implementation, since the ComWrappers runtime returns a pointer to the IUnknown interface implementation - Guid streamIID = Interop.Ole32.IStreamComWrapper.IID; + Guid streamIID = IStreamIID; CheckStatus(Marshal.QueryInterface(pstm, ref streamIID, out IntPtr pstmImpl)); try diff --git a/src/libraries/System.Drawing.Common/src/System/Drawing/Internal/GPStream.cs b/src/libraries/System.Drawing.Common/src/System/Drawing/Internal/GPStream.cs index 4b505101a44dea..c2d23afea39845 100644 --- a/src/libraries/System.Drawing.Common/src/System/Drawing/Internal/GPStream.cs +++ b/src/libraries/System.Drawing.Common/src/System/Drawing/Internal/GPStream.cs @@ -6,7 +6,7 @@ namespace System.Drawing.Internal { - internal sealed class GPStream : Interop.Ole32.IStream, Interop.Ole32.IStreamComWrapper + internal sealed class GPStream : Interop.Ole32.IStream { private readonly Stream _dataStream; @@ -50,15 +50,6 @@ public Interop.Ole32.IStream Clone() }; } - Interop.Ole32.IStreamComWrapper Interop.Ole32.IStreamComWrapper.Clone() - { - // The cloned object should have the same current "position" - return new GPStream(_dataStream) - { - _virtualPosition = _virtualPosition - }; - } - public void Commit(uint grfCommitFlags) { _dataStream.Flush(); @@ -104,43 +95,6 @@ public unsafe void CopyTo(Interop.Ole32.IStream pstm, ulong cb, ulong* pcbRead, *pcbWritten = totalWritten; } - unsafe void Interop.Ole32.IStreamComWrapper.CopyTo(Interop.Ole32.IStreamComWrapper pstm, ulong cb, ulong* pcbRead, ulong* pcbWritten) - { - byte[] buffer = ArrayPool.Shared.Rent(4096); - - ulong remaining = cb; - ulong totalWritten = 0; - ulong totalRead = 0; - - fixed (byte* b = buffer) - { - while (remaining > 0) - { - uint read = remaining < (ulong)buffer.Length ? (uint)remaining : (uint)buffer.Length; - Read(b, read, &read); - remaining -= read; - totalRead += read; - - if (read == 0) - { - break; - } - - uint written; - pstm.Write(b, read, &written); - totalWritten += written; - } - } - - ArrayPool.Shared.Return(buffer); - - if (pcbRead != null) - *pcbRead = totalRead; - - if (pcbWritten != null) - *pcbWritten = totalWritten; - } - public unsafe void Read(byte* pv, uint cb, uint* pcbRead) { ActualizeVirtualPosition(); From 75a7ddded62a1d6ab4c261e0e4431663283e50e8 Mon Sep 17 00:00:00 2001 From: Eric Erhardt Date: Thu, 24 Jun 2021 14:34:53 -0500 Subject: [PATCH 07/11] More PR feedback * No need for a VTable struct, just set each function pointer right into the table * Wrap all managed calls in try-catch and return HResult --- .../src/System/Drawing/DrawingComWrappers.cs | 252 ++++++++++++------ 1 file changed, 164 insertions(+), 88 deletions(-) diff --git a/src/libraries/System.Drawing.Common/src/System/Drawing/DrawingComWrappers.cs b/src/libraries/System.Drawing.Common/src/System/Drawing/DrawingComWrappers.cs index 76995f4e3c5bae..6940cab48ddb72 100644 --- a/src/libraries/System.Drawing.Common/src/System/Drawing/DrawingComWrappers.cs +++ b/src/libraries/System.Drawing.Common/src/System/Drawing/DrawingComWrappers.cs @@ -18,7 +18,9 @@ internal unsafe class DrawingComWrappers : ComWrappers { private const int OK = 0; private static readonly Guid IStreamIID = new Guid(0x0000000C, 0x0000, 0x0000, 0xC0, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x46); + private static readonly ComInterfaceEntry* s_wrapperEntry = InitializeComInterfaceEntry(); + internal static DrawingComWrappers Instance { get; } = new DrawingComWrappers(); private DrawingComWrappers() { } @@ -27,6 +29,12 @@ internal static void CheckStatus(int result) { if (result != OK) { + Exception? ex = Marshal.GetExceptionForHR(result); + if (ex != null) + { + throw ex; + } + throw new ExternalException() { HResult = result }; } } @@ -35,12 +43,11 @@ internal static void CheckStatus(int result) { GetIUnknownImpl(out IntPtr fpQueryInteface, out IntPtr fpAddRef, out IntPtr fpRelease); - IntPtr iStreamVtblRaw = RuntimeHelpers.AllocateTypeAssociatedMemory(typeof(IStreamVtbl), sizeof(IStreamVtbl)); - IStreamVtbl.Fill((IStreamVtbl*)iStreamVtblRaw, fpQueryInteface, fpAddRef, fpRelease); + IntPtr iStreamVtbl = IStreamVtbl.Create(fpQueryInteface, fpAddRef, fpRelease); - ComInterfaceEntry* wrapperEntry = (ComInterfaceEntry*)RuntimeHelpers.AllocateTypeAssociatedMemory(typeof(IStreamVtbl), sizeof(ComInterfaceEntry)); + ComInterfaceEntry* wrapperEntry = (ComInterfaceEntry*)RuntimeHelpers.AllocateTypeAssociatedMemory(typeof(DrawingComWrappers), sizeof(ComInterfaceEntry)); wrapperEntry->IID = IStreamIID; - wrapperEntry->Vtable = iStreamVtblRaw; + wrapperEntry->Vtable = iStreamVtbl; return wrapperEntry; } @@ -73,141 +80,210 @@ protected override void ReleaseObjects(IEnumerable objects) throw new NotImplementedException(); } - internal struct IUnknownVtbl + internal static class IStreamVtbl { - public IntPtr QueryInterface; - public IntPtr AddRef; - public IntPtr Release; - } - - internal unsafe struct IStreamVtbl - { - public IUnknownVtbl IUnknownImpl; - public delegate* unmanaged Read; - public delegate* unmanaged Write; - public delegate* unmanaged Seek; - public delegate* unmanaged SetSize; - public delegate* unmanaged CopyTo; - public delegate* unmanaged Commit; - public delegate* unmanaged Revert; - public delegate* unmanaged LockRegion; - public delegate* unmanaged UnlockRegion; - public delegate* unmanaged Stat; - public delegate* unmanaged Clone; - - public static void Fill(IStreamVtbl* vtable, IntPtr fpQueryInteface, IntPtr fpAddRef, IntPtr fpRelease) - { - vtable->IUnknownImpl = new IUnknownVtbl() - { - QueryInterface = fpQueryInteface, - AddRef = fpAddRef, - Release = fpRelease - }; - vtable->Read = &ReadImplementation; - vtable->Write = &WriteImplementation; - vtable->Seek = &SeekImplementation; - vtable->SetSize = &SetSizeImplementation; - vtable->CopyTo = &CopyToImplementation; - vtable->Commit = &CommitImplementation; - vtable->Revert = &RevertImplementation; - vtable->LockRegion = &LockRegionImplementation; - vtable->UnlockRegion = &UnlockRegionImplementation; - vtable->Stat = &StatImplementation; - vtable->Clone = &CloneImplementation; + public static IntPtr Create(IntPtr fpQueryInteface, IntPtr fpAddRef, IntPtr fpRelease) + { + IntPtr* vtblRaw = (IntPtr*)RuntimeHelpers.AllocateTypeAssociatedMemory(typeof(IStreamVtbl), IntPtr.Size * 14); + vtblRaw[0] = fpQueryInteface; + vtblRaw[1] = fpAddRef; + vtblRaw[2] = fpRelease; + vtblRaw[3] = (IntPtr)(delegate* unmanaged)&ReadImplementation; + vtblRaw[4] = (IntPtr)(delegate* unmanaged)&WriteImplementation; + vtblRaw[5] = (IntPtr)(delegate* unmanaged)&SeekImplementation; + vtblRaw[6] = (IntPtr)(delegate* unmanaged)&SetSizeImplementation; + vtblRaw[7] = (IntPtr)(delegate* unmanaged)&CopyToImplementation; + vtblRaw[8] = (IntPtr)(delegate* unmanaged)&CommitImplementation; + vtblRaw[9] = (IntPtr)(delegate* unmanaged)&RevertImplementation; + vtblRaw[10] = (IntPtr)(delegate* unmanaged)&LockRegionImplementation; + vtblRaw[11] = (IntPtr)(delegate* unmanaged)&UnlockRegionImplementation; + vtblRaw[12] = (IntPtr)(delegate* unmanaged)&StatImplementation; + vtblRaw[13] = (IntPtr)(delegate* unmanaged)&CloneImplementation; + + return (IntPtr)vtblRaw; } [UnmanagedCallersOnly] - private static Interop.HRESULT ReadImplementation(IntPtr thisPtr, byte* pv, uint cb, uint* pcbRead) + private static int ReadImplementation(IntPtr thisPtr, byte* pv, uint cb, uint* pcbRead) { - Interop.Ole32.IStream inst = ComInterfaceDispatch.GetInstance((ComInterfaceDispatch*)thisPtr); - inst.Read(pv, cb, pcbRead); - return Interop.HRESULT.S_OK; + try + { + Interop.Ole32.IStream inst = ComInterfaceDispatch.GetInstance((ComInterfaceDispatch*)thisPtr); + inst.Read(pv, cb, pcbRead); + } + catch (Exception e) + { + return e.HResult; + } + + return OK; } [UnmanagedCallersOnly] - private static Interop.HRESULT WriteImplementation(IntPtr thisPtr, byte* pv, uint cb, uint* pcbWritten) + private static int WriteImplementation(IntPtr thisPtr, byte* pv, uint cb, uint* pcbWritten) { - Interop.Ole32.IStream inst = ComInterfaceDispatch.GetInstance((ComInterfaceDispatch*)thisPtr); - inst.Write(pv, cb, pcbWritten); - return Interop.HRESULT.S_OK; + try + { + Interop.Ole32.IStream inst = ComInterfaceDispatch.GetInstance((ComInterfaceDispatch*)thisPtr); + inst.Write(pv, cb, pcbWritten); + } + catch (Exception e) + { + return e.HResult; + } + + return OK; } [UnmanagedCallersOnly] - private static Interop.HRESULT SeekImplementation(IntPtr thisPtr, long dlibMove, SeekOrigin dwOrigin, ulong* plibNewPosition) + private static int SeekImplementation(IntPtr thisPtr, long dlibMove, SeekOrigin dwOrigin, ulong* plibNewPosition) { - Interop.Ole32.IStream inst = ComInterfaceDispatch.GetInstance((ComInterfaceDispatch*)thisPtr); - inst.Seek(dlibMove, dwOrigin, plibNewPosition); - return Interop.HRESULT.S_OK; + try + { + Interop.Ole32.IStream inst = ComInterfaceDispatch.GetInstance((ComInterfaceDispatch*)thisPtr); + inst.Seek(dlibMove, dwOrigin, plibNewPosition); + } + catch (Exception e) + { + return e.HResult; + } + + return OK; } [UnmanagedCallersOnly] - private static Interop.HRESULT SetSizeImplementation(IntPtr thisPtr, ulong libNewSize) + private static int SetSizeImplementation(IntPtr thisPtr, ulong libNewSize) { - Interop.Ole32.IStream inst = ComInterfaceDispatch.GetInstance((ComInterfaceDispatch*)thisPtr); - inst.SetSize(libNewSize); - return Interop.HRESULT.S_OK; + try + { + Interop.Ole32.IStream inst = ComInterfaceDispatch.GetInstance((ComInterfaceDispatch*)thisPtr); + inst.SetSize(libNewSize); + } + catch (Exception e) + { + return e.HResult; + } + + return OK; } [UnmanagedCallersOnly] - private static Interop.HRESULT CopyToImplementation(IntPtr thisPtr, IntPtr pstm, ulong cb, ulong* pcbRead, ulong* pcbWritten) + private static int CopyToImplementation(IntPtr thisPtr, IntPtr pstm, ulong cb, ulong* pcbRead, ulong* pcbWritten) { - Interop.Ole32.IStream inst = ComInterfaceDispatch.GetInstance((ComInterfaceDispatch*)thisPtr); - Interop.Ole32.IStream pstmStream = ComInterfaceDispatch.GetInstance((ComInterfaceDispatch*)pstm); + try + { + Interop.Ole32.IStream inst = ComInterfaceDispatch.GetInstance((ComInterfaceDispatch*)thisPtr); + Interop.Ole32.IStream pstmStream = ComInterfaceDispatch.GetInstance((ComInterfaceDispatch*)pstm); - inst.CopyTo(pstmStream, cb, pcbRead, pcbWritten); - return Interop.HRESULT.S_OK; + inst.CopyTo(pstmStream, cb, pcbRead, pcbWritten); + } + catch (Exception e) + { + return e.HResult; + } + + return OK; } [UnmanagedCallersOnly] - private static Interop.HRESULT CommitImplementation(IntPtr thisPtr, uint grfCommitFlags) + private static int CommitImplementation(IntPtr thisPtr, uint grfCommitFlags) { - Interop.Ole32.IStream inst = ComInterfaceDispatch.GetInstance((ComInterfaceDispatch*)thisPtr); - inst.Commit(grfCommitFlags); - return Interop.HRESULT.S_OK; + try + { + Interop.Ole32.IStream inst = ComInterfaceDispatch.GetInstance((ComInterfaceDispatch*)thisPtr); + inst.Commit(grfCommitFlags); + } + catch (Exception e) + { + return e.HResult; + } + + + return OK; } [UnmanagedCallersOnly] - private static Interop.HRESULT RevertImplementation(IntPtr thisPtr) + private static int RevertImplementation(IntPtr thisPtr) { - Interop.Ole32.IStream inst = ComInterfaceDispatch.GetInstance((ComInterfaceDispatch*)thisPtr); - inst.Revert(); - return Interop.HRESULT.S_OK; + try + { + Interop.Ole32.IStream inst = ComInterfaceDispatch.GetInstance((ComInterfaceDispatch*)thisPtr); + inst.Revert(); + } + catch (Exception e) + { + return e.HResult; + } + + + return OK; } [UnmanagedCallersOnly] - private static Interop.HRESULT LockRegionImplementation(IntPtr thisPtr, ulong libOffset, ulong cb, uint dwLockType) + private static int LockRegionImplementation(IntPtr thisPtr, ulong libOffset, ulong cb, uint dwLockType) { - Interop.Ole32.IStream inst = ComInterfaceDispatch.GetInstance((ComInterfaceDispatch*)thisPtr); - return inst.LockRegion(libOffset, cb, dwLockType); + try + { + Interop.Ole32.IStream inst = ComInterfaceDispatch.GetInstance((ComInterfaceDispatch*)thisPtr); + return (int)inst.LockRegion(libOffset, cb, dwLockType); + } + catch (Exception e) + { + return e.HResult; + } } [UnmanagedCallersOnly] - private static Interop.HRESULT UnlockRegionImplementation(IntPtr thisPtr, ulong libOffset, ulong cb, uint dwLockType) + private static int UnlockRegionImplementation(IntPtr thisPtr, ulong libOffset, ulong cb, uint dwLockType) { - Interop.Ole32.IStream inst = ComInterfaceDispatch.GetInstance((ComInterfaceDispatch*)thisPtr); - return inst.UnlockRegion(libOffset, cb, dwLockType); + try + { + Interop.Ole32.IStream inst = ComInterfaceDispatch.GetInstance((ComInterfaceDispatch*)thisPtr); + return (int)inst.UnlockRegion(libOffset, cb, dwLockType); + } + catch (Exception e) + { + return e.HResult; + } } [UnmanagedCallersOnly] - private static Interop.HRESULT StatImplementation(IntPtr thisPtr, out Interop.Ole32.STATSTG pstatstg, Interop.Ole32.STATFLAG grfStatFlag) + private static int StatImplementation(IntPtr thisPtr, out Interop.Ole32.STATSTG pstatstg, Interop.Ole32.STATFLAG grfStatFlag) { - Interop.Ole32.IStream inst = ComInterfaceDispatch.GetInstance((ComInterfaceDispatch*)thisPtr); - inst.Stat(out pstatstg, grfStatFlag); - return Interop.HRESULT.S_OK; + try + { + Interop.Ole32.IStream inst = ComInterfaceDispatch.GetInstance((ComInterfaceDispatch*)thisPtr); + inst.Stat(out pstatstg, grfStatFlag); + } + catch (Exception e) + { + pstatstg = default; + return e.HResult; + } + + return OK; } [UnmanagedCallersOnly] - private static Interop.HRESULT CloneImplementation(IntPtr thisPtr, IntPtr* ppstm) + private static int CloneImplementation(IntPtr thisPtr, IntPtr* ppstm) { if (ppstm == null) { - return Interop.HRESULT.STG_E_INVALIDPOINTER; + return (int)Interop.HRESULT.STG_E_INVALIDPOINTER; } - Interop.Ole32.IStream inst = ComInterfaceDispatch.GetInstance((ComInterfaceDispatch*)thisPtr); + try + { + Interop.Ole32.IStream inst = ComInterfaceDispatch.GetInstance((ComInterfaceDispatch*)thisPtr); + + *ppstm = Instance.GetOrCreateComInterfaceForObject(inst.Clone(), CreateComInterfaceFlags.None); + } + catch (Exception e) + { + return e.HResult; + } - *ppstm = Instance.GetOrCreateComInterfaceForObject(inst.Clone(), CreateComInterfaceFlags.None); - return Interop.HRESULT.S_OK; + return OK; } } From 12f52f8b46377bbd5ad010bd430ee14c5155a212 Mon Sep 17 00:00:00 2001 From: Eric Erhardt Date: Thu, 24 Jun 2021 14:39:53 -0500 Subject: [PATCH 08/11] Revert unnecessary changes. --- .../Common/src/Interop/Windows/Ole32/Interop.IStream.cs | 1 - .../src/System/Drawing/Internal/GPStream.cs | 1 + 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/Common/src/Interop/Windows/Ole32/Interop.IStream.cs b/src/libraries/Common/src/Interop/Windows/Ole32/Interop.IStream.cs index 2eda49a3774eb4..1b78ce6f4745a6 100644 --- a/src/libraries/Common/src/Interop/Windows/Ole32/Interop.IStream.cs +++ b/src/libraries/Common/src/Interop/Windows/Ole32/Interop.IStream.cs @@ -1,7 +1,6 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. -using System; using System.IO; using System.Runtime.InteropServices; diff --git a/src/libraries/System.Drawing.Common/src/System/Drawing/Internal/GPStream.cs b/src/libraries/System.Drawing.Common/src/System/Drawing/Internal/GPStream.cs index c2d23afea39845..6040cd85c982c6 100644 --- a/src/libraries/System.Drawing.Common/src/System/Drawing/Internal/GPStream.cs +++ b/src/libraries/System.Drawing.Common/src/System/Drawing/Internal/GPStream.cs @@ -3,6 +3,7 @@ using System.Buffers; using System.IO; +using System.Runtime.InteropServices; namespace System.Drawing.Internal { From 6cb2d997d4cea49ccce2f5a26751ec7e8748f8f2 Mon Sep 17 00:00:00 2001 From: Eric Erhardt Date: Thu, 24 Jun 2021 18:22:09 -0500 Subject: [PATCH 09/11] More PR feedback * Rename methods * Use COM naming * Fix method signature to use pointer instead of out. * CheckStatus => ThrowExceptionForHR --- .../Interop/Windows/Ole32/Interop.IStream.cs | 4 +- .../Interop/Windows/Ole32/Interop.STATFLAG.cs | 2 +- .../src/System/Drawing/DrawingComWrappers.cs | 93 ++++++++----------- .../Drawing/Icon.Windows.COMWrappers.cs | 4 +- .../src/System/Drawing/Internal/GPStream.cs | 12 ++- 5 files changed, 51 insertions(+), 64 deletions(-) diff --git a/src/libraries/Common/src/Interop/Windows/Ole32/Interop.IStream.cs b/src/libraries/Common/src/Interop/Windows/Ole32/Interop.IStream.cs index 1b78ce6f4745a6..6fdf6718f050a2 100644 --- a/src/libraries/Common/src/Interop/Windows/Ole32/Interop.IStream.cs +++ b/src/libraries/Common/src/Interop/Windows/Ole32/Interop.IStream.cs @@ -56,8 +56,8 @@ HRESULT UnlockRegion( ulong cb, uint dwLockType); - void Stat( - out STATSTG pstatstg, + unsafe void Stat( + STATSTG* pstatstg, STATFLAG grfStatFlag); IStream Clone(); diff --git a/src/libraries/Common/src/Interop/Windows/Ole32/Interop.STATFLAG.cs b/src/libraries/Common/src/Interop/Windows/Ole32/Interop.STATFLAG.cs index 0847f1075983fc..38cd9844524c15 100644 --- a/src/libraries/Common/src/Interop/Windows/Ole32/Interop.STATFLAG.cs +++ b/src/libraries/Common/src/Interop/Windows/Ole32/Interop.STATFLAG.cs @@ -6,7 +6,7 @@ internal static partial class Interop internal static partial class Ole32 { /// - /// Stat flags for . + /// Stat flags for . /// /// internal enum STATFLAG : uint diff --git a/src/libraries/System.Drawing.Common/src/System/Drawing/DrawingComWrappers.cs b/src/libraries/System.Drawing.Common/src/System/Drawing/DrawingComWrappers.cs index 6940cab48ddb72..bdc06d9d738cee 100644 --- a/src/libraries/System.Drawing.Common/src/System/Drawing/DrawingComWrappers.cs +++ b/src/libraries/System.Drawing.Common/src/System/Drawing/DrawingComWrappers.cs @@ -16,8 +16,8 @@ namespace System.Drawing /// internal unsafe class DrawingComWrappers : ComWrappers { - private const int OK = 0; - private static readonly Guid IStreamIID = new Guid(0x0000000C, 0x0000, 0x0000, 0xC0, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x46); + private const int S_OK = (int)Interop.HRESULT.S_OK; + private static readonly Guid IID_IStream = new Guid(0x0000000C, 0x0000, 0x0000, 0xC0, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x46); private static readonly ComInterfaceEntry* s_wrapperEntry = InitializeComInterfaceEntry(); @@ -25,20 +25,6 @@ internal unsafe class DrawingComWrappers : ComWrappers private DrawingComWrappers() { } - internal static void CheckStatus(int result) - { - if (result != OK) - { - Exception? ex = Marshal.GetExceptionForHR(result); - if (ex != null) - { - throw ex; - } - - throw new ExternalException() { HResult = result }; - } - } - private static ComInterfaceEntry* InitializeComInterfaceEntry() { GetIUnknownImpl(out IntPtr fpQueryInteface, out IntPtr fpAddRef, out IntPtr fpRelease); @@ -46,7 +32,7 @@ internal static void CheckStatus(int result) IntPtr iStreamVtbl = IStreamVtbl.Create(fpQueryInteface, fpAddRef, fpRelease); ComInterfaceEntry* wrapperEntry = (ComInterfaceEntry*)RuntimeHelpers.AllocateTypeAssociatedMemory(typeof(DrawingComWrappers), sizeof(ComInterfaceEntry)); - wrapperEntry->IID = IStreamIID; + wrapperEntry->IID = IID_IStream; wrapperEntry->Vtable = iStreamVtbl; return wrapperEntry; } @@ -67,7 +53,7 @@ protected override object CreateObject(IntPtr externalComObject, CreateObjectFla Guid pictureIID = IPicture.IID; int hr = Marshal.QueryInterface(externalComObject, ref pictureIID, out IntPtr comObject); - if (hr == 0) + if (hr == S_OK) { return new PictureWrapper(comObject); } @@ -88,23 +74,23 @@ public static IntPtr Create(IntPtr fpQueryInteface, IntPtr fpAddRef, IntPtr fpRe vtblRaw[0] = fpQueryInteface; vtblRaw[1] = fpAddRef; vtblRaw[2] = fpRelease; - vtblRaw[3] = (IntPtr)(delegate* unmanaged)&ReadImplementation; - vtblRaw[4] = (IntPtr)(delegate* unmanaged)&WriteImplementation; - vtblRaw[5] = (IntPtr)(delegate* unmanaged)&SeekImplementation; - vtblRaw[6] = (IntPtr)(delegate* unmanaged)&SetSizeImplementation; - vtblRaw[7] = (IntPtr)(delegate* unmanaged)&CopyToImplementation; - vtblRaw[8] = (IntPtr)(delegate* unmanaged)&CommitImplementation; - vtblRaw[9] = (IntPtr)(delegate* unmanaged)&RevertImplementation; - vtblRaw[10] = (IntPtr)(delegate* unmanaged)&LockRegionImplementation; - vtblRaw[11] = (IntPtr)(delegate* unmanaged)&UnlockRegionImplementation; - vtblRaw[12] = (IntPtr)(delegate* unmanaged)&StatImplementation; - vtblRaw[13] = (IntPtr)(delegate* unmanaged)&CloneImplementation; + vtblRaw[3] = (IntPtr)(delegate* unmanaged)&Read; + vtblRaw[4] = (IntPtr)(delegate* unmanaged)&Write; + vtblRaw[5] = (IntPtr)(delegate* unmanaged)&Seek; + vtblRaw[6] = (IntPtr)(delegate* unmanaged)&SetSize; + vtblRaw[7] = (IntPtr)(delegate* unmanaged)&CopyTo; + vtblRaw[8] = (IntPtr)(delegate* unmanaged)&Commit; + vtblRaw[9] = (IntPtr)(delegate* unmanaged)&Revert; + vtblRaw[10] = (IntPtr)(delegate* unmanaged)&LockRegion; + vtblRaw[11] = (IntPtr)(delegate* unmanaged)&UnlockRegion; + vtblRaw[12] = (IntPtr)(delegate* unmanaged)&Stat; + vtblRaw[13] = (IntPtr)(delegate* unmanaged)&Clone; return (IntPtr)vtblRaw; } [UnmanagedCallersOnly] - private static int ReadImplementation(IntPtr thisPtr, byte* pv, uint cb, uint* pcbRead) + private static int Read(IntPtr thisPtr, byte* pv, uint cb, uint* pcbRead) { try { @@ -116,11 +102,11 @@ private static int ReadImplementation(IntPtr thisPtr, byte* pv, uint cb, uint* p return e.HResult; } - return OK; + return S_OK; } [UnmanagedCallersOnly] - private static int WriteImplementation(IntPtr thisPtr, byte* pv, uint cb, uint* pcbWritten) + private static int Write(IntPtr thisPtr, byte* pv, uint cb, uint* pcbWritten) { try { @@ -132,11 +118,11 @@ private static int WriteImplementation(IntPtr thisPtr, byte* pv, uint cb, uint* return e.HResult; } - return OK; + return S_OK; } [UnmanagedCallersOnly] - private static int SeekImplementation(IntPtr thisPtr, long dlibMove, SeekOrigin dwOrigin, ulong* plibNewPosition) + private static int Seek(IntPtr thisPtr, long dlibMove, SeekOrigin dwOrigin, ulong* plibNewPosition) { try { @@ -148,11 +134,11 @@ private static int SeekImplementation(IntPtr thisPtr, long dlibMove, SeekOrigin return e.HResult; } - return OK; + return S_OK; } [UnmanagedCallersOnly] - private static int SetSizeImplementation(IntPtr thisPtr, ulong libNewSize) + private static int SetSize(IntPtr thisPtr, ulong libNewSize) { try { @@ -164,11 +150,11 @@ private static int SetSizeImplementation(IntPtr thisPtr, ulong libNewSize) return e.HResult; } - return OK; + return S_OK; } [UnmanagedCallersOnly] - private static int CopyToImplementation(IntPtr thisPtr, IntPtr pstm, ulong cb, ulong* pcbRead, ulong* pcbWritten) + private static int CopyTo(IntPtr thisPtr, IntPtr pstm, ulong cb, ulong* pcbRead, ulong* pcbWritten) { try { @@ -182,11 +168,11 @@ private static int CopyToImplementation(IntPtr thisPtr, IntPtr pstm, ulong cb, u return e.HResult; } - return OK; + return S_OK; } [UnmanagedCallersOnly] - private static int CommitImplementation(IntPtr thisPtr, uint grfCommitFlags) + private static int Commit(IntPtr thisPtr, uint grfCommitFlags) { try { @@ -198,12 +184,11 @@ private static int CommitImplementation(IntPtr thisPtr, uint grfCommitFlags) return e.HResult; } - - return OK; + return S_OK; } [UnmanagedCallersOnly] - private static int RevertImplementation(IntPtr thisPtr) + private static int Revert(IntPtr thisPtr) { try { @@ -215,12 +200,11 @@ private static int RevertImplementation(IntPtr thisPtr) return e.HResult; } - - return OK; + return S_OK; } [UnmanagedCallersOnly] - private static int LockRegionImplementation(IntPtr thisPtr, ulong libOffset, ulong cb, uint dwLockType) + private static int LockRegion(IntPtr thisPtr, ulong libOffset, ulong cb, uint dwLockType) { try { @@ -234,7 +218,7 @@ private static int LockRegionImplementation(IntPtr thisPtr, ulong libOffset, ulo } [UnmanagedCallersOnly] - private static int UnlockRegionImplementation(IntPtr thisPtr, ulong libOffset, ulong cb, uint dwLockType) + private static int UnlockRegion(IntPtr thisPtr, ulong libOffset, ulong cb, uint dwLockType) { try { @@ -248,24 +232,23 @@ private static int UnlockRegionImplementation(IntPtr thisPtr, ulong libOffset, u } [UnmanagedCallersOnly] - private static int StatImplementation(IntPtr thisPtr, out Interop.Ole32.STATSTG pstatstg, Interop.Ole32.STATFLAG grfStatFlag) + private static int Stat(IntPtr thisPtr, Interop.Ole32.STATSTG* pstatstg, Interop.Ole32.STATFLAG grfStatFlag) { try { Interop.Ole32.IStream inst = ComInterfaceDispatch.GetInstance((ComInterfaceDispatch*)thisPtr); - inst.Stat(out pstatstg, grfStatFlag); + inst.Stat(pstatstg, grfStatFlag); } catch (Exception e) { - pstatstg = default; return e.HResult; } - return OK; + return S_OK; } [UnmanagedCallersOnly] - private static int CloneImplementation(IntPtr thisPtr, IntPtr* ppstm) + private static int Clone(IntPtr thisPtr, IntPtr* ppstm) { if (ppstm == null) { @@ -283,7 +266,7 @@ private static int CloneImplementation(IntPtr thisPtr, IntPtr* ppstm) return e.HResult; } - return OK; + return S_OK; } } @@ -313,8 +296,8 @@ public void Dispose() public unsafe int SaveAsFile(IntPtr pstm, int fSaveMemCopy, int* pcbSize) { // Get the IStream implementation, since the ComWrappers runtime returns a pointer to the IUnknown interface implementation - Guid streamIID = IStreamIID; - CheckStatus(Marshal.QueryInterface(pstm, ref streamIID, out IntPtr pstmImpl)); + Guid streamIID = IID_IStream; + Marshal.ThrowExceptionForHR(Marshal.QueryInterface(pstm, ref streamIID, out IntPtr pstmImpl)); try { diff --git a/src/libraries/System.Drawing.Common/src/System/Drawing/Icon.Windows.COMWrappers.cs b/src/libraries/System.Drawing.Common/src/System/Drawing/Icon.Windows.COMWrappers.cs index 47de3032558581..2552cd67d3e36e 100644 --- a/src/libraries/System.Drawing.Common/src/System/Drawing/Icon.Windows.COMWrappers.cs +++ b/src/libraries/System.Drawing.Common/src/System/Drawing/Icon.Windows.COMWrappers.cs @@ -30,7 +30,7 @@ public unsafe void Save(Stream outputStream) PICTDESC pictdesc = PICTDESC.CreateIconPICTDESC(Handle); Guid iid = DrawingComWrappers.IPicture.IID; IntPtr lpPicture; - DrawingComWrappers.CheckStatus(OleCreatePictureIndirect(&pictdesc, &iid, fOwn: 0, &lpPicture)); + Marshal.ThrowExceptionForHR(OleCreatePictureIndirect(&pictdesc, &iid, fOwn: 0, &lpPicture)); IntPtr streamPtr = IntPtr.Zero; try @@ -40,7 +40,7 @@ public unsafe void Save(Stream outputStream) var gpStream = new GPStream(outputStream, makeSeekable: false); streamPtr = DrawingComWrappers.Instance.GetOrCreateComInterfaceForObject(gpStream, CreateComInterfaceFlags.None); - DrawingComWrappers.CheckStatus(picture.SaveAsFile(streamPtr, -1, null)); + Marshal.ThrowExceptionForHR(picture.SaveAsFile(streamPtr, -1, null)); } finally { diff --git a/src/libraries/System.Drawing.Common/src/System/Drawing/Internal/GPStream.cs b/src/libraries/System.Drawing.Common/src/System/Drawing/Internal/GPStream.cs index 6040cd85c982c6..dd6a8a16350ff0 100644 --- a/src/libraries/System.Drawing.Common/src/System/Drawing/Internal/GPStream.cs +++ b/src/libraries/System.Drawing.Common/src/System/Drawing/Internal/GPStream.cs @@ -3,7 +3,6 @@ using System.Buffers; using System.IO; -using System.Runtime.InteropServices; namespace System.Drawing.Internal { @@ -177,9 +176,14 @@ public void SetSize(ulong value) _dataStream.SetLength(checked((long)value)); } - public void Stat(out Interop.Ole32.STATSTG pstatstg, Interop.Ole32.STATFLAG grfStatFlag) + public unsafe void Stat(Interop.Ole32.STATSTG* pstatstg, Interop.Ole32.STATFLAG grfStatFlag) { - pstatstg = new Interop.Ole32.STATSTG + if (pstatstg == null) + { + throw new ArgumentNullException(nameof(pstatstg)); + } + + *pstatstg = new Interop.Ole32.STATSTG { cbSize = (ulong)_dataStream.Length, type = Interop.Ole32.STGTY.STGTY_STREAM, @@ -195,7 +199,7 @@ public void Stat(out Interop.Ole32.STATSTG pstatstg, Interop.Ole32.STATFLAG grfS if (grfStatFlag == Interop.Ole32.STATFLAG.STATFLAG_DEFAULT) { // Caller wants a name - pstatstg.AllocName(_dataStream is FileStream fs ? fs.Name : _dataStream.ToString()); + pstatstg->AllocName(_dataStream is FileStream fs ? fs.Name : _dataStream.ToString()); } } From 3c657bcef0e65048f2fa4f834131f8dc4794e350 Mon Sep 17 00:00:00 2001 From: Eric Erhardt Date: Fri, 25 Jun 2021 19:11:09 -0500 Subject: [PATCH 10/11] Fix tests * Pass -1 to Marshal.GetExceptionForHR so it doesn't query GetErrorInfo, and always returns the correct exception type * Create the PictureWrapper with UniqueInstance, so it doesn't get cached. Caching it causes lifetime issues. --- .../src/System/Drawing/DrawingComWrappers.cs | 2 +- .../Drawing/Icon.Windows.COMWrappers.cs | 22 +++++++++++++++---- 2 files changed, 19 insertions(+), 5 deletions(-) diff --git a/src/libraries/System.Drawing.Common/src/System/Drawing/DrawingComWrappers.cs b/src/libraries/System.Drawing.Common/src/System/Drawing/DrawingComWrappers.cs index bdc06d9d738cee..7895381853c4a4 100644 --- a/src/libraries/System.Drawing.Common/src/System/Drawing/DrawingComWrappers.cs +++ b/src/libraries/System.Drawing.Common/src/System/Drawing/DrawingComWrappers.cs @@ -49,7 +49,7 @@ private DrawingComWrappers() { } protected override object CreateObject(IntPtr externalComObject, CreateObjectFlags flags) { - Debug.Assert(flags == CreateObjectFlags.None); + Debug.Assert(flags == CreateObjectFlags.UniqueInstance); Guid pictureIID = IPicture.IID; int hr = Marshal.QueryInterface(externalComObject, ref pictureIID, out IntPtr comObject); diff --git a/src/libraries/System.Drawing.Common/src/System/Drawing/Icon.Windows.COMWrappers.cs b/src/libraries/System.Drawing.Common/src/System/Drawing/Icon.Windows.COMWrappers.cs index 2552cd67d3e36e..973211c47e76db 100644 --- a/src/libraries/System.Drawing.Common/src/System/Drawing/Icon.Windows.COMWrappers.cs +++ b/src/libraries/System.Drawing.Common/src/System/Drawing/Icon.Windows.COMWrappers.cs @@ -35,12 +35,14 @@ public unsafe void Save(Stream outputStream) IntPtr streamPtr = IntPtr.Zero; try { - using DrawingComWrappers.IPicture picture = (DrawingComWrappers.IPicture)DrawingComWrappers.Instance.GetOrCreateObjectForComInstance(lpPicture, CreateObjectFlags.None); + // Use UniqueInstance here because we never want to cache the wrapper. It only gets used once and then disposed. + using DrawingComWrappers.IPicture picture = (DrawingComWrappers.IPicture)DrawingComWrappers.Instance + .GetOrCreateObjectForComInstance(lpPicture, CreateObjectFlags.UniqueInstance); var gpStream = new GPStream(outputStream, makeSeekable: false); streamPtr = DrawingComWrappers.Instance.GetOrCreateComInterfaceForObject(gpStream, CreateComInterfaceFlags.None); - Marshal.ThrowExceptionForHR(picture.SaveAsFile(streamPtr, -1, null)); + CheckSaveAsFileResult(picture.SaveAsFile(streamPtr, -1, null)); } finally { @@ -52,13 +54,25 @@ public unsafe void Save(Stream outputStream) if (lpPicture != IntPtr.Zero) { - // don't assert the count went to 0 here because some other thread could be using the same lpPicture - Marshal.Release(lpPicture); + int count = Marshal.Release(lpPicture); + Debug.Assert(count == 0); } + } } } + private static void CheckSaveAsFileResult(int errorCode) + { + // Pass -1 for errorInfo to indicate that Windows' GetErrorInfo shouldn't be called, and only + // return the Exception corresponding to the specified errorCode. + Exception? ex = Marshal.GetExceptionForHR(errorCode, errorInfo: new IntPtr(-1)); + if (ex != null) + { + throw ex; + } + } + [DllImport(Interop.Libraries.Oleaut32)] private static unsafe extern int OleCreatePictureIndirect(PICTDESC* pictdesc, Guid* refiid, int fOwn, IntPtr* lplpvObj); From 39e5fc64a4b66f1e3a8a39081bc5e7f3b23ea263 Mon Sep 17 00:00:00 2001 From: Eric Erhardt Date: Fri, 25 Jun 2021 19:18:29 -0500 Subject: [PATCH 11/11] GetExceptionForHR => ThrowExceptionForHR --- .../src/System/Drawing/Icon.Windows.COMWrappers.cs | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/src/libraries/System.Drawing.Common/src/System/Drawing/Icon.Windows.COMWrappers.cs b/src/libraries/System.Drawing.Common/src/System/Drawing/Icon.Windows.COMWrappers.cs index 973211c47e76db..a03ad5ed133bbe 100644 --- a/src/libraries/System.Drawing.Common/src/System/Drawing/Icon.Windows.COMWrappers.cs +++ b/src/libraries/System.Drawing.Common/src/System/Drawing/Icon.Windows.COMWrappers.cs @@ -57,7 +57,6 @@ public unsafe void Save(Stream outputStream) int count = Marshal.Release(lpPicture); Debug.Assert(count == 0); } - } } } @@ -65,12 +64,8 @@ public unsafe void Save(Stream outputStream) private static void CheckSaveAsFileResult(int errorCode) { // Pass -1 for errorInfo to indicate that Windows' GetErrorInfo shouldn't be called, and only - // return the Exception corresponding to the specified errorCode. - Exception? ex = Marshal.GetExceptionForHR(errorCode, errorInfo: new IntPtr(-1)); - if (ex != null) - { - throw ex; - } + // throw the Exception corresponding to the specified errorCode. + Marshal.ThrowExceptionForHR(errorCode, errorInfo: new IntPtr(-1)); } [DllImport(Interop.Libraries.Oleaut32)]