Skip to content

Commit dbe3236

Browse files
dellis1972dellis1972
authored
Fix corrupt data when Deleting entries from zip files. (#103)
Fixes #102 The initial report showed the following error ``` 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) ``` The really odd thing was that this only happened if the example files were added to the archive in a certain order. This pointed to some kind of memory corruption or stream offset issue. Digging into the documentation for [zip_source_function]](https://libzip.org/documentation/zip_source_function.html#DESCRIPTION) we can being to see what the problem is. Our current implementation of this user callback treats the SourceCommand's of `ZIP_SOURCE_BEGIN_WRITE` `ZIP_SOURCE_COMMIT_WRITE`, `ZIP_SOURCE_TELL_WRITE` and `ZIP_SOURCE_SEEK_WRITE` in the same way as their non `WRITE` counter parts. In that when we get a `ZIP_SOURCE_SEEK_WRITE` we seek in the current archive `Stream` to the appropriate location. Similarly `ZIP_SOURCE_BEGIN_WRITE` seeks to the start of the current archive `Stream`. This implementation is incorrect. The documentation for `ZIP_SOURCE_BEGIN_WRITE` is as follows ``` Prepare the source for writing. Use this to create any temporary file(s). ``` This suggests that a new temporary file is expected to be created. Also if we look at the following descriptions ZIP_SOURCE_TELL Return the current read offset in the source, like ftell(3). ZIP_SOURCE_TELL_WRITE Return the current write offset in the source, like ftell(3). We can see that there are supposed to be two different offsets. One for reading and one for writing. This leads us to the reason why this problem was occurring. Because both the `READ` and `WRITE` are operating on the exact same `Stream` were are getting into a position where the original data was being overwritten by us deleting an entry. What we should have been doing was creating a temp `Stream` when we got the `ZIP_SOURCE_BEGIN_WRITE` SourceCommand. Then use this temp `Stream` to write all the required changes to before finally overwriting the original `Stream` when we get the `ZIP_SOURCE_COMMIT_WRITE` SourceCommand. This will ensure that the original archive data will remain intact until all the processing is done. So rather than passing in a `GCHandle` to the `Stream` directly , a new class has been introduced. The `CallbackContext` class will be used to pass data between the managed and unmanaged code. It will contain the properties for the `Source` stream as well as `Destination` stream. The `Source` will always be used for reading, the `Destination` (if present) will be used for writing. Now on `ZIP_SOURCE_BEGIN_WRITE` we create a new temp file stream which we will use to create the updated zip file. This new stream will be stored in the `CallbackContext.Destination` property so that all the other `WRITE` based commands will work on it. Finally when we get the `ZIP_SOURCE_COMMIT_WRITE` SourceCommand, we will copy the data to the `CallbackContext.Source` stream. Then finally we will dispose of the `CallbackContext.Destination`.
1 parent b5c90a1 commit dbe3236

File tree

9 files changed

+186
-17
lines changed

9 files changed

+186
-17
lines changed

‎.vscode/settings.json‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,4 @@
11
{
2-
"nxunitExplorer.nunit": "/Users/dean/.nuget/packages/nunit.consolerunner/3.10.0/tools/nunit3-console.exe",
32
"nxunitExplorer.modules": [
43
"LibZipSharp.UnitTest/bin/Debug/net471/LibZipSharp.UnitTest.dll",
54
]

‎LibZipSharp.UnitTest/LibZipSharp.UnitTest.csproj‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,15 @@
4242
<None Include="packaged_resources">
4343
<CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
4444
</None>
45+
<None Include="info.json">
46+
<CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
47+
</None>
48+
<None Include="characters_players.json">
49+
<CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
50+
</None>
51+
<None Include="object_spawn.json">
52+
<CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
53+
</None>
4554
</ItemGroup>
4655

4756
<Target Name="RunNunitTests" DependsOnTargets="Build">

‎LibZipSharp.UnitTest/ZipTests.cs‎

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -257,6 +257,54 @@ public void CheckForUnknownCompressionMethods ()
257257
}
258258
}
259259

260+
[Test]
261+
public void InMemoryZipFile ()
262+
{
263+
string filePath = Path.GetFullPath ("info.json");
264+
if (!File.Exists (filePath)) {
265+
filePath = Path.GetFullPath (Path.Combine ("LibZipSharp.UnitTest", "info.json"));
266+
}
267+
string fileRoot = Path.GetDirectoryName (filePath);
268+
using (var stream = new MemoryStream ()) {
269+
using (var zip = ZipArchive.Create (stream)) {
270+
zip.AddFile (filePath, "info.json");
271+
zip.AddFile (Path.Combine (fileRoot, "characters_players.json"), "characters_players.json");
272+
zip.AddFile (Path.Combine (fileRoot, "object_spawn.json"), "object_spawn.json");
273+
}
274+
275+
stream.Position = 0;
276+
using (var zip = ZipArchive.Open (stream)) {
277+
Assert.AreEqual (3, zip.EntryCount);
278+
foreach (var e in zip) {
279+
Console.WriteLine (e.FullName);
280+
}
281+
zip.DeleteEntry ("info.json");
282+
}
283+
284+
stream.Position = 0;
285+
using (var zip = ZipArchive.Open (stream)) {
286+
Assert.AreEqual (2, zip.EntryCount);
287+
zip.AddEntry ("info.json", File.ReadAllText (filePath), Encoding.UTF8, CompressionMethod.Deflate);
288+
}
289+
290+
stream.Position = 0;
291+
using (var zip = ZipArchive.Open (stream)) {
292+
Assert.AreEqual (3, zip.EntryCount);
293+
Assert.IsTrue (zip.ContainsEntry ("info.json"));
294+
var entry1 = zip.ReadEntry ("info.json");
295+
using (var s = new MemoryStream ()) {
296+
entry1.Extract (s);
297+
s.Position = 0;
298+
using (var sr = new StreamReader (s)) {
299+
var t = sr.ReadToEnd ();
300+
Assert.AreEqual (File.ReadAllText (filePath), t);
301+
}
302+
303+
}
304+
}
305+
}
306+
}
307+
260308
[TestCase (false)]
261309
[TestCase (true)]
262310
public void EnumerateSkipDeletedEntries (bool deleteFromExistingFile)

‎LibZipSharp.UnitTest/characters_players.json‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
{
2+
"players": []
3+
}

‎LibZipSharp.UnitTest/info.json‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
{
2+
"version": 22,
3+
"type": "Object",
4+
"id": "c9bb1a03-cb8a-4f9f-b298-49ee26fa1fd9",
5+
"ownerId": "47b9d124-6421-4055-abf4-b5fc8fbfe784",
6+
"name": "OBJECT NAME",
7+
"description": "",
8+
"blockCount": 9,
9+
"created": "2019-06-21T14:20:42.000Z",
10+
"lastChanged": "0001-01-01T00:00:00.000Z",
11+
"tags": [
12+
{
13+
"value": "some_tag",
14+
"origin": "category"
15+
},
16+
{
17+
"value": "some_other_tag",
18+
"origin": "category"
19+
}
20+
]
21+
}

‎LibZipSharp.UnitTest/object_spawn.json‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
{
2+
"spawnRotationOffset":
3+
{
4+
"pitch": -16.274194717407227,
5+
"yaw": -34.644023895263672,
6+
"roll": 65.772232055664063
7+
},
8+
"spawnScaleOffset":
9+
{
10+
"x": 0.52916890382766724,
11+
"y": 0.52916890382766724,
12+
"z": 0.52916890382766724
13+
},
14+
"bounds":
15+
{
16+
"origin":
17+
{
18+
"x": -9086.74609375,
19+
"y": -6475.60498046875,
20+
"z": 1128.5997314453125
21+
},
22+
"boxExtent":
23+
{
24+
"x": 400.17755126953125,
25+
"y": 474.9906005859375,
26+
"z": 502.65240478515625
27+
},
28+
"sphereRadius": 530.2637939453125
29+
}
30+
}

‎LibZipSharp.props‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
<Project DefaultTargets="Build" ToolsVersion="4.0" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
22
<PropertyGroup>
3-
<_LibZipSharpAssemblyVersion>2.0.1</_LibZipSharpAssemblyVersion>
3+
<_LibZipSharpAssemblyVersion>2.0.2</_LibZipSharpAssemblyVersion>
44
<!--
55
Nuget Version. You can append things like -alpha-1 etc to this value.
66
But always leave the $(_LibZipSharpAssemblyVersion) value at the start.

‎LibZipSharp/Xamarin.Tools.Zip/Native.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -356,7 +356,7 @@ public static extern IntPtr zip_source_function_create (
356356
[MarshalAs (UnmanagedType.FunctionPtr)]zip_source_callback callback, IntPtr user_data, out zip_error_t errorp);
357357

358358
[DllImport (ZIP_LIBNAME, SetLastError = true, CallingConvention = CallingConvention.Cdecl)]
359-
public static extern UInt64 zip_source_seek_compute_offset (UInt64 offset, UInt64 length, IntPtr data, UInt64 data_length, out zip_error_t error);
359+
public static extern Int64 zip_source_seek_compute_offset (UInt64 offset, UInt64 length, IntPtr data, UInt64 data_length, out zip_error_t error);
360360

361361
[DllImport (ZIP_LIBNAME, CallingConvention = CallingConvention.Cdecl)]
362362
public static extern Int64 zip_dir_add (IntPtr archive, IntPtr name, OperationFlags flags);

‎LibZipSharp/Xamarin.Tools.Zip/ZipArchive.cs‎

Lines changed: 73 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,11 @@ namespace Xamarin.Tools.Zip
4040
/// </summary>
4141
public abstract partial class ZipArchive : IDisposable, IEnumerable <ZipEntry>
4242
{
43+
internal class CallbackContext {
44+
public Stream Source { get; set; } = null;
45+
public Stream Destination {get; set;} = null;
46+
public string DestinationFileName {get; set; } = null;
47+
}
4348
public const EntryPermissions DefaultFilePermissions = EntryPermissions.OwnerRead | EntryPermissions.OwnerWrite | EntryPermissions.GroupRead | EntryPermissions.WorldRead;
4449
public const EntryPermissions DefaultDirectoryPermissions = EntryPermissions.OwnerAll | EntryPermissions.GroupRead | EntryPermissions.GroupExecute | EntryPermissions.WorldRead | EntryPermissions.WorldExecute;
4550

@@ -87,9 +92,12 @@ internal ZipArchive (Stream stream, IPlatformOptions options, OpenFlags flags =
8792
throw new ArgumentNullException (nameof (options));
8893
Options = options;
8994
Native.zip_error_t errorp;
90-
var streamHandle = GCHandle.Alloc (stream, GCHandleType.Normal);
91-
IntPtr h = GCHandle.ToIntPtr (streamHandle);
92-
IntPtr source = Native.zip_source_function_create (callback, h, out errorp);
95+
CallbackContext context = new CallbackContext () {
96+
Source = stream,
97+
Destination = null,
98+
};
99+
var contextHandle = GCHandle.Alloc (context, GCHandleType.Normal);
100+
IntPtr source = Native.zip_source_function_create (callback, GCHandle.ToIntPtr (contextHandle), out errorp);
93101
archive = Native.zip_open_from_source (source, flags, out errorp);
94102
if (archive == IntPtr.Zero) {
95103
// error;
@@ -323,7 +331,10 @@ public ZipEntry AddStream (Stream stream, string archivePath, EntryPermissions p
323331
throw new ArgumentNullException (nameof (stream));
324332
sources.Add (stream);
325333
string destPath = EnsureArchivePath (archivePath);
326-
var handle = GCHandle.Alloc (stream, GCHandleType.Normal);
334+
var context = new CallbackContext () {
335+
Source = stream,
336+
};
337+
var handle = GCHandle.Alloc (context, GCHandleType.Normal);
327338
IntPtr h = GCHandle.ToIntPtr (handle);
328339
IntPtr source = Native.zip_source_function (archive, callback, h);
329340
long index = Native.zip_file_add (archive, destPath, source, overwriteExisting ? OperationFlags.Overwrite : OperationFlags.None);
@@ -758,9 +769,13 @@ internal static unsafe Int64 stream_callback (IntPtr state, IntPtr data, UInt64
758769
var handle = GCHandle.FromIntPtr (state);
759770
if (!handle.IsAllocated)
760771
return -1;
761-
var stream = handle.Target as Stream;
772+
var context = handle.Target as CallbackContext;
773+
if (context == null)
774+
return -1;
775+
var stream = context.Source;
762776
if (stream == null)
763777
return -1;
778+
var destination = context.Destination ?? context.Source;
764779
switch (cmd) {
765780
case SourceCommand.Stat:
766781
if (len < (UInt64)sizeof (Native.zip_stat_t))
@@ -773,34 +788,68 @@ internal static unsafe Int64 stream_callback (IntPtr state, IntPtr data, UInt64
773788
return (Int64)sizeof (Native.zip_stat_t);
774789

775790
case SourceCommand.Tell:
776-
case SourceCommand.TellWrite:
777791
return (Int64)stream.Position;
792+
case SourceCommand.TellWrite:
793+
return (Int64)destination.Position;
778794

779795
case SourceCommand.Write:
780796
buffer = ArrayPool<byte>.Shared.Rent (length);
781797
try {
782798
Marshal.Copy (data, buffer, 0, length);
783-
stream.Write (buffer, 0, length);
799+
destination.Write (buffer, 0, length);
784800
return length;
785801
} finally {
786802
ArrayPool<byte>.Shared.Return (buffer);
787803
}
788804

789805
case SourceCommand.SeekWrite:
790-
case SourceCommand.Seek:
791806
Native.zip_error_t error;
792-
UInt64 offset = Native.zip_source_seek_compute_offset ((UInt64)stream.Position, (UInt64)stream.Length, data, len, out error);
793-
stream.Seek ((long)offset, SeekOrigin.Begin);
807+
Int64 offset = Native.zip_source_seek_compute_offset ((UInt64)destination.Position, (UInt64)destination.Length, data, len, out error);
808+
if (offset < 0) {
809+
return offset;
810+
}
811+
if (offset != stream.Seek (offset, SeekOrigin.Begin)) {
812+
return -1;
813+
}
814+
break;
815+
case SourceCommand.Seek:
816+
offset = Native.zip_source_seek_compute_offset ((UInt64)stream.Position, (UInt64)stream.Length, data, len, out error);
817+
if (offset < 0) {
818+
return offset;
819+
}
820+
if (offset != stream.Seek (offset, SeekOrigin.Begin)) {
821+
return -1;
822+
}
794823
break;
795824

796825
case SourceCommand.CommitWrite:
826+
destination.Flush ();
827+
stream.Position = 0;
828+
destination.Position = 0;
829+
stream.SetLength (destination.Length);
830+
destination.CopyTo (stream);
797831
stream.Flush ();
832+
stream.Position = 0;
833+
destination.Dispose ();
834+
context.Destination = null;
835+
if (!string.IsNullOrEmpty (context.DestinationFileName)&& File.Exists (context.DestinationFileName)) {
836+
try {
837+
File.Delete (context.DestinationFileName);
838+
} catch (Exception) {
839+
// we are deleting a temp file. So we can ignore any error.
840+
// it will get cleaned up eventually.
841+
Console.WriteLine ($"warning: Could not delete {context.DestinationFileName}.");
842+
}
843+
context.DestinationFileName = null;
844+
}
845+
break;
846+
case SourceCommand.RollbackWrite:
847+
destination.Dispose ();
848+
context.Destination = null;
798849
break;
799850

800851
case SourceCommand.Read:
801-
if (length > stream.Length - stream.Position) {
802-
length = (int)(stream.Length - stream.Position);
803-
}
852+
length = (int)Math.Min (stream.Length - stream.Position, length);
804853
buffer = ArrayPool<byte>.Shared.Rent (length);
805854
try {
806855
int bytesRead = stream.Read (buffer, 0, length);
@@ -809,8 +858,18 @@ internal static unsafe Int64 stream_callback (IntPtr state, IntPtr data, UInt64
809858
} finally {
810859
ArrayPool<byte>.Shared.Return (buffer);
811860
}
812-
813861
case SourceCommand.BeginWrite:
862+
try {
863+
string tempFile = Path.GetTempFileName ();
864+
context.Destination = File.Open (tempFile, FileMode.OpenOrCreate, FileAccess.ReadWrite);
865+
context.DestinationFileName = tempFile;
866+
} catch (IOException) {
867+
// ok use a memory stream as a backup
868+
context.Destination = new MemoryStream ();
869+
}
870+
destination = context.Destination;
871+
destination.Position = 0;
872+
break;
814873
case SourceCommand.Open:
815874
stream.Position = 0;
816875
return 0;

0 commit comments

Comments
 (0)