From d2bcfcf8b9d22d86d7ac192479a8f5d14b4ba91b Mon Sep 17 00:00:00 2001 From: Stephen Toub Date: Tue, 18 Apr 2023 15:07:42 -0400 Subject: [PATCH] Revert Deflater/Inflater changes around SafeHandle initialization 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. --- .../src/System/IO/Compression/DeflateZLib/Deflater.cs | 6 +----- .../src/System/IO/Compression/DeflateZLib/Inflater.cs | 6 +----- 2 files changed, 2 insertions(+), 10 deletions(-) diff --git a/src/libraries/System.IO.Compression/src/System/IO/Compression/DeflateZLib/Deflater.cs b/src/libraries/System.IO.Compression/src/System/IO/Compression/DeflateZLib/Deflater.cs index 8ceb38f13eb8c2..f2d02cde7296bf 100644 --- a/src/libraries/System.IO.Compression/src/System/IO/Compression/DeflateZLib/Deflater.cs +++ b/src/libraries/System.IO.Compression/src/System/IO/Compression/DeflateZLib/Deflater.cs @@ -64,21 +64,17 @@ internal Deflater(CompressionLevel compressionLevel, int windowBits) ZLibNative.CompressionStrategy strategy = ZLibNative.CompressionStrategy.DefaultStrategy; - ZLibNative.ZLibStreamHandle? zlibStream = null; ZErrorCode errC; try { - errC = ZLibNative.CreateZLibStreamForDeflate(out zlibStream, zlibCompressionLevel, + errC = ZLibNative.CreateZLibStreamForDeflate(out _zlibStream, zlibCompressionLevel, windowBits, memLevel, strategy); } catch (Exception cause) { - zlibStream?.Dispose(); throw new ZLibException(SR.ZLibErrorDLLLoadError, cause); } - _zlibStream = zlibStream; - switch (errC) { case ZErrorCode.Ok: diff --git a/src/libraries/System.IO.Compression/src/System/IO/Compression/DeflateZLib/Inflater.cs b/src/libraries/System.IO.Compression/src/System/IO/Compression/DeflateZLib/Inflater.cs index 493a6f47d8cb29..4544353ac5406b 100644 --- a/src/libraries/System.IO.Compression/src/System/IO/Compression/DeflateZLib/Inflater.cs +++ b/src/libraries/System.IO.Compression/src/System/IO/Compression/DeflateZLib/Inflater.cs @@ -234,20 +234,16 @@ public void Dispose() [MemberNotNull(nameof(_zlibStream))] private void InflateInit(int windowBits) { - ZLibNative.ZLibStreamHandle? zlibStream = null; ZLibNative.ErrorCode error; try { - error = ZLibNative.CreateZLibStreamForInflate(out zlibStream, windowBits); + error = ZLibNative.CreateZLibStreamForInflate(out _zlibStream, windowBits); } catch (Exception exception) // could not load the ZLib dll { - zlibStream?.Dispose(); throw new ZLibException(SR.ZLibErrorDLLLoadError, exception); } - _zlibStream = zlibStream; - switch (error) { case ZLibNative.ErrorCode.Ok: // Successful initialization