GCHandle.Free should raise errors on misuse

[gregg-miskelly]

I own some code that that uses GCHandle which runs in a large managed application that runs lots of code in a single process. We face issues with this code occasionally crashing on users machines because something in the process has freed our handle value, so that when we attempt to obtain our object back we fetch either no object, or the wrong object. Playing around with GCHandle I found that part of the problem is that GCHandle raises no error on API misuse, making it hard for folks using GCHandle to notice that they have a bug.

Request: GCHandle should raise an exception or an MDA if there is an attempt to either call GCHandle.FromIntPtr or GCHandle.Free using a handle value that has ready been free.

Example code:

using System;
using System.Runtime.InteropServices;

namespace GCHandleFreeTest
{
    class Program
    {
        static void Main(string[] args)
        {
            IntPtr handleValue;
            {
                GCHandle handle = GCHandle.Alloc(new MyObject());
                handleValue = GCHandle.ToIntPtr(handle);
            }

            {
                GCHandle handle = GCHandle.FromIntPtr(handleValue);
                handle.Free();
            }

            {
                GCHandle handle = GCHandle.FromIntPtr(handleValue);
                handle.Free();
            }
        }
    }

    class MyObject
    { };
}

[jkotas]

GCHandle.Alloc/Free is a variant of classic malloc/free. I do not think we would want to pay for the protection against incorrect use by default, for the same reasons classic malloc/free is not protected against incorrect use by default - it comes with performance penalty. We can consider making this diagnostic opt-in.

Note that you can turn on ETW events for handle allocation / deallocation. It should allow you to trace down the something that is freeing the handle incorrectly pretty quickly.

[jkotas]

.NET Framework has invalidGCHandleCookie MDA that tried to solve this problem, but it had number of issues: changes meaning of GCHandle IntPtr and it has significant performance cost, even when it is turned off. VS does not ever offer it in the UI. We would want to come up with a different design for this that does not have these issues.

[gregg-miskelly]

One note: one important difference, at least on, Windows, for malloc/free vs. GCHandle is that the most trivial case --malloc+free+free -- will break on the attempt to free already-freed memory for heap but not for GCHandle.

[AaronRobinsonMSFT]

@karpinsn This isn't something we are likely to do, but the VS memory tool should be able to listen to the ETW events mentioned above and automate the tracking. Any chance you could help @gregg-miskelly out with a feature in the memory tool?

[AaronRobinsonMSFT]

Reference for the destroy event:

https://github.com/dotnet/runtime/blob/8640eed087d0155fbd76c2ded706b2599e06410a/src/coreclr/src/vm/ClrEtwAll.man#L2993-L2997

https://github.com/dotnet/runtime/blob/8640eed087d0155fbd76c2ded706b2599e06410a/src/coreclr/src/vm/ClrEtwAll.man#L1284-L1293

[karpinsn]

We could add something to the .NET Object Allocation tool for this. I assume we would just need to listen for SetGCHandle events to register an active handler and then watch for DestroyGCHandle to unregister it? Then if we get double freed we would show some event in the tool?

[AaronRobinsonMSFT]

@karpinsn Yep. The "set" is indicating the OBJECTHANDLE now has an object associated with it and can be accessed through the handle. OBJECTHANDLEs are reused containers so you can think of the "set" as a create. The DestroyGCHandle is putting the OBJECTHANDLE back into the queue. Making sure you had both would be required to avoid false positives for failure.

Lots of statistics possible here involving outstanding handle count and even the average duration a handle is used. Creating lots of handles does put additional pressure on the GC so there is value in knowing that too. Creating the tracking and lifetime management is probably the best external customer feature, but for performance people like @stephentoub knowing how many handles and how long they are used in a scenario might be useful.

https://github.com/dotnet/runtime/blob/8640eed087d0155fbd76c2ded706b2599e06410a/src/coreclr/src/gc/handletable.cpp#L356-L389

https://github.com/dotnet/runtime/blob/8640eed087d0155fbd76c2ded706b2599e06410a/src/coreclr/src/gc/handletable.cpp#L511-L540

[dotnet-policy-service[bot]]

Due to lack of recent activity, this issue has been marked as a candidate for backlog cleanup. It will be closed if no further activity occurs within 14 more days. Any new comment (by anyone, not necessarily the author) will undo this process.

This process is part of our issue cleanup automation.