Bug 22414 - mono_marshal_free_ccw freeing ccw on object that aren't ccw but happen to have the same object hash as other ccw's
Summary: mono_marshal_free_ccw freeing ccw on object that aren't ccw but happen to hav...
Status: RESOLVED FIXED
Alias: None
Product: Runtime
Classification: Mono
Component: Interop ()
Version: 3.2.x
Hardware: PC Windows
: --- normal
Target Milestone: ---
Assignee: Bugzilla
URL:
Depends on:
Blocks:
 
Reported: 2014-08-27 12:50 UTC by Tom Hindle
Modified: 2014-08-29 16:06 UTC (History)
3 users (show)

Tags:
Is this bug a regression?: ---
Last known good build:


Attachments
Patch that demos a fix - NOT final fix. (1.30 KB, patch)
2014-08-27 14:44 UTC, Tom Hindle
Details
Patch that fixes the issue by using ptr address in the ccw_hash rather than an hash of a ptr address. (1.77 KB, patch)
2014-08-27 14:59 UTC, Tom Hindle
Details
proposed patch (1.32 KB, patch)
2014-08-27 16:30 UTC, Tom Hindle
Details


Notice (2018-05-24): bugzilla.xamarin.com is now in read-only mode.

Please join us on Visual Studio Developer Community and in the Xamarin and Mono organizations on GitHub to continue tracking issues. Bugzilla will remain available for reference in read-only mode. We will continue to work on open Bugzilla bugs, copy them to the new locations as needed for follow-up, and add the new items under Related Links.

Our sincere thanks to everyone who has contributed on this bug tracker over the years. Thanks also for your understanding as we make these adjustments and improvements for the future.


Please create a new report on GitHub or Developer Community with your current version information, steps to reproduce, and relevant error messages or log files if you are hitting an issue that looks similar to this resolved bug and you do not yet see a matching new report.

Related Links:
Status:
RESOLVED FIXED

Description Tom Hindle 2014-08-27 12:50:38 UTC
Testing with mono 3.2.8 and with sgen on a 64bit Ubuntu 14.04 Linux machine.

I suspect this may be a 64bit only issue.

In cominterop.c : mono_marshal_free_ccw

There is a block of code that looks like this:


MonoObject* handle_target = mono_gchandle_get_target (ccw_iter->gc_handle);

/* Looks like the GC NULLs the weakref handle target before running the
* finalizer. So if we get a NULL target, destroy the CCW as well. */
if (!handle_target || handle_target == object) {


However given that this can be true:

mono_object_hash(objA) == mono_object_hash(objB) when objA and objB are different MonoObject ptrs.

then its possible that handle_target can be NULL but the object ptr passed to mono_marshal_free_ccw isn't/doesn't have a ccw.

This leads to infrequent random crashes.

I don't have a sharable small test case for this, but I can reliability reproduce in less than 10 seconds by creating and forgetting 500 instances of a complex object that uses many CCW's.

I've confirmed that if I remove the check for "!handle_target" the problem does not occur, but I don't yet have a suggested fix.

Example stack trace.

#1  0x00000000005ab130 in mono_gc_run_finalize (obj=obj@entry=0x7f60af5bbb70, data=data@entry=0x0) at gc.c:192
#2  0x00000000005deb83 in mono_gc_invoke_finalizers () at sgen-gc.c:3898
#3  0x00000000005aba71 in finalizer_thread (unused=unused@entry=0x0) at gc.c:1102
Comment 1 Tom Hindle 2014-08-27 14:44:14 UTC
Created attachment 7826 [details]
Patch that demos a fix - NOT final fix.

While that patch fixes the issue, its not intended to be a final fix due to the adding of an extra field to the MonoCCW struct.
Comment 2 Tom Hindle 2014-08-27 14:59:01 UTC
Created attachment 7828 [details]
Patch that fixes the issue by using ptr address in the ccw_hash rather than an hash of a ptr address.

The problem with this fix is that it will increase the number of items in the ccw_hash.
Comment 3 Tom Hindle 2014-08-27 15:16:15 UTC
patch in attachment 7828 [details] with probably break things if HAVE_MOVING_COLLECTOR is defined.
Comment 4 Tom Hindle 2014-08-27 16:30:30 UTC
Created attachment 7838 [details]
proposed patch
Comment 5 Zoltan Varga 2014-08-29 15:20:45 UTC
Thanks for the patch. Could you state that it is licensed under the MIT/X11 license ?
Comment 6 Tom Hindle 2014-08-29 15:30:09 UTC
Yep. 

This patch is licensed under the MIT/X11.

And so is any patch that I submit to mono.
Comment 7 Zoltan Varga 2014-08-29 16:06:30 UTC
Applied in bb672ad95e5b236313e67f3f19edec7d663ceceb.