Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix enumerating zip with deleted entries #53

Merged
merged 6 commits into from
May 15, 2020
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 60 additions & 0 deletions LibZipSharp.UnitTest/ZipTests.cs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
using NUnit.Framework;
using System;
using System.Collections.Generic;
using System.IO;
using System.Text;
using System.Threading;
Expand Down Expand Up @@ -157,5 +158,64 @@ public void SmallTextFile ()
}
}
}

[TestCase(false)]
[TestCase(true)]
public void EnumerateSkipDeletedEntries(bool deleteFromExistingFile)
{
var ms = new MemoryStream (Encoding.UTF8.GetBytes(TEXT));
File.WriteAllText ("file1.txt", "1111");
string filePath = Path.GetFullPath("file1.txt");
if (File.Exists ("test-archive-write.zip"))
File.Delete ("test-archive-write.zip");

ZipArchive zip = null;
try {
zip = ZipArchive.Open ("test-archive-write.zip", FileMode.CreateNew);

ZipEntry e;
e = zip.AddFile (filePath, "/path/ZipTestCopy1.exe");
e = zip.AddFile (filePath, "/path/ZipTestCopy2.exe");
var text = "Hello World";
e = zip.AddEntry ("/data/foo1.txt", text, Encoding.UTF8, CompressionMethod.Store);
e = zip.AddEntry ("/data/foo2.txt", File.OpenRead(filePath), CompressionMethod.Store);

if (deleteFromExistingFile) {
zip.Close ();
zip = ZipArchive.Open ("test-archive-write.zip", FileMode.Open);
}

ValidateEnumeratedEntries (zip, "path/ZipTestCopy1.exe", "path/ZipTestCopy2.exe", "data/foo1.txt", "data/foo2.txt");

// Delete first
zip.DeleteEntry ("path/ZipTestCopy1.exe");
ValidateEnumeratedEntries (zip, "path/ZipTestCopy2.exe", "data/foo1.txt", "data/foo2.txt");

// Delete last
zip.DeleteEntry ("data/foo2.txt");
ValidateEnumeratedEntries (zip, "path/ZipTestCopy2.exe", "data/foo1.txt");

// Delete middle
zip.DeleteEntry ("path/ZipTestCopy2.exe");
ValidateEnumeratedEntries (zip, "data/foo1.txt");

// Delete all
zip.DeleteEntry ("data/foo1.txt");
ValidateEnumeratedEntries (zip);
}
finally {
zip?.Dispose();
}
}

void ValidateEnumeratedEntries (ZipArchive zip, params string[] expectedEntries)
{
var actualEntries = new List<string>();
foreach (var entry in zip) {
actualEntries.Add(entry.FullName);
}

Assert.AreEqual(expectedEntries, actualEntries.ToArray());
}
}
}
39 changes: 34 additions & 5 deletions ZipArchive.cs
Original file line number Diff line number Diff line change
Expand Up @@ -368,8 +368,8 @@ public ZipEntry AddFileToDirectory (string sourcePath, string archiveDirectory =
string destDir = NormalizeArchivePath (true, archiveDirectory);
string destFile = useFileDirectory ? GetRootlessPath (sourcePath) : Path.GetFileName (sourcePath);
return AddFile (sourcePath,
String.IsNullOrEmpty (destDir) ? null : destDir + "/" + destFile,
permissions, compressionMethod, overwriteExisting);
String.IsNullOrEmpty (destDir) ? null : destDir + "/" + destFile,
permissions, compressionMethod, overwriteExisting);
}

/// <summary>
Expand Down Expand Up @@ -691,12 +691,41 @@ public ZipEntry ReadEntry (string entryName, bool caseSensitive = false)
return ReadEntry ((ulong)LookupEntry (entryName, caseSensitive));
}

public ZipEntry ReadEntry (ulong index)
public ZipEntry ReadEntry(ulong index)
{
return ReadEntry (index, throwIfDeleted: true);
}

/// <summary>
/// Read a zip entry, given an index.
///
/// When throwIfDeleted is true, if the entry is deleted then an exception is thrown (the error will be
/// ErrorCode.Deleted or ErrorCode.Inval, depending on whether the deleted entry previously existed in
/// the zip or was newly added - that's just how libzip handles that). If throwIfDeleted is false then
/// null is returned for deleted entries and an exception is just thrown for other errors.
/// </summary>
/// <param name="index">index to read</param>
/// <param name="throwIfDeleted">whether to return null or throw an exception for deleted entries</param>
/// <returns></returns>
public ZipEntry ReadEntry (ulong index, bool throwIfDeleted)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Presumably at some point in the future, we'll want to enable C#8 Nullable Reference Types in this repo.

Should we do so, this method would need to return ZipEntry?.

An alternative approach would be to leave this method unchanged as ZipEntry ReadEntry(long index), and add a new method which returns a ZipEntry?.

Thus, a "new' method instead of a ReadEntry() overload.

Question is, what should said new method be named?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We leave it with the same name. Overloads don't need to have identical return types.

{
Native.zip_stat_t stat;
int ret = Native.zip_stat_index (archive, index, OperationFlags.None, out stat);
if (ret < 0)
throw GetErrorException ();
if (ret < 0) {
if (! throwIfDeleted) {
IntPtr error = Native.zip_get_error(archive);

if (error != IntPtr.Zero) {
int zip_error = Native.zip_error_code_zip(error);
// Deleted is returned when the deleted entry existed when the zip was opened
// Inval is returned when the deleted entry was newly added to the zip, then deleted
if (zip_error == (int) ErrorCode.Deleted || zip_error == (int)ErrorCode.Inval)
return null;
}
}

throw GetErrorException();
}

var ze = ZipEntry.Create (this, stat);
ze.Init ();
Expand Down
12 changes: 10 additions & 2 deletions ZipEntryEnumerator.cs
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,15 @@ public bool MoveNext ()

// Calling it each time because the archive can change in the meantime
long nentries = archive.EntryCount;
if (nentries < 0 || index >= (ulong)nentries)
if (nentries < 0)
return false;

// Skip past any deleted entires
while (index < (ulong)nentries && ReadEntry(index) == null) {
++index;
}

if (index >= (ulong)nentries)
return false;
return true;
}
Expand All @@ -87,7 +95,7 @@ ZipEntry ReadEntry (ulong index)
if (current != null && current.Index == index)
return current;

current = archive.ReadEntry (index);
current = archive.ReadEntry (index, throwIfDeleted:false);
return current;
}
}
Expand Down
Empty file modified build_native
100755 → 100644
Empty file.
Empty file modified build_windows
100755 → 100644
Empty file.