Ensure ZipPackage can handle file lengths over int.MaxValue … (dotnet/corefx#35660)
authorEric StJohn <ericstj@microsoft.com>
Fri, 1 Mar 2019 00:41:22 +0000 (16:41 -0800)
committerGitHub <noreply@github.com>
Fri, 1 Mar 2019 00:41:22 +0000 (16:41 -0800)
* Ensure ZipPackage can handle file lengths over int.MaxValue when using FileAccess.Read and FileAccess.Write.

This helps unblock the scenario of using large files with System.IO.Packaging

FileAccess.Write was completely broken before because ZipPackage would create an entry
and immediately delete it, which is disallowed when you're writing an archive.  This
was a porting error, the desktop code had this under an else that was incorrectly removed.

Additionally when reading an archive with a large file, accessing the length of the
stream would cause the entire stream to be read into memory uncompressed and
overflow since this used a MemoryStream backed by a int.max-limited managed array.
Instead use the length info from the archive entry.

* Fix new packaging tests on desktop, respond to CR feedback

Desktop does not support FileAccess.Write on Package.Open: it throws when attempting to
create the ZipArchive.  It only supports FileAccess.Read or FileAccess.ReadWrite.

Also clean up the test code.

* Code review feedback

Commit migrated from https://github.com/dotnet/corefx/commit/125eac5808fc63bde5c238acdbaba9cac055855e

src/libraries/System.IO.Packaging/src/System/IO/Packaging/ZipPackage.cs
src/libraries/System.IO.Packaging/src/System/IO/Packaging/ZipWrappingStream.cs
src/libraries/System.IO.Packaging/tests/Tests.cs

index ff83ae7..6c70b1d 100644 (file)
@@ -669,15 +669,16 @@ namespace System.IO.Packaging
                         _contentTypeZipArchiveEntry = _zipArchive.CreateEntry(s_contentTypesFile, _cachedCompressionLevel);
                         _contentTypeStreamExists = true;
                     }
-
-                    // delete and re-create entry for content part.  When writing this, the stream will not truncate the content
-                    // if the XML is shorter than the existing content part.
-                    var contentTypefullName = _contentTypeZipArchiveEntry.FullName;
-                    var thisArchive = _contentTypeZipArchiveEntry.Archive;
-                    _zipStreamManager.Close(_contentTypeZipArchiveEntry);
-                    _contentTypeZipArchiveEntry.Delete();
-                    _contentTypeZipArchiveEntry = thisArchive.CreateEntry(contentTypefullName);
-
+                    else
+                    {
+                        // delete and re-create entry for content part.  When writing this, the stream will not truncate the content
+                        // if the XML is shorter than the existing content part.
+                        var contentTypefullName = _contentTypeZipArchiveEntry.FullName;
+                        var thisArchive = _contentTypeZipArchiveEntry.Archive;
+                        _zipStreamManager.Close(_contentTypeZipArchiveEntry);
+                        _contentTypeZipArchiveEntry.Delete();
+                        _contentTypeZipArchiveEntry = thisArchive.CreateEntry(contentTypefullName);
+                    }
 
                     using (Stream s = _zipStreamManager.Open(_contentTypeZipArchiveEntry, _packageFileMode, FileAccess.ReadWrite))
                     {
index 2f8d851..fa77650 100644 (file)
@@ -51,33 +51,33 @@ namespace System.IO.Packaging
         }
 
         public override void Write(
-               byte[] buffer,
-               int offset,
-               int count
+            byte[] buffer,
+            int offset,
+            int count
         )
         {
             _baseStream.Write(buffer, offset, count);
         }
 
         public override int Read(
-               byte[] buffer,
-               int offset,
-               int count
+            byte[] buffer,
+            int offset,
+            int count
         )
         {
             return _baseStream.Read(buffer, offset, count);
         }
 
         public override void SetLength(
-               long value
+            long value
         )
         {
             _baseStream.SetLength(value);
         }
 
         public override long Seek(
-               long offset,
-               SeekOrigin origin
+            long offset,
+            SeekOrigin origin
         )
         {
             return _baseStream.Seek(offset, origin);
@@ -100,36 +100,18 @@ namespace System.IO.Packaging
             }
         }
 
-        // For some strange reason, if the package FileAccess == Read,
-        // then can't get length from the stream.  Not supported.
-        // The workaround - write the stream to a memory stream, then return the length of the
-        // memory stream.
         public override long Length
         {
             get {
+                // ZipArchiveEntry's read stream doesn't provide a length since it's a raw DeflateStream
+                // Return length from the archive entry.
                 if (_packageFileAccess == FileAccess.Read)
-                {
-                    using (MemoryStream ms = new MemoryStream())
-                    using (Stream zs = _zipArchiveEntry.Open())
-                    {
-                        CopyStream(zs, ms);
-                        return ms.Length;
-                    }
-                }
+                    return _zipArchiveEntry.Length;
                 else
                     return _baseStream.Length;
             }
         }
 
-        private static void CopyStream(Stream source, Stream target)
-        {
-            const int BufSize = 0x4096;
-            byte[] buf = new byte[BufSize];
-            int bytesRead = 0;
-            while ((bytesRead = source.Read(buf, 0, BufSize)) > 0)
-                target.Write(buf, 0, bytesRead);
-        }
-
         public override bool CanSeek
         {
             get { return _baseStream.CanSeek; }
index cf26373..d436a03 100644 (file)
@@ -873,7 +873,7 @@ namespace System.IO.Packaging.Tests
                     }
                 }
             }
-                       tempGuidName.Delete();
+            tempGuidName.Delete();
         }
 
         [Fact]
@@ -2420,7 +2420,7 @@ namespace System.IO.Packaging.Tests
                 }
                 Assert.Equal(sumLen, 44768);
             }
-                       fiGuidName.Delete();
+            fiGuidName.Delete();
         }
 
         private string NL = Environment.NewLine;
@@ -2437,7 +2437,7 @@ namespace System.IO.Packaging.Tests
             {
                 Package package = Package.Open(tempGuidName.FullName, FileMode.Truncate, FileAccess.ReadWrite);
             });
-                       tempGuidName.Delete();
+            tempGuidName.Delete();
         }
 
         [Fact]
@@ -2452,7 +2452,7 @@ namespace System.IO.Packaging.Tests
             {
                 Package package = Package.Open(tempGuidName.FullName, FileMode.Truncate, FileAccess.Write);
             });
-                       tempGuidName.Delete();
+            tempGuidName.Delete();
         }
 
         [Fact]
@@ -2467,7 +2467,7 @@ namespace System.IO.Packaging.Tests
             {
                 Package package = Package.Open(tempGuidName.FullName, FileMode.Truncate, FileAccess.Read);
             });
-                       tempGuidName.Delete();
+            tempGuidName.Delete();
         }
 
         [Fact]
@@ -2692,7 +2692,7 @@ namespace System.IO.Packaging.Tests
             {
                 Package package = Package.Open(tempGuidName.FullName, FileMode.OpenOrCreate, FileAccess.Write);
             });
-                       tempGuidName.Delete();
+            tempGuidName.Delete();
         }
 
         [Fact]
@@ -2706,7 +2706,7 @@ namespace System.IO.Packaging.Tests
             {
                 Package package = Package.Open(tempGuidName.FullName, FileMode.OpenOrCreate, FileAccess.Read);
             });
-                       tempGuidName.Delete();
+            tempGuidName.Delete();
         }
 
         [Fact]
@@ -2813,7 +2813,7 @@ namespace System.IO.Packaging.Tests
             {
                 Package package = Package.Open(tempGuidName.FullName, FileMode.Open, FileAccess.Write);
             });
-                       tempGuidName.Delete();
+            tempGuidName.Delete();
         }
 
         [Fact]
@@ -3077,7 +3077,7 @@ namespace System.IO.Packaging.Tests
                     Assert.Equal(2, xd.DescendantNodes().Count());
                 }
             }
-                       tempGuidName.Delete();
+            tempGuidName.Delete();
         }
 
         [Fact]
@@ -3104,7 +3104,7 @@ namespace System.IO.Packaging.Tests
                     }
                 });
             }
-                       tempGuidName.Delete();
+            tempGuidName.Delete();
         }
 
         [Fact]
@@ -3130,7 +3130,7 @@ namespace System.IO.Packaging.Tests
                     }
                 });
             }
-                       tempGuidName.Delete();
+            tempGuidName.Delete();
         }
 
         [Fact]
@@ -3159,7 +3159,7 @@ namespace System.IO.Packaging.Tests
                     Assert.Equal(2, xd.DescendantNodes().Count());
                 }
             }
-                       tempGuidName.Delete();
+            tempGuidName.Delete();
         }
 
         [Fact]
@@ -3191,7 +3191,7 @@ namespace System.IO.Packaging.Tests
                     }
                 });
             }
-                       tempGuidName.Delete();
+            tempGuidName.Delete();
         }
 
         [Fact]
@@ -3220,7 +3220,7 @@ namespace System.IO.Packaging.Tests
                     Assert.Equal(2, xd.DescendantNodes().Count());
                 }
             }
-                       tempGuidName.Delete();
+            tempGuidName.Delete();
         }
 
         [Fact]
@@ -3231,7 +3231,7 @@ namespace System.IO.Packaging.Tests
             {
                 Package package = Package.Open(tempGuidName.FullName, FileMode.CreateNew, FileAccess.Write);
             });
-                       tempGuidName.Delete();
+            tempGuidName.Delete();
         }
 
         [Fact]
@@ -3242,7 +3242,7 @@ namespace System.IO.Packaging.Tests
             {
                 Package package = Package.Open(tempGuidName.FullName, FileMode.CreateNew, FileAccess.Read);
             });
-                       tempGuidName.Delete();
+            tempGuidName.Delete();
         }
 
         [Fact]
@@ -3251,7 +3251,7 @@ namespace System.IO.Packaging.Tests
             var tempGuidName = GetTempFileInfoWithExtension(".docx");
             // opening the package attempts to read the package, and no permissions.
             AssertExtensions.Throws<ArgumentException>(null, () => Package.Open(tempGuidName.FullName, FileMode.Create, FileAccess.Write));
-                       tempGuidName.Delete();
+            tempGuidName.Delete();
         }
 
         [Fact]
@@ -3280,7 +3280,7 @@ namespace System.IO.Packaging.Tests
                     Assert.Equal(2, xd.DescendantNodes().Count());
                 }
             }
-                       tempGuidName.Delete();
+            tempGuidName.Delete();
         }
 
         [Fact]
@@ -3309,7 +3309,7 @@ namespace System.IO.Packaging.Tests
                     Assert.Equal(2, xd.DescendantNodes().Count());
                 }
             }
-                       tempGuidName.Delete();
+            tempGuidName.Delete();
         }
 
         [Fact]
@@ -3338,7 +3338,7 @@ namespace System.IO.Packaging.Tests
                     Assert.Equal(2, xd.DescendantNodes().Count());
                 }
             }
-                       tempGuidName.Delete();
+            tempGuidName.Delete();
         }
 
         [Fact]
@@ -3367,7 +3367,7 @@ namespace System.IO.Packaging.Tests
                     Assert.Equal(2, xd.DescendantNodes().Count());
                 }
             }
-                       tempGuidName.Delete();
+            tempGuidName.Delete();
         }
 
         [Fact]
@@ -3396,7 +3396,7 @@ namespace System.IO.Packaging.Tests
                     Assert.Equal(2, xd.DescendantNodes().Count());
                 }
             }
-                       tempGuidName.Delete();
+            tempGuidName.Delete();
         }
 
         [Fact]
@@ -3424,7 +3424,7 @@ namespace System.IO.Packaging.Tests
                     });
                 }
             }
-                       tempGuidName.Delete();
+            tempGuidName.Delete();
         }
 
         [Fact]
@@ -3447,7 +3447,7 @@ namespace System.IO.Packaging.Tests
                     sw.Write(s_DocumentXml);
                 }
             }
-                       tempGuidName.Delete();
+            tempGuidName.Delete();
         }
 
         [Fact]
@@ -3471,7 +3471,7 @@ namespace System.IO.Packaging.Tests
                     Assert.Equal(0, partStream.Length);
                 }
             }
-                       tempGuidName.Delete();
+            tempGuidName.Delete();
         }
 
         [Fact]
@@ -3497,7 +3497,7 @@ namespace System.IO.Packaging.Tests
                     }
                 });
             }
-                       tempGuidName.Delete();
+            tempGuidName.Delete();
         }
 
         [Fact]
@@ -3531,7 +3531,7 @@ namespace System.IO.Packaging.Tests
                     Stream partStream = packagePartDocument.GetStream(FileMode.Append, FileAccess.Read);
                 });
             }
-                       tempGuidName.Delete();
+            tempGuidName.Delete();
         }
 
         [Fact]
@@ -3639,6 +3639,110 @@ namespace System.IO.Packaging.Tests
             }
         }
 
+        [Fact]
+        [SkipOnTargetFramework(TargetFrameworkMonikers.NetFramework, "Desktop doesn't support Package.Open with FileAccess.Write")]
+        public void CreateWithFileAccessWrite()
+        {
+            string[] fileNames = new [] { "file1.txt", "file2.txt", "file3.txt" };
+
+            using (Stream stream = new MemoryStream())
+            {
+                using (Package package = Package.Open(stream, FileMode.Create, FileAccess.Write))
+                {
+                    foreach (string fileName in fileNames)
+                    {
+                        PackagePart part = package.CreatePart(PackUriHelper.CreatePartUri(new Uri(fileName, UriKind.Relative)),
+                                                              System.Net.Mime.MediaTypeNames.Text.Plain,
+                                                              CompressionOption.Fast);
+                        using (StreamWriter writer = new StreamWriter(part.GetStream(), Encoding.ASCII))
+                        {
+                            // just write the filename as content
+                            writer.Write(fileName);
+                        }
+                    }
+                }
+
+                // reopen for read and validate the content
+                stream.Seek(0, SeekOrigin.Begin);
+                using (Package readPackage = Package.Open(stream))
+                {
+                    foreach (string fileName in fileNames)
+                    {
+                        PackagePart part = readPackage.GetPart(PackUriHelper.CreatePartUri(new Uri(fileName, UriKind.Relative)));
+
+                        using (Stream partStream = part.GetStream())
+                        using (StreamReader reader = new StreamReader(partStream, Encoding.ASCII))
+                        {
+                            Assert.Equal(fileName.Length, partStream.Length);
+                            Assert.Equal(fileName, reader.ReadToEnd());
+                        }
+                    }
+                }
+            }
+        }
+
+        [Fact]
+        [OuterLoop]
+        public void VeryLargePart()
+        {
+            // FileAccess.Write is important, this tells ZipPackage to open the underlying ZipArchive in 
+            // ZipArchiveMode.Create mode as opposed to ZipArchiveMode.Update
+            // When ZipArchive is opened in Create it will write entries directly to the zip stream
+            // When ZipArchive is opened in Update it will write uncompressed data to memory until 
+            // the archive is closed.
+            using (Stream stream = new MemoryStream())
+            {
+                Uri partUri = PackUriHelper.CreatePartUri(new Uri("test.bin", UriKind.Relative));
+
+                // should compress *very well*
+                byte[] buffer =  new byte[1024 * 1024];
+                for (int i = 0; i < buffer.Length; i++)
+                {
+                    buffer[i] = (byte)(i % 2);
+                }
+                
+                const long SizeInMb = 6 * 1024; // 6GB
+                long totalLength = SizeInMb * buffer.Length;
+
+                // issue on desktop we cannot use FileAccess.Write on a ZipArchive
+                using (Package package = Package.Open(stream, FileMode.Create, PlatformDetection.IsFullFramework ? FileAccess.ReadWrite : FileAccess.Write))
+                {
+                    PackagePart part = package.CreatePart(partUri,
+                                                          System.Net.Mime.MediaTypeNames.Application.Octet,
+                                                          CompressionOption.Fast);
+
+
+                    using (Stream partStream = part.GetStream())
+                    {
+                        for (long i = 0; i < SizeInMb; i++)
+                        {
+                            partStream.Write(buffer, 0, buffer.Length);
+                        }
+                    }
+                }
+
+                // reopen for read and make sure we can get the part length & data matches
+                stream.Seek(0, SeekOrigin.Begin);
+                using (Package readPackage = Package.Open(stream))
+                {
+                    PackagePart part = readPackage.GetPart(partUri);
+
+                    using (Stream partStream = part.GetStream())
+                    {
+                        Assert.Equal(totalLength, partStream.Length);
+                        byte[] readBuffer = new byte[buffer.Length];
+                        for (long i = 0; i < SizeInMb; i++)
+                        {
+                            int actualRead = partStream.Read(readBuffer, 0, readBuffer.Length);
+
+                            Assert.Equal(actualRead, readBuffer.Length);
+                            Assert.True(buffer.AsSpan().SequenceEqual(readBuffer));
+                        }
+                    }
+                }
+            }
+        }
+
         private const string DocumentRelationshipType = "http://schemas.openxmlformats.org/officeDocument/2006/relationships/officeDocument";
     }