More reliably clean up more SafeHandle instances

[stephentoub]

This calls Dispose on SafeHandles (or things wrapping SafeHandles) that were otherwise being left for finalization:

1. Some of these are fixing finalization happening even on success paths (typically where the implementation isn't disposing of something that directly or indirectly wraps a SafeHandle).
2. Some of these are fixing finalization happening for invalid SafeHandles. Their IsInvalid is true, but they still get finalized.
3. Some of these are fixing tests to finalize less. My goal with fixing the tests was to eliminate the noise in order to find instances of the other two cases.

These were found primarily via two means:

• Debug-instrumentation in SafeHandle to log when one is finalized. This instrumentation is built into debug/checked builds of SafeHandle and requires setting the DEBUG_SAFEHANDLE_FINALIZATION environment variable to "1".
• Auditing use of SafeHandle.IsInvalid

There's a lot more that can be cleaned up using the SafeHandle instrumentation, but I'm pausing here for now. The System.IO.Pipes, System.IO.FileSystem, and System.Security.Cryptography tests are clean on Windows (with the exception of a few tests that are purposefully leaving objects for finalization to test finalization code paths).

[ghost]

Tagging subscribers to this area: @dotnet/area-system-security, @vcsjones
See info in area-owners.md if you want to be subscribed.

Issue Details

---

This calls Dispose on SafeHandles (or things wrapping SafeHandles) that were otherwise being left for finalization:

1. Some of these are fixing finalization happening even on success paths (typically where the implementation isn't disposing of something that directly or indirectly wraps a SafeHandle).
2. Some of these are fixing finalization happening for invalid SafeHandles. Their IsInvalid is true, but they still get finalized.
3. Some of these are fixing tests to finalize less. My goal with fixing the tests was to eliminate the noise in order to find instances of the other two cases.

These were found primarily via two means:

• Debug-instrumentation in SafeHandle to log when one is finalized. This instrumentation is built into debug/checked builds of SafeHandle and requires setting the DEBUG_SAFEHANDLE_FINALIZATION environment variable to "1".
• Auditing use of SafeHandle.IsInvalid

There's a lot more that can be cleaned up using the SafeHandle instrumentation, but I'm pausing here for now. The System.IO.Pipes, System.IO.FileSystem, and System.Security.Cryptography tests are clean on Windows (with the exception of a few tests that are purposefully leaving objects for finalization to test finalization code paths).

Author:	stephentoub
Assignees:	-
Labels:	area-System.Security
Milestone:	-

[jkotas]

The runtime and SafeHandle.cs changes LGTM. I have done cursory review of the rest.

[bartonjs]

LGTM, other than braceless usings in product code (which I believed we had banned in style outside of tests, but I guess not?)

[stephentoub]

other than braceless usings in product code (which I believed we had banned in style outside of tests

I don't know why we'd ban them.

[danmoseley]

Yay, glad you added this switch. Do you plan to make an "up for grabs" issue to continue, in case folks are interested? eg., with a checkbox for each library that has hits (if you have that data gathered)?

Can you imagine us getting clean - I'm guessing there are test cases for finalizers, though.

How much slower is it to run tests when the switch is flipped? I assume it's expensive to gather stacks.

[stephentoub]

How much slower is it to run tests when the switch is flipped? I assume it's expensive to gather stacks.

A lot... starting with the fact that it's only built into a debug/checked runtime, and then layering on a stack walk for every safe handle created.

Can you imagine us getting clean - I'm guessing there are test cases for finalizers, though.

Without workarounds that'd be challenging, e.g. because of tests for finalizers as you say, tests that purposefully allow resources to not be disposed to avoid race conditions, etc.

if you have that data gathered

I don't.

[danmoseley]

Makes sense -- probably we can use this flag on rare occasions, eg., having written a new feature with non trivial use of safehandles.

[bartonjs]

If you have to spin again, Steve, I'm indifferent about keeping or removing the version in

runtime/src/libraries/Common/src/Microsoft/Win32/SafeHandles/SafeX509Handles.Unix.cs

Lines 12 to 26 in 388dd6c

	#if DEBUG
	private static readonly bool s_captureTrace =
	Environment.GetEnvironmentVariable("DEBUG_SAFEX509HANDLE_FINALIZATION") != null;
	private readonly StackTrace? _stacktrace =
	s_captureTrace ? new StackTrace(fNeedFileInfo: true) : null;
	~SafeX509Handle()
	{
	if (s_captureTrace)
	{
	Console.WriteLine($"0x{handle.ToInt64():x} {_stacktrace?.ToString() ?? "no stacktrace..."}");
	}
	}
	#endif

that I left in from getting Linux PKCS7 tidy.

[stephentoub]

I'm indifferent about keeping or removing the version

Me, too. I will need to resolve a conflict, so I can remove it if you like.

[bartonjs]

Eh, if you're indifferent and I'm indifferent, let's leave it, so I can have a "smaller than the whole world" experience :)

[AndyAyersMS]

Possible regression: dotnet/perf-autofiling-issues#6973

[bartonjs]

Since CryptoConfig.CreateFromName is just a lookup in ConcurrentDictionary, reflection to find a suitable ctor, and then reflection invocation of said ctor, I don't think this change was involved.

[stephentoub]

@bartonjs, I did touch this:
https://github.com/dotnet/runtime/pull/71991/files#diff-b5e1d8d2d47fbe472dd7de0d3d5450bc58de31363bee3ae108cf295bf879b4e4R44
I guess that could have caused this (an extra non-inlineable function call, an extra generic dictionary lookup, etc.)?

That said, I'm not sure how much a few hundred nanoseconds on this particular method matters. We don't want folks using this method anyway, right?

[bartonjs]

Yeah, I had that as a suspect, but then looked at the perf test:

https://github.com/dotnet/performance/blob/main/src/benchmarks/micro/libraries/System.Security.Cryptography/Perf.CryptoConfig.cs

It goes straight to https://source.dot.net/#System.Security.Cryptography/System/Security/Cryptography/CryptoConfig.cs,2ea13b091ee4b642, not through the generic wrapper.

[stephentoub]

In that case, yeah, I'm not on the hook :)