Why is adding/removing a reference to a SafeHandle "dangerous"?

[colejohnson66]

When using Win32, many P/Invoked functions take a HANDLE, which can be given from a managed SafeHandle object. However, to pass in the HANDLE safely, you need to add/remove a reference before/after invocation. Despite this, adding/removing references are considered "dangerous", as noted in their names: SafeHandle.DangerousAddRef(ref bool) and SafeHandle.DangerousRelease. I can understand SafeHandle.DangerousGetHandle() being dangerous, but not those two other ones.

Is there a reason for them being labeled as "dangerous"?

[stephentoub]

Because once you call add, the handle will be leaked unless a corresponding release call is made, and finalization won't help. And because anyone who has that reference can call release even if they didn't call add, which means code that's using the handle correctly can still experience use-after-free bugs if code elsewhere misuses that instance. The naming is just there to say "be really careful here, there be dragons".

[colejohnson66]

Would it be reasonable for the BCL to have some IDisposable scope system for adding/removing references? Something like using (safeHandle.Acquire())? Or is that best left for library authors to do themselves with extension methods?

[stephentoub]

Personally I don't think it's worth it to make such an advanced use a little bit better. The vast majority of uses is in generated code where such a wrapper would just be overhead, and while there are certainly some hand-coded uses that could benefit, when you're at the level of working with these methods directly, it's reasonably common that the use doesn't fall into that nice pattern such an IDisposable struct would enable. For example, working with two SafeHandle instances with a single try/finally, taking advantage of the ref bool argument. Or using the APIs to enable a parent/child relationship between handles. Or doing acquisition and release in two completely different code paths.

[colejohnson66]

Fair enough. My initial thinking was based on the fact that the ref bool argument will never be false. You either succeed or get ObjectDisposedException. As such, you're still forced to use a nested try/finally in case adding a reference to "handle A" succeeds, but "handle B" throws. One could abstract out the nesting of try/finally blocks with using statements. I'll defer to your judgement, however.

On the flip side, should DangerousAddRef throw ArgumentException if the ref bool is true on entry? I know Monitor.Enter does so.

[stephentoub]

As such, you're still forced to use a nested try/finally in case adding a reference to "handle A" succeeds, but "handle B" throws.

Can you elaborate? Cases like this work fine:

runtime/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicApi.NativeMethods.cs

Lines 231 to 252 in 8f3c687

	public int ConnectionStart(MsQuicSafeHandle connection, MsQuicSafeHandle configuration, ushort family, sbyte* serverName, ushort serverPort)
	{
	bool connectionSuccess = false;
	bool configurationSuccess = false;
	try
	{
	connection.DangerousAddRef(ref connectionSuccess);
	configuration.DangerousAddRef(ref configurationSuccess);
	return ApiTable->ConnectionStart(connection.QuicHandle, configuration.QuicHandle, family, serverName, serverPort);
	}
	finally
	{
	if (connectionSuccess)
	{
	connection.DangerousRelease();
	}
	if (configurationSuccess)
	{
	configuration.DangerousRelease();
	}
	}
	}

On the flip side, should DangerousAddRef throw ArgumentException if the ref bool is true on entry? I know Monitor.Enter does so.

That would be a breaking change for anyone doing the simple thing in core and happening to use true. Might have been good to do that from the get-go, but not worth it now.

[colejohnson66]

I can't elaborate about the try/finally because I had a brain fart. facepalm. I was imagining usage where the DangerousAddRef is outside the try, similar to how using statements are lowered.

bool aSuccess = false;
handleA.DangerousAddRef(ref aSuccess);
try
{
    bool bSuccess = false;
    handleB.DangerousAddRef(ref bSuccess);
    try
    {
        // ...
    }
    finally
    {
        if (bSuccess)
            handleB.DangerousRelease();
    }
}
finally
{
    if (aSuccess)
        handleA.DangerousRelease();
}