.NET 7 regression in ZipArchive

[jonathanpeppers]

Description

I ran into this trying to get MAUI workloads running on .NET 7. Our CI caught some test failures here.

Append to an existing ZipEntry with code such as:

using var fileStream = File.Open(path, FileMode.Open, FileAccess.ReadWrite);
using var zip = new ZipArchive(fileStream, ZipArchiveMode.Update);

var oldEntry = zip.GetEntry("foo.txt");
ArgumentNullException.ThrowIfNull(oldEntry);
var newEntry = zip.CreateEntry("foo.txt");

using var newStream = newEntry.Open();

using (var oldStream = oldEntry.Open())
{
    oldStream.CopyTo(newStream);
}

using var writer = new StreamWriter(newStream);
writer.Write("foo");

oldEntry.Delete();

In .NET 6 the code works, in .NET 7 it throws:

Message: 
System.InvalidOperationException : An entry named 'foo.txt' already exists in the archive.

Stack Trace: 
ZipArchive.DoCreateEntry(String entryName, Nullable`1 compressionLevel)
ZipArchive.CreateEntry(String entryName)
ZipArchiveTest.AppendToZip(String path) line 42
ZipArchiveTest.Test1() line 17
RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
RuntimeMethodInfo.InvokeNonEmitUnsafe(Object obj, IntPtr* arguments, Span`1 argsForTemporaryMonoSupport, BindingFlags invokeAttr)

Reproduction Steps

Here is an xunit project targeting both .NET 6 and .NET 7: zippy.zip

If you just unzip and run dotnet test you'll see it passes on 6 and fails in 7.

Expected behavior

ZipEntry.CreateEntry() doesn't throw in this example.

Actual behavior

ZipEntry.CreateEntry() throws an exception.

Regression?

Yes, works in .NET 6.

Known Workarounds

There may be a different way to write the above code, but I ported this example from existing code.

Configuration

> dotnet --version
7.0.100-preview.5.22229.2

I'm running on Windows 11, but this also happens on macOS where I saw this on CI.

Other information

I think this may have been introduced in: #60973

[ghost]

Tagging subscribers to this area: @dotnet/area-system-io-compression
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Description

I ran into this trying to get MAUI workloads running on .NET 7. Our CI caught some test failures here.

Append to an existing ZipEntry with code such as:

using var fileStream = File.Open(path, FileMode.Open, FileAccess.ReadWrite);
using var zip = new ZipArchive(fileStream, ZipArchiveMode.Update);

var oldEntry = zip.GetEntry("foo.txt");
ArgumentNullException.ThrowIfNull(oldEntry);
var newEntry = zip.CreateEntry("foo.txt");

using var newStream = newEntry.Open();

using (var oldStream = oldEntry.Open())
{
    oldStream.CopyTo(newStream);
}

using var writer = new StreamWriter(newStream);
writer.Write("foo");

oldEntry.Delete();

In .NET 6 the code works, in .NET 7 it throws:

Message: 
System.InvalidOperationException : An entry named 'foo.txt' already exists in the archive.

Stack Trace: 
ZipArchive.DoCreateEntry(String entryName, Nullable`1 compressionLevel)
ZipArchive.CreateEntry(String entryName)
ZipArchiveTest.AppendToZip(String path) line 42
ZipArchiveTest.Test1() line 17
RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
RuntimeMethodInfo.InvokeNonEmitUnsafe(Object obj, IntPtr* arguments, Span`1 argsForTemporaryMonoSupport, BindingFlags invokeAttr)

Reproduction Steps

Here is an xunit project targeting both .NET 6 and .NET 7: zippy.zip

If you just unzip and run dotnet test you'll see it passes on 6 and fails in 7.

Expected behavior

ZipEntry.CreateEntry() doesn't throw in this example.

Actual behavior

ZipEntry.CreateEntry() throws an exception.

Regression?

Yes, works in .NET 6.

Known Workarounds

There may be a different way to write the above code, but I ported this example from existing code.

Configuration

> dotnet --version
7.0.100-preview.5.22229.2

I'm running on Windows 11, but this also happens on macOS where I saw this on CI.

Other information

I think this may have been introduced in: #60973

Author:	jonathanpeppers
Assignees:	-
Labels:	area-System.IO.Compression, untriaged
Milestone:	-

[danmoseley]

@jonathanpeppers Can you work around this or are you actively blocked?

[jonathanpeppers]

Let me check on Monday the actual number of test failures this causes. If it’s a small number we might be able to disable some tests for now.

[danmoseley]

The docs do say " If an entry with the specified path and name already exists in the archive, a second entry is created with the same path and name." That should be considered (if it wasn't already) when we evaluate the breaking change. Although, it seems that the previous behavior was buggy, which is why it was disabled.

I would imagine that the code above could be changed to write the old data into a MemoryStream (or even file) so that you could remove the old entry before writing the new one.

[jonathanpeppers]

So these tests are tricky, they are integration tests that fail in an MSBuild task inside a NuGet package. We'd have to patch the package and push it somewhere.

It looked like I can just add a category on 4 tests to skip them for now. This should be ok, because we will continue running these tests for Xamarin.Android projects alongside .NET 7 Android tests.

[danmoseley]

Cool. My inclination would be to leave the code as is, pending further feedback on previews. (And document the breaking change and fix the docs) cc @dotnet/area-system-io-compression

[jozkee]

I also believe the breaking change should stay cc @carlossanlop.

@jonathanpeppers can you use a different implementation to append to a zip file (avoiding having a duplicated entry)? e.g:

using var fileStream = File.Open(path, FileMode.Open, FileAccess.ReadWrite);
using var zip = new ZipArchive(fileStream, ZipArchiveMode.Update);

var entry = zip.GetEntry("foo.txt");
ArgumentNullException.ThrowIfNull(entry);
//var newEntry = zip.CreateEntry("foo.txt");

using var stream = entry.Open();
stream.Seek(0, SeekOrigin.End);
//using (var oldStream = oldEntry.Open())
//{
//    oldStream.CopyTo(newStream);
//}

using var writer = new StreamWriter(stream);
writer.Write("foo");

//oldEntry.Delete();

[jonathanpeppers]

We ended up deleting the problematic code and updated the NuGet package.

We are unblocked here, so now it just depends on how many customers will hit this, and if it is an acceptable breaking change. Completely up to you, thanks!

[jozkee]

Closing for now, we can reopen if someone brings a convincing scenario (unable to workaround, probably).

[akoeplinger]

@jozkee would you mind providing a bit more detail about why this breaking change is acceptable? E.g. does it violate the ZIP specs etc. and how widespread we think it is.

Should we apply the breaking-change label to track getting this added to https://docs.microsoft.com/en-us/dotnet/core/compatibility/7.0 ?

[carlossanlop]

@jozkee can you create the breaking change issue in dotnet/docs?https://github.com/dotnet/docs/issues/new?assignees=gewarren&labels=breaking-change%2CPri1%2Cdoc-idea&template=breaking-change.yml&title=%5BBreaking+change%5D%3A+

Here's the PR that introduced this breaking change: #60973

[jozkee]

@akoeplinger the problem was that .NET's zip impl. allows to create multiple entries with the same name, but if you attempt multiple operations on archives in such state, some operations won't work e.g: if you attempt to extract the archive with ZipFile.ExtractToDirectory("archive.zip", "archive"); it will throw The file 'duplicate-entry.txt' already exists. archive.GetEntry(duplicateEntryName); also is unable to retrieve duplicate entries, because it is backed by a Dictionary<string, ZipArchiveEntry>.

However duplicate entries are also OK in the spec. I have seen many libraries/tools disallowing this scenario, e.g. 7zip.

The only way to see duplicate entries is on the actual entry list, (archive.Entries), and that still works. The changed only broke creating them.

Again, I think we can undo the change if there's an scenario for duplicate entries. From a quick search, I've seen mixed opinions on both sides.

[stephentoub]

However duplicate entries are also OK in the spec

If it's allowed by the spec, I don't think we should be enforcing our own policy to the contrary. If we allow it, a higher level tool could choose its own policy here using our APIs, but if we prohibit it, there's no wiggle room.

[TonyValenti]

Glad to see this fixed!

The change prohibiting duplicate entries broke interesting code used in steganographic contexts.

[jozkee]

Thanks to @jonathanpeppers for consuming our previews and reporting the issue.