Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add an analyzer to catch inadvertent ephemeral delegates #1240

Open
asklar opened this issue Jul 16, 2024 · 4 comments
Open

Add an analyzer to catch inadvertent ephemeral delegates #1240

asklar opened this issue Jul 16, 2024 · 4 comments

Comments

@asklar
Copy link
Member

asklar commented Jul 16, 2024

There are several Win32 APIs that take callbacks, e.g. WNDPROCs or DLGPROCs. These are currently projected as delegates, and if you do the "natural thing" and assign static methods to fields of these *PROC types, the runtime will create a temporary delegate, which gets garbage collected from under you, and you will start getting ExecutionEngineExceptions at random.
These are hard to debug (they don't hint at what went wrong), so they constitute a dangerous sharp edge.

cswin32 should either have a build-time warning, or an analyzer, that validates that you are not falling into the trap of doing the natural-yet-unsafe way of assigning these callbacks.

An alternative is to change the projection of structs taking callbacks to always use function pointers instead of delegates.

See #623 (comment)

@AArnott
Copy link
Member

AArnott commented Jul 16, 2024

Thanks for filing this. This has come up from two others in the last week besides you, yet before that I hardly heard about it at all. Funny how perfect storms come up like that.

I am curious about your failure. In studying code with others, it seems that modern C# compilers actually store a delegate over a static method in a static field just as an optimization, but with the nice side effect of avoiding the very problem you're seeing. Can you confirm by looking at the compiled IL whether a field for the delegate isn't implicitly created?

Here is a sample of the delegate being cached.

@asklar
Copy link
Member Author

asklar commented Jul 16, 2024

looking through the IL dump, I see that it just creates an instance of WNDPROC which is a delegate, no field is created for the delegate.

WNDCLASSEXW param = new WNDCLASSEXW
{
	cbSize = (uint)Marshal.SizeOf<WNDCLASSEXW>(),
	hInstance = PInvoke.GetModuleHandle((PCWSTR)null),
	lpszClassName = ptr,
	lpfnWndProc = WndProc
};
IL_004c: ldftn valuetype Windows.Win32.Foundation.LRESULT [redacted]::WndProc(valuetype Windows.Win32.Foundation.HWND, uint32, valuetype Windows.Win32.Foundation.WPARAM, valuetype Windows.Win32.Foundation.LPARAM)
IL_0052: newobj instance void Windows.Win32.UI.WindowsAndMessaging.WNDPROC::.ctor(object, native int)
IL_0057: stfld class Windows.Win32.UI.WindowsAndMessaging.WNDPROC Windows.Win32.UI.WindowsAndMessaging.WNDCLASSEXW::lpfnWndProc

@AArnott
Copy link
Member

AArnott commented Jul 16, 2024

I see the same. Switching from method group to lambda syntax triggers the C# compiler optimization that I expected to see here.

Implicit delegate creation is still not a problem so long as the p/invoke doesn't store the function pointer beyond its own lifetime. In your particular case of storing it in a field, that is the problematic storage, by definition because the struct is used for interop rather than permanent storage. So ironically the analyzer would be noticing you store a new delegate into a field, but that the field is on an interop struct, and I guess the message would advise that you also store the delegate elsewhere and then assign from there. It wouldn't catch every use of an implicit delegate (since they can appear as direct arguments to methods), and some would be false positives. So it's not going to have a high hit rate, which makes me hesitant to write it in the first place.

But we can leave the issue active to track the design thoughts on this, in case I or other have a chance to write the analyzer.

@AArnott AArnott changed the title Reduce sharp edges with APIs that take callbacks Add an analyzer to catch inadvertent ephemeral delegates Jul 16, 2024
@AArnott
Copy link
Member

AArnott commented Jul 16, 2024

We could potentially trigger the analyzer when the ephemeral delegate is used to initialize an argument or struct field that is annotated with the [Retained] attribute (which is described at microsoft/win32metadata#1726). We'd need to work with the win32metadata repo to get the appropriate places attributed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants