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

Fix IOStreamStreamReader to account for alignment padding #1047

Merged

Conversation

josalem
Copy link
Contributor

@josalem josalem commented Nov 13, 2019

When reading a non-seekable stream, the IOStreamStreamReader class can be put in a state where it attempts to read a count greater than length - offset of its internal buffer.

simple repro
class Program
    {
        public static int align = 8;
        public static int size = 0x4000;

        static void Main(string[] args)
        {
            using var memoryStream = new MemoryStream();

            for (int i = 0; i < 20 * size; i++)
            {
                memoryStream.WriteByte(0xFD);
            }

            memoryStream.Seek(0, SeekOrigin.Begin);
            var nonSeekableStream = new NonSeekableStream(memoryStream);

            var dst = new byte[size * 2];
            using var streamReader = new IOStreamStreamReader(nonSeekableStream);

            // Put the stream reader in a bad state
            streamReader.Read(dst, 0, 14); // Read a length in (8,16)

            streamReader.Read(dst, 0, size + align); // read a length in (size, size + align]
        }
    }

This happens because the constructor for the class pads the buffer size by align bytes so that it has room to round up the number of bytes read to an aligned value. The reading logic doesn't preserve the padding and uses the requestedSize + align length for some of its logic which is how we get into this corner case. It is a rare occurrence, but this has been what is causing CI failures in CoreCLR (see: dotnet/coreclr#26241).

I'm going to be testing this locally with EventPipe workloads (a main use case for this class).

CC - @tommcdon, @brianrob, @noahfalk, @sywhang

of the alignment padding in its buffer
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants