Add strict validation to System.IO.Compression.GZipStream

[mfkl]

EDIT:

This turned out to be handled as a bug rather than an API proposal, see #47563 (comment) for details.

Original description:

Background and Motivation

The System.IO.Compression.GZipStream implemention over zlib does not currently do strict native error reporting.

See https://stackoverflow.com/questions/9456563/gzipstream-doesnt-detect-corrupt-data-even-crc32-passes for more info.

Other zlib managed implementation or bindings over the native API do report issues on corrupt data.

Proposed API

Add a new GZipStream contructor overload.

namespace System.IO.Compression.cs
{
+            public GZipStream(System.IO.Stream stream, System.IO.Compression.CompressionMode mode, bool leaveOpen, bool strictValidation) { }
}

Usage Examples

new GZipStream(decomStream, CompressionMode.Decompress, false, strictValidation: true)

#47560

Alternative Designs

There may better ways to expose this optional behavior. Ideas are welcome.

Risks

Since this is explicitly opt-in, and that the current behavior is not changed, I don't expect any breaking changes or behavior with existing code calling into this API.

[adamsitnik]

Triage: we like this idea, but we should most probably add ctor for the other enum too

    public partial class GZipStream : System.IO.Stream
    {
        public GZipStream(System.IO.Stream stream, System.IO.Compression.CompressionLevel compressionLevel, bool leaveOpen)
+       public GZipStream(System.IO.Stream stream, System.IO.Compression.CompressionLevel compressionLevel, bool leaveOpen, bool strictValidation)
        public GZipStream(System.IO.Stream stream, System.IO.Compression.CompressionMode mode, bool leaveOpen)
+       public GZipStream(System.IO.Stream stream, System.IO.Compression.CompressionMode mode, bool leaveOpen, bool strictValidation)

and verify if other compression methods allow for such validation and extend them as well

[adamsitnik]

@mfkl is there any chance that you could research if other compression methods allow for such validation? In such a case, we should add overloads to other ctors too. Once we get there, we are going to mark the issue as ready for review, go through API Design Review, get the API accepted (with possible modifications of the proposal), and then re-open your PR.

[danmoseley]

If we had such a boolean, and then discovered new ways to be strict, would we add those checks or would it be a breaking change (requiring yet another flag)? I'm guessing we would add them because the flag suggests you want to know.

[Wraith2]

Failing to report errors in the data and producing incorrect output seems more like a bug to be fixed then a new feature request. Shouldn't the default be to produce correct data?

[mfkl]

is there any chance that you could research if other compression methods allow for such validation?

I can have a look.

Shouldn't the default be to produce correct data?

I considered this, but since I don't know the impact on existing users I decided to go the API route. Happy to revisit.

Another thing to consider from the comment on this line

runtime/src/libraries/System.IO.Compression/src/System/IO/Compression/DeflateZLib/Deflater.cs

Lines 250 to 251 in 42bc286

	case ZErrorCode.BufError:
	return errC; // This is a recoverable error

Is throwing on all buffer errors the correct behavior? I'd think so.

[mfkl]

I've tried enabling strict validation by default (e.g. throwing on all BufErrors) to check for potential regressions in tests, and it fails half the 600+ compression test suite.

Additionally, the docs makes me think that maybe throwing on every BufErrors may not be the correct approach:

inflate() returns Z_OK if some progress has been made (more input processed or more output produced), Z_STREAM_END if the end of the compressed data has been reached and all uncompressed output has been produced, Z_NEED_DICT if a preset dictionary is needed at this point, Z_DATA_ERROR if the input data was corrupted (input stream not conforming to the zlib format or incorrect check value, in which case strm->msg points to a string with a more specific error), Z_STREAM_ERROR if the stream structure was inconsistent (for example next_in or next_out was Z_NULL, or the state was inadvertently written over by the application), Z_MEM_ERROR if there was not enough memory, Z_BUF_ERROR if no progress was possible or if there was not enough room in the output buffer when Z_FINISH is used. Note that Z_BUF_ERROR is not fatal, and inflate() can be called again with more input and more output space to continue decompressing

deflate() returns Z_OK if some progress has been made (more input processed or more output produced), Z_STREAM_END if all input has been consumed and all output has been produced (only when flush is set to Z_FINISH), Z_STREAM_ERROR if the stream state was inconsistent (for example if next_in or next_out was Z_NULL or the state was inadvertently written over by the application), or Z_BUF_ERROR if no progress is possible (for example avail_in or avail_out was zero). Note that Z_BUF_ERROR is not fatal, and deflate() can be called again with more input and more output space to continue compressing.

As for other compression methods, I had a look at Brotli which has a similar enum value for corrupted data

/** Decoding error, e.g. corrupted input or memory allocation problem. */
  BROTLI_DECODER_RESULT_ERROR = 0,

which is correctly handled on the C# side currently (similarly to what I had done for zlib):

int brotliResult = Interop.Brotli.BrotliDecoderDecompressStream(_state, ref availableInput, &inBytes, ref availableOutput, &outBytes, out _);
if (brotliResult == 0) // Error
{
     return OperationStatus.InvalidData;
}

[danmoseley]

Presumably the goal is to flag inputs that will decompress incorrectly and not flag those that will decompress correctly in all major decompression tools. So at a minimum zero of the test bed should fail the checks because we know they decompress correctly. (Unless we can find another tools that cannot.)