From 313eca38094d4ded6f816f602f7e5a60e5facd98 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Fri, 15 Oct 2021 09:47:33 +0200 Subject: [PATCH 1/5] add a failing test --- .../tests/FileStream/Read.cs | 34 +++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/src/libraries/System.IO.FileSystem/tests/FileStream/Read.cs b/src/libraries/System.IO.FileSystem/tests/FileStream/Read.cs index b6c555920bca3b..df15e537febb1b 100644 --- a/src/libraries/System.IO.FileSystem/tests/FileStream/Read.cs +++ b/src/libraries/System.IO.FileSystem/tests/FileStream/Read.cs @@ -14,5 +14,39 @@ public void NegativeReadRootThrows() Assert.Throws(() => new FileStream(Path.GetPathRoot(Directory.GetCurrentDirectory()), FileMode.Open, FileAccess.Read)); } + + [Fact] + public void NoInt32OverflowInTheBufferingLogic() + { + const long positon1 = 10; + const long positon2 = (1L << 32) + positon1; + + string filePath = GetTestFilePath(); + byte[] data1 = new byte[] { 1, 2, 3, 4, 5 }; + byte[] data2 = new byte[] { 6, 7, 8, 9, 10 }; + byte[] buffer = new byte[5]; + + using (var stream = new FileStream(filePath, FileMode.Create, FileAccess.Write)) + { + stream.Seek(positon1, SeekOrigin.Begin); + stream.Write(data1, 0, data1.Length); + + stream.Seek(positon2, SeekOrigin.Begin); + stream.Write(data2, 0, data2.Length); + } + + using (var stream = new FileStream(filePath, FileMode.Open, FileAccess.Read)) + { + stream.Seek(positon1, SeekOrigin.Begin); + Assert.Equal(buffer.Length, stream.Read(buffer)); + Assert.Equal(data1, buffer); + + Array.Clear(buffer); + + stream.Seek(positon2, SeekOrigin.Begin); + Assert.Equal(buffer.Length, stream.Read(buffer)); + Assert.Equal(data2, buffer); + } + } } } From 65342059202a5b1201b94bfcf69e68d3fde62bd5 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Fri, 15 Oct 2021 10:08:21 +0200 Subject: [PATCH 2/5] fix the bug in BufferedFileStreamStrategy --- .../src/System/IO/Strategies/BufferedFileStreamStrategy.cs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/BufferedFileStreamStrategy.cs b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/BufferedFileStreamStrategy.cs index 00d0d2a59b9a6a..9de42145c00ded 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/BufferedFileStreamStrategy.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/BufferedFileStreamStrategy.cs @@ -974,11 +974,12 @@ public override long Seek(long offset, SeekOrigin origin) // Otherwise we will throw away the buffer. This can only happen on read, as we flushed write data above. // The offset of the new/updated seek pointer within _buffer: - _readPos = (int)(newPos - (oldPos - _readPos)); + long readPos = (newPos - (oldPos - _readPos)); // If the offset of the updated seek pointer in the buffer is still legal, then we can keep using the buffer: - if (0 <= _readPos && _readPos < _readLen) + if (0 <= readPos && readPos < _readLen) { + _readPos = (int)readPos; // Adjust the seek pointer of the underlying stream to reflect the amount of useful bytes in the read buffer: _strategy.Seek(_readLen - _readPos, SeekOrigin.Current); } From 4df8f9c0c83112cb5880fde8fd5365387c759221 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Fri, 15 Oct 2021 10:20:12 +0200 Subject: [PATCH 3/5] extend the test and fix it in the BufferedStream as well --- .../System.IO.FileSystem/tests/FileStream/Read.cs | 11 ++++++++++- .../src/System/IO/BufferedStream.cs | 5 +++-- 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/src/libraries/System.IO.FileSystem/tests/FileStream/Read.cs b/src/libraries/System.IO.FileSystem/tests/FileStream/Read.cs index df15e537febb1b..fb83c1fe4f4c0a 100644 --- a/src/libraries/System.IO.FileSystem/tests/FileStream/Read.cs +++ b/src/libraries/System.IO.FileSystem/tests/FileStream/Read.cs @@ -41,7 +41,16 @@ public void NoInt32OverflowInTheBufferingLogic() Assert.Equal(buffer.Length, stream.Read(buffer)); Assert.Equal(data1, buffer); - Array.Clear(buffer); + stream.Seek(positon2, SeekOrigin.Begin); + Assert.Equal(buffer.Length, stream.Read(buffer)); + Assert.Equal(data2, buffer); + } + + using (var stream = new BufferedStream(new FileStream(filePath, FileMode.Open, FileAccess.Read, FileShare.None, bufferSize: 0))) + { + stream.Seek(positon1, SeekOrigin.Begin); + Assert.Equal(buffer.Length, stream.Read(buffer)); + Assert.Equal(data1, buffer); stream.Seek(positon2, SeekOrigin.Begin); Assert.Equal(buffer.Length, stream.Read(buffer)); diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/BufferedStream.cs b/src/libraries/System.Private.CoreLib/src/System/IO/BufferedStream.cs index bcdbc292ab415b..c7c1434d69cbae 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/BufferedStream.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/BufferedStream.cs @@ -1222,11 +1222,12 @@ public override long Seek(long offset, SeekOrigin origin) // Otherwise we will throw away the buffer. This can only happen on read, as we flushed write data above. // The offset of the new/updated seek pointer within _buffer: - _readPos = (int)(newPos - (oldPos - _readPos)); + long readPos = (newPos - (oldPos - _readPos)); // If the offset of the updated seek pointer in the buffer is still legal, then we can keep using the buffer: - if (0 <= _readPos && _readPos < _readLen) + if (0 <= readPos && readPos < _readLen) { + _readPos = (int)readPos; // Adjust the seek pointer of the underlying stream to reflect the amount of useful bytes in the read buffer: _stream.Seek(_readLen - _readPos, SeekOrigin.Current); } From 340e596de453f9bd18f4aeb9dbb62c139cd20522 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Fri, 15 Oct 2021 15:39:03 +0200 Subject: [PATCH 4/5] large files are currently not supported on WASM --- src/libraries/System.IO.FileSystem/tests/FileStream/Read.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/libraries/System.IO.FileSystem/tests/FileStream/Read.cs b/src/libraries/System.IO.FileSystem/tests/FileStream/Read.cs index fb83c1fe4f4c0a..fff4284d637682 100644 --- a/src/libraries/System.IO.FileSystem/tests/FileStream/Read.cs +++ b/src/libraries/System.IO.FileSystem/tests/FileStream/Read.cs @@ -16,6 +16,7 @@ public void NegativeReadRootThrows() } [Fact] + [ActiveIssue("https://github.com/dotnet/runtime/issues/45954", TestPlatforms.Browser)] public void NoInt32OverflowInTheBufferingLogic() { const long positon1 = 10; From a6fc59c6e38baa6baa3f79f8a5e9ba1c20157d07 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Fri, 15 Oct 2021 18:58:20 +0200 Subject: [PATCH 5/5] Apply suggestions from code review Co-authored-by: Carlos Sanchez <1175054+carlossanlop@users.noreply.github.com> --- .../tests/FileStream/Read.cs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/libraries/System.IO.FileSystem/tests/FileStream/Read.cs b/src/libraries/System.IO.FileSystem/tests/FileStream/Read.cs index fff4284d637682..c65529b6c39b9e 100644 --- a/src/libraries/System.IO.FileSystem/tests/FileStream/Read.cs +++ b/src/libraries/System.IO.FileSystem/tests/FileStream/Read.cs @@ -19,8 +19,8 @@ public void NegativeReadRootThrows() [ActiveIssue("https://github.com/dotnet/runtime/issues/45954", TestPlatforms.Browser)] public void NoInt32OverflowInTheBufferingLogic() { - const long positon1 = 10; - const long positon2 = (1L << 32) + positon1; + const long position1 = 10; + const long position2 = (1L << 32) + position1; string filePath = GetTestFilePath(); byte[] data1 = new byte[] { 1, 2, 3, 4, 5 }; @@ -29,31 +29,31 @@ public void NoInt32OverflowInTheBufferingLogic() using (var stream = new FileStream(filePath, FileMode.Create, FileAccess.Write)) { - stream.Seek(positon1, SeekOrigin.Begin); + stream.Seek(position1, SeekOrigin.Begin); stream.Write(data1, 0, data1.Length); - stream.Seek(positon2, SeekOrigin.Begin); + stream.Seek(position2, SeekOrigin.Begin); stream.Write(data2, 0, data2.Length); } using (var stream = new FileStream(filePath, FileMode.Open, FileAccess.Read)) { - stream.Seek(positon1, SeekOrigin.Begin); + stream.Seek(position1, SeekOrigin.Begin); Assert.Equal(buffer.Length, stream.Read(buffer)); Assert.Equal(data1, buffer); - stream.Seek(positon2, SeekOrigin.Begin); + stream.Seek(position2, SeekOrigin.Begin); Assert.Equal(buffer.Length, stream.Read(buffer)); Assert.Equal(data2, buffer); } using (var stream = new BufferedStream(new FileStream(filePath, FileMode.Open, FileAccess.Read, FileShare.None, bufferSize: 0))) { - stream.Seek(positon1, SeekOrigin.Begin); + stream.Seek(position1, SeekOrigin.Begin); Assert.Equal(buffer.Length, stream.Read(buffer)); Assert.Equal(data1, buffer); - stream.Seek(positon2, SeekOrigin.Begin); + stream.Seek(position2, SeekOrigin.Begin); Assert.Equal(buffer.Length, stream.Read(buffer)); Assert.Equal(data2, buffer); }