Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CryptoStream calling flush when only reads have been performed #61325

Open
aromaa opened this issue Nov 8, 2021 · 7 comments
Open

CryptoStream calling flush when only reads have been performed #61325

aromaa opened this issue Nov 8, 2021 · 7 comments

Comments

@aromaa
Copy link
Contributor

aromaa commented Nov 8, 2021

Description

Deserializing JSON from DeflateStream/ZLibStream with inner stream of CryptoStream seems to cause the CryptoStream to call Flush on its inner stream while disposing. If the inner stream does not permit writes (we are only supposed to be reading from it!), it can lead to a exception to be thrown.

This is most likely related to the breaking changed around partial and zero-byte reads in DeflateStream, GZipStream, and CryptoStream.

Reproduction Steps

using System;
using System.IO;
using System.IO.Compression;
using System.Security.Cryptography;
using System.Text.Json;
using System.Threading;
using System.Threading.Tasks;

static async Task<MemoryStream> Serialize<T>(T test)
{
	MemoryStream stream = new();

	using ToBase64Transform base64Transformer = new();
	await using CryptoStream cryptoStream = new(stream, base64Transformer, CryptoStreamMode.Write, leaveOpen: true);
	await using DeflateStream deflate = new(cryptoStream, CompressionMode.Compress);

	await JsonSerializer.SerializeAsync(deflate, test);

	return stream;
}

static async Task<T> Deserialize<T>(Stream stream)
{
	using FromBase64Transform base64Transformer = new();
	await using CryptoStream cryptoStream = new(stream, base64Transformer, CryptoStreamMode.Read, leaveOpen: true);
	await using DeflateStream deflate = new(cryptoStream, CompressionMode.Decompress);

	return await JsonSerializer.DeserializeAsync<T>(deflate);
}

MemoryStream json = await Serialize("Hello World!");
string output = await Deserialize<string>(new WrapperStream(json));

Console.WriteLine(output);

public sealed class WrapperStream : MemoryStream
{
	public WrapperStream(MemoryStream stream) : base(stream.ToArray())
	{
	}

	public override void Flush() => throw new NotSupportedException();
	public override Task FlushAsync(CancellationToken cancellationToken) => throw new NotSupportedException();
}

Expected behavior

The code should run just fine and not call Flush.

Actual behavior

The code calls Flush and throws NotSupportedException.

Regression?

Regression from .NET 5.

Known Workarounds

No response

Configuration

Running on .NET 6.

Other information

No response

@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.IO untriaged New issue has not been triaged by the area owner labels Nov 8, 2021
@ghost
Copy link

ghost commented Nov 8, 2021

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

Issue Details

Description

Deserializing JSON from DeflateStream/ZLibStream with inner stream of CryptoStream seems to cause the CryptoStream to call Flush on its inner stream while disposing. If the inner stream does not permit writes (we are only supposed to be reading from it!), it can lead to a exception to be thrown.

This is most likely related to the breaking changed around partial and zero-byte reads in DeflateStream, GZipStream, and CryptoStream.

Reproduction Steps

using System;
using System.IO;
using System.IO.Compression;
using System.Security.Cryptography;
using System.Text.Json;
using System.Threading;
using System.Threading.Tasks;

static async Task<MemoryStream> Serialize<T>(T test)
{
	MemoryStream stream = new();

	using ToBase64Transform base64Transformer = new();
	await using CryptoStream cryptoStream = new(stream, base64Transformer, CryptoStreamMode.Write, leaveOpen: true);
	await using DeflateStream deflate = new(cryptoStream, CompressionMode.Compress);

	await JsonSerializer.SerializeAsync(deflate, test);

	return stream;
}

static async Task<T> Deserialize<T>(Stream stream)
{
	using FromBase64Transform base64Transformer = new();
	await using CryptoStream cryptoStream = new(stream, base64Transformer, CryptoStreamMode.Read, leaveOpen: true);
	await using DeflateStream deflate = new(cryptoStream, CompressionMode.Decompress);

	return await JsonSerializer.DeserializeAsync<T>(deflate);
}

MemoryStream json = await Serialize("Hello World!");
string output = await Deserialize<string>(new WrapperStream(json));

Console.WriteLine(output);

Expected behavior

The code should run just fine and not call Flush.

Actual behavior

The code calls Flush and throws NotSupportedException.

Regression?

Regression from .NET 5.

Known Workarounds

No response

Configuration

Running on .NET 6.

Other information

No response

Author: aromaa
Assignees: -
Labels:

area-System.IO, untriaged

Milestone: -

@ghost
Copy link

ghost commented Nov 8, 2021

Tagging subscribers to this area: @bartonjs, @vcsjones, @krwq, @GrabYourPitchforks
See info in area-owners.md if you want to be subscribed.

Issue Details

Description

Deserializing JSON from DeflateStream/ZLibStream with inner stream of CryptoStream seems to cause the CryptoStream to call Flush on its inner stream while disposing. If the inner stream does not permit writes (we are only supposed to be reading from it!), it can lead to a exception to be thrown.

This is most likely related to the breaking changed around partial and zero-byte reads in DeflateStream, GZipStream, and CryptoStream.

Reproduction Steps

using System;
using System.IO;
using System.IO.Compression;
using System.Security.Cryptography;
using System.Text.Json;
using System.Threading;
using System.Threading.Tasks;

static async Task<MemoryStream> Serialize<T>(T test)
{
	MemoryStream stream = new();

	using ToBase64Transform base64Transformer = new();
	await using CryptoStream cryptoStream = new(stream, base64Transformer, CryptoStreamMode.Write, leaveOpen: true);
	await using DeflateStream deflate = new(cryptoStream, CompressionMode.Compress);

	await JsonSerializer.SerializeAsync(deflate, test);

	return stream;
}

static async Task<T> Deserialize<T>(Stream stream)
{
	using FromBase64Transform base64Transformer = new();
	await using CryptoStream cryptoStream = new(stream, base64Transformer, CryptoStreamMode.Read, leaveOpen: true);
	await using DeflateStream deflate = new(cryptoStream, CompressionMode.Decompress);

	return await JsonSerializer.DeserializeAsync<T>(deflate);
}

MemoryStream json = await Serialize("Hello World!");
string output = await Deserialize<string>(new WrapperStream(json));

Console.WriteLine(output);

public sealed class WrapperStream : MemoryStream
{
	public WrapperStream(MemoryStream stream) : base(stream.ToArray())
	{
	}

	public override void Flush() => throw new NotSupportedException();
	public override Task FlushAsync(CancellationToken cancellationToken) => throw new NotSupportedException();
}

Expected behavior

The code should run just fine and not call Flush.

Actual behavior

The code calls Flush and throws NotSupportedException.

Regression?

Regression from .NET 5.

Known Workarounds

No response

Configuration

Running on .NET 6.

Other information

No response

Author: aromaa
Assignees: -
Labels:

area-System.Security, untriaged

Milestone: -

@GSPP
Copy link

GSPP commented Nov 9, 2021

Why is flush being called at all? Flushing is an expensive operation that is not automatically required. Normally, flushing should be initiated by application code. Only the application knows how to correctly orchestrate stream flushing.

@stephentoub
Copy link
Member

stephentoub commented Nov 9, 2021

Stream.Flush{Async} is expected to nop even when a stream has been used only for reads. From the docs:
https://docs.microsoft.com/en-us/dotnet/api/system.io.stream.flush?view=net-5.0#remarks
"In a class derived from Stream that doesn't support writing, Flush is typically implemented as an empty method to ensure full compatibility with other Stream types since it's valid to flush a read-only stream."

This also isn't a regression in CryptoStream or DeflateStream. You can see, for example, this throws the same NotSupportedException on both .NET 5 and .NET Framework 4.8:

using System;
using System.IO;
using System.IO.Compression;
using System.Security.Cryptography;
using System.Threading;
using System.Threading.Tasks;

internal class Program
{
    static void Main()
    {
        using (var base64Transformer = new FromBase64Transform())
        using (var cryptoStream = new CryptoStream(new WrapperStream(new MemoryStream()), base64Transformer, CryptoStreamMode.Read, leaveOpen: true))
        using (var deflate = new DeflateStream(cryptoStream, CompressionMode.Decompress))
        {
        }
    }

    public sealed class WrapperStream : MemoryStream
    {
        public WrapperStream(MemoryStream stream) : base(stream.ToArray()) { }
        public override void Flush() => throw new NotSupportedException();
        public override Task FlushAsync(CancellationToken cancellationToken) => throw new NotSupportedException();
    }
}

I believe what's different here in your example between .NET 5 and .NET 6 is that in .NET 5 JsonSerializer.DeserializeAsync was reading until EOF whereas in .NET 6 it doesn't appear to be doing so. When it reads until EOF, that causes CryptoStream.Dispose to not follow the code path that flushes, but if it hasn't read until EOF, CryptoStream.Dispose will (and always has) flush. You can see that by running this tweak to your repro... this outputs True on .NET 5 but False on .NET 6:

using System.IO.Compression;
using System.Security.Cryptography;
using System.Text.Json;

static async Task<MemoryStream> Serialize<T>(T test)
{
    MemoryStream stream = new();
    using ToBase64Transform base64Transformer = new();
    await using CryptoStream cryptoStream = new(stream, base64Transformer, CryptoStreamMode.Write, leaveOpen: true);
    await using DeflateStream deflate = new(cryptoStream, CompressionMode.Compress);
    await JsonSerializer.SerializeAsync(deflate, test);
    return stream;
}

static async Task<T> Deserialize<T>(Stream stream)
{
    using FromBase64Transform base64Transformer = new();
    using CryptoStream cryptoStream = new(stream, base64Transformer, CryptoStreamMode.Read, leaveOpen: true);
    using DeflateStream deflate = new(cryptoStream, CompressionMode.Decompress);
    return await JsonSerializer.DeserializeAsync<T>(deflate);
}

var s = new TrackingStream(await Serialize("Hello World!"));
await Deserialize<string>(s);
Console.WriteLine(s.ReadToEof);

class TrackingStream : MemoryStream
{
    public bool ReadToEof;

    public TrackingStream(MemoryStream stream) : base(stream.ToArray()) { }

    public override async ValueTask<int> ReadAsync(Memory<byte> buffer, CancellationToken cancellationToken = default)
    {
        int r = await base.ReadAsync(buffer, cancellationToken);
        if (r == 0) ReadToEof = true;
        return r;
    }
}

cc: @dotnet/area-system-text-json for comment on whether that change in reading is by design

@ghost
Copy link

ghost commented Nov 9, 2021

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

Issue Details

Description

Deserializing JSON from DeflateStream/ZLibStream with inner stream of CryptoStream seems to cause the CryptoStream to call Flush on its inner stream while disposing. If the inner stream does not permit writes (we are only supposed to be reading from it!), it can lead to a exception to be thrown.

This is most likely related to the breaking changed around partial and zero-byte reads in DeflateStream, GZipStream, and CryptoStream.

Reproduction Steps

using System;
using System.IO;
using System.IO.Compression;
using System.Security.Cryptography;
using System.Text.Json;
using System.Threading;
using System.Threading.Tasks;

static async Task<MemoryStream> Serialize<T>(T test)
{
	MemoryStream stream = new();

	using ToBase64Transform base64Transformer = new();
	await using CryptoStream cryptoStream = new(stream, base64Transformer, CryptoStreamMode.Write, leaveOpen: true);
	await using DeflateStream deflate = new(cryptoStream, CompressionMode.Compress);

	await JsonSerializer.SerializeAsync(deflate, test);

	return stream;
}

static async Task<T> Deserialize<T>(Stream stream)
{
	using FromBase64Transform base64Transformer = new();
	await using CryptoStream cryptoStream = new(stream, base64Transformer, CryptoStreamMode.Read, leaveOpen: true);
	await using DeflateStream deflate = new(cryptoStream, CompressionMode.Decompress);

	return await JsonSerializer.DeserializeAsync<T>(deflate);
}

MemoryStream json = await Serialize("Hello World!");
string output = await Deserialize<string>(new WrapperStream(json));

Console.WriteLine(output);

public sealed class WrapperStream : MemoryStream
{
	public WrapperStream(MemoryStream stream) : base(stream.ToArray())
	{
	}

	public override void Flush() => throw new NotSupportedException();
	public override Task FlushAsync(CancellationToken cancellationToken) => throw new NotSupportedException();
}

Expected behavior

The code should run just fine and not call Flush.

Actual behavior

The code calls Flush and throws NotSupportedException.

Regression?

Regression from .NET 5.

Known Workarounds

No response

Configuration

Running on .NET 6.

Other information

No response

Author: aromaa
Assignees: -
Labels:

area-System.Text.Json, untriaged

Milestone: -

@eiriktsarpalis
Copy link
Member

@stephentoub I ran your reproduction on the debugger, and it seems that it's an issue with DeflateStream. System.Text.Json will read the outer stream to EOF however DeflateStream does not. Likely introduced by #53644.

Here's a reproduction demonstrating that this is the case:

using System;
using System.IO;
using System.IO.Compression;
using System.Security.Cryptography;
using System.Text.Json;
using System.Threading;
using System.Threading.Tasks;

var memoryStream = new InstrumentedMemoryStream(await Serialize("x"));
memoryStream.Position = 0;
(InstrumentedDeflateStream deflateStream, _) = await Deserialize<string>(memoryStream);
Console.WriteLine($"{memoryStream.ReadToEof}, {deflateStream.ReadToEof}"); // False, True

static async Task<MemoryStream> Serialize<T>(T test)
{
    MemoryStream stream = new();
    using ToBase64Transform base64Transformer = new();
    await using CryptoStream cryptoStream = new(stream, base64Transformer, CryptoStreamMode.Write, leaveOpen: true);
    await using DeflateStream deflate = new(cryptoStream, CompressionMode.Compress);
    await JsonSerializer.SerializeAsync(deflate, test);
    return stream;
}

static async Task<(InstrumentedDeflateStream, T?)> Deserialize<T>(Stream stream)
{
    using FromBase64Transform base64Transformer = new();
    using CryptoStream cryptoStream = new(stream, base64Transformer, CryptoStreamMode.Read, leaveOpen: true);
    using InstrumentedDeflateStream deflate = new(cryptoStream, CompressionMode.Decompress);
    var result = await JsonSerializer.DeserializeAsync<T>(deflate);
    return (deflate, result);
}

class InstrumentedMemoryStream : MemoryStream
{
    public bool ReadToEof;

    public InstrumentedMemoryStream(MemoryStream stream) : base(stream.ToArray()) { }

    public override async ValueTask<int> ReadAsync(Memory<byte> buffer, CancellationToken cancellationToken = default)
    {
        int r = await base.ReadAsync(buffer, cancellationToken);
        if (r == 0) ReadToEof = true;
        return r;
    }
}

class InstrumentedDeflateStream : DeflateStream
{
    public bool ReadToEof;

    public InstrumentedDeflateStream(Stream stream, CompressionMode mode) : base(stream, mode) { }

    public override async ValueTask<int> ReadAsync(Memory<byte> buffer, CancellationToken cancellationToken = default)
    {
        int r = await base.ReadAsync(buffer, cancellationToken);
        if (r == 0) ReadToEof = true;
        return r;
    }
}

@ghost
Copy link

ghost commented Nov 10, 2021

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

Deserializing JSON from DeflateStream/ZLibStream with inner stream of CryptoStream seems to cause the CryptoStream to call Flush on its inner stream while disposing. If the inner stream does not permit writes (we are only supposed to be reading from it!), it can lead to a exception to be thrown.

This is most likely related to the breaking changed around partial and zero-byte reads in DeflateStream, GZipStream, and CryptoStream.

Reproduction Steps

using System;
using System.IO;
using System.IO.Compression;
using System.Security.Cryptography;
using System.Text.Json;
using System.Threading;
using System.Threading.Tasks;

static async Task<MemoryStream> Serialize<T>(T test)
{
	MemoryStream stream = new();

	using ToBase64Transform base64Transformer = new();
	await using CryptoStream cryptoStream = new(stream, base64Transformer, CryptoStreamMode.Write, leaveOpen: true);
	await using DeflateStream deflate = new(cryptoStream, CompressionMode.Compress);

	await JsonSerializer.SerializeAsync(deflate, test);

	return stream;
}

static async Task<T> Deserialize<T>(Stream stream)
{
	using FromBase64Transform base64Transformer = new();
	await using CryptoStream cryptoStream = new(stream, base64Transformer, CryptoStreamMode.Read, leaveOpen: true);
	await using DeflateStream deflate = new(cryptoStream, CompressionMode.Decompress);

	return await JsonSerializer.DeserializeAsync<T>(deflate);
}

MemoryStream json = await Serialize("Hello World!");
string output = await Deserialize<string>(new WrapperStream(json));

Console.WriteLine(output);

public sealed class WrapperStream : MemoryStream
{
	public WrapperStream(MemoryStream stream) : base(stream.ToArray())
	{
	}

	public override void Flush() => throw new NotSupportedException();
	public override Task FlushAsync(CancellationToken cancellationToken) => throw new NotSupportedException();
}

Expected behavior

The code should run just fine and not call Flush.

Actual behavior

The code calls Flush and throws NotSupportedException.

Regression?

Regression from .NET 5.

Known Workarounds

No response

Configuration

Running on .NET 6.

Other information

No response

Author: aromaa
Assignees: -
Labels:

area-System.IO.Compression, area-System.Text.Json, untriaged

Milestone: -

@jozkee jozkee added this to the .NET 7.0 milestone Nov 24, 2021
@jozkee jozkee removed the untriaged New issue has not been triaged by the area owner label Nov 24, 2021
@karelz karelz modified the milestones: .NET 7.0, 7.0.0 Nov 25, 2021
@jeffhandley jeffhandley modified the milestones: 7.0.0, Future Aug 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants