Revert Deflater/Inflater changes around SafeHandle initialization

[stephentoub]

Deflater/Inflater's ctor calls a P/Invoke that initializes a SafeHandle. Previously this was being done to directly initialize a field, but I'd changed that months ago due to it leaving a handle for finalization. What I failed to notice, however, was that these types themselves defined finalizers, and those finalizers expected that SafeHandle field to have been initialized; now that it's not, if a rare zlib initialization error occurs (e.g. zlib couldn't be found/loaded), the finalizer may crash the process due to an unhandled null reference exception.

For Deflater, it'd be possible to just call GC.SuppressFinalize(this) inside the existing catch block that's disposing of the SafeHandle in the event of an exception. But it's more complicated for Inflater, where the SafeHandle might be recreated during the Inflater's lifetime, and thus the existing catch block is inside of a helper method that's used from more than just the ctor, and we shouldn't be suppressing finalization in that case.

So, rather than do something complicated for the small gains this provided (it was part of a much larger sweep to clean up non-disposed SafeHandles), I've just reverted these cases.

Fixes #84994
cc: @rickbrew, @dotnet/area-system-io-compression

[ghost]

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

Issue Details

---

Deflater/Inflater's ctor calls a P/Invoke that initializes a SafeHandle. Previously this was being done to directly initialize a field, but I'd changed that months ago due to it leaving a handle for finalization. What I failed to notice, however, was that these types themselves defined finalizers, and those finalizers expected that SafeHandle field to have been initialized; now that it's not, if a rare zlib initialization error occurs (e.g. zlib couldn't be found/loaded), the finalizer may crash the process due to an unhandled null reference exception.

For Deflater, it'd be possible to just call GC.SuppressFinalize(this) inside the existing catch block that's disposing of the SafeHandle in the event of an exception. But it's more complicated for Inflater, where the SafeHandle might be recreated during the Inflater's lifetime, and thus the existing catch block is inside of a helper method that's used from more than just the ctor, and we shouldn't be suppressing finalization in that case.

So, rather than do something complicated for the small gains this provided (it was part of a much larger sweep to clean up non-disposed SafeHandles), I've just reverted these cases.

Fixes #84994
cc: @rickbrew, @dotnet/area-system-io-compression

Author:	stephentoub
Assignees:	-
Labels:	area-System.IO.Compression
Milestone:	-

[rickbrew]

I'm not sure I understand how this fixes the crash ... the field is initialized differently, and the object is no longer disposed in the catch, but won't DeallocateInputBufferHandle() still bump into a null if ZLibNative.CreateZLibStreamForDeflate() has some kind of error during the constructor?

Is the SafeHandle always non-null upon returning from ZLibNative.CreateZLibStreamForDeflate() ? Even in exceptional cases like when the DLL doesn't exist? 🤔 (I don't use them all that much!)

[stephentoub]

Is the SafeHandle always non-null upon returning from ZLibNative.CreateZLibStreamForDeflate() ?

That method is:

runtime/src/libraries/Common/src/System/IO/Compression/ZLibNative.cs

Lines 349 to 354 in 9e2a565

	public static ErrorCode CreateZLibStreamForDeflate(out ZLibStreamHandle zLibStreamHandle, CompressionLevel level,
	int windowBits, int memLevel, CompressionStrategy strategy)
	{
	zLibStreamHandle = new ZLibStreamHandle();
	return zLibStreamHandle.DeflateInit2_(level, windowBits, memLevel, strategy);
	}

so yes, the SafeHandle is always non-null. The only way it wouldn't be is if there were some kind of asynchronous exception (e.g. a thread abort), and very little code is truly reliable in such situations.

[ViktorHofer]

/backport to release/7.0-staging

[github-actions]

Started backporting to release/7.0-staging: https://github.com/dotnet/runtime/actions/runs/5403746757