Fix DataObject.GetData issues #3039

weltkante · 2020-04-05T14:42:53Z

Fixes #3025
Contributes to #1268

Proposed changes

use ReleaseStgMedium for ownership handling as suggested by win32 docs
do not assume third party implementations of IDataObject.GetData return the medium which was asked for (Office sometimes ignores the medium the caller asks for and always returns the medium it happens to be using internally). Since Windows/OLE does not pose constraints on what the DataObject source returns WinForms needs to be robust and handle this case instead of crashing the whole process.
clean up dead code related to metafiles, most code supporting them is already gone

Customer Impact

no more crashing when asking Office for "EnhancedMetafile"

Regression?

Yes (earlier Desktop Framework versions which still supported metafiles did not crash; latest Desktop Framework however also crashes the whole process)

Risk

low
- For the scenario crashing the process there is no risk, (almost) nothing is worse than crashing the process
- Metafile support was already removed and did not work anyways. Removing dead code related to metafiles should not be a problem since it was not possible to retrieve them.

Test methodology

Manual test of scenario Fatal error. Internal CLR error. (0x80131506) when DataObject.GetData(EnhancedMetaFile) #3025 by copy/pasting a metafile from Word to .NET Core, ensuring it no longer crashes the process and instead properly delivers no content as expected.
Unit test which exercises the code path which was previously crashing

Microsoft Reviewers: Open in CodeFlow

weltkante

I didn't do any code style cleanup to not distract from how the ownership/release logic changed. Adding some pre-resolved comments to explain whats happening. Any other place which released something was replaced by ReleaseStgMedium.

Note that the previous code did not respect punkForRelease and released stuff even if it didn't own it. I can only conclude that punkForRelease is not used by most applications, otherwise Clipboard support would have been totally broken in Desktop Framework probably since its initial release.

src/System.Windows.Forms/src/System/Windows/Forms/DataObject.cs

weltkante · 2020-04-05T15:04:48Z

Not sure why tests are failing, I don't think the AxHost/IPictureDisp test has anything to do with DataObject, and for the other CI failure I don't even see what failed (or don't know where to look).

src/System.Windows.Forms/src/System/Windows/Forms/DataObject.cs

hughbe · 2020-04-06T10:56:01Z

src/System.Windows.Forms/src/System/Windows/Forms/DataObject.cs

@@ -1414,6 +1416,10 @@ private object GetDataFromOleHGLOBAL(string format, out bool done)
                    catch


should we have these catch-all blocks?

Unfortunately its required here for two reasons:

GetDataFromHGLOBAL reports failure via exception, but you don't know which exception it is. It does complex dispatching and some branches can execute arbitrary user code while deserializing a serialized object (not for the builtin formats, but custom formats still support arbitrary serialized objects). Failure to deserialize shouldn't throw and needs to return null, it is actually a pretty common scenario not being able to resolve an assembly from a 3rd party process.

For the interop calls, while you could be more specific, you really want to catch all HRESULT-based exceptions, but some HRESULT-based exceptions get translated. So being specific requires a lot of research to cover exactly the set of HRESULT-exceptions that can get translated.

If you want to improve this you have to

move the deserialization try-catch to where its really needed (i.e. where the deserialization happens)

make it TryGetDataFromHGLOBAL so it can report failures without throwing arbitrary exceptions

stop using IDataObject from corefx, make your own declaration which returns HRESULT explicitely so it doesn't get translated to a thrown exception when you don't want it to

Certainly not something I'd want to be doing in a bugfix PR. If you think its worth to improve the locality of the catch-all to just the deserialization (can't get rid of it there) we should create a separate issue.

src/System.Windows.Forms/src/System/Windows/Forms/DataObject.cs

weltkante

Added unit test which repros the faulting scenario without having to copy/paste a metafile from Office

src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/DataObjectTests.cs

weltkante · 2020-04-07T19:17:26Z

src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/DataObjectTests.cs

+            // even when asked for a stream. This used to crash the process when DataObject interpreted the
+            // handle as a pointer to a COM IStream without checking the storage medium it retrieved.
+
+            Assert.Null(data.GetData(DataFormats.EnhancedMetafile));


This will crash the runtime without the fix, is the CI unit test infrastructure up to the task to detect crashing as test failures? When running the test inside VS without fix it is not detected as test failure, it just looks like the test never ran even though I explicitly told it to run the test.

weltkante · 2020-04-07T19:25:05Z

Since the remaining changes for #1268 are supposed to go into a separate PR I think this is now ready. Diff is slightly easier to read when ignoring whitespace changes due to try/finally indentation.

src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/DataObjectTests.cs

RussKie

🤯 👍

src/System.Windows.Forms.Primitives/src/Interop/Ole32/Interop.ReleaseStgMedium.cs

src/System.Windows.Forms/src/System/Windows/Forms/DataObject.cs

src/System.Windows.Forms.Primitives/src/Interop/Ole32/Interop.ReleaseStgMedium.cs

JeremyKuhne · 2020-05-11T20:45:59Z

Looks good. I'd love to see this code thoroughly cleaned up in the future. There are efficiency issues and we're not being particularly robust/safe.

RussKie · 2020-05-12T22:58:56Z

@weltkante could you please resolve the MC.

JeremyKuhne · 2020-05-13T00:31:41Z

To be clear- I'm ok with this modulo a sanity check after the conflict resolution.

…rop constants

weltkante · 2020-05-13T08:31:54Z

rebased, original is here for comparison purposes; unless I messed up the rebase there are no additional changes except moving diffs into separated files

RussKie · 2020-05-19T07:17:25Z

Thank you

weltkante requested a review from a team as a code owner April 5, 2020 14:42

ghost assigned weltkante Apr 5, 2020

weltkante commented Apr 5, 2020

View reviewed changes

weltkante mentioned this pull request Apr 5, 2020

Clipboard.ContainsImage inconsistent with Clipboard.GetImage #1268

Closed

asklar reviewed Apr 6, 2020

View reviewed changes

src/System.Windows.Forms/src/System/Windows/Forms/DataObject.cs Outdated Show resolved Hide resolved

hughbe reviewed Apr 6, 2020

View reviewed changes

hughbe reviewed Apr 7, 2020

View reviewed changes

src/System.Windows.Forms/src/System/Windows/Forms/DataObject.cs Outdated Show resolved Hide resolved

weltkante commented Apr 7, 2020

View reviewed changes

weltkante changed the title ~~WIP: fix DataObject.GetData issues~~ Fix DataObject.GetData issues Apr 7, 2020

merriemcgaw requested a review from JeremyKuhne April 16, 2020 23:59

RussKie reviewed Apr 17, 2020

View reviewed changes

src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/DataObjectTests.cs Outdated Show resolved Hide resolved

src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/DataObjectTests.cs Outdated Show resolved Hide resolved

RussKie added the waiting-review This item is waiting on review by one or more members of team label Apr 17, 2020

RussKie reviewed May 7, 2020

View reviewed changes

RussKie closed this May 7, 2020

RussKie reopened this May 7, 2020

JeremyKuhne reviewed May 11, 2020

View reviewed changes

src/System.Windows.Forms.Primitives/src/Interop/Ole32/Interop.ReleaseStgMedium.cs Outdated Show resolved Hide resolved

RussKie added the 📭 waiting-author-feedback The team requires more information from the author label May 12, 2020

weltkante added 5 commits May 13, 2020 10:27

Use ReleaseStgMedium to clean up shared resources

93cdd83

PR feedback

a63e2bb

Unit test for bugfix

85d1171

reference System.Windows.Forms.Primitives instead of duplicating inte…

4ba7901

…rop constants

move private nested class to end of containing class

c3d9df8

weltkante force-pushed the DataObject branch from 60830c2 to c3d9df8 Compare May 13, 2020 08:29

ghost removed the 📭 waiting-author-feedback The team requires more information from the author label May 13, 2020

JeremyKuhne approved these changes May 13, 2020

View reviewed changes

RussKie removed the waiting-review This item is waiting on review by one or more members of team label May 19, 2020

RussKie merged commit 1d17041 into dotnet:master May 19, 2020

ghost added this to the 5.0 Preview6 milestone May 19, 2020

weltkante deleted the DataObject branch July 12, 2020 17:42

ghost locked as resolved and limited conversation to collaborators Feb 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix DataObject.GetData issues #3039

Fix DataObject.GetData issues #3039

weltkante commented Apr 5, 2020 •

edited

Loading

weltkante left a comment •

edited

Loading

weltkante commented Apr 5, 2020 •

edited

Loading

hughbe Apr 6, 2020

weltkante Apr 6, 2020 •

edited

Loading

weltkante left a comment

weltkante Apr 7, 2020

weltkante commented Apr 7, 2020 •

edited

Loading

RussKie left a comment

JeremyKuhne commented May 11, 2020

RussKie commented May 12, 2020

JeremyKuhne commented May 13, 2020

weltkante commented May 13, 2020 •

edited

Loading

RussKie commented May 19, 2020

		@@ -1414,6 +1416,10 @@ private object GetDataFromOleHGLOBAL(string format, out bool done)
		catch

Fix DataObject.GetData issues #3039

Fix DataObject.GetData issues #3039

Conversation

weltkante commented Apr 5, 2020 • edited Loading

Proposed changes

Customer Impact

Regression?

Risk

Test methodology

Microsoft Reviewers: Open in CodeFlow

weltkante left a comment • edited Loading

Choose a reason for hiding this comment

weltkante commented Apr 5, 2020 • edited Loading

hughbe Apr 6, 2020

Choose a reason for hiding this comment

weltkante Apr 6, 2020 • edited Loading

Choose a reason for hiding this comment

weltkante left a comment

Choose a reason for hiding this comment

weltkante Apr 7, 2020

Choose a reason for hiding this comment

weltkante commented Apr 7, 2020 • edited Loading

RussKie left a comment

Choose a reason for hiding this comment

JeremyKuhne commented May 11, 2020

RussKie commented May 12, 2020

JeremyKuhne commented May 13, 2020

weltkante commented May 13, 2020 • edited Loading

RussKie commented May 19, 2020

weltkante commented Apr 5, 2020 •

edited

Loading

weltkante left a comment •

edited

Loading

weltkante commented Apr 5, 2020 •

edited

Loading

weltkante Apr 6, 2020 •

edited

Loading

weltkante commented Apr 7, 2020 •

edited

Loading

weltkante commented May 13, 2020 •

edited

Loading