Consider using SafeHandle in Libgit2Object instead of void*

[EgorBo]

We've had a couple of bug reports against dotnet/runtime with rarely-reproduceable crashes coming from libgit2sharp. We believe that one possible reason could be an interop anti-pattern in Libgit2Object - void*/IntPtr representation of a native handle that can be freed in finalizer. E.g. in this case the finalizer in Libgit2Object may end up calling native free here (and other overloads).

The reason why it's called an anti-pattern can be explained by a short repro in this issue: dotnet/runtime#103522 and a general solution is to use SafeHandle for such handles. Also, see https://learn.microsoft.com/en-us/dotnet/standard/native-interop/best-practices

Reproduction steps

Expected behavior

Actual behavior

Version of LibGit2Sharp (release number or SHA1)

Operating system(s) tested; .NET runtime tested

[christian-clausen]

We are seeing System.ExecutionEngineException also in dotnet 9 RC2 (9.0.100-rc.2.24474.11) when running our app in debug mode.

[bording]

@EgorBo Thanks for raising the issue. I've converted Libgit2Object to derive from SafeHandleZeroOrMinusOneIsInvalid in #2127 which if I'm understanding everything properly should be enough to avoid the problem.

[jkotas]

@bording In order to benefit from SafeHandles reliability and security guarantees, the code has to AddRef/Release around all places where it operates on the underlying raw handle. It does not look like that the conversions of SafeHandle to IntPtr like https://github.com/libgit2/libgit2sharp/pull/2127/files#diff-f210a201602dfd11231de2914b00197fa6ae2dd165e88d9f602fbfc950c98082R111 do proper AddRef/Release. As far as a I can tell, the code has the same problems as before since it is not using SafeHandle correctly.

[bording]

@jkotas Can you point me to documentation that explains what I need to change? The docs didn't seem to explain anything regarding "proper usage".

[jkotas]

The red Caution box at https://learn.microsoft.com/en-us/dotnet/api/system.runtime.interopservices.safehandle.dangerousgethandle?view=net-9.0 describes the problem.

https://github.com/dotnet/runtime/blob/09b30a4c0618ff378abd523fd02e113b2474e958/src/libraries/Common/src/Interop/OSX/Interop.CoreFoundation.CFString.cs#L56-L78 is a random example of a typical use.

[bording]

@jkotas If the derived handle type usage is wrapped in a using statement, is it still also required to pair up DangerousAddRef/DangerousRelease calls as well?

For example, this method is using this implicit operator to access the underlying IntPtr, but it's called from this method which wraps the ReferenceHandle in a using statement.

You're saying that's not enough and it also needs to be wrapped in DangerousAddRef/DangerousRelease calls?

[jkotas]

If the calling method ensures lifetime like in your example, additional DangerousAddRef / DangerousRelease is not strictly required. Unless the code is perf sensitive, it is easier to do DangerousAddRef / DangerousRelease everywhere. If it is perf sensitive and you really want to avoid the extra cost DangerousAddRef / DangerousRelease, it can use a comment to make it clear that the DangerousGetHandle is safe.

Also, another common pattern is leave it to the P/Invoke marshaller to auto-generate the DangerousAddRef / DangerousRelease for you by using the SafeHandle in the P/Invoke signature. For example, change

libgit2sharp/LibGit2Sharp/Core/NativeMethods.cs

Lines 241 to 242 in a88e2a0

	internal static extern unsafe int git_branch_delete(
	git_reference* reference);

to int git_branch_delete(ReferenceHandle reference);, so that you do not have to write the DangerousAddRef / DangerousRelease manually.

[rickbrew]

I use a disposable struct and an extension method to help with this https://gist.github.com/rickbrew/0c1643b5186f8ac061be092de7a9a40a

SafeHandle handle = ...;

using (SafeHandleValue handleValue = handle.UseValue())
{
    SomeNativeMethod(handleValue.Value); // or omit .Value and use the implicit cast to `IntPtr` / `nint`
}