Fix bug in ZipArchiveEntry data descriptor (#37601)

carlossanlop · web-flow · commit 05ada9bc9674 · 2019-05-14T14:26:12.000-07:00
* 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.
diff --git a/src/System.IO.Compression/src/System/IO/Compression/ZipArchiveEntry.cs b/src/System.IO.Compression/src/System/IO/Compression/ZipArchiveEntry.cs
@@ -823,6 +823,8 @@ private bool WriteLocalFileHeader(bool isEmptyFile)
                 }
                 else // if we are not in streaming mode, we have to decide if we want to write zip64 headers
                 {
+                    // We are in seekable mode so we will not need to write a data descriptor
+                    _generalPurposeBitFlag &= ~BitFlagValues.DataDescriptor;
                     if (SizesTooLarge()
 #if DEBUG_FORCE_ZIP64
                         || (_archive._forceZip64 && _archive.Mode == ZipArchiveMode.Update)
@@ -1020,6 +1022,9 @@ private void WriteCrcAndSizesInLocalHeader(bool zip64HeaderUsed)
 
         private void WriteDataDescriptor()
         {
+            // We enter here because we cannot seek, so the data descriptor bit should be on
+            Debug.Assert((_generalPurposeBitFlag & BitFlagValues.DataDescriptor) != 0);
+
             // data descriptor can be 32-bit or 64-bit sizes. 32-bit is more compatible, so use that if possible
             // signature is optional but recommended by the spec
 
@@ -1231,7 +1236,6 @@ protected override void Dispose(bool disposing)
                         // go back and finish writing
                         if (_entry._archive.ArchiveStream.CanSeek)
                             // finish writing local header if we have seek capabilities
-
                             _entry.WriteCrcAndSizesInLocalHeader(_usedZip64inLH);
                         else
                             // write out data descriptor if we don't have seek capabilities
diff --git a/src/System.IO.Compression/tests/ZipArchive/zip_CreateTests.cs b/src/System.IO.Compression/tests/ZipArchive/zip_CreateTests.cs
@@ -149,10 +149,58 @@ public static void CreateUncompressedArchive()
             }
         }
 
+        [Fact]
+        public static void CreateNormal_VerifyDataDescriptor()
+        {
+            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);
+
+            // 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))
+            {
+                CreateEntry(archive, "A", "xxx");
+                CreateEntry(archive, "B", "yyy");
+            }
+
+            AssertDataDescriptor(memoryStream, true);
+
+            // Update should flip the data descriptor bit to zero on save
+            using (var archive = new ZipArchive(memoryStream, ZipArchiveMode.Update))
+            {
+                ZipArchiveEntry entry = archive.Entries[0];
+                using Stream entryStream = entry.Open();
+                StreamReader reader = new StreamReader(entryStream);
+                string content = reader.ReadToEnd();
+                
+                // Append a string to this entry
+                entryStream.Seek(0, SeekOrigin.End);
+                StreamWriter writer = new StreamWriter(entryStream);
+                writer.Write("zzz");
+                writer.Flush();
+            }
+
+            AssertDataDescriptor(memoryStream, false);
+        }
+
         private static string ReadStringFromSpan(Span<byte> input)
         {
             return Text.Encoding.UTF8.GetString(input.ToArray());
         }
+
+        private static void CreateEntry(ZipArchive archive, string fileName, string fileContents)
+        {
+            ZipArchiveEntry entry = archive.CreateEntry(fileName);
+            using StreamWriter writer = new StreamWriter(entry.Open());
+            writer.Write(fileContents);
+        }
+
+        private static void AssertDataDescriptor(MemoryStream memoryStream, bool hasDataDescriptor)
+        {
+            byte[] fileBytes = memoryStream.ToArray();
+            Assert.Equal(hasDataDescriptor ? 8 : 0, fileBytes[6]);
+            Assert.Equal(0, fileBytes[7]);
+        }
     }
 }
 
