Fix bug in ZipArchiveEntry data descriptor#37601
Fix bug in ZipArchiveEntry data descriptor#37601carlossanlop merged 4 commits intodotnet:masterfrom carlossanlop:datadescriptor
Conversation
…ls) when data descriptor bit is turned on in a seekable file that is being updated.
…ile that was created with an unseekable stream.
src/System.IO.Compression/src/System/IO/Compression/ZipArchiveEntry.cs
Outdated
Show resolved
Hide resolved
src/System.IO.Pipes/tests/NamedPipeTests/NamedPipeTest.Specific.cs
Outdated
Show resolved
Hide resolved
… the data descriptor.
|
Ran an automation job that targets the Compression unit tests: Edit: It passed. |
| { | ||
| using var memoryStream = new MemoryStream(); | ||
| // We need an non-seekable stream so the data descriptor bit is turned on when saving | ||
| var wrappedStream = new WrappedStream(memoryStream, true, true, false, null); |
There was a problem hiding this comment.
nit, not worth going back and changing now, but when passing booleans into a method, it's nice to name them since it often makes it clearer what's going on. eg Foo(bar, baz, enableFrob:true)
| { | ||
| ZipArchiveEntry entry = archive.Entries[0]; | ||
| using Stream entryStream = entry.Open(); | ||
| StreamReader reader = new StreamReader(entryStream); |
There was a problem hiding this comment.
technically this Reader is a disable object that will close the stream when disposed. If the GC decided to collect after the last reference to reader it would close your MemoryStream and your other calls can fail with ObjectDisposed.
| ZipArchiveEntry entry = archive.Entries[0]; | ||
| using Stream entryStream = entry.Open(); | ||
| StreamReader reader = new StreamReader(entryStream); | ||
| string content = reader.ReadToEnd(); |
There was a problem hiding this comment.
nit: you read the stream, didn't do anything with the output, and also seeked to the end (which I believe is a noop if you read to the end already). Maybe just delete this code that's using the reader and just keep the seek call.
|
|
||
| // Append a string to this entry | ||
| entryStream.Seek(0, SeekOrigin.End); | ||
| StreamWriter writer = new StreamWriter(entryStream); |
There was a problem hiding this comment.
I believe StreamWriter has the same problem as the reader, you need to specify leaveOpen = true
| var wrappedStream = new WrappedStream(memoryStream, true, true, false, null); | ||
|
|
||
| // Creation will go through the path that sets the data descriptor bit when the stream is unseekable | ||
| using (var archive = new ZipArchive(wrappedStream, ZipArchiveMode.Create)) |
There was a problem hiding this comment.
ZipArchive usage here and below also need leaveOpen=true.
* Prevent data corruption (can't drag drop files using external zip tools) when data descriptor bit is turned on in a seekable file that is being updated. * Add unit test to verify data descriptor is off after updating a zip file that was created with an unseekable stream. Commit migrated from dotnet/corefx@05ada9b
5 participants
Fixes https://github.com/dotnet/corefx/issues/29872
When a zip file is created using an unseekable stream, the data descriptor bit is turned on. If we update that same zip file, we should expect the data descriptor to get turned off. If it doesn't get turned off, the file gets in a bad state: we can open it with File Explorer or 7-Zip without problems, but we cannot drag-drop files into that zip file anymore; those programs say the file is corrupted.
Added a unit test to verify the data descriptor value is the expected one in the first local file header, after creating a zip file with an unseekable stream and after updating the zip file.