GetModuleHandle shouldn't return FreeLibrarySafeHandle

[HavenDV]

Actual behavior

GetModuleHandle returns FreeLibrarySafeHandle. Which will call FreeLibrary on cleanup. But, according to the documentation, this is not necessary to avoid errors:

The GetModuleHandle function returns a handle to a mapped module without incrementing its reference count. However, if this handle is passed to the FreeLibrary function, the reference count of the mapped module will be decremented. Therefore, do not pass a handle returned by GetModuleHandle to the FreeLibrary function. Doing so can cause a DLL module to be unmapped prematurely.
https://docs.microsoft.com/en-us/windows/win32/api/libloaderapi/nf-libloaderapi-getmodulehandlew#remarks

GetModuleHandleEx partly also affected by this problem, because the cleanup behavior depends on the arguments passed
https://docs.microsoft.com/en-us/windows/win32/api/libloaderapi/nf-libloaderapi-getmodulehandleexw#parameters

Expected behavior

A clear and concise description of what you expected to happen.

Repro steps

1. NativeMethods.txt content:

GetModuleHandle

2. NativeMethods.json content (if present):

3. Any of your own code that should be shared?

Context

• CsWin32 version: [e.g. 0.1.635-beta]
• Win32Metadata version (if explicitly set by project):
• Target Framework: [e.g. netstandard2.0]
• LangVersion (if explicitly set by project): [e.g. 9]

[AArnott]

Great feedback, thank you. Moving to the metadata repo so we can fix it at the right place for all projections.

[elachlan]

@mikebattista This issue also affects winforms.

[mikebattista]

What is the specific change in the metadata? It currently returns an HINSTANCE which has RAIIFree("FreeLibrary"). Do you expect a different return type that has no RAIIFree? What about for the -Ex API where the behavior is dependent on the arguments?

[elachlan]

For CsWin32, I guess if the meta data flagged GetModuleHandle with an attribute to exclude the RAIIFree, then we can code for it. Otherwise, the only other way to do it is to have a different return type. I am definitely not an expert in this area though.

GetModuleHandleEx is even harder. Because its only when it uses certain parameters that it needs to call FreeLibrary. We will need to add conditional generation around that to handle it.

@AArnott knows better than myself.

[AArnott]

I feel like this has come up before, and if so we may already have a way to express the simple case in metadata. I think GetModuleHandle should still return an HINSTANCE so that type equivalence is there, but a [return: NoFree] attribute on the method could indicate that the caller doesn't own the returned value and thus doesn't need to call the RAIIFree method. In CsWin32 we would interpret this and create a SafeHandle where ownsHandle: false to avoid the FreeLibrary call.

For GetModuleHandleEx, we may just have to assume it must be released. Methods whose return values vary by argument can be arbitrary complex and I don't know that we want to try to represent them all in metadata.

[mikebattista]

I feel like this has come up before, and if so we may already have a way to express the simple case in metadata. I think GetModuleHandle should still return an HINSTANCE so that type equivalence is there, but a [return: NoFree] attribute on the method could indicate that the caller doesn't own the returned value and thus doesn't need to call the RAIIFree method. In CsWin32 we would interpret this and create a SafeHandle where ownsHandle: false to avoid the FreeLibrary call.

Do we use [DoNotRelease] for this? I see that applied to GetCurrentProcess and GetProcessHeap's return handles.

For GetModuleHandleEx, we may just have to assume it must be released. Methods whose return values vary by argument can be arbitrary complex and I don't know that we want to try to represent them all in metadata.

I will leave it as is.

[elachlan]

https://github.com/microsoft/win32metadata/blob/42ce1543a9d83a1a6cc03c71e3ca84d8854a1c01/generation/WinSDK/emitter.settings.rsp#L491-L492

They both have [DoNotRelease], Do we handle for that in CsWin32?

[mikebattista]

Assigning to @AArnott to resolve whether this is a metadata issue or C#/Win32 issue. It would seem that [DoNotRelease] is properly applied for this scenario and C#/Win32 just needs to honor it.

[elachlan]

We need something like this, maybe in DeclareFriendlyOverloads?

internal const string DoNotReleaseAttribute = "DoNotReleaseAttribute";
bool doNotRelease = false;
foreach (CustomAttributeHandle attHandle in this.GetReturnTypeCustomAttributes(methodDefinition) ?? Enumerable.Empty<CustomAttributeHandle>())
{
    CustomAttribute att = this.Reader.GetCustomAttribute(attHandle);
    if (this.IsAttribute(att, InteropDecorationNamespace, DoNotReleaseAttribute))
    {
        doNotRelease = true;
        break;
    }
}

[AArnott]

Wanna take this, @elachlan? We might have a helper method that does the loop-search for you.

[elachlan]

I did implement it and it worked. But not exactly how I'd hoped. Since there was still a friendly overloads, when it probably shouldn't be generated at all.

Also if we implement IDisposable on the structs(#596), then we are pushing the problem down the line. I am not sure how to fix that. Maybe wrap the return type?

[AArnott]

The friendly overload should still return a SafeHandle, since all the other friendly APIs we generate will take a SafeHandle as an argument. But the SafeHandle should be created, passing in ownsHandle: false to the constructor.

[elachlan]

ahhhh, thanks. I should be able to get it sorted in the next few days in my free time.