dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.32k stars 4.74k forks source link

PipeWriter.UnflushedBytes `long` property wraps to negative numbers after `int.MaxValue` #109513

Open AArnott opened 1 week ago

AArnott commented 1 week ago

Description

The PipeWriter.UnflushedBytes property is a (signed) long and may report negative numbers. I wonder if it has wrapped around to the negative range. This surely will cause malfunctions in apps that want to take action based on the number being above some threshold.

Reproduction Steps

Run this simple console app on .NET 8 for several seconds, then break into a debugger:

PipeWriter writer = PipeWriter.Create(Stream.Null);
while (true)
{
    Memory<byte> buffer = writer.GetMemory();
    if (buffer.Length < 47)
    {
        writer.Advance(21);
        buffer = writer.GetMemory(47);
        writer.Advance(14);
    }
    else
    {
        writer.Advance(35);
    }
}

Expected behavior

I expected UnflushedBytes to accurately represent the count, and in particular always be non-negative to avoid malfunction in the application.

Actual behavior

The UnflushedBytes property has a value of -1981799737.

Image

After another iteration of the loop, the property value transitioned like so: -1981799737 -> -1981799702 That indeed added 35 bytes, making the magnitude of the negative number smaller.

Regression?

No response

Known Workarounds

Don't let the unflushed bytes climb high enough to wrap around?

Configuration

.NET 8 on Windows x64.

Other information

Seems to me the property should have been a ulong and/or checked operators should be used to guard against this.

AArnott commented 1 week ago

When I add this to the loop:

if (writer.UnflushedBytes < 0)
{
    Debugger.Break();
}

It breaks fairly quickly, with UnflushedBytes set to -2147483636. That's a lot smaller than long.MinValue. In fact it's very close to int.MinValue == -2147483648. Looks to me that the wraparound problem may be around some other 32-bit signed integer even though the property is exposed as a long.

In fact when I record the last positive value before it dives to a negative number, I can see the last positive value is 2147483625.

BrennanConroy commented 1 week ago

Looks like it's because the backing field in the StreamPipeWriter is an int https://source.dot.net/#System.IO.Pipelines/System/IO/Pipelines/StreamPipeWriter.cs,23

BrennanConroy commented 6 days ago

What is the scenario where you're hitting this? Trying to judge how likely someone is to hit this. You would need to buffer 2GB without flushing in order to hit this scenario which seems unlikely.

AArnott commented 5 days ago

I'm writing a serializer. A very large amount of data may be serialized before the serializer has a chance to flush, depending on the types being serialized. In my case, I hit this accidentally while exploring what I thought was another bug. So I don't have an immediate application where 2GB is expected to be written between flushes. But it wouldn't be a bug for someone to write a custom converter that wrote that much without flushing.