From a5228d05df94cce45491cb59d8015057aaa20062 Mon Sep 17 00:00:00 2001 From: Richard Webb Date: Thu, 21 Feb 2019 18:00:25 +0000 Subject: [PATCH 1/2] Refactor TarArchive.ExtractEntry() to use using blocks for stream disposal --- src/ICSharpCode.SharpZipLib/Tar/TarArchive.cs | 63 ++++++++++--------- 1 file changed, 32 insertions(+), 31 deletions(-) diff --git a/src/ICSharpCode.SharpZipLib/Tar/TarArchive.cs b/src/ICSharpCode.SharpZipLib/Tar/TarArchive.cs index 2161ab12a..133b33081 100644 --- a/src/ICSharpCode.SharpZipLib/Tar/TarArchive.cs +++ b/src/ICSharpCode.SharpZipLib/Tar/TarArchive.cs @@ -623,20 +623,32 @@ private void ExtractEntry(string destDir, TarEntry entry) if (process) { - bool asciiTrans = false; - - Stream outputStream = File.Create(destFile); - if (this.asciiTranslate) + using (var outputStream = File.Create(destFile)) { - asciiTrans = !IsBinary(destFile); + if (this.asciiTranslate) + { + // May need to translate the file. + ExtractAndTranslateEntry(destFile, outputStream); + } + else + { + // If translation is disabled, just copy the entry across directly. + tarIn.CopyEntryContents(outputStream); + } } + } + } + } - StreamWriter outw = null; - if (asciiTrans) - { - outw = new StreamWriter(outputStream); - } + // Extract a TAR entry, and perform an ASCII translation if required. + private void ExtractAndTranslateEntry(string destFile, Stream outputStream) + { + bool asciiTrans = !IsBinary(destFile); + if (asciiTrans) + { + using (var outw = new StreamWriter(outputStream, new UTF8Encoding(false), 1024, true)) + { byte[] rdbuf = new byte[32 * 1024]; while (true) @@ -648,34 +660,23 @@ private void ExtractEntry(string destDir, TarEntry entry) break; } - if (asciiTrans) + for (int off = 0, b = 0; b < numRead; ++b) { - for (int off = 0, b = 0; b < numRead; ++b) + if (rdbuf[b] == 10) { - if (rdbuf[b] == 10) - { - string s = Encoding.ASCII.GetString(rdbuf, off, (b - off)); - outw.WriteLine(s); - off = b + 1; - } + string s = Encoding.ASCII.GetString(rdbuf, off, (b - off)); + outw.WriteLine(s); + off = b + 1; } } - else - { - outputStream.Write(rdbuf, 0, numRead); - } - } - - if (asciiTrans) - { - outw.Dispose(); - } - else - { - outputStream.Dispose(); } } } + else + { + // No translation required. + tarIn.CopyEntryContents(outputStream); + } } /// From d1519087dcac8e1ecb877d4fe4c8c5fb4d0b2c09 Mon Sep 17 00:00:00 2001 From: Richard Webb Date: Thu, 21 Feb 2019 18:01:23 +0000 Subject: [PATCH 2/2] Unit test to check that TarArchive.ExtractContents() doesn't leak file handles when an exception occurs --- .../Tar/TarTests.cs | 47 +++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/test/ICSharpCode.SharpZipLib.Tests/Tar/TarTests.cs b/test/ICSharpCode.SharpZipLib.Tests/Tar/TarTests.cs index 33f9ae4df..c7945f142 100644 --- a/test/ICSharpCode.SharpZipLib.Tests/Tar/TarTests.cs +++ b/test/ICSharpCode.SharpZipLib.Tests/Tar/TarTests.cs @@ -1,4 +1,5 @@ using ICSharpCode.SharpZipLib.Tar; +using ICSharpCode.SharpZipLib.GZip; using ICSharpCode.SharpZipLib.Tests.TestSupport; using NUnit.Framework; using System; @@ -787,5 +788,51 @@ public void SingleLargeEntry() } ); } + + // Test for corruption issue described @ https://github.com/icsharpcode/SharpZipLib/issues/321 + [Test] + [Category("Tar")] + public void ExtractingCorruptTarShouldntLeakFiles() + { + using (var memoryStream = new MemoryStream()) + { + //Create a tar.gz in the output stream + using (var gzipStream = new GZipOutputStream(memoryStream)) + { + gzipStream.IsStreamOwner = false; + + using (var tarOut = TarArchive.CreateOutputTarArchive(gzipStream)) + using (var dummyFile = Utils.GetDummyFile(32000)) + { + tarOut.IsStreamOwner = false; + tarOut.WriteEntry(TarEntry.CreateEntryFromFile(dummyFile.Filename), false); + } + } + + // corrupt archive - make sure the file still has more than one block + memoryStream.SetLength(16000); + memoryStream.Seek(0, SeekOrigin.Begin); + + // try to extract + using (var gzipStream = new GZipInputStream(memoryStream)) + { + string tempDirName; + gzipStream.IsStreamOwner = false; + + using (var tempDir = new Utils.TempDir()) + { + tempDirName = tempDir.Fullpath; + + using (var tarIn = TarArchive.CreateInputTarArchive(gzipStream)) + { + tarIn.IsStreamOwner = false; + Assert.Throws(() => tarIn.ExtractContents(tempDir.Fullpath)); + } + } + + Assert.That(Directory.Exists(tempDirName), Is.False, "Temporary folder should have been removed"); + } + } + } } }