Reliable clean-up of SafeHandles in Deflater/Inflater #114826

edwardneal · 2025-04-18T20:09:11Z

This reimplements #71991 in Deflater and Inflater. The PR was previously reverted in #85001 as a result of issue #84994.

As noted in the reverting PR, Deflater simply needed to dispose of the returned SafeHandle and supress its own finalizer if CreateZLibStreamForDeflate threw an exception. We now do this.

We couldn't do this for Inflater at the time - it had to support concatenated GZip data, and as part of that support it would recreate its ZLibStreamHandle. As a result of #113587, this recreation no longer happens and we can treat Inflater as we would treat Deflater. I've refactored Inflater.InflateInit out of existence and made _zlibStream readonly to reflect this.

This also incorporates a change to both class' DeallocateBufferHandle methods (when called by Dispose) to prevent them dereferencing a ZLibStreamHandle after it's been disposed of. This is roughly how #84994 was originally discovered.

If the call to CreateZLibStreamForXXflate throws an exception from the constructor, explicitly dispose of the ZLibStreamHandle (rather than relying on the finalizer to do it.) Also incorporates a change to DeallocateBufferHandle (called from the Dispose path) to prevent it using a ZlibStreamHandle after it's been Disposed.

This aligns Inflater with Deflater. Removed now-unnecessary usings.

stephentoub · 2025-05-28T22:24:30Z

src/libraries/System.IO.Compression/src/System/IO/Compression/DeflateZLib/Deflater.cs

+                }

-                DeallocateInputBufferHandle();
+                // Unpin the input buffer, but avoid modifying the ZLibStreamHandle (which may have been disposed of.)


Suggested change

// Unpin the input buffer, but avoid modifying the ZLibStreamHandle (which may have been disposed of.)

// Unpin the input buffer, but avoid modifying the ZLibStreamHandle (which may have been disposed of).

avoid modifying the ZLibStreamHandle (which may have been disposed of.)

Why would setting those fields on the disposed instance be a problem?

It's not a problem yet - but that's largely because ZLibStreamHandle's underlying handle field isn't used properly. Setting these properties on an ZLibStreamHandle instance sets fields on a ZStream struct variable inside the instance; calling InflateInit2_ fixes that variable in memory and passes a pointer to it to the underlying PInvoke. At the moment, setting a property on the disposed instance will always work.

There's a bug open to fix those handle semantics (#89445), and I'm testing a few approaches. Most of them involve allocating and freeing native memory though - at which point, setting properties on a disposed ZLibStreamHandle instance becomes a use-after-free bug.

src/libraries/System.IO.Compression/src/System/IO/Compression/DeflateZLib/Deflater.cs

stephentoub · 2025-05-29T13:13:17Z

src/libraries/System.IO.Compression/src/System/IO/Compression/DeflateZLib/Deflater.cs

-                errC = ZLibNative.CreateZLibStreamForDeflate(out _zlibStream, compressionLevel, windowBits, memLevel, strategy);
+                errC = ZLibNative.CreateZLibStreamForDeflate(out zlibStream, compressionLevel, windowBits, memLevel, strategy);
+
+                _zlibStream = zlibStream;


I don't think this dance should be done here. CreateZLibStreamForDeflate should only be setting the out once it's successful.

I've pushed a speculative commit so we can see roughly what that looks like.

We could turn CreateZLibStreamForXxflate into TryCreateZLibStreamForXxflate and add the right nullability annotations. It's the smallest change at the call sites and it'd simplify the nullability dance but introduce a new one as we shuffle the error handling state around. I'd prefer to avoid it.

I've currently folded most of the error handling into CreateZLibStreamForXxflate, which will either return a non-null ZLibStreamHandle or throw. This simplifies the System.IO.Compression call sites slightly. ZLibNative.cs is also source-included in System.Net.WebSockets though, which means that we can't throw ZLibException directly (it's excluded from the System.IO.Compression ref assembly as per 112440) and have to throw a new shim exception class.

More broadly, I was planning to split the ZLibStreamHandle class into separate ZLibStreamInflateHandle and ZLibStreamDeflateHandle classes; the CreateZLibStreamForXxflate methods would just become constructors. I'm inclined to roll most of the work from this commit into that future PR, and keep this one limited to the finalization fixes. What do you think?

rzikm

Looks reasonable so far, @edwardneal, are you still looking to contribute this change? Have you had time to look into addressing #89445 as well?

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

src/libraries/System.IO.Compression/src/System/IO/Compression/DeflateZLib/Inflater.cs

Separates the initialization of a ZLibStreamHandle and only instantiates a Deflater or Inflater instance if this initialization succeeds

rzikm

I think this looks good now, besides the few nits from @stephentoub .

Stephen, do you want to take another look?

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

stephentoub · 2025-08-14T17:40:12Z

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


-        public static ErrorCode CreateZLibStreamForDeflate(out ZLibStreamHandle zLibStreamHandle, CompressionLevel level,
-            int windowBits, int memLevel, CompressionStrategy strategy)
+        public sealed class ZLibNativeException : Exception


Why is this needed, rather than just throwing the ZLibException that should propagate?

It's a workaround, unfortunately. The root cause of this is that ZLibNative.cs is source-included in both System.IO.Compression and System.Net.WebSockets, that we've centralised the error code -> exception transition in ZLibNative.cs, and that ZLibException isn't in the ref assemblies for S.IO.C.

Taking the example of deflation, main has the CreateZLibStreamForDeflate method in ZLibNative.cs. In System.IO.Compression, the Deflater constructor throws a ZLibException based on the error code; in System.Net.WebSockets, there's a CreateDeflater method which does the same thing and throws a WebSocketException. I can't reference ZLibException in order to translate the exception to a WebSocketException, because it's been specifically excluded from the S.IO.C ref assembly.

It's always been like that (earliest version of that file is from 2018), and the only comment I've found states that it's only publicly exposed in the implementation for serialization compatibility.

If we expose ZLibException publicly and document that its HResult property will store the ZLib error code then we don't need this "transport" exception type. I imagine this'll probably need an API proposal though?

If only the class visibility is the problem, we were able to workaround that in the past by using SkipUseReferenceAssembly="true" on the ProjectReference tag in the csproj file.

It's purely a problem with class visibility, but the csproj doesn't have an explicit reference to System.IO.Compression, it just source-includes the files it needs. With that said, S.IO.C's part of the runtime, so I think it's safe to add a ProjectReference without having any knock-on impacts on the consumers of System.Net.WebSockets.

Adding the ProjectReference builds locally and lets me reference ZLibException - do I need to do anything else as a result of adding that dependency?

I can't think of any possible problems at this moment, but please add a comment to the in the csproj file as to why we are bypassing the ref assembly.

do I need to do anything else as a result of adding that dependency?

no, just the project reference should be enough.

Thanks rzikm - local builds work properly after adding the new reference, and the PR's diff looks more sensible now.

Add explanatory comment to WebSockets csproj.

rzikm · 2025-09-08T06:31:38Z

@stephentoub I think your comments are addressed now, do you want to give this PR another review?

rzikm · 2025-09-29T08:14:58Z

@stephentoub Gentle ping in case this slipped your inbox

stephentoub · 2025-10-15T16:05:58Z

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

+                {
+                    errC = zLibStreamHandle.DeflateInit2_(level, windowBits, memLevel, strategy);
+                }
+                catch (Exception cause) // could not load the ZLib dll


Nit: cause is an unusual name for an exception. We typically use e, ex, or something like that.

stephentoub · 2025-10-15T16:09:39Z

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

+                if (errC is not ErrorCode.Ok)
+                {
+                    string zlibErrorMessage = zLibStreamHandle.GetErrorMessage();
+                    string exceptionMessage = GenerateExceptionMessage(errC);
+
+                    zLibStreamHandle.Dispose();
+                    throw new ZLibException(exceptionMessage, "inflateInit2_", (int)errC, zlibErrorMessage);
+                }


Is this identical to the same block in the deflate method, other than the const function name passed to the throw? Can you lift this into a shared helper? Could you dedup both methods almost entirely with a delegate that's just used to parameterize the Deflate/InflateInit2 call? You could have that delegate accept a tuple to avoid a closure allocation.

I've created the shared helper and moved the logic down into DeflateInit2_ and InflateInit2_, and this renders CreateForDeflate and CreateForInflate nearly identical.

Do you mind if I deal with the duplication in a future PR? I'm planning to adjust ZLibStreamHandle and the PAL so that it uses its handle field and we can marshal usable handles directly, and this'll change these methods anyway.

src/libraries/System.IO.Compression/src/Resources/Strings.resx

src/libraries/System.IO.Compression/src/System/IO/Compression/DeflateZLib/Inflater.cs

stephentoub · 2025-10-15T16:13:56Z

src/libraries/System.Net.WebSockets/src/Resources/Strings.resx

+  </data>
+  <data name="ZLibErrorIncorrectInitParameters" xml:space="preserve">
+    <value>The underlying compression routine received incorrect initialization parameters.</value>
+  </data>


Where are these used?

We moved the logic in Deflater & Inflaters' constructors which created a ZLibStreamHandle into factory methods on the type. That pushed references to string resources (and to ZLibException) into ZLibNative.cs, and that file is source-included in System.Net.WebSockets.

In theory, this would seem to bring a behaviour change: if a web socket fails to initialize ZLib, the inner exception's message could change from The underlying compression routine returned an unexpected error code: '{0}' to The underlying compression routine received incorrect initialization parameters. or The version of the underlying compression routine does not match expected version. if the error code returned is -2 or -6 respectively. In practice though, this can't happen: the compression routine versions are locked by the version of ZLib we package, and the only initialization parameters are pre-validated in WebSocketDeflateOptions. The resources are just there to overcome a compilation error.

This is also why we have the new reference in System.Net.WebSockets: ZLibException is a public type which doesn't appear in the S.IO.C reference assembly.

stephentoub · 2025-10-15T16:23:56Z

src/libraries/System.Net.WebSockets/src/System.Net.WebSockets.csproj

    <ProjectReference Include="$(LibrariesProjectRoot)System.Collections\src\System.Collections.csproj" />
    <ProjectReference Include="$(LibrariesProjectRoot)System.Diagnostics.Tracing\src\System.Diagnostics.Tracing.csproj" />
+    <!-- Bypass System.IO.Compression's reference assembly in order to allow ZLibNative to access the ZLibException type. -->
+    <ProjectReference Include="$(LibrariesProjectRoot)System.IO.Compression\src\System.IO.Compression.csproj" SkipUseReferenceAssembly="true" />


Is this still needed?

Moved error handling to XxflateInit2_. These methods will either return successfully or throw an exception. Resource string and code style changes.

edwardneal added 2 commits April 18, 2025 18:22

Replace InflateInit, make _zlibStream readonly

8c353ba

This aligns Inflater with Deflater. Removed now-unnecessary usings.

ghost added the area-System.IO.Compression label Apr 18, 2025

dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Apr 18, 2025

ericstj assigned carlossanlop Apr 21, 2025

ericstj self-assigned this May 13, 2025

stephentoub reviewed May 28, 2025

View reviewed changes

src/libraries/System.IO.Compression/src/System/IO/Compression/DeflateZLib/Deflater.cs Show resolved Hide resolved

stephentoub reviewed May 29, 2025

View reviewed changes

ericstj unassigned carlossanlop May 29, 2025

edwardneal added 3 commits May 29, 2025 21:59

Merge branch 'main' into zlib-safehandle-finalization

9dc6313

Move ZLibStreamHandle initialisation into ZLibNative

38a97f1

Merge branch 'main' into zlib-safehandle-finalization

0046881

ericstj removed their assignment Jul 31, 2025

Merge branch 'main' into zlib-safehandle-finalization

0611b30

rzikm reviewed Aug 11, 2025

View reviewed changes

src/libraries/Common/src/System/IO/Compression/ZLibNative.cs Outdated Show resolved Hide resolved

src/libraries/System.IO.Compression/src/System/IO/Compression/DeflateZLib/Inflater.cs Outdated Show resolved Hide resolved

build-analysis bot mentioned this pull request Aug 11, 2025

[android] System.Net.Http.Functional.Tests failure with TimeoutException #118118

Closed

Move factory methods to ZLibStreamHandle

8079487

This was referenced Aug 12, 2025

Unable to pull image from mcr.microsoft.com #117164

Open

[Android][CoreCLR] System.Security.Cryptography.Tests killed by lowmemorykiller #118603

Closed

Create Xxflaters using factory methods

08f783e

Separates the initialization of a ZLibStreamHandle and only instantiates a Deflater or Inflater instance if this initialization succeeds

rzikm approved these changes Aug 14, 2025

View reviewed changes

rzikm requested a review from stephentoub August 14, 2025 08:11

stephentoub reviewed Aug 14, 2025

View reviewed changes

edwardneal added 4 commits September 4, 2025 07:34

Add ProjectReference, remove now-unnecessary ZLibNativeException class

9a4798d

Merge main

1510a4f

Post-merge cleanup

e52adf2

Add explanatory comment to WebSockets csproj.

Wrap up an earlier review comment

50d5f8d

rzikm self-assigned this Sep 11, 2025

stephentoub reviewed Oct 15, 2025

View reviewed changes

Code review response

2ddf194

Moved error handling to XxflateInit2_. These methods will either return successfully or throw an exception. Resource string and code style changes.

	// Unpin the input buffer, but avoid modifying the ZLibStreamHandle (which may have been disposed of.)
	// Unpin the input buffer, but avoid modifying the ZLibStreamHandle (which may have been disposed of).

Reliable clean-up of SafeHandles in Deflater/Inflater #114826

Are you sure you want to change the base?

Reliable clean-up of SafeHandles in Deflater/Inflater #114826

Conversation

edwardneal commented Apr 18, 2025 • edited by karelz Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rzikm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rzikm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rzikm commented Sep 8, 2025

Uh oh!

rzikm commented Sep 29, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

edwardneal commented Apr 18, 2025 •

edited by karelz

Loading