Fixing a possible null reference error in WebSocket deflate.

[zlatanov]

There is a possible situation (although I've been unable to reproduce) when a deflate with FlushCode.NoFlush flush code consumes the entire output buffer but also doesn't return a buffer error to indicate that it needs more output. In this case the WebSocket would happily call Flush with an empty Span<byte> buffer, which when used in fixed statement would result in null pointer.

@QuinnDamerell is there any way you could try and run your code against this build, or this error happens only in production environment?

Fixes #62422

[ghost]

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

Issue Details

---

There is a possible situation (although I've been unable to reproduce) when a deflate with FlushCode.NoFlush flush code consumes the entire output buffer but also doesn't return a buffer error to indicate that it needs more output. In this case the WebSocket would happily call Flush with an empty Span<byte> buffer, which when used in fixed statement would result in null pointer.

@QuinnDamerell is there any way you could try and run your code against this build, or this error happens only in production environment?

Fixes #62422

Author:	zlatanov
Assignees:	-
Labels:	area-System.Net
Milestone:	-

[stephentoub]

So we have tests that exercise all of these conditions?

[zlatanov]

The first two yes. They are easy to be tested when we provide payload that is incompressible.

The written == output.Length I couldn't reproduce, but it is my only conclusion on how could an empty Span<byte> buffer reach UnsafeFlush.

[zlatanov]

@stephentoub, if you're interested, I was able to reproduce this, but I had to reduce the memory level with which the deflater is initialized. By default it's 8 (Deflate_DefaultMemLevel) and I changed it to 1 so I can iterate faster with different buffers.

In order to change it, modify line 213 to memLevel: 1

runtime/src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/Compression/WebSocketDeflater.cs

Lines 204 to 215 in 47b9cb5

	private ZLibStreamHandle CreateDeflater()
	{
	ZLibStreamHandle stream;
	ErrorCode errorCode;
	try
	{
	errorCode = CreateZLibStreamForDeflate(out stream,
	level: CompressionLevel.DefaultCompression,
	windowBits: _windowBits,
	memLevel: Deflate_DefaultMemLevel,
	strategy: CompressionStrategy.DefaultStrategy);
	}

Now run the following test:

[Fact]
public async Task Issue()
{
    WebSocketTestStream stream = new();
    using WebSocket server = WebSocket.CreateFromStream(stream, new WebSocketCreationOptions
    {
        IsServer = true,
        KeepAliveInterval = TimeSpan.Zero,
        DangerousDeflateOptions = new()
    });

    var blob = new byte[16136];
    new Random(0).NextBytes(blob);

    await server.SendAsync(blob, WebSocketMessageType.Binary, endOfMessage: true, CancellationToken);
    stream.Remote.Clear();
}

[CarnaViire]

@zlatanov I wonder, if the condition for having an empty span is so specific, why the failure is not easily reproducible? And how does memLevel affect that?

[zlatanov]

I wonder, if the condition for having an empty span is so specific, why the failure is not easily reproducible? And how does memLevel affect that?

The memory level for the deflater represents how much memory it is allowed to use internally before flushing to output buffer. Larger memory, better performance and better compression ratios - the classic trade-off between speed and memory. The default is 8, which in turn means that the deflater is allowed to allocate up to 2^8 KB of memory.

So for example, if we try to deflate 10_000 bytes of data in a single deflate call, it deflater in most cases will not produce any output unless we explicitly say we want it to. In our case it is essential to avoid flushing and only flush when we're done deflating.

However there might be cases when the deflater has reached some limit in its internal buffer and will flush bytes. It might even flush as many bytes as we've given it opportunity to. My original assumption was that the deflater will always return ErrorCode.BufError when the provided buffer is full and internally there is more outstanding data, but because we're not explicitly specifying that we're flushing, I guess my assumption is wrong.

This is why working with memLevel 1 = 2KB internal memory is easier to reproduce the error - I can craft messages, more easily, that would exhaust its internal memory and cause it to flush.

I hope this makes sense, let me know if I can help further.

[zlatanov]

@ManickaP should we go ahead and proceed merging this into main so it can be also backported for NET 6? @QuinnDamerell reports no more errors with the build that you've provided.

@stephentoub I was unable to produce such data that would trigger this behavior so a test could be written.

[ManickaP]

Let's merge this.
@karelz should we start 6.0 port?

[karelz]

Sounds reasonable to me.
@CarnaViire do you have any additional concerns? If not, can you please help push it forward?

[CarnaViire]

Thanks!

[CarnaViire]

/backport to release/6.0

[github-actions]

Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/1572912323