throw exception when creating an entry name that already exists in Zi…

…pArchive (#60973) * throw exception when creating an entry name that already exists in ZipArchive * secure code at the top of CreateEntry call * specify entry name in exception message * fix exception message, make AddEntry retrocompatible, and fix test issue with globalization * restore the EmptyEntryTest code as before
dotnet · Dec 1, 2021 · 7a62468 · 7a62468
1 parent c370c2d
commit 7a62468
Show file tree

Hide file tree

Showing 4 changed files with 51 additions and 13 deletions.
diff --git a/src/libraries/System.IO.Compression/src/Resources/Strings.resx b/src/libraries/System.IO.Compression/src/Resources/Strings.resx
@@ -308,4 +308,7 @@
   <data name="Zip64EOCDNotWhereExpected" xml:space="preserve">
     <value>Zip 64 End of Central Directory Record not where indicated.</value>
   </data>
+  <data name="EntryNameAlreadyExists" xml:space="preserve">
+    <value>An entry named '{0}' already exists in the archive.</value>
+  </data>
 </root>
diff --git a/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchive.cs b/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchive.cs
@@ -388,6 +388,11 @@ private ZipArchiveEntry DoCreateEntry(string entryName, CompressionLevel? compre
             if (_mode == ZipArchiveMode.Read)
                 throw new NotSupportedException(SR.CreateInReadMode);
 
+            if (_entriesDictionary.ContainsKey(entryName))
+            {
+                throw new InvalidOperationException(string.Format(SR.EntryNameAlreadyExists, entryName));
+            }
+
             ThrowIfDisposed();
 
 
@@ -421,12 +426,7 @@ internal void AcquireArchiveStream(ZipArchiveEntry entry)
         private void AddEntry(ZipArchiveEntry entry)
         {
             _entries.Add(entry);
-
-            string entryName = entry.FullName;
-            if (!_entriesDictionary.ContainsKey(entryName))
-            {
-                _entriesDictionary.Add(entryName, entry);
-            }
+            _entriesDictionary.TryAdd(entry.FullName, entry);
         }
 
         [Conditional("DEBUG")]

diff --git a/src/libraries/System.IO.Compression/tests/ZipArchive/zip_CreateTests.cs b/src/libraries/System.IO.Compression/tests/ZipArchive/zip_CreateTests.cs
@@ -178,6 +178,23 @@ public static void CreateNormal_VerifyDataDescriptor()
             AssertDataDescriptor(memoryStream, false);
         }
 
+        [Fact]
+        public static void CreateNormal_With2SameEntries_ThrowException()
+        {
+            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);
+
+            // 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))
+            {
+                string entryName = "duplicate.txt";
+                CreateEntry(archive, entryName, "xxx");
+                AssertExtensions.ThrowsContains<InvalidOperationException>(() => CreateEntry(archive, entryName, "yyy"),
+                    entryName);
+            }
+        }
+
         private static string ReadStringFromSpan(Span<byte> input)
         {
             return Text.Encoding.UTF8.GetString(input.ToArray());

diff --git a/src/libraries/System.IO.Compression/tests/ZipArchive/zip_UpdateTests.cs b/src/libraries/System.IO.Compression/tests/ZipArchive/zip_UpdateTests.cs
@@ -92,20 +92,38 @@ public static void EmptyEntryTest(ZipArchiveMode mode)
             baseline = baseline.Clone();
             using (ZipArchive archive = new ZipArchive(baseline, mode))
             {
-                AddEntry(archive, "data1.txt", data1, lastWrite);
+                if (mode == ZipArchiveMode.Create)
+                {
+                    AddEntry(archive, "data1.txt", data1, lastWrite);
 
-                ZipArchiveEntry e = archive.CreateEntry("empty.txt");
-                e.LastWriteTime = lastWrite;
-                using (Stream s = e.Open()) { }
+                    ZipArchiveEntry e = archive.CreateEntry("empty.txt");
+                    e.LastWriteTime = lastWrite;
+                    using (Stream s = e.Open()) { }
+                }
+                else
+                {
+                    Assert.Throws<InvalidOperationException>(() => AddEntry(archive, "data1.txt", data1, lastWrite));
+
+                    Assert.Throws<InvalidOperationException>(() => archive.CreateEntry("empty.txt"));
+                }
             }
 
             test = test.Clone();
             using (ZipArchive archive = new ZipArchive(test, mode))
             {
-                AddEntry(archive, "data1.txt", data1, lastWrite);
+                if (mode == ZipArchiveMode.Create)
+                {
+                    AddEntry(archive, "data1.txt", data1, lastWrite);
 
-                ZipArchiveEntry e = archive.CreateEntry("empty.txt");
-                e.LastWriteTime = lastWrite;
+                    ZipArchiveEntry e = archive.CreateEntry("empty.txt");
+                    e.LastWriteTime = lastWrite;
+                }
+                else
+                {
+                    Assert.Throws<InvalidOperationException>(() => AddEntry(archive, "data1.txt", data1, lastWrite));
+
+                    Assert.Throws<InvalidOperationException>(() => archive.CreateEntry("empty.txt"));
+                }
             }
             //compare
             Assert.True(ArraysEqual(baseline.ToArray(), test.ToArray()), "Arrays didn't match after update");