Fix the elusive invalid zip archive issue that has been a problem for…

… ages! (#142) * Fix the elusive invalid zip archive issue that has been a problem for ages! Fixes dotnet/android#8988 We had this odd corrupt zip file issue which kept cropping up on our Azure Pipelines builds. We had no idea what caused it until now. Some of the data for the local headers of an item (not the central directory) would be written incorrectly. This would result in a zip which may or may not be extractable, it would depend on how resilient the software extracting the data would be. So, what was happening here was that (sometimes) libzip would start writing some data (most likely the local file header) using our stream source callback, and it would seek a few bytes into the data and then tried to seek back to the beginning. The latter seek was done by giving the seek operation of the callback an offset of 0 which, unfortunately, was also used by the code as a guard as to whether or not to even perform the seek operation. The effect was that we ignored the seek to 0 and the stream remained at whatever the previous seek location was requested, thus corrupting data. It happened only on the very first entry, since that was the only one which would have position 0 within its range. We discovered that just enabling the strict consistency checks would uncover the issue, so that has been enabled in a number of unit tests. Once we did that it turns out we were writting the corrupt data ALL the TIME!. Fixing up the seeking code to take into account that we might want to see to 0 fixed the issue. * Bump to 3.3.0 due to ABI changes
dotnet · Jun 3, 2024 · b541b87 · b541b87
1 parent c2ae332
commit b541b87
Show file tree

Hide file tree

Showing 5 changed files with 69 additions and 41 deletions.
diff --git a/.vscode/tasks.json b/.vscode/tasks.json
@@ -6,7 +6,7 @@
         {
             "label": "Build LibZipSharp",
             "type": "shell",
-            "command": "msbuild LibZipSharp/libZipSharp.csproj /restore /t:Build",
+            "command": "dotnet build LibZipSharp/libZipSharp.csproj -t:Build",
             "group": {
                 "kind": "build",
                 "isDefault": true
@@ -18,7 +18,7 @@
         {
             "label": "Pack LibZipSharp Nuget",
             "type": "shell",
-            "command": "msbuild LibZipSharp/libZipSharp.csproj /t:Pack",
+            "command": "dotnet pack LibZipSharp/libZipSharp.csproj",
             "group": {
                 "kind": "build",
                 "isDefault": true
@@ -30,7 +30,7 @@
         {
             "label": "Clean LibZipSharp",
             "type": "shell",
-            "command": "msbuild LibZipSharp/libZipSharp.csproj /restore /t:Clean",
+            "command": "dotnet build LibZipSharp/libZipSharp.csproj -t:Clean",
             "group": {
                 "kind": "build",
                 "isDefault": true
@@ -42,7 +42,7 @@
         {
             "label": "Build LibZipSharp Unit Tests",
             "type": "shell",
-            "command": "msbuild LibZipSharp.UnitTest/LibZipSharp.UnitTest.csproj /restore /t:Build /p:ReferenceNuget=False",
+            "command": "dotnet build LibZipSharp.UnitTest/LibZipSharp.UnitTest.csproj -t:Build -p:ReferenceNuget=False",
             "group": {
                 "kind": "build",
                 "isDefault": true
@@ -51,17 +51,5 @@
                 "$msCompile"
             ]
         },
-        {
-            "label": "Run LibZipSharp Unit Tests",
-            "type": "shell",
-            "command": "msbuild LibZipSharp.UnitTest/LibZipSharp.UnitTest.csproj /restore /t:RunNunitTests /p:ReferenceNuget=False",
-            "group": {
-                "kind": "test",
-                "isDefault": true
-            },
-            "problemMatcher": [
-                "$msCompile"
-            ]
-        }
     ]
 }
diff --git a/LibZipSharp.UnitTest/ZipTests.cs b/LibZipSharp.UnitTest/ZipTests.cs
@@ -345,7 +345,7 @@ public void InMemoryZipFile ()
 				}
 
 				stream.Position = 0;
-				using (var zip = ZipArchive.Open (stream)) {
+				using (var zip = ZipArchive.Open (stream, strictConsistencyChecks: true)) {
 					Assert.AreEqual (3, zip.EntryCount);
 					foreach (var e in zip) {
 						Console.WriteLine (e.FullName);
@@ -354,7 +354,7 @@ public void InMemoryZipFile ()
 				}
 
 				stream.Position = 0;
-				using (var zip = ZipArchive.Open (stream)) {
+				using (var zip = ZipArchive.Open (stream, strictConsistencyChecks: true)) {
 					Assert.AreEqual (2, zip.EntryCount);
 					zip.AddEntry ("info.json", File.ReadAllText (filePath), Encoding.UTF8, CompressionMethod.Deflate);
 				}
@@ -377,6 +377,40 @@ public void InMemoryZipFile ()
 			}
 		}
 
+		[Test]
+		public void EnumerateOnEmptyThenAddFiles ()
+		{
+			File.WriteAllText ("file1.txt", TEXT);
+			File.WriteAllText ("file2.txt", TEXT);
+			string filePath = Path.GetFullPath ("file1.txt");
+			if (File.Exists ("enumerateonempty.zip"))
+				File.Delete ("enumerateonempty.zip");
+
+
+			using (var stream = File.Create ("test-archive-write.zip"))
+			using (var zip = ZipArchive.Create (stream)) {
+				foreach (var entry in zip) {
+					Console.Write (entry.FullName);
+				}
+				ZipEntry e;
+				e = zip.AddFile (filePath, "/path/ZipTestCopy1.exe");
+				filePath = Path.GetFullPath ("file2.txt");
+				e = zip.AddFile (filePath, "/path/ZipTestCopy2.exe");
+
+				foreach (var entry in zip) {
+					Console.Write (entry.FullName);
+				}
+
+				zip.Close ();
+			}
+
+			using (var zip = ZipArchive.Open ("test-archive-write.zip", FileMode.Open, strictConsistencyChecks: true)) {
+				foreach (var entry in zip) {
+					Console.Write (entry.FullName);
+				}
+			}
+		}
+
 		[TestCase (false)]
 		[TestCase (true)]
 		public void EnumerateSkipDeletedEntries (bool deleteFromExistingFile)

diff --git a/LibZipSharp.props b/LibZipSharp.props
@@ -1,7 +1,7 @@
 <Project DefaultTargets="Build" ToolsVersion="4.0" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
   <PropertyGroup>
     <_LibZipSharpAssemblyVersionMajor>3</_LibZipSharpAssemblyVersionMajor>
-    <_LibZipSharpAssemblyVersionMinor>2</_LibZipSharpAssemblyVersionMinor>
+    <_LibZipSharpAssemblyVersionMinor>3</_LibZipSharpAssemblyVersionMinor>
     <_LibZipSharpAssemblyVersionPatch>0</_LibZipSharpAssemblyVersionPatch>
     <_LibZipSharpAssemblyVersion>$(_LibZipSharpAssemblyVersionMajor).$(_LibZipSharpAssemblyVersionMinor).$(_LibZipSharpAssemblyVersionPatch)</_LibZipSharpAssemblyVersion>
     <_NativeLibraryVersionForName>$(_LibZipSharpAssemblyVersionMajor)-$(_LibZipSharpAssemblyVersionMinor)</_NativeLibraryVersionForName>

diff --git a/LibZipSharp/Xamarin.Tools.Zip/ZipArchive.cs b/LibZipSharp/Xamarin.Tools.Zip/ZipArchive.cs
@@ -28,6 +28,7 @@
 using System.Buffers;
 using System.Collections;
 using System.Collections.Generic;
+using System.Diagnostics;
 using System.IO;
 using System.Runtime.InteropServices;
 using System.Text;
@@ -195,19 +196,26 @@ ErrorCode Open (string path, OpenFlags flags)
 		/// </summary>
 		/// <param name="stream">The stream to open</param>
 		/// <param name="options">Platform-specific options</param>
-		public static ZipArchive Open (Stream stream, IPlatformOptions options = null)
+		/// <param name="strictConsistencyChecks"></param>
+		public static ZipArchive Open (Stream stream, IPlatformOptions options = null, bool strictConsistencyChecks = false)
 		{
-			return ZipArchive.CreateInstanceFromStream (stream, OpenFlags.None, options);
+			OpenFlags flags = OpenFlags.None;
+			if (strictConsistencyChecks)
+				flags |= OpenFlags.CheckCons;
+			return ZipArchive.CreateInstanceFromStream (stream, flags, options);
 		}
 
 		/// <summary>
 		/// Create a new archive using the Stream provided. The steam should be an empty stream, any existing data will be overwritten.
 		/// </summary>
 		/// <param name="stream">The stream to create the arhive in</param>
 		/// <param name="options">Platform-specific options</param>
-		public static ZipArchive Create (Stream stream, IPlatformOptions options = null)
+		public static ZipArchive Create (Stream stream, IPlatformOptions options = null, bool strictConsistencyChecks = false)
 		{
-			return ZipArchive.CreateInstanceFromStream (stream, OpenFlags.Create | OpenFlags.Truncate, options);
+			OpenFlags flags = OpenFlags.Create | OpenFlags.Truncate;
+			if (strictConsistencyChecks)
+				flags |= OpenFlags.CheckCons;
+			return ZipArchive.CreateInstanceFromStream (stream, flags, options);
 		}
 
 		/// <summary>
@@ -788,6 +796,7 @@ internal ZipException GetErrorException ()
 		internal static unsafe Int64 stream_callback (IntPtr state, IntPtr data, UInt64 len, SourceCommand cmd)
 		{
 			byte [] buffer = null;
+			Native.zip_error_t error;
 			int length = (int)len;
 			var handle = GCHandle.FromIntPtr (state);
 			if (!handle.IsAllocated)
@@ -833,30 +842,26 @@ internal static unsafe Int64 stream_callback (IntPtr state, IntPtr data, UInt64
 						return -1;
 					}
 
-					long firstOffset, secondOffset;
+					SeekOrigin seek = Native.ConvertWhence (args.whence);
 					if (args.offset > Int64.MaxValue) {
 						// Stream.Seek uses a signed 64-bit value for the offset, we need to split it up
-						firstOffset = Int64.MaxValue;
-						secondOffset = (long)(args.offset - Int64.MaxValue);
+						if (!Seek (destination, seek, Int64.MaxValue) || !Seek (destination, seek, (long)(args.offset - Int64.MaxValue))) {
+							return -1;
+						}
 					} else {
-						firstOffset = (long)args.offset;
-						secondOffset = 0;
-					}
-
-					SeekOrigin seek = Native.ConvertWhence (args.whence);
-					Native.zip_error_t error;
-
-					if (!SeekIfNeeded (destination, seek, firstOffset) || !SeekIfNeeded (destination, seek, secondOffset)) {
-						return -1;
+						if (!Seek (destination, seek, (long)args.offset)) {
+							return -1;
+						}
 					}
+
 					break;
 
 				case SourceCommand.Seek:
 					long offset = Native.zip_source_seek_compute_offset ((UInt64)stream.Position, (UInt64)stream.Length, data, len, out error);
 					if (offset < 0) {
 						return offset;
 					}
-					if (offset != stream.Seek (offset, SeekOrigin.Begin)) {
+					if (!Seek (stream, SeekOrigin.Begin, offset)) {
 						return -1;
 					}
 					break;
@@ -962,12 +967,8 @@ internal static unsafe Int64 stream_callback (IntPtr state, IntPtr data, UInt64
 
 			return 0;
 
-			bool SeekIfNeeded (Stream s, SeekOrigin origin, long offset)
+			bool Seek (Stream s, SeekOrigin origin, long offset)
 			{
-				if (offset == 0) {
-					return true;
-				}
-
 				return s.Seek (offset, origin) == offset;
 			}
 		}

diff --git a/azure-pipelines.yml b/azure-pipelines.yml
@@ -105,6 +105,11 @@ extends:
                 archiveType: 7z
                 replaceExistingArchive: true
                 archiveFile: $(Build.ArtifactStagingDirectory)\libzip-windows-arm-x64.7z
+            - script: |
+                7z t $(Build.ArtifactStagingDirectory)\libzip-windows-arm-x64.7z
+                7z t $(Build.ArtifactStagingDirectory)\libzip-windows-x64.7z
+                7z t $(Build.ArtifactStagingDirectory)\libzip-windows-x86.7z
+              displayName: Test Archives
 
         - job: buildLinux
           pool: