Use Stream.ReadAtLeast when loading ZipArchives
#114256
Conversation
dotnet-policy-service
bot
added
the
community-contribution
|
Tagging subscribers to this area: @dotnet/area-system-io-compression |
There was a problem hiding this comment.
Thanks for this change, @edwardneal. I left some comments/questions for you to consider.
| Stream clamped = new ClampedReadStream(stream, readSizeLimit: 1); | ||
|
|
||
| IsZipSameAsDir(clamped, zfolder(zipFolder), ZipArchiveMode.Read, requireExplicit: true, checkTimes: true); | ||
| Assert.False(clamped.CanRead, "Clamped stream should be closed at this point"); //check that it was closed |
There was a problem hiding this comment.
Could you please explain this check? Is CanRead supposed to change in the BaseStream of ClampedReadStream if the stream is properly disposed?
There was a problem hiding this comment.
The TestPartialReads test aligned with the TestStreamingRead test above it, but I agree - making sure that the stream is closed isn't really a focus of the test. I've removed it, so now it's equivalent to the ReadNormal test.
src/libraries/System.IO.Compression/tests/System.IO.Compression.Tests.csproj
Outdated
Show resolved
Hide resolved
| @@ -557,8 +557,10 @@ public static bool TrySkipBlock(Stream stream) | |||
| { | |||
| Span<byte> blockBytes = stackalloc byte[4]; | |||
There was a problem hiding this comment.
I don't think we need the assert below:
| Span<byte> blockBytes = stackalloc byte[4]; | |
| Span<byte> blockBytes = stackalloc byte[FieldLengths.Signature]; |
|
|
||
| Debug.Assert(blockBytes.Length == FieldLengths.Signature); |
There was a problem hiding this comment.
Just make the blockBytes length the expected size:
| Debug.Assert(blockBytes.Length == FieldLengths.Signature); |
| @@ -572,7 +574,8 @@ public static bool TrySkipBlock(Stream stream) | |||
| // Already read the signature, so make the filename length field location relative to that | |||
| stream.Seek(FieldLocations.FilenameLength - FieldLengths.Signature, SeekOrigin.Current); | |||
|
|
|||
| bytesRead = stream.Read(blockBytes); | |||
| Debug.Assert(blockBytes.Length == FieldLengths.FilenameLength + FieldLengths.ExtraFieldLength); | |||
There was a problem hiding this comment.
I'm not sure I understand this debug. Wasn't blockBytes.Length fixed to 4 bytes? The debug above (which I suggested to remove) was checking a different length, but I don't see that blockBytes is getting replaced with an array of different size.
There was a problem hiding this comment.
I've added them because blockBytes is used twice: once to store the four signature bytes, and once to hold the filename length & extra field length (which also both fill four bytes.) The asserts were to make it clearer that this was happening, but I've added a comment too. I can remove the assertions and just use the comment if you prefer
src/libraries/System.IO.Compression/src/System/IO/Compression/ZipBlocks.cs
Show resolved
Hide resolved
src/libraries/System.IO.Compression/tests/Utilities/ClampedReadStream.cs
Show resolved
Hide resolved
| @@ -61,7 +61,6 @@ public static async Task TestPartialReads(string zipFile, string zipFolder) | |||
| Stream clamped = new ClampedReadStream(stream, readSizeLimit: 1); | |||
|
|
|||
| IsZipSameAsDir(clamped, zfolder(zipFolder), ZipArchiveMode.Read, requireExplicit: true, checkTimes: true); | |||
| Assert.False(clamped.CanRead, "Clamped stream should be closed at this point"); //check that it was closed | |||
There was a problem hiding this comment.
I was thinking about this and since you're using your own custom clamped stream, You could override Dispose so that it sets the value of a public property called IsDisposed (besides also doing the usual disposing tasks) and then in this test you can check the value of that property to see if we disposed it. But it's ok as it is currently. Just a note for the future.
Fixes #114026. Swaps references to
Stream.Readout withStream.ReadAtLeast, adds a test to prove the correct behaviour.In every case except for ZipArchive, we use a pattern of calling
Stream.ReadAtLeast(Span, Span.Length, throwOnEndOfStream: false)rather thanStream.ReadExactly(Span). The surrounding code checks the number of bytes read and returns false, and I didn't want to change that pattern.The ZipArchive case takes place when reading the central directory, and it makes sure to always read enough bytes for the constant component of a ZipCentralDirectoryFileHeader.
/cc @carlossanlop