pinvoke
pinvoke copied to clipboard
dotnet
Review and improve our Handle and SafeHandle policies
I understand the need and meaning of SafeHandles as the ones that we need to Close/Destroy after use in order to avoid memory leaks and other memory and referencing problems. They are also good for us to keep people using the right handles on the right functions as they will have more trouble to use it improperly.
This kind of type safety could be used on all other handle referencing APIs, specially if the Windows original API do have a specialized handle type for that. HMONITOR is an example of a handle that does not need to be disposed, as a matter of fact there is not even a way to Close/Destroy them on the public API. We could introduce NotSoSafeHandles to ensure type safety on these calls instead of dealing with IntPtr wich is a big risk of having someone sending wrong values to the APIs.
Also there is a very clever/nice type (HandleRef and an usage sample on MSDN) that can bundle managed types with unmanaged handles so that GC does not clean one of them at one time and the other later, which might be a problem if the unmanaged function/API tries to use it to point to something that was already destroyed on the managed world.
We also have some SafeHandles on Microsoft.Win32.SafeHandles Namespace that we could use, like:
- SafeAccessTokenHandle: Provides a safe handle to a Windows thread or process access token. For more information see Access Tokens
- SafeFileHandle: Represents a wrapper class for a file handle.
- SafeMemoryMappedFileHandle: Provides a safe handle that represents a memory-mapped file for sequential access.
- SafeMemoryMappedViewHandle: Provides a safe handle that represents a view of a block of unmanaged memory for random access.
- SafePipeHandle: Represents a wrapper class for a pipe handle.
- SafeProcessHandle: Provides a managed wrapper for a process handle.
- SafeRegistryHandle: Represents a safe handle to the Windows registry.
- SafeWaitHandle: Represents a wrapper class for a wait handle.
A little more references for information:
One of the things to note on Microsoft.Win32.SafeHandles is that it's desktop only for now. The current choice was to re-implement them to keep the same code for both portable and Desktop versions of the SafeHandle.
Regarding NotSoSafeHandles it's a good idea I think and really simple to implement:
public static class User32
{
[DllImport("user32.dll", SetLastError = false)]
public static extern HWND GetDesktopWindow();
}
[StructLayout(LayoutKind.Sequential)]
public struct HWND
{
public readonly IntPtr Hwnd;
public HWND(IntPtr hwnd)
{
Hwnd = hwnd;
}
public override string ToString() => Hwnd.ToString();
}
Compatibility wise it's a bigger problem :wink:
BTW I love how the HandleRef sample brushes over the problem with OVERLAPPED they propose 2 different solution that won't work in the async case :) Asychronous APIs tend to disagree strongly when you release memory they will write to when the operation complete :boom: :boom: :boom:
Thanks for the thoughts on this. I think we can develop it into a good solution too. I'd suggest we wait and do it all at once when we're ready to make a breaking change release.
@vbfox, if you don't mind, could you explain a little more the problem you see on the Overlapped solution with Async APIs and, if you do have have an opinion on that, what are the proper approaches to that?
they propose 2 different solution that won't work in the async case :) Asychronous APIs tend to disagree strongly when you release memory they will write to when the operation complete.
See also: https://social.msdn.microsoft.com/Forums/vstudio/en-US/a170a15f-aa4e-4c1b-ad2a-c96259bc3948/pinvoke-signatures-and-intptr-vs-handleref-vs-safehandle?forum=csharpgeneral to consider various options.
@vbfox, if you don't mind, could you explain a little more the problem you see on the Overlapped solution with Async APIs and, if you do have have an opinion on that, what are the proper approaches to that?
The overlapped pointer that is passed IN when the asynchronous operation begin and is kept by the OS and when the asynchronous operation finishes or update, the memory pointed is changed. You can see it for example in the way the HasOverlappedIoCompleted works (It's also a pretty well documented behavior, i'm just too lazy to search MSDN) :
#define HasOverlappedIoCompleted(lpOverlapped) (((DWORD)(lpOverlapped)->Internal) != STATUS_PENDING)
The result is that of the 2 examples one can't work and the other is dangerous:
- In the case of a "ref struct" we let the OS keep a pointer to part of the stack, that mean that we can't return from the calling function until the async method is finished. Otherwise the OS will at some later point write into our stack and corrupt it. So it will "work" but it's not much of an async method if we can't return from it.
- In the case of a class the marshaller allocate memory, copy the content, call the method, copy back the values from the temporary memory and then free that memory. So it will free memory that the OS expects to write to... Never a good idea.
The proper approach is to handle memory by hand either using pointers or IntPtr. The fact is that the memory blob that represent the OVERLAPPED structure must not move and not be freed until the asynchronous operation is marked as finished by the OS.
@vbfox: did you add a comment to the MSDN topic explaining its defect? They'll sometimes correct samples as a result of such feedback.
@arlm My suggestion in #302 is to have a base class which can do nothing in ReleaseHandle method. This adds support for functions like GetCurrentProcess and others such as your example of HMONITOR.
I can see a couple of solutions that haven't been mentioned (possibly because they are terrible). In no particular order:
I'm unclear on why using the standard SafeHandle base class isn't sufficient in the case of arguments. It seems like you could eliminate some boilerplate conversions by returning a SafeHandle-derived handle of a specific type, but allowing any kind of safe handle as input (as a general rule).
You could theoretically change the library structure so that the primary access point is a singleton instance of a public interface rather than a static class. That would add a bit of overhead, but it would let people define additional PInvoke signatures and attach them to the instance with pass-through extension methods. You could reorganize the whole thing into a toolkit with struct and enum definitions, and a set of pre-made implementation librarys. Big change, not pushing it, just an idea.
You could also provide a compatibility layer package that provides interop between types that exist both in this library and in the standard library. This would cover at least safe handles and Overlapped (though from my experimenting the OVERLAPPED/NativeOverlapped issue can be solved with a pointer cast).
I'm unclear on why using the standard SafeHandle base class isn't sufficient in the case of arguments. It seems like you could eliminate some boilerplate conversions by returning a SafeHandle-derived handle of a specific type, but allowing any kind of safe handle as input (as a general rule).
Not a bad idea.
You could theoretically change the library structure so that the primary access point is a singleton instance of a public interface rather than a static class.
This possibility was discussed fairly exhaustively in #55