DeflateStream truncation detection is buggy

[MihaZupan]

Description

After #61768, DeflateStream is no longer able to decompress (seemingly valid) input.

Noticed this as .NET 7 can no longer communicate with Discord servers after updating the preview.

Reproduction Steps

It seems like it can't even roundtrip:

using System.IO.Compression;
using System.Text;

string input = "foo";

var compressed = new MemoryStream();
var compressor = new DeflateStream(compressed, CompressionLevel.Optimal);

compressor.Write(Encoding.ASCII.GetBytes(input));
compressor.Flush();
compressed.Position = 0;

var decompressor = new DeflateStream(compressed, CompressionMode.Decompress);
var decompressed = new MemoryStream();
decompressor.CopyTo(decompressed); // Blows up in 7.0 preview 5 or newer

Console.WriteLine(Encoding.ASCII.GetString(decompressed.ToArray()));

Expected behavior

DeflateStream is able to roundtrip.

Actual behavior

System.IO.InvalidDataException: Found truncated data while decoding.
   at System.IO.Compression.DeflateStream.ThrowTruncatedInvalidData()
   at System.IO.Compression.DeflateStream.CopyToStream.CopyFromSourceToDestination()

Regression?

Works in 7.0 preview 4.
Throws in 7.0 preview 5 or newer (#61768).

Known Workarounds

N/A

Configuration

No response

Other information

No response

[ghost]

Tagging subscribers to this area: @dotnet/area-system-io-compression
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Description

After #61768, DeflateStream is no longer able to decompress (seemingly valid) input.

Noticed this as .NET 7 can no longer communicate with Discord servers after updating the preview.

Reproduction Steps

I seems like it can't even roundtrip:

using System.IO.Compression;
using System.Text;

string input = "foo";

var compressed = new MemoryStream();
var compressor = new DeflateStream(compressed, CompressionLevel.Optimal);

compressor.Write(Encoding.ASCII.GetBytes(input));
compressor.Flush();
compressed.Position = 0;

var decompressor = new DeflateStream(compressed, CompressionMode.Decompress);
var decompressed = new MemoryStream();
decompressor.CopyTo(decompressed); // Blows up in 7.0 preview 5 or newer

Console.WriteLine(Encoding.ASCII.GetString(decompressed.ToArray()));

Expected behavior

DeflateStream is able to roundtrip.

Actual behavior

System.IO.InvalidDataException: Found truncated data while decoding.
   at System.IO.Compression.DeflateStream.ThrowTruncatedInvalidData()
   at System.IO.Compression.DeflateStream.CopyToStream.CopyFromSourceToDestination()

Regression?

Works in 7.0 preview 4.
Throws in 7.0 preview 5 or newer (#61768).

Known Workarounds

N/A

Configuration

No response

Other information

No response

Author:	MihaZupan
Assignees:	-
Labels:	area-System.IO.Compression
Milestone:	-

[MihaZupan]

cc: @mfkl @jozkee

[stephentoub]

It seems like it can't even roundtrip:

DeflateStream writes a footer as part of closing the stream; Flush'ing doesn't generate such a footer because there might be additional data written afterwards. The shared repro isn't closing the stream, hence the resulting data lacks the footer and technically is invalid. If you change the repro to pass , leaveOpen: true to the compressor's ctor and add a compressor.Dispose(); between flushing and resetting the position, roundtripping succeeds.

That said, I think we should revert #61768. It's marked as a breaking change, so possible breaks were recognized, but if it's actually preventing us from communicating with real-world servers, it doesn't seem worth it, and we should instead take another run at enabling such validation in an opt-in manner for .NET 8.
cc: @jeffhandley, @adamsitnik, @jozkee

@MihaZupan, can you share an example that talks to a server that results in such failures?

[MihaZupan]

This example will connect to and read one message from the Discord WebSocket gateway.
The code is mimicking what the Discord.Net library does.

It should print something along the lines of

{"t":null,"s":null,"op":10,"d":{"heartbeat_interval":41250,"_trace":["[\"gateway-prd-main-2vsv\",{\"micros\":0.0}]"]}}

but will instead throw at the CopyTo

using System.Diagnostics;
using System.IO.Compression;
using System.Net.WebSockets;
using System.Text;

using var ws = new ClientWebSocket();
await ws.ConnectAsync(new Uri("wss://gateway.discord.gg/?v=10&encoding=json&compress=zlib-stream"), CancellationToken.None);

var compressed = new MemoryStream();
var decompressor = new DeflateStream(compressed, CompressionMode.Decompress);
var decompressed = new MemoryStream();

var buffer = new byte[1024];
var result = await ws.ReceiveAsync(buffer, CancellationToken.None);

Debug.Assert(result.Count > 2 && buffer[0] == 0x78 && result.EndOfMessage); // This is just a POC

compressed.Write(buffer.AsSpan(2, result.Count - 2));
compressed.Position = 0;
decompressor.CopyTo(decompressed);

Console.WriteLine(Encoding.ASCII.GetString(decompressed.ToArray()));

// Further messages would require auth

[yretenai]

If you know the exact size that is present, ReadExactly will still throw (in this case, we're aware it's a partial stream but only care for say, the first frame.) ReadAtLeast when passing false to the ignore exceptions argument will yield the data, but it's a very ugly hackfix.

[danmoseley]

@mfkl

[mfkl]

Noted, thanks for letting me know.

Do revert and I will put together a new PR with the truncation behavior under a constructor opt-in flag.

[marcin-krystianc]

There are other libraries (like SharpZibLib) which would also fail on the Discord repro, e.g.:

{
    using var ws = new ClientWebSocket();
    await ws.ConnectAsync(new Uri("wss://gateway.discord.gg/?v=10&encoding=json&compress=zlib-stream"), CancellationToken.None);

    var compressed = new MemoryStream();
    var decompressor = new ICSharpCode.SharpZipLib.Zip.Compression.Streams.InflaterInputStream(compressed, new ICSharpCode.SharpZipLib.Zip.Compression.Inflater(noHeader: true));
    var decompressed = new MemoryStream();

    var buffer = new byte[1024];
    var result = await ws.ReceiveAsync(buffer, CancellationToken.None);

    Debug.Assert(result.Count > 2 && buffer[0] == 0x78 && result.EndOfMessage); // This is just a POC

    compressed.Write(buffer.AsSpan(2, result.Count - 2));
    // compressed.WriteByte(0x03); add finishing sequence
    // compressed.WriteByte(0x00);

    compressed.Position = 0;
    decompressor.CopyTo(decompressed);

    Console.WriteLine(Encoding.ASCII.GetString(decompressed.ToArray()));
}

throws:

Unhandled exception. ICSharpCode.SharpZipLib.SharpZipBaseException: Unexpected EOF
   at ICSharpCode.SharpZipLib.Zip.Compression.Streams.InflaterInputStream.Fill()
   at ICSharpCode.SharpZipLib.Zip.Compression.Streams.InflaterInputStream.Read(Byte[] buffer, Int32 offset, Int32 count)
   at System.IO.Stream.CopyTo(Stream destination, Int32 bufferSize)
   at Program.<Main>$(String[] args) in D:\workspace\CodeBIn\ConsoleApp8\ConsoleApp8\Program.cs:line 51
   at Program.<Main>(String[] args)

Apparently Discord server doesn't send the finishing sequence (0x03, 0x00) so some of the libraries reject decoding such data.
Note, that it can be worked around by adding these missing bytes on the client side.

There is an argument for reverting #61768 because it was a breaking change. But there is also an argument for not reverting it. Mainly, because stricter checks on decompression prevent from much more serious problems, like when the incomplete data is silently decompressed without errors.

[stephentoub]

Apparently Discord server doesn't send the finishing sequence (0x03, 0x00) so some of the libraries reject decoding such data.

Yup, this is the same issue I noted above with the repro:
"The shared repro isn't closing the stream, hence the resulting data lacks the footer and technically is invalid"

like when the incomplete data is silently decompressed without errors.

The server is in the wrong here. However, I think the cons of the break outweigh the benefits. The only thing this helps with is the missing footer, and until that point, data will happily be decompressed. The client can just as easily stop reading and not know about such silent missing data from the server, and there are a myriad other ways the server could miss sending some data, even with a footer. In this case, the server is taking an explicit action (e.g. specifying payload length) to signal a complete data payload. At this point if we want that codition to be an error, it should be opt-in in some fashion, such as when constructing the stream or as a property you can query after use to see whether the footer was received.

[jozkee]

Do revert and I will put together a new PR with the truncation behavior under a constructor opt-in flag.

@mfkl we would need an API proposal before the PR, so please consider filing one using the template from https://github.com/dotnet/runtime/issues/new/choose.