Fix race between zlib Inflate/Deflate and Dispose

[rzikm]

Fixes #128550.

Inflater/Deflater synchronize their native zlib calls via a managed lock, but Dispose is not synchronized, so a concurrent Dispose on one thread could run InflateEnd/DeflateEnd and free the underlying z_stream while another thread was mid-call inside the native inflate/deflate, leading to use-after-free crashes.

The fix moves the protection down into ZLibStreamHandle. Inflate, InflateReset2_, and Deflate now bracket their native invocations with DangerousAddRef/DangerousRelease, which is exactly what SafeHandle is designed for: it defers ReleaseHandle (and therefore the InflateEnd/DeflateEnd P/Invoke) until all in-flight callers have released their refs. The redundant EnsureNotDisposed check is removed from these methods since DangerousAddRef already throws ObjectDisposedException for a closed handle.

The other ZLibStreamHandle members (DeflateEnd, InflateEnd, InflateInit2_, DeflateInit2_) are intentionally left as-is: they only run from initialization or from ReleaseHandle/Dispose paths that are not the racing party.

[Copilot]

Pull request overview

This PR hardens the shared zlib stream handle used by System.IO.Compression and System.Net.WebSockets so concurrent disposal cannot release native zlib state while an inflate/deflate native call is in flight.

Changes:

• Wraps Deflate, InflateReset2_, and Inflate native calls with DangerousAddRef/DangerousRelease.
• Relies on SafeHandle ref-counting instead of the previous disposed-state check at these call sites.

[dotnet-policy-service]

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

[github-actions]

Note

This review was generated by Copilot.

🤖 Copilot Code Review — PR #128752

Holistic Assessment

Motivation: The PR fixes a real use-after-free race (issue #128550) where concurrent Dispose frees the native z_stream while Inflate/Deflate is in-flight. The bug is valid and the fix is warranted.

Approach: DangerousAddRef/DangerousRelease is the canonical SafeHandle pattern for exactly this scenario — deferring ReleaseHandle until all in-flight native calls complete. This is the right fix at the right layer.

Summary: ✅ LGTM. The change is minimal, correctly scoped, and applies the idiomatic SafeHandle ref-counting pattern. No blocking issues found.

---

Detailed Findings

✅ Correctness — DangerousAddRef/DangerousRelease pattern is correct

All three protected methods (Deflate, Inflate, InflateReset2_) follow the correct pattern:

1. bool refAdded = false declared before try
2. DangerousAddRef(ref refAdded) as first statement in try
3. DangerousRelease() in finally, guarded by if (refAdded)

This ensures ReleaseHandle (and therefore DeflateEnd/InflateEnd) cannot execute while any of these calls are in-flight.

✅ Removal of EnsureNotDisposed — correct

DangerousAddRef already throws ObjectDisposedException when the handle is closed/disposed, making the explicit EnsureNotDisposed check redundant. Removing it from DeflateEnd/InflateEnd is also fine since those are only called from ReleaseHandle (which runs after all refs are released) or during initialization failure cleanup.

✅ Scope — unprotected methods are correctly identified

• DeflateInit2_/InflateInit2_ run only during construction before the handle is shared
• DeflateEnd/InflateEnd run only from ReleaseHandle, which is itself deferred by the ref count

These don't need protection.

✅ Cross-cutting — WebSocket callers also benefit

WebSocketDeflater and WebSocketInflater call the same ZLibStreamHandle.Deflate/Inflate methods. They have similar Dispose patterns (non-synchronized _stream?.Dispose()). This fix protects those callers as well without requiring changes there.

💡 Observation — EnsureState ordering

EnsureState is called after DangerousAddRef, so if it throws, the ref is still correctly released in the finally block. The ordering is sound.

Generated by Code Review for issue #128752 · ● 2M · ◷

[jkotas]

Does the fix need to be applied here as well?

[jkotas]

And here

[jkotas]

Is the Zlib implementation robust against multiple threads calling Deflate at the same time on the same state? It is fine for it to produce corrupted results, but it is not fine for it to crash or corrupt memory that it does not own.