Reliable clean-up of SafeHandles in Deflater/Inflater #114826
Conversation
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.
ghost
added
the
dotnet-policy-service
bot
added
the
community-contribution
|
Tagging subscribers to this area: @dotnet/area-system-io-compression |
|
|
||
| DeallocateInputBufferHandle(); | ||
| // Unpin the input buffer, but avoid modifying the ZLibStreamHandle (which may have been disposed of.) |
There was a problem hiding this comment.
| // 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). |
There was a problem hiding this comment.
avoid modifying the ZLibStreamHandle (which may have been disposed of.)
Why would setting those fields on the disposed instance be a problem?
There was a problem hiding this comment.
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.
| { | ||
| _zlibStream.AvailIn = 0; | ||
| _zlibStream.NextIn = ZLibNative.ZNullPtr; | ||
| } |
| errC = ZLibNative.CreateZLibStreamForDeflate(out _zlibStream, compressionLevel, windowBits, memLevel, strategy); | ||
| errC = ZLibNative.CreateZLibStreamForDeflate(out zlibStream, compressionLevel, windowBits, memLevel, strategy); | ||
|
|
||
| _zlibStream = zlibStream; |
There was a problem hiding this comment.
I don't think this dance should be done here. CreateZLibStreamForDeflate should only be setting the out once it's successful.
There was a problem hiding this comment.
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?
This reimplements #71991 in
DeflaterandInflater. The PR was previously reverted in #85001 as a result of issue #84994.As noted in the reverting PR,
Deflatersimply needed to dispose of the returned SafeHandle and supress its own finalizer ifCreateZLibStreamForDeflatethrew an exception. We now do this.We couldn't do this for
Inflaterat the time - it had to support concatenated GZip data, and as part of that support it would recreate itsZLibStreamHandle. As a result of #113587, this recreation no longer happens and we can treatInflateras we would treatDeflater. I've refactoredInflater.InflateInitout of existence and made_zlibStreamreadonly to reflect this.This also incorporates a change to both class'
DeallocateBufferHandlemethods (when called byDispose) to prevent them dereferencing aZLibStreamHandleafter it's been disposed of. This is roughly how #84994 was originally discovered.