Breakage when files are added to archive in the "wrong" order

[mrclayman]

Hi guys,

I have been battling for a few days with a curious issue. My use case is one where I need to replace the contents of one (JSON) text file after fetching it from the archive, deserializing it into a dedicated class instance, making the necessary changes and then serializing the class instance back to a new file and adding it to the archive as a new entry. When I do that, however, I get the following exception

Unhandled exception. System.IO.IOException: An attempt was made to move the position before the beginning of the stream.
   at System.IO.MemoryStream.Seek(Int64 offset, SeekOrigin loc)
   at Xamarin.Tools.Zip.ZipArchive.stream_callback(IntPtr state, IntPtr data, UInt64 len, SourceCommand cmd)

I have spent a good few hours getting a debug environment working and tracing through the native libzip library code. It does indeed try to seek to an apparently nonsensical position within the stream when trying to read extra fields for the archive entries at one point (we're talking position shifts by tens of thousands of bytes in an archive that is just barely over a kilobyte in compressed size). I don't have the details on the problematic spot in libzip with me at the moment, but I should be able to provide them easily if need be.

In addition to that, I have discovered that the order in which I add files to the archive does matter to LibZipSharp/libzip. To demonstrate that, I have put together a simple project. See this GitHub repository for the source code and test files. The info.json file is the one getting read and subsequently replaced in the archive.

During my experiments, I tried different orders of files on the command line to see the impact of those changes. I use the standard zip command in Linux. I have tried these permutations of files on the command line.

zip -9 object.zip characters_players.json info.json object_spawn.json,
zip -9 object.zip object_spawn.json info.json characters_players.json,
zip -9 object.zip info.json object_spawn.json characters_players.json, and
zip -9 object.zip object_spawn.json characters_players.json info.json
produce an archive file that the test application is able to successfully process and run to completion.

However, commands
zip -9 object.zip info.json characters_players.json object_spawn.json and
zip -9 object.zip characters_players.json object_spawn.json info.json
produce an archive file that the app fails to process and fails with the traceback listed above.

To complicate this even more, I found that when I try to iterate through the entries of the archive and then do the read/update/replace operation, most of the archive variations created by the commands above run to completion except

zip -9 object.zip info.json object_spawn.json characters_players.json and
zip -9 object.zip object_spawn.json characters_players.json info.json.

Lines 38-39 are commented out and uncommenting them will print the contents of the archive to stdout together with position information of the archive stream.

That's really all I can think of at the moment. I hope this helps. Let me know if you need anything else. 🙂

[dellis1972]

Given one of the problematic zip files you can narrow down the test case to

var data = GetInputFileData ();
using (var stream = new MemoryStream ()) {
    stream.Write (data);
    stream.Position = 0;
    using (var zip = ZipArchive.Open (stream)) {
        zip.DeleteEntry (InfoJson);
    }
}

Interestingly. If you open the zip using ZipArchive.Open (InputFile) directly rather than going through a stream, it works as expected as the file is deleted.

Need to do some more investigation.

[dellis1972]

@mrclayman something to remember about how libzip works. Changes to zip files are not "committed" until the archive is closed. All modifications happen in memory.

What this means is that until you call archive.Close() none of the changes will take effect, close is called automatically by the Dispose call.
We get around this in Xamarin.Android by closing then re-opening the archive. So something like

stream.Position = 0;
using (var zip = ZipArchive.Open (stream)) {
	Assert.AreEqual (3, zip.EntryCount);
	zip.DeleteEntry ("Entry1");
        //Assert.AreEqual (2, zip.EntryCount); // This will Fail as the count will still be 3
}

stream.Position = 0;
using (var zip = ZipArchive.Open (stream)) {
	Assert.AreEqual (2, zip.EntryCount); // This will now pass 
}

[mrclayman]

Thanks for the feedback, guys. @dellis1972 , I thought as much since the library does not have any facility to obtain the end result (especially when working with an in-memory stream). Anyway, although it's not apparent from the traceback I showed in my original post, the exception that breaks the app is raised during the execution of the Close() method of ZipArchive. I figure you will see the same when you try to trace the test app.

I am tempted to write a small test C app with the same execution flow as the C# one, just to see if the same issue occurs while using only the pure libzip library. In the meantime, if you guys find something, I'm all ears. 🙂 Thanks very much for your time!

[dellis1972]

@mrclayman just to keep you posted. I have managed to create a repo using the files you provided. You can track our progress at #103. It is beginning to look like its a bug in libzip itself, we will continue to investigate but we might need to a native repo so we can create an issue at https://github.com/nih-at/libzip.
So if you made any progress in creating a native repo please let us know :)

[mrclayman]

Thanks very much for the update, @dellis1972 . 😊 I have not begun with it yet, but seeing as you guys are making progress, I will do my best to whip up one ASAP. I will let you know when I have it, for sure. 👍

[dellis1972]

@mrclayman don't worry abou the C example. I put one together.

[dellis1972]

@mrclayman looks like we figured out the probem. Just going though the code review process now. Once that is done and its merged we'll be releasing a new Nuget with the fix, probably tomorrow.

[mrclayman]

Awesome! Thanks for the heads-up, @dellis1972 . 😊