From 33f55d45ebac39c0309f59b6ba0c877d555dc85c Mon Sep 17 00:00:00 2001 From: Fredric Silberberg Date: Wed, 14 May 2025 15:35:18 -0700 Subject: [PATCH 1/7] Fix aligned buffer write This is a particularly subtle bug that requires a chunk (or remaining bit of a chunk) and the destination buffer being _exactly_ the same size. When this happens, the following sequence occurs: 1. `source.Length` and `destination.Length` are exactly aligned. This means that we do not enter the `if` on line 450, and `endOfChunkWritten` will be true. 2. We slice `destination` by `source.Length`. This leaves us with a `destination` that has 0 elements, and `IsEmpty` is true. 3. Because `endOfChunkWritten` is true, we enter the `if` on line 466. This increments us to the next chunk. 4. On line 472 (in the before diff), `destination.IsEmpty` is true, so we enter the `if`, and break out of the loop, never reseting `charIndex` to 0. 5. 4 happens again, this time on line 480 (in the before diff). Again, we do not reset `charIndex` to 0. To fix this, we want to unconditionally reset `charIndex` when we increment a chunk, not wait until after the destination has been checked for empty. --- .../src/Language/CodeGeneration/CodeWriter.cs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Language/CodeGeneration/CodeWriter.cs b/src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Language/CodeGeneration/CodeWriter.cs index cff8797938a..e07586b84f8 100644 --- a/src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Language/CodeGeneration/CodeWriter.cs +++ b/src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Language/CodeGeneration/CodeWriter.cs @@ -466,6 +466,7 @@ public override int Read(char[] buffer, int index, int count) if (endOfChunkWritten) { chunkIndex++; + charIndex = 0; } // Break if we are done writing. chunkIndex and charIndex should have their correct values at this point. @@ -473,8 +474,6 @@ public override int Read(char[] buffer, int index, int count) { break; } - - charIndex = 0; } if (destination.IsEmpty) From 1e3dab5d812008717548b9cae83c14ccf404938e Mon Sep 17 00:00:00 2001 From: Fredric Silberberg Date: Wed, 14 May 2025 15:40:52 -0700 Subject: [PATCH 2/7] Ensure that no more than `count` characters are read into the destination buffer. --- .../src/Language/CodeGeneration/CodeWriter.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Language/CodeGeneration/CodeWriter.cs b/src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Language/CodeGeneration/CodeWriter.cs index e07586b84f8..c6da9fdfe6e 100644 --- a/src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Language/CodeGeneration/CodeWriter.cs +++ b/src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Language/CodeGeneration/CodeWriter.cs @@ -413,7 +413,7 @@ public override int Read(char[] buffer, int index, int count) return -1; } - var destination = buffer.AsSpan(index); + var destination = buffer.AsSpan(index, count); var charsWritten = 0; var page = _page; From 28ce614725b74cc66edd50d500522fbecffedef1 Mon Sep 17 00:00:00 2001 From: Fredric Silberberg Date: Wed, 14 May 2025 16:14:13 -0700 Subject: [PATCH 3/7] Add tests. --- .../CodeGeneration/CSharpCodeWriterTest.cs | 43 +++++++++++++++++++ .../src/Language/CodeGeneration/CodeWriter.cs | 14 ++++-- 2 files changed, 53 insertions(+), 4 deletions(-) diff --git a/src/Compiler/Microsoft.AspNetCore.Razor.Language/test/CodeGeneration/CSharpCodeWriterTest.cs b/src/Compiler/Microsoft.AspNetCore.Razor.Language/test/CodeGeneration/CSharpCodeWriterTest.cs index 2d2133483bc..422691bcd88 100644 --- a/src/Compiler/Microsoft.AspNetCore.Razor.Language/test/CodeGeneration/CSharpCodeWriterTest.cs +++ b/src/Compiler/Microsoft.AspNetCore.Razor.Language/test/CodeGeneration/CSharpCodeWriterTest.cs @@ -6,6 +6,7 @@ using System; using System.Collections.Generic; using Microsoft.AspNetCore.Razor.Language.Intermediate; +using Roslyn.Test.Utilities; using Xunit; namespace Microsoft.AspNetCore.Razor.Language.CodeGeneration; @@ -439,4 +440,46 @@ class C """, output); } + + [Fact, WorkItem("https://github.com/dotnet/core/issues/9885")] + public void AlignedPages_WritesCorrectlyWhenPageAndBufferAreAligned() + { + var pages = new LinkedList[]>(); + + const string FirstLine = "First Line"; + pages.AddLast([(FirstLine + FirstLine).AsMemory(), "Second".AsMemory()]); + + var testWriter = CodeWriter.GetTestTextReader(pages); + var output = new char[FirstLine.Length]; + + testWriter.Read(output, 0, output.Length); + Assert.Equal(output, FirstLine.AsSpan()); + Array.Clear(output); + + testWriter.Read(output, 0, output.Length); + Assert.Equal(output, FirstLine.AsSpan()); + Array.Clear(output); + + testWriter.Read(output, 0, output.Length); + Assert.Equal(output, "Second\0\0\0\0".AsSpan()); + } + + [Fact] + public void ReaderOnlyReadsAsMuchAsRequested() + { + var pages = new LinkedList[]>(); + + const string FirstLine = "First Line"; + pages.AddLast([FirstLine.AsMemory()]); + + var testWriter = CodeWriter.GetTestTextReader(pages); + var output = new char[FirstLine.Length]; + + testWriter.Read(output, 0, 2); + Assert.Equal(output, "Fi\0\0\0\0\0\0\0\0".AsSpan()); + Array.Clear(output); + + testWriter.Read(output, 0, output.Length); + Assert.Equal(output, "rst Line\0\0".AsSpan()); + } } diff --git a/src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Language/CodeGeneration/CodeWriter.cs b/src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Language/CodeGeneration/CodeWriter.cs index c6da9fdfe6e..4f9bca0444a 100644 --- a/src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Language/CodeGeneration/CodeWriter.cs +++ b/src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Language/CodeGeneration/CodeWriter.cs @@ -323,14 +323,20 @@ public CodeWriter WriteLine([InterpolatedStringHandlerArgument("")] ref WriteInt public SourceText GetText() { - using var reader = new Reader(this); + using var reader = new Reader(_pages, Length); return SourceText.From(reader, Length, Encoding.UTF8); } - private sealed class Reader(CodeWriter codeWriter) : TextReader + // Internal for testing + internal static TextReader GetTestTextReader(LinkedList[]> pages) { - private LinkedListNode[]>? _page = codeWriter._pages.First; - private int _remainingLength = codeWriter.Length; + return new Reader(pages, pages.Count); + } + + private sealed class Reader(LinkedList[]> pages, int length) : TextReader + { + private LinkedListNode[]>? _page = pages.First; + private int _remainingLength = length; private int _chunkIndex; private int _charIndex; From 4b0813d9eca9e81519f28da7bba8be5529e0e6f3 Mon Sep 17 00:00:00 2001 From: Fredric Silberberg Date: Wed, 14 May 2025 16:38:25 -0700 Subject: [PATCH 4/7] Simplify code around chunk tracking --- .../src/Language/CodeGeneration/CodeWriter.cs | 18 +++++------------- 1 file changed, 5 insertions(+), 13 deletions(-) diff --git a/src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Language/CodeGeneration/CodeWriter.cs b/src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Language/CodeGeneration/CodeWriter.cs index 4f9bca0444a..1496438f940 100644 --- a/src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Language/CodeGeneration/CodeWriter.cs +++ b/src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Language/CodeGeneration/CodeWriter.cs @@ -449,18 +449,17 @@ public override int Read(char[] buffer, int index, int count) } } - var endOfChunkWritten = true; - // Are we about to write past the end of the buffer? If so, adjust source. // This will be the last chunk we write, so be sure to update charIndex. if (source.Length > destination.Length) { source = source[..destination.Length]; charIndex += source.Length; - - // There's more to this chunk to write! Note this so that we don't update - // chunkIndex later. - endOfChunkWritten = false; + } + else + { + chunkIndex++; + charIndex = 0; } source.CopyTo(destination); @@ -468,13 +467,6 @@ public override int Read(char[] buffer, int index, int count) charsWritten += source.Length; - // Be careful not to increment chunkIndex unless we actually wrote to the end of the chunk. - if (endOfChunkWritten) - { - chunkIndex++; - charIndex = 0; - } - // Break if we are done writing. chunkIndex and charIndex should have their correct values at this point. if (destination.IsEmpty) { From 2984408910dbf9ff7e690388cd141bbb2e84d562 Mon Sep 17 00:00:00 2001 From: Fredric Silberberg Date: Wed, 14 May 2025 16:44:02 -0700 Subject: [PATCH 5/7] .NET Framework build fixes --- .../test/CodeGeneration/CSharpCodeWriterTest.cs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/Compiler/Microsoft.AspNetCore.Razor.Language/test/CodeGeneration/CSharpCodeWriterTest.cs b/src/Compiler/Microsoft.AspNetCore.Razor.Language/test/CodeGeneration/CSharpCodeWriterTest.cs index 422691bcd88..a9ab11579fa 100644 --- a/src/Compiler/Microsoft.AspNetCore.Razor.Language/test/CodeGeneration/CSharpCodeWriterTest.cs +++ b/src/Compiler/Microsoft.AspNetCore.Razor.Language/test/CodeGeneration/CSharpCodeWriterTest.cs @@ -453,15 +453,15 @@ public void AlignedPages_WritesCorrectlyWhenPageAndBufferAreAligned() var output = new char[FirstLine.Length]; testWriter.Read(output, 0, output.Length); - Assert.Equal(output, FirstLine.AsSpan()); - Array.Clear(output); + Assert.Equal(output, FirstLine); + Array.Clear(output, 0, output.Length); testWriter.Read(output, 0, output.Length); - Assert.Equal(output, FirstLine.AsSpan()); - Array.Clear(output); + Assert.Equal(output, FirstLine); + Array.Clear(output, 0, output.Length); testWriter.Read(output, 0, output.Length); - Assert.Equal(output, "Second\0\0\0\0".AsSpan()); + Assert.Equal(output, "Second\0\0\0\0"); } [Fact] @@ -476,10 +476,10 @@ public void ReaderOnlyReadsAsMuchAsRequested() var output = new char[FirstLine.Length]; testWriter.Read(output, 0, 2); - Assert.Equal(output, "Fi\0\0\0\0\0\0\0\0".AsSpan()); - Array.Clear(output); + Assert.Equal(output, "Fi\0\0\0\0\0\0\0\0"); + Array.Clear(output, 0, output.Length); testWriter.Read(output, 0, output.Length); - Assert.Equal(output, "rst Line\0\0".AsSpan()); + Assert.Equal(output, "rst Line\0\0"); } } From 32999e15d80174fc45045570e460f1270d78c46a Mon Sep 17 00:00:00 2001 From: Fredric Silberberg Date: Wed, 14 May 2025 17:21:30 -0700 Subject: [PATCH 6/7] .NET FRAMEWORK!!!!!!!!!!!!!!!!!!!!!!!!1 --- .../test/CodeGeneration/CSharpCodeWriterTest.cs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/Compiler/Microsoft.AspNetCore.Razor.Language/test/CodeGeneration/CSharpCodeWriterTest.cs b/src/Compiler/Microsoft.AspNetCore.Razor.Language/test/CodeGeneration/CSharpCodeWriterTest.cs index a9ab11579fa..f669f432466 100644 --- a/src/Compiler/Microsoft.AspNetCore.Razor.Language/test/CodeGeneration/CSharpCodeWriterTest.cs +++ b/src/Compiler/Microsoft.AspNetCore.Razor.Language/test/CodeGeneration/CSharpCodeWriterTest.cs @@ -453,15 +453,15 @@ public void AlignedPages_WritesCorrectlyWhenPageAndBufferAreAligned() var output = new char[FirstLine.Length]; testWriter.Read(output, 0, output.Length); - Assert.Equal(output, FirstLine); + Assert.Equal(FirstLine, string.Join("", output)); Array.Clear(output, 0, output.Length); testWriter.Read(output, 0, output.Length); - Assert.Equal(output, FirstLine); + Assert.Equal(FirstLine, string.Join("", output)); Array.Clear(output, 0, output.Length); testWriter.Read(output, 0, output.Length); - Assert.Equal(output, "Second\0\0\0\0"); + Assert.Equal("Second\0\0\0\0", string.Join("", output)); } [Fact] @@ -476,10 +476,10 @@ public void ReaderOnlyReadsAsMuchAsRequested() var output = new char[FirstLine.Length]; testWriter.Read(output, 0, 2); - Assert.Equal(output, "Fi\0\0\0\0\0\0\0\0"); + Assert.Equal("Fi\0\0\0\0\0\0\0\0", string.Join("", output)); Array.Clear(output, 0, output.Length); testWriter.Read(output, 0, output.Length); - Assert.Equal(output, "rst Line\0\0"); + Assert.Equal("rst Line\0\0", string.Join("", output)); } } From 593e15bf90c1536e0ff775fff2d429271013fd43 Mon Sep 17 00:00:00 2001 From: Fredric Silberberg Date: Wed, 14 May 2025 17:25:01 -0700 Subject: [PATCH 7/7] Feedback --- .../test/CodeGeneration/CSharpCodeWriterTest.cs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/Compiler/Microsoft.AspNetCore.Razor.Language/test/CodeGeneration/CSharpCodeWriterTest.cs b/src/Compiler/Microsoft.AspNetCore.Razor.Language/test/CodeGeneration/CSharpCodeWriterTest.cs index f669f432466..ad130db29d9 100644 --- a/src/Compiler/Microsoft.AspNetCore.Razor.Language/test/CodeGeneration/CSharpCodeWriterTest.cs +++ b/src/Compiler/Microsoft.AspNetCore.Razor.Language/test/CodeGeneration/CSharpCodeWriterTest.cs @@ -449,18 +449,18 @@ public void AlignedPages_WritesCorrectlyWhenPageAndBufferAreAligned() const string FirstLine = "First Line"; pages.AddLast([(FirstLine + FirstLine).AsMemory(), "Second".AsMemory()]); - var testWriter = CodeWriter.GetTestTextReader(pages); + var testReader = CodeWriter.GetTestTextReader(pages); var output = new char[FirstLine.Length]; - testWriter.Read(output, 0, output.Length); + testReader.Read(output, 0, output.Length); Assert.Equal(FirstLine, string.Join("", output)); Array.Clear(output, 0, output.Length); - testWriter.Read(output, 0, output.Length); + testReader.Read(output, 0, output.Length); Assert.Equal(FirstLine, string.Join("", output)); Array.Clear(output, 0, output.Length); - testWriter.Read(output, 0, output.Length); + testReader.Read(output, 0, output.Length); Assert.Equal("Second\0\0\0\0", string.Join("", output)); } @@ -472,14 +472,14 @@ public void ReaderOnlyReadsAsMuchAsRequested() const string FirstLine = "First Line"; pages.AddLast([FirstLine.AsMemory()]); - var testWriter = CodeWriter.GetTestTextReader(pages); + var testReader = CodeWriter.GetTestTextReader(pages); var output = new char[FirstLine.Length]; - testWriter.Read(output, 0, 2); + testReader.Read(output, 0, 2); Assert.Equal("Fi\0\0\0\0\0\0\0\0", string.Join("", output)); Array.Clear(output, 0, output.Length); - testWriter.Read(output, 0, output.Length); + testReader.Read(output, 0, output.Length); Assert.Equal("rst Line\0\0", string.Join("", output)); } }