LoadResource generates an unexpected GlobalFreeSafeHandle

[sharwell]

Actual behavior

internal static unsafe GlobalFreeSafeHandle LoadResource(SafeHandle hModule, winmdroot.Foundation.HRSRC hResInfo)
{
  // ...
  return new GlobalFreeSafeHandle(__result, ownsHandle: true);
}

Expected behavior

One of the following:

1. Return a type which is not GlobalFreeSafeHandle
2. Construct the GlobalFreeSafeHandle with ownsHandle: false

Repro steps

1. NativeMethods.txt content:

LoadResource

3. NativeMethods.json content (if present):

{
  "$schema": "https://aka.ms/CsWin32.schema.json",
  "className": "PInvoke2"
}

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

Context

• CsWin32 version: 0.3.18-beta
• Win32Metadata version (if explicitly set by project):
• Target Framework: netstandard2.0
• LangVersion (if explicitly set by project): preview

[AArnott]

The documentation for this function explicitly calls out that the return value should not be passed to GlobalFree.

I believe the metadata may have a way to attribute a particular return value with special release handling (in this case, none at all.) I'll move this to the win32metadata repo so we can apply an attribute to this particular method's return value and go from there.

[mikebattista]

We have a [DoNotRelease] attribute. Should we apply it to the return value here?

[mikebattista]

We also have [ReturnsUnownedHandle].

[KalleOlaviNiemitalo]

Can you define a wholly new handle type for LoadResource and LockResource? To minimise confusion if the HGLOBAL used in the headers is not actually valid for any other functions.

[mikebattista]

@kennykerr

Generally we try to avoid changing the type from what's declared in the headers. HGLOBAL is true to the headers.

[riverar]

I agree with annotating the LoadResource retval with [DoNotRelease]. [ReturnsUnownedHandle] is only used on one function (GetClipboardData), so it may be worth consolidating and deleting that attribute, preferring [DoNotRelease] instead.

(For those curious, LoadResource returns a pointer to the requested resource in the module. Subsequently, LockResource does nothing on current Windows targets.)

[AArnott]

I'm curious about the semantic difference between the two attributes myself. If there really isn't one, I support consolidating as @riverar proposed.

[mikebattista]

[ReturnsUnownedHandle] was discussed at #792.

[riverar]

Reviewing that discussion, it seems to confirm [ReturnsUnownedHandle] was intended to drive whether or not projections will involve RAII semantics (e.g. C# and SafeHandle). I'd imagine the same would occur with [DoNotRelease] or am I missing something?

[mikebattista]

Is there a scenario where you would own a handle but still shouldn't free it? If the semantic distinction is important, then the two attributes make sense. Otherwise, we can consolidate on one.

[AArnott]

No, I can't think of any case where we need distinct concepts for ownership and the need to release a handle.

[riverar]

Agreed.