1

I just recently learned about SafeHandle and, for a test, I implemented it for the SDL2 library, creating and destroying a window:

[DllImport(_libName, CallingConvention = CallingConvention.Cdecl)]
internal static extern IntPtr SDL_CreateWindow(
    [MarshalAs(UnmanagedType.LPStr)] string title, int x, int y, int w, int h, uint flags);

[DllImport(_libName, CallingConvention = CallingConvention.Cdecl)]
internal static extern void SDL_DestroyWindow(IntPtr window);

public class Window : SafeHandleZeroOrMinusOneIsInvalid
{
    public Window() : base(true)
    {
        SetHandle(SDL_CreateWindow("Hello", 400, 400, 800, 600, 0));
    }

    protected override bool ReleaseHandle()
    {
        SDL_DestroyWindow(handle);
        return true;
    }
}

This works fine, then I learned of another advantage of using SafeHandle: The possibility to use the class in the p/invoke signature directly, like so:

[DllImport(_libName, CallingConvention = CallingConvention.Cdecl)]
internal static extern Window SDL_CreateWindow(
    [MarshalAs(UnmanagedType.LPStr)] string title, int x, int y, int w, int h, uint flags);

[DllImport(_libName, CallingConvention = CallingConvention.Cdecl)]
internal static extern void SDL_DestroyWindow(Window window);

This is of course much better than generic IntPtr parameters / returns, because I have type safety passing / retrieving actual Window (handles) to / from those methods.

While this works for SDL_CreateWindow, which correctly returns a Window instance now, it doesn't work for SDL_DestroyWindow, which is called by me inside of Window.ReleaseHandle like this:

public Window() : base(true)
{
    SetHandle(SDL_CreateWindow("Hello", 400, 400, 800, 600, 0).handle);
}

protected override bool ReleaseHandle()
{
    SDL_DestroyWindow(this);
    return true;
}

When trying to pass this to SDL_DestroyWindow, I get an ObjectDisposedException: Safe handle has been closed. Indeed the IsClosed property is true, which I didn't expect to be at this time. Apparently it internally tries to increase a reference count, but notices IsClosed is true. According to the documentation, it has been set to true because "The Dispose method or Close method was called and there are no references to the SafeHandle object on other threads.", so I guess Dispose was implicitly called before in the call stack to invoke my ReleaseHandle.

ReleaseHandle apparently isn't the correct place to clean up if I want to use a class parameter in the p/invoke signature, so I wonder if there is any method where I could clean up without breaking SafeHandle internals?

Ray
  • 7,940
  • 7
  • 58
  • 90
  • You should not change `IntPtr` to `Window` in `SDL_DestroyWindow`. – user4003407 Dec 22 '17 at 00:28
  • Yes, but then I lose type safety. My question is if it is still possible to clean up without losing the specific signature. – Ray Dec 22 '17 at 00:42
  • 2
    What do you mean here by "type safety"? In ReleaseHandle you're supposed to use the protected member `handle`, not `this`, so the p/invoke method that does release the handle must use an IntPtr argument. This is a very last call, doc says *The garbage collector calls ReleaseHandle after normal finalizers have been run for objects that were garbage collected at the same time* where you're not supposed to fail or do fancy things – Simon Mourier Dec 22 '17 at 06:08
  • With that I meant the "type safety" I can achieve by using the class rather than IntPtr in the signature (an IntPtr could point to structs which don't make sense for the argument, and the SafeHandle based class ensured I use a pointer to the correct opaque type). Anyway, as you confirmed, this "feature" of using a SafeHandle class does really not seem to work for clean up calls, which I thought would be part of the "advantage" of using such. I also realized a serious issue in my code above, so if you want I can answer with pointing it out, or accept your comment as an answer. – Ray Dec 22 '17 at 07:56
  • Microsoft meant something *radically* different with the word "safe" in SafeHandle. Nothing to do with type safety. This is about the handle not becoming invalid while an interop call is executing, not crashing the program when the finalizer fails and not being exploitable with a handle recycle attack. It is only truly useful on reference-counted handles, so not a window handle. – Hans Passant Dec 24 '17 at 01:10

1 Answers1

2

My question above is slightly misguided by wrong information I learned about SafeHandle (via some blog posts I won't mention). While I was told that replacing IntPtr parameters in P/Invoke methods with class instances is "the main advantage provided by SafeHandle" and definitely nice, it turns out to be only partly useful:

Careful with automatic SafeHandle creation by the marshaller

For one, I say this because my code above has a big issue I didn't see at first. I have written this code:

void DoStuff()
{
    Window window = new Window();
}

public class Window : SafeHandleZeroOrMinusOneIsInvalid
{
    public Window() : base(true)
    {
        // SDL_CreateWindow will create another `Window` instance internally!!
        SetHandle(SDL_CreateWindow("Hello", 400, 400, 800, 600, 0).handle);
    }

    protected override bool ReleaseHandle()
    {
        SDL_DestroyWindow(handle); // Since "this" won't work here (s. below)
        return true;
    }

    // Returns Window instance rather than IntPtr via the automatic SafeHandle creation
    [DllImport(_libName, CallingConvention = CallingConvention.Cdecl)]
    private static extern Window SDL_CreateWindow(
        [MarshalAs(UnmanagedType.LPStr)] string title, int x, int y, int w, int h, uint flags);

    // Accept Window instance rather than IntPtr (won't work out, s. below)
    [DllImport(_libName, CallingConvention = CallingConvention.Cdecl)]
    private static extern void SDL_DestroyWindow(Window window);
}

When the marshaller calls the P/Invoke method of SDL_CreateWindow in the Window constructor, it internally creates another instance of the Window class for the return value (calling the required parameterless constructor and then setting the handle member internally). This means I now have two SafeHandle instances:

  • one returned by the SDL_CreateWindow method - which I don't use anywhere (only stripping out the handle property)
  • one created by my user code calling new Window(), the SafeHandle class itself

The only correct way to implement the SafeHandle here is to let SDL_CreateWindow return an IntPtr again, so no internal marshalling SafeHandle instances are created anymore.

Can't pass SafeHandle around in ReleaseHandle

As Simon Mourier explained / cited in the comments, the SafeHandle itself cannot be used at all anymore when cleaning up in ReleaseHandle, since the object is garbage collected and trying to do "fancy" things like passing it to a P/Invoke method is not safe anymore / doomed to fail. (Given I were told that the replacement of IntPtr parameters in the P/Invoke is one of "the main features" of SafeHandle, it surprised me first that this is not supported and deemed "fancy"). That is also why the ObjectDisposedException I receive is very justified.

I can still access the handle property here, but then, again, my P/Invoke method no longer accepts a Window instance, but the "classic" IntPtr.

Am I better off with IntPtr for P/invoke parameters again?

I'd say so, my final implementation looks like this and solves the above two problems while still using the advantages of SafeHandle, just without the fancy P/Invoke argument replacements. And as an extra feature, I can still connote the IntPtr parameters to "accept" an SDL_Window (the native type pointed to) with a using alias.

using SDL_Window = System.IntPtr;

public class Window : SafeHandleZeroOrMinusOneIsInvalid
{
    private Window(IntPtr handle) : base(true)
    {
        SetHandle(handle);
    }

    public Window() : this(SDL_CreateWindow("Hello", 400, 400, 800, 600, 0)) { }

    protected override bool ReleaseHandle()
    {
        SDL_DestroyWindow(handle);
        return true;
    }

    [DllImport(_libName, CallingConvention = CallingConvention.Cdecl)]
    private static extern SDL_Window SDL_CreateWindow(
        [MarshalAs(UnmanagedType.LPStr)] string title, int x, int y, int w, int h, uint flags);

    [DllImport(_libName, CallingConvention = CallingConvention.Cdecl)]
    private static extern void SDL_DestroyWindow(SDL_Window window);
}
Ray
  • 7,940
  • 7
  • 58
  • 90
  • I know this is an old question, but I just wanted to reiterate that this is really odd behavior. Feels like a super obvious thing to be able to pass it to the release function as part of the final release. – William Oct 09 '19 at 22:24