Skip to content
This repository has been archived by the owner on Jul 26, 2023. It is now read-only.

Review and improve our Handle and SafeHandle policies #272

Closed
arlm opened this issue Aug 11, 2016 · 12 comments
Closed

Review and improve our Handle and SafeHandle policies #272

arlm opened this issue Aug 11, 2016 · 12 comments
Milestone

Comments

@arlm
Copy link
Contributor

arlm commented Aug 11, 2016

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:

A little more references for information:

@vbfox
Copy link
Collaborator

vbfox commented Aug 11, 2016

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 😉

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 💥 💥 💥

@AArnott
Copy link
Collaborator

AArnott commented Aug 12, 2016

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.

@AArnott AArnott added this to the 2.0 milestone Aug 12, 2016
@arlm
Copy link
Contributor Author

arlm commented Aug 13, 2016

@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.

@vbfox
Copy link
Collaborator

vbfox commented Aug 14, 2016

@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.

@AArnott
Copy link
Collaborator

AArnott commented Aug 14, 2016

@vbfox: did you add a comment to the MSDN topic explaining its defect? They'll sometimes correct samples as a result of such feedback.

@vbfox
Copy link
Collaborator

vbfox commented Aug 14, 2016

No but i'll try.

@NN---
Copy link
Contributor

NN--- commented Jun 22, 2017

@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.

@arlm
Copy link
Contributor Author

arlm commented Jun 22, 2017

Ohh that sounds great!

@BenjaminHolland
Copy link

BenjaminHolland commented Jan 5, 2019

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).

@BenjaminHolland
Copy link

Started a discussion about the overlapped issue here: #419

@AArnott
Copy link
Collaborator

AArnott commented Jan 16, 2019

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

@AArnott AArnott closed this as completed Jul 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants