Winforms: Fatal error. Internal CLR error. (0x80131506) when DataObject.GetData(EnhancedMetaFile)

[asklar]

• .NET Core Version:
  .NET Core SDK (reflecting any global.json):
  Version: 3.1.201
  Commit: b1768b4ae7

Runtime Environment:
OS Name: Windows
OS Version: 10.0.19041
OS Platform: Windows
RID: win10-x64
Base Path: C:\Program Files\dotnet\sdk\3.1.201\

Host (useful for support):
Version: 3.1.3
Commit: 4a9f85e9f8

.NET Core SDKs installed:
3.1.201 [C:\Program Files\dotnet\sdk]

.NET Core runtimes installed:
Microsoft.AspNetCore.App 3.1.3 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
Microsoft.NETCore.App 3.1.3 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
Microsoft.WindowsDesktop.App 3.1.3 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]

• Have you experienced this same bug with .NET Framework?:
  No

Problem description:
I have a console app that prints out the clipboard contents using System.Windows.Forms.IDataObject.
When I do IDataObject.GetData("EnhancedMetafile") (verified that the format is available), the program crashes with this error:

Fatal error. Internal CLR error. (0x80131506)
   at System.Runtime.InteropServices.Marshal.GetObjectForIUnknown(IntPtr)
   at System.Windows.Forms.DataObject+OleConverter.GetDataFromOleIStream(System.String)
   at System.Windows.Forms.DataObject+OleConverter.GetDataFromBoundOleDataObject(System.String, Boolean ByRef)
   at System.Windows.Forms.DataObject+OleConverter.GetData(System.String, Boolean)
   at System.Windows.Forms.DataObject.GetData(System.String, Boolean)
   at System.Windows.Forms.DataObject.GetData(System.String)

Expected behavior:
No crash, should be able to access the data in the EMF

Minimal repro:

[weltkante]

Just FYI automatic conversion of metafile to WinForms image has been removed because of potential security issues. So while that exception crash should of course be fixed and DataObject.GetData should work you'd be getting binary data now and have to process it yourself (or getting null if the source application is too restrictive and only provides a metafile handle, WinForms will probably reject to unpack that). You'd also not be able to call DataFile.GetImage to get a WinForms image object (returns null). See #493 for previous discussion:

We were restricting de-serialization of some of the types in Clipboard, for security reasons. Metafile type is one of them. This was a design decision for Core. Resolving this as by design and we will revisit if there are customer issues.

[weltkante]

What application (or API if its your own) did you use to provide the metafile data? I can repro with copying a metafile from inside Word, but I'm wondering what the scope of the problem is.

Have you experienced this same bug with .NET Framework?: No

On which version of Desktop Framework have you tested this? (Installed version, the target version against which the project is compiled is irrelevant.) For me this crashes on fully patched Desktop Framework 4.8 as well, so this is not a regression in .NET Core.

For what its worth the crash happens here. Contrary to the documentation we get a TYMED_ENHMF even though WinForms asked for a TYMED_ISTREAM. Unconditionally turning that pointer into a COM interface crashes because there is no COM interface stored in the struct.

This looks like a regression in Windows and/or the application which provided the data as the doc says:

If the data object cannot comply with the information specified in the FORMATETC, the method should return DV_E_FORMATETC.

If an error code were returned then innerData.GetData should have thrown and the method been exited early.

If you want to make WinForms more robust you can check the medium by replacing this check to make sure the code is operating on what it assumed to operate on

- if (medium.unionmember != IntPtr.Zero)
+ if (formatetc.tymed == TYMED.TYMED_ISTREAM && medium.unionmember != IntPtr.Zero)

Though technically now you created a memory leak because you are not properly releasing the pointed-to data. Before adding general release logic to work around what looks like a bug in Windows and/or Office I'd like to dig into this a bit more since it looks like a Windows API semantic is violated here.

[asklar]

For me this repros when copying an image inside an email in Outlook (desktop). It is possible this broke in Windows or Office. I'm trying to find other sources of EMF to validate whether this is an Office or Windows bug, stay tuned.

[RussKie]

```cs
- if (medium.unionmember != IntPtr.Zero)
+ if (formatetc.tymed == TYMED.TYMED_ISTREAM && medium.unionmember != IntPtr.Zero)
```

if you use ```diff it will render as a diff:

- if (medium.unionmember != IntPtr.Zero)
+ if (formatetc.tymed == TYMED.TYMED_ISTREAM && medium.unionmember != IntPtr.Zero)

[asklar]

Filed Office Client bug 4066227

[asklar]

@weltkante @RussKie Ok I've done some research.

The MSDN docs are ambiguous about the meaning of the 2nd param to GetData, the STGMEDIUM (I'll work on that) but if you read the IDL (see objidl.idl), you see that the STGMEDIUM passed in to IDataObject::GetData is an [out] parameter, not an [inout].

The docs even go as far as to say:

The medium must be allocated and filled in by GetData.

Moreover, when talking about the TYMED that is inside the FORMATETC struct, the docs say

It is important that GetData render the requested aspect and, if possible, use the requested medium

So the docs indicate it is possible for the data object to choose to render the results in a different medium than requested. The comment you mentioned:

If the data object cannot comply with the information specified in the FORMATETC, the method should return DV_E_FORMATETC.

this comment above refers to the format part of FORMATETC, not the TYMED (since the previous comment mentions "if possible").

In other words, you tell the data object _what type of medium you would like_, but it tells you what it gave you. So whatever value is being set in the STGMEDIUM that's passed in, the consumer needs to handle the medium type being different than what it asked for.

[weltkante]

You tell what medium you want in the other (input) parameter. The fact that WinForms sets the type on the out-parameter is redundant but not a bug.

The primary question is whether Windows always behaved this way, passing the returned medium unfiltered from the source application to the target application. Normally OLE sits in between the applications and does some sanity checking.

If Windows/OLE always behaved this way and lets GetData return arbitrary storage mediums regardless what the source application asked then its a documentation bug and WinForms needs to be made more robust.

If its a regression introduced by Windows/OLE and it didn't work this way before then it may not be worth adding complex workarounds in WinForms to work around an OS regression which (hopefully) gets fixed soon. (Whether to add a workaround anyways is a decision of the WinForms maintainers.)

As a side note: earlier Desktop Framework versions wouldn't have run into this bug because they check for Metafile before checking for IStream medium. They would have returned early with a decoded metafile. Since that security update which removed metafile support it now gets to the code path which wants to try IStream medium.

I already tested with native applications and they see the same behavior, next I'll try setting up a test environment to figure out how older Windows versions behave.

[asklar]

@weltkante Yes, I've updated my comment when I realized that there are two TYMEDs in play, see my comment about the "if possible" clause in the docs

[weltkante]

You are probably right about the docs, could be written better, its all very unclear how ownership of the storage medium works (not just this section). The way I'm reading it I suspect there may actually be major memory/resource leaks in WinForms, because it neither uses ReleaseStgMedium nor respects pUnkForRelease.

Anyways, my tests in VMs confirm that Windows always worked this way and OLE doesn't verify the returned TYMED matches what was asked for. I also tested a few other applications and only Office behaved this way.

I think WinForms needs to be made more robust, since they stopped accepting Metafiles it now will go through this code path and needs to handle the way Office (and potentially other applications) behave.

I'll create a PR for fixing the problem - including the other call sites, even though they do not crash they look equally broken. They also really need to start using ReleaseStgMedium for robustness, I can only guess usage of pUnkForRelease is not widespread.

This bug might actually be desireable to fix in Desktop Framework as well, removing Metafile support for security reasons isn't a great security improvement when it uncovers/triggers a bug that makes applications crash unrecoverably. Office isn't exactly an uncommon application to copy from, either. If someone has an old application still supporting metafiles running on a runtime updated to .NET 4.8 this would cause bad crashes.

[weltkante]

Also, to be clear, when this bug is fixed WinForms just returns null. The EnhancedMetafile format provided by Office has no TYMED_ISTREAM support, WinForms just stumbles over how Office returns its response. If you try it with WordPad you already get null. For all practical purposes .NET applications should be ignoring this format (you should generally program your clipboard code to only use formats you can deal with, since metafiles are no longer supported they can never be useful for .NET applications)

If you need metafile support for some reason you probably should pick up discussion based on #493, as far as I read the comments the decision is still open for discussion if there is justifyable demand (but it probably takes more than "I just want to support all clipboard formats", you'll probably need a good reason why you can't use any of the other formats).

[asklar]

thanks for your reply @weltkante - note that returning null for a format that the dataobject says it supports is still a violation of the API contract.
1) I don't really understand why .net is deeming metafile support as being a security vulnerability, when it is fully supported in win32.
2) If .net were still to want to not support metafiles, the "right" behavior would be to not only return null (in fact it should probably throw), but also when you try to enumerate the formats available, it should filter out those formats that are not supported by .net. This would still be a functional regression, but at least it would be consistent (no crash)
3) the medium for metafiles is not IStream, it's GDI handles.
It sounds like I need to not rely on the winforms implementation and instead roll my own and reinvent the wheel by p/invoking the win32 apis, which I was trying to avoid :(
4) Agree that winforms has major leaks if it isn't respecting the pUnkForRelease / ReleaseStgMedium part of the API

[weltkante]

Not commenting on security decisions since I'm not working for Microsoft and can only guess what their intentions are.

I generally agree, except with how the API should behave:

the "right" behavior would be to not only return null (in fact it should probably throw)

As far as I'm aware GetData always returned null for formats it can't handle, changing this to throwing would be wrong.

but also when you try to enumerate the formats available, it should filter out those formats that are not supported by .net

There is #1268 for making the APIs consistent around metafile removal. I'll have a look at how Desktop Framework behaves with formats it couldn't understand. If Desktop Frameworks also filters the formats I'll try doing it for metafiles as well, but if Desktop Framework doesn't filter unknown formats there's no reason to start doing it just for metafiles.

Also note that the way Desktop Framework uses the clipboard with custom formats, by putting serialized objects into it and making the format name optional instead of a contract, it is often not possible to determine whether a format is supported before you have seen the data. In my opinion that was a bad design that skewed the API into a way OLE doesn't intend to, but Desktop Framework has been around long enough that you have to consider its behavior as common practice.

As such at this point the GetFormats and GetDataPresent APIs are only advisorial so you can prioritize formats and you have to live and deal with false positives.

It sounds like I need to not rely on the winforms implementation and instead roll my own and reinvent the wheel by p/invoking the win32 apis

Definitely the best way to do things if you want maximum compatibility with arbitrary applications, including Desktop Framework. I have hit the limitations of WinForms/WPF DataObject API way before this already (their IDataObject API was never implemented fully) and am probably going the same route to avoid compatibility issues. Note that WinForms and WPF expose access to the native COM IDataObject API if you want to make use of it.

In summary the WinForms/WPF implementation is only good for targeted usage of commonly used formats, or for data transfer between applications all based on the same framework. Cross framework of custom formats is not going to work well. Uncommon usage of IDataObject with native applications has never been working well, even in Desktop Framework.

As another upcoming compatibility issue be aware that Binary Formatter, which is heavily used in Desktop Framework custom clipboard/drag'n'drop formats, currently only has partial compatibility. Commonly used classes are likely compatible, but less commonly used classes may have been refactored or be no longer marked serializable. In addition Binary Formatter is gradually staged out for security reasons oustide clipboard usage, see dotnet/runtime#29976 for future plans of moving it entirely out of the framework into a separate package, with the possibility that cross framework clipboard compatibility may regress further.