Data corruption in BinaryReader.ReadChars

[carlossanlop]

This issue was first reported internally as a bug in .NET Framework, but it is also reproducible in .NET Core.

• .NET Framework code: https://referencesource.microsoft.com/#mscorlib/system/io/binaryreader.cs,344
• .NET Core 2.2 code: dotnet/coreclr/blob/v2.2.6/src/mscorlib/src/System/IO/BinaryReader.cs#L358
• .NET Core 3.0 code: dotnet/coreclr/blob/v3.0.0-preview8.19379.2/src/System.Private.CoreLib/shared/System/IO/BinaryReader.cs#L388
• .NET Core 5.0 code: dotnet/corefx/blob/2fac89f63c644ab81d936d62c91e6003302e718d/src/Common/src/CoreLib/System/IO/BinaryReader.cs#L362

UTF-16

When using a 2-byte encoding (i.e. UTF-16), and _stream.Read() returns an uneven number of bytes (e.g. a network stream), then in the next iteration of the loop, numBytes can be off by one, and the next read can go past the end of the encoded data in the stream. This can either cause an unexpected end of stream, and/or corruption of any subsequent data read from it.

The repro below is supposed to print all "True"s, as it does when a MemoryStream is used. However, when using a mock NetStream that simulates short reads, data corruption occurs.

Update: It seems that this issue has always been there, so it's not a new regression.

class Program
{
    class NetStream : MemoryStream
    {
        public override int Read(byte[] buffer, int offset, int count)
        {
            return base.Read(buffer, offset, count > 10 ? count - 3 : count);
        }

        public override int Read(Span<byte> destination)
        {
            return base.Read(destination.Length > 10 ? destination.Slice(0, destination.Length - 3) : destination);
        }
    }

    static void Main(string[] args)
    {
        char[] data1 = "hello world".ToCharArray();
        uint data2 = 0xABCDEF01;
        uint data3 = 0;

        using (Stream stream = new NetStream())
        {
            using (BinaryWriter writer = new BinaryWriter(stream, Encoding.Unicode, leaveOpen: true))
            {
                writer.Write(data1);
                writer.Write(data2);
                writer.Write(data3);
            }

            Console.WriteLine(stream.Length == data1.Length * sizeof(char) + 2 * sizeof(uint));

            stream.Seek(0, SeekOrigin.Begin);
            using (BinaryReader reader = new BinaryReader(stream, encoding: Encoding.Unicode, leaveOpen: true))
            {
                char[] data1b = reader.ReadChars(data1.Length);
                Console.WriteLine(data1.AsSpan().SequenceEqual(data1b));
                Console.WriteLine(stream.Position == data1.Length * sizeof(char));

                uint data2b = reader.ReadUInt32();
                Console.WriteLine(data2 == data2b);
                Console.WriteLine(stream.Position == data1.Length * sizeof(char) + sizeof(uint));
            }
        }
    }
}

UTF-8

The code below passes on .NET Framework and .NET Core 2.2, but fails on 3.0, so it's a new regression:

class Program
{
    // version for UTF-8: over-reads if 2 bytes, 3 chars left
    // Fails on .NET Core 3.0, passes on 2.2.
    static void Main(string[] args)
    {
        char[] data1 = "hello world 😃x".ToCharArray(); // 14 code points, 15 chars in UTF-16, 17 bytes in UTF-8
        uint data2 = 0xABCDEF01;
        uint data3 = 0;

        using (Stream stream = new MemoryStream())
        {
            using (BinaryWriter writer = new BinaryWriter(stream, Encoding.UTF8, leaveOpen: true))
            {
                writer.Write(data1);
                writer.Write(data2);
                writer.Write(data3);
            }

            Console.WriteLine(stream.Length == (data1.Length - 1) + 3 + 2 * sizeof(uint));

            stream.Seek(0, SeekOrigin.Begin);
            using (BinaryReader reader = new BinaryReader(stream, encoding: Encoding.UTF8, leaveOpen: true))
            {
                char[] data1b = reader.ReadChars(data1.Length);
                Console.WriteLine(data1.AsSpan().SequenceEqual(data1b));
                Console.WriteLine(stream.Position == (data1.Length - 1) + 3);

                uint data2b = reader.ReadUInt32();
                Console.WriteLine(data2 == data2b);
                Console.WriteLine(stream.Position == (data1.Length - 1) + 3 + sizeof(uint));
            }
        }
    }
}

Remarks

• Both versions of BinaryReader fail the UTF-16 repro.
• The UTF-8 case does not need a short read (i.e. network stream), it fails deterministically on any kind of underlying stream.
• It seems that the reason why the UTF-8 code fails in 3.0 but passes in 2.2, is because the 2.2 runtime uses a version of BinaryReader that has a special case for handling t`his edge case for single-byte encodings:

The special case in 2.2: dotnet/coreclr/blob/ce1d090d33b400a25620c0145046471495067cc7/src/mscorlib/src/System/IO/BinaryReader.cs#L377-L386

...
numBytes = charsRemaining;

// special case for DecoderNLS subclasses when there is a hanging byte from the previous loop
DecoderNLS decoder = _decoder as DecoderNLS;
if (decoder != null && decoder.HasState && numBytes > 1)
{
    numBytes -= 1;
}

if (_2BytesPerChar)
...

No special case in 3.0 and 5.0: dotnet/coreclr/blob/9642de76d4f5e563150a90f5923b304d87d7a8b1/src/System.Private.CoreLib/shared/System/IO/BinaryReader.cs#L387-L389

...
numBytes = charsRemaining;

if (_2BytesPerChar)
...

[carlossanlop]

Since this issue exists since .NET Framework, I will give it the 5.0 milestone.

[stephentoub]

Since this issue exists since .NET Framework, I will give it the 5.0 milestone.

@carlossanlop, you wrote above for the UTF8 issue "The code below passes on .NET Core 2.2 and fails on 3.0 when using UTF-8"... that issue passes or fails on .NET Framework? Regardless, it sounds like a regression from 2.2 that should be addressed in 3.0, no?

[carlossanlop]

It fails on .NET Framework, which is why it was originally opened as an internal bug. Then we tested it on Core, and since it fails, I opened a bug here too.

[stephentoub]

It fails on .NET Framework

I just tried your UTF8 example from above... it outputs true/true/true/true on both .NET Framework 4.8 and .NET Core 2.2, and fails with an exception on .NET Core 3.0.

[carlossanlop]

Ah you're correct. You quoted the text I wrote above the second case.
The first case is the one that fails for all versions.
I'll update the second case example to mention it passes in Framework.

[carlossanlop]

This issue can be split into two:

• The UTF16 problem has always been there.
• The UTF8 was fixed in Framework (I found the changeset that fixed it internally back in 2010), but it got removed for 3.0.

[nagya]

I believe there is only one root cause, with two different symptoms. The code attempts to establish an upper limit for the number of bytes remaining in the stream that represent the char array being read, but it fails to account for the internal state of the decoder being used, and sometimes chooses an upper limit that’s too high. In 3.0, it ignores such state altogether, so the UTF-8 and UTF-16 symptoms manifest. In 2.2, it tries to account for the state, but in a manner that’s only sufficient for UTF-8, not for UTF-16, so only the UTF-16 symptom manifests. However, the root cause in either case is the same.

[jkotas]

The UTF16 problem has always been there.

It is even listed as known problem in the documentation: https://docs.microsoft.com/en-us/dotnet/api/system.io.binaryreader.readchars?view=netcore-3.0#remarks

[carlossanlop]

I ran the UTF8 example in both 2.2 and 3.0 and compared the code that is being executed:

• In both versions, the BinaryReader constructor sets the value of the private readonly Decoder _decoder member by calling encoding.GetDecoder().
• The GetDecoder() method is different between versions:
  • In 2.2, we do the following: return new UTF8Decoder(this);.
  • in 3.0, we do the following: return new DecoderNLS(this);.
• In 2.2 and 3.0, DecoderNLS has a property internal virtual bool HasState, which always returns false.
  • In 2.2, because UTF8Decoder inherits from DecoderNLS, it overrides the HasState property with: return (this.bits != 0);.
  • In 3.0, UTF8Decoder was deleted.
• The UTF8 example pasted above:
  • Passes in 2.2 because we have the special case inside InternalReadChars(), and when it calls HasState, it returns true when there are hanging bytes from the previous loop.
  • Fails in 3.0 because we do not have the special case. Even if I add it back, we will call HasState and it will always return false.
• When we call _decoder.GetChars() at the end of the while loop in InternalReadChars(), it:
  • Passes in 2.2.
  • Throws in 3.0 when we call GetChars. The code is very different to the code we used to call in 2.2.

This is the exception callstack:

        System.ArgumentException : The output char buffer is too small to contain the decoded characters, encoding 'Unicode (UTF-8)' fallback 'System.Text.DecoderReplacementFallback'. (Parameter 'chars')
        Stack Trace:
          D:\coreclr\src\System.Private.CoreLib\shared\System\Text\Encoding.cs(1155,0): at System.Text.Encoding.ThrowCharsOverflow(DecoderNLS decoder, Boolean nothingDecoded)
          D:\coreclr\src\System.Private.CoreLib\shared\System\Text\Encoding.Internal.cs(1211,0): at System.Text.Encoding.GetCharsWithFallback(ReadOnlySpan`1 bytes, Int32 originalBytesLength, Span`1 chars, Int32 originalCharsLength, DecoderNLS decoder)
          D:\coreclr\src\System.Private.CoreLib\shared\System\Text\UTF8Encoding.cs(646,0): at System.Text.UTF8Encoding.GetCharsWithFallback(ReadOnlySpan`1 bytes, Int32 originalBytesLength, Span`1 chars, Int32 originalCharsLength, DecoderNLS decoder)
          D:\coreclr\src\System.Private.CoreLib\shared\System\Text\Encoding.Internal.cs(1165,0): at System.Text.Encoding.GetCharsWithFallback(Byte* pOriginalBytes, Int32 originalByteCount, Char* pOriginalChars, Int32 originalCharCount, Int32 bytesConsumedSoFar, Int32 charsWrittenSoFar, DecoderNLS decoder)
          D:\coreclr\src\System.Private.CoreLib\shared\System\Text\Encoding.Internal.cs(1017,0): at System.Text.Encoding.GetChars(Byte* pBytes, Int32 byteCount, Char* pChars, Int32 charCount, DecoderNLS decoder)
          D:\coreclr\src\System.Private.CoreLib\shared\System\Text\DecoderNLS.cs(144,0): at System.Text.DecoderNLS.GetChars(Byte* bytes, Int32 byteCount, Char* chars, Int32 charCount, Boolean flush)
          D:\coreclr\src\System.Private.CoreLib\shared\System\IO\BinaryReader.cs(443,0): at System.IO.BinaryReader.InternalReadChars(Span`1 buffer)
          D:\coreclr\src\System.Private.CoreLib\shared\System\IO\BinaryReader.cs(475,0): at System.IO.BinaryReader.ReadChars(Int32 count)
          D:\corefx\src\System.IO\tests\BinaryReader\BinaryReaderTests.cs(214,0): at System.IO.Tests.BinaryReaderTests.ReadChars_UTF8()

@jkotas since your PR refactored the BinaryReader/Writer code, can you please take a look at the UTF8 regression?

CC @JeremyKuhne