From b541b87f8ef244b81a0c1d7e937b002d35b4aef5 Mon Sep 17 00:00:00 2001 From: Dean Ellis Date: Mon, 3 Jun 2024 10:04:22 +0100 Subject: [PATCH] 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 xamarin/xamarin-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 --- .vscode/tasks.json | 20 ++------- LibZipSharp.UnitTest/ZipTests.cs | 38 ++++++++++++++++- LibZipSharp.props | 2 +- LibZipSharp/Xamarin.Tools.Zip/ZipArchive.cs | 45 +++++++++++---------- azure-pipelines.yml | 5 +++ 5 files changed, 69 insertions(+), 41 deletions(-) diff --git a/.vscode/tasks.json b/.vscode/tasks.json index 5ad1cd2..fecea5f 100644 --- 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" - ] - } ] } \ No newline at end of file diff --git a/LibZipSharp.UnitTest/ZipTests.cs b/LibZipSharp.UnitTest/ZipTests.cs index 64367c2..a7642e8 100644 --- 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 index d0e4180..623188d 100644 --- a/LibZipSharp.props +++ b/LibZipSharp.props @@ -1,7 +1,7 @@ <_LibZipSharpAssemblyVersionMajor>3 - <_LibZipSharpAssemblyVersionMinor>2 + <_LibZipSharpAssemblyVersionMinor>3 <_LibZipSharpAssemblyVersionPatch>0 <_LibZipSharpAssemblyVersion>$(_LibZipSharpAssemblyVersionMajor).$(_LibZipSharpAssemblyVersionMinor).$(_LibZipSharpAssemblyVersionPatch) <_NativeLibraryVersionForName>$(_LibZipSharpAssemblyVersionMajor)-$(_LibZipSharpAssemblyVersionMinor) diff --git a/LibZipSharp/Xamarin.Tools.Zip/ZipArchive.cs b/LibZipSharp/Xamarin.Tools.Zip/ZipArchive.cs index 66b31f9..abead13 100644 --- 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,9 +196,13 @@ ErrorCode Open (string path, OpenFlags flags) /// /// The stream to open /// Platform-specific options - public static ZipArchive Open (Stream stream, IPlatformOptions options = null) + /// + 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); } /// @@ -205,9 +210,12 @@ public static ZipArchive Open (Stream stream, IPlatformOptions options = null) /// /// The stream to create the arhive in /// Platform-specific options - 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); } /// @@ -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,22 +842,18 @@ 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: @@ -856,7 +861,7 @@ internal static unsafe Int64 stream_callback (IntPtr state, IntPtr data, UInt64 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 index ca31b51..42f40d8 100644 --- 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: