[release/7.0-staging] Revert Deflater/Inflater changes around SafeHandle initialization #88153
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
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.
|
Tagging subscribers to this area: @dotnet/area-system-io-compression Issue DetailsBackport of #85001 to release/7.0-staging Customer ImpactTestingRiskIMPORTANT: If this backport is for a servicing release, please verify that:
|
|
@ViktorHofer Reminder: Code Complete for the August Release is tomorrow Monday July 10th. If you intend to include this fix in that release, please make sure to fill out the template, add the servicing-consider label, send the email to Tactics requesting approval. If approved, please switch to the servicing-approved label, and merge it before 4pm because that's when I'll start the staging merge process into internal. |
ViktorHofer
added
Servicing-approved
ghost
locked as resolved and limited conversation to collaborators
Backport of #85001 to release/7.0-staging
Fixes #87791
/cc @ViktorHofer @stephentoub
Customer Impact
Customer reported (Rick Brewster - Paint.NET), see #87791 for more details.
Infrequent application crashes are reported because of an unhandled
NullReferenceExceptionwhen initializing the Deflater or Inflater compression type.Testing
The fix has been in main since April 19th and we haven't noticed any issues with it.
Risk
Low risk. The code changes are isolated to the two types in question and the overall diff is small. This changes restores the previous .NET 6 behavior around SafeHandle initialization.