Add strict validation to System.IO.Compression.GZipStream

[mfkl]

The current implementation of System.IO.Compression.GzipStream seems to not be as strict regarding error handling as it could be when it comes to corrupt data.

For context, this is a good thread about this exact issue, I believe. There is likely even more info in the old connect ticket but it's been unaccessible for a while.

So I have compared the System.IO.Compression.GzipStream implementation with its managed and bindings counterparts, namely zlib.managed, ICSharpCode.SharpZipLib and SharpCompress when it comes to handling corrupt data.

As for test cases, I retrieved the code from the SO question and also added another test which hits a different code path.

This is the other test case https://github.com/mfkl/zlib-validation/blob/main/ConsoleApp1/Program.cs

This is the result:

System.IO.Compression
Expected: xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
Actual  : ////////////////////////////////////////////////////

zlib.managed
Expected: xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
Actual  : xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx

ICSharpCode.SharpZipLib
Expected: xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
Actual  : /xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx

SharpCompress
Expected: xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
Actual  : /xxxxxxxxx//xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx                                                                                                                                                                                  

My understanding is that the System.IO.Compression.GzipStream implementation ignores ZLibNative.ErrorCode.BufError errors in most cases, while other implementions are stricter about it.

My proposed fix is to add an overload with a simple boolean which, when set to true, would take into account the BufError returned by the native lib, and throw accordingly.

I've run the compression test suites and made sure I did not break anything. If there is another approach to go at this, feel free to let me know.

Feedback welcome!

[ghost]

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

[danmoseley]

@mfkl thanks for your investigation and contribution. The owners of this area will take a look, but note that any changes to public API have to go through a proposal and approval process described here: https://github.com/dotnet/runtime/blob/master/docs/project/api-review-process.md

[mfkl]

note that any changes to public API have to go through a proposal and approval process described here: https://github.com/dotnet/runtime/blob/master/docs/project/api-review-process.md

Indeed, sorry about that. Let's take the conversation there #47563

[adamsitnik]

Hello @mfkl

Big thanks for your work and contribution. As @danmosemsft mentioned, we have a dedicated process for adding new APIs to .NET.

Let's take the conversation to the issue that you have created (#47563), get a good API proposal, API approval, and then reopen the PR. I hope that you don't mind that. At our scale, we need to follow the process to ensure that we meet all the quality requirements.

Thank you,
Adam