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
src/libraries/System.IO.Compression/src/System/IO/Compression/DeflateZLib/Deflater.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.IO.Compression/src/System/IO/Compression/DeflateZLib/Deflater.cs
Show resolved
Hide resolved
src/libraries/System.IO.Compression/src/System/IO/Compression/DeflateZLib/Deflater.cs
Outdated
Show resolved
Hide resolved
rzikm
left a comment
rzikm
left a comment
There was a problem hiding this comment.
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/System.IO.Compression/src/System/IO/Compression/DeflateZLib/Inflater.cs
Outdated
Show resolved
Hide resolved
Separates the initialization of a ZLibStreamHandle and only instantiates a Deflater or Inflater instance if this initialization succeeds
rzikm
left a comment
rzikm
left a comment
There was a problem hiding this comment.
I think this looks good now, besides the few nits from @stephentoub .
Stephen, do you want to take another look?
Add explanatory comment to WebSockets csproj.
|
@stephentoub I think your comments are addressed now, do you want to give this PR another review? |
|
@stephentoub Gentle ping in case this slipped your inbox |
| if (errC is not ErrorCode.Ok) | ||
| { | ||
| string zlibErrorMessage = zLibStreamHandle.GetErrorMessage(); | ||
| string exceptionMessage = GenerateExceptionMessage(errC); | ||
|
|
||
| zLibStreamHandle.Dispose(); | ||
| throw new ZLibException(exceptionMessage, "inflateInit2_", (int)errC, zlibErrorMessage); | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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/System/IO/Compression/DeflateZLib/Inflater.cs
Show resolved
Hide resolved
| </data> | ||
| <data name="ZLibErrorIncorrectInitParameters" xml:space="preserve"> | ||
| <value>The underlying compression routine received incorrect initialization parameters.</value> | ||
| </data> |
There was a problem hiding this comment.
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.
| <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" /> |
Moved error handling to XxflateInit2_. These methods will either return successfully or throw an exception. Resource string and code style changes.
|
Hey @edwardneal, there still seem to be some unaddressed code review feedback, can you look into it? |
|
There was one review comment on the name for the exception in Another two comments (the string resource and the reference) are covered in this response. I think I've answered the question, but I've left it open in case there's anything to follow up. This leaves some feedback on code duplication between |
5 participants
Fixes #89445
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.