ExtractContent() of a corrupt file holds the extracting file open

[sslyle]

Steps to reproduce

1. zip a file, open the .zip file in Notepad++, intention delete the second half of the file.
2. Create a folder and ExtractContent() into the new folder.
3. Try to delete the file (being a developer you want to clean up after yourself don't you?)

Expected behavior

will be successfully deleted

Actual behavior

ExtractContent() throws "Unexpected EOF"
You can then .Close() the TarArchive, even .Dispose() but your code still has the file open and it can't be deleted. Not even from Windows File Explorer.
Neither can you delete the folder you just created to unzip the archive into.
You can not clean up after attempting to unzip a corrupted archive file

Version of SharpZipLib

(1.0.0)

Obtained from (only keep the relevant lines)

• Compiled from source, commit: _______ - N/A
• Downloaded from GitHub - NO
• Package installed using NuGet - YES

[decipherer]

I tried to reproduce this in a unit test, but:

• I'm not sure I understand the problem exactly (you talk about zipping, but also about tar archives)
• I can't reproduce that the file can't be deleted.

Does this unit test reflect your problem?

[Test]
[Category("Tar")]
public void Corrupted_Tar_Should_Be_Disposable()
{
	var tarfilename = Utils.GetDummyFileName();
	using (var tarfile = new FileStream(tarfilename, FileMode.OpenOrCreate, FileAccess.ReadWrite))
	{
		using (var tarOut = TarArchive.CreateOutputTarArchive(tarfile))
		using (var dummyFile = Utils.GetDummyFile(1024))
		{
			tarOut.IsStreamOwner = false;
			tarOut.WriteEntry(TarEntry.CreateEntryFromFile(dummyFile.Filename), false);
		}
		tarfile.SetLength(200); // corrupt archive
		tarfile.Seek(0, SeekOrigin.Begin);

		using (var tarIn = TarArchive.CreateInputTarArchive(tarfile))
		using (var tempDir = new Utils.TempDir())
		{
			tarIn.IsStreamOwner = false;
			try
			{
				tarIn.ExtractContents(tempDir.Fullpath);
			}
			catch (Exception)
			{ }
		}
	}
	File.Delete(tarfilename);
	Assert.IsFalse(File.Exists(tarfilename));
}

[sslyle]

Take a file and zip it; I use 7-Zip Open the zip file in Notepad++ and delete the second half of the file. Save the file. Pass the file to ExtractContents(); I have ExtractContents() extracting to the folder pointed to by the Windows' TEMP environment variable. ExtractContents() you should see ExtractContent() throw the "EOF" file error. -- Correct me if I am wrong but in your code you will never know if ExtractContent() throws anything because your catch will just toss any problems await unseen. After ExtractContents() crashes open Windows File Explorer and try to delete THE PARTIALLY UNZIPPED CONTENTS. Do this on a WINDOWS box, not Linux.

…

On Fri, Feb 15, 2019 at 5:16 AM decipherer ***@***.***> wrote: I tried to reproduce this in a unit test, but: - I'm not sure I understand the problem exactly (you talk about zipping, but also about tar archives) - I can not reproduce that the file cannot be deleted. Does this unit test reflect your problem? [Test] [Category("Tar")] public void Corrupted_Tar_Should_Be_Disposable() { var tarfilename = Utils.GetDummyFileName(); using (var tarfile = new FileStream(tarfilename, FileMode.OpenOrCreate, FileAccess.ReadWrite)) { using (var tarOut = TarArchive.CreateOutputTarArchive(tarfile)) using (var dummyFile = Utils.GetDummyFile(1024)) { tarOut.IsStreamOwner = false; tarOut.WriteEntry(TarEntry.CreateEntryFromFile(dummyFile.Filename), false); } tarfile.SetLength(200); // corrupt archive tarfile.Seek(0, SeekOrigin.Begin); using (var tarIn = TarArchive.CreateInputTarArchive(tarfile)) using (var tempDir = new Utils.TempDir()) { tarIn.IsStreamOwner = false; try { tarIn.ExtractContents(tempDir.Fullpath); } catch (Exception) { } } } File.Delete(tarfilename); Assert.IsFalse(File.Exists(tarfilename)); } — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#321 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFZh2s3IaTlA3fouWtgIpnU1VHgbb6owks5vNokZgaJpZM4a8wdQ> .

-- "In preparing for battle I have always found that plans are useless, but planning is indispensable." - General Dwight D. Eisenhower “For the things we have to learn before we can do them, we learn by doing them.” - Aristotle, The Nicomachean Ethics "A leader is someone who steps back from the entire system and tries to build a more collaborative, more innovative system that will work over the long term." - Robert Reich

[sslyle]

PS in addition to my reply of a minute ago I also opened your source code. Line 644 of TarArchive.cs doesn't do anything to clean up file handles in the advent of a Exception. I don't see a try-catch or anything else trying to handle critical errors thrown by .Net nor the OS in ExtractContents() or ExtractEntry(); lines 549-680.

…

On Fri, Feb 15, 2019 at 5:16 AM decipherer ***@***.***> wrote: I tried to reproduce this in a unit test, but: - I'm not sure I understand the problem exactly (you talk about zipping, but also about tar archives) - I can not reproduce that the file cannot be deleted. Does this unit test reflect your problem? [Test] [Category("Tar")] public void Corrupted_Tar_Should_Be_Disposable() { var tarfilename = Utils.GetDummyFileName(); using (var tarfile = new FileStream(tarfilename, FileMode.OpenOrCreate, FileAccess.ReadWrite)) { using (var tarOut = TarArchive.CreateOutputTarArchive(tarfile)) using (var dummyFile = Utils.GetDummyFile(1024)) { tarOut.IsStreamOwner = false; tarOut.WriteEntry(TarEntry.CreateEntryFromFile(dummyFile.Filename), false); } tarfile.SetLength(200); // corrupt archive tarfile.Seek(0, SeekOrigin.Begin); using (var tarIn = TarArchive.CreateInputTarArchive(tarfile)) using (var tempDir = new Utils.TempDir()) { tarIn.IsStreamOwner = false; try { tarIn.ExtractContents(tempDir.Fullpath); } catch (Exception) { } } } File.Delete(tarfilename); Assert.IsFalse(File.Exists(tarfilename)); } — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#321 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFZh2s3IaTlA3fouWtgIpnU1VHgbb6owks5vNokZgaJpZM4a8wdQ> .

-- "In preparing for battle I have always found that plans are useless, but planning is indispensable." - General Dwight D. Eisenhower “For the things we have to learn before we can do them, we learn by doing them.” - Aristotle, The Nicomachean Ethics "A leader is someone who steps back from the entire system and tries to build a more collaborative, more innovative system that will work over the long term." - Robert Reich

[Numpsy]

You say that you zip the file, but then talk about extracting it with TarArchive - are you actually trying to extract a corrupt zip file with the Tar reader?

[Numpsy]

Does look like the file stream created at

SharpZipLib/src/ICSharpCode.SharpZipLib/Tar/TarArchive.cs

Line 628 in 25ee644

Stream outputStream = File.Create(destFile);

might not be closed in some error cases though. (I guess the exact behaviour there would depend on exactly where the input data is corrupted though?)

[decipherer]

I was confused by trying to open (half) a zip as a tar file, as they’re two different formats. But you’re right, ExtractContents should always clean up after itself, no matter the input. I’ll try changing the unit test to half a zip file and play with the exact length a bit to see if I’m able to reproduce the problem.

I also noticed the lack of exception handling, therefore thinking I could easily reproduce the problem.

The unit test in fact does throw an exception, but I’m not so much interested in the exception itself, therefore it just gets swallowed in the test. I’m interested in the lack of closing any file handles, even when the calling code is implemented properly. Therefore I try to delete the file at the end of the test, if the file handle isn’t closed I’d expect that to fail, throwing an exception either there or on the next line where it makes sure the file is no longer there.

I do realize the test only covers half the problem you mention, only testing for the closing of the original tar file, not the extracted contents.

[sslyle]

I'm happy to modify the code and build for my purposes but honestly would like to see your approach/what you would do?

…

On Fri, Feb 15, 2019 at 3:14 PM Richard Webb ***@***.***> wrote: Does look like the file stream created at https://github.com/icsharpcode/SharpZipLib/blob/25ee6440f444aa326ee8f0b98e6223296f35fa6d/src/ICSharpCode.SharpZipLib/Tar/TarArchive.cs#L628 might not be closed in some error cases though. (I guess the exact behaviour there would depend on exactly where the input data is corrupted though?) — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#321 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFZh2qUjlLNJVmbRLnrEXXlVhmlmqGP2ks5vNxU4gaJpZM4a8wdQ> .

-- "In preparing for battle I have always found that plans are useless, but planning is indispensable." - General Dwight D. Eisenhower “For the things we have to learn before we can do them, we learn by doing them.” - Aristotle, The Nicomachean Ethics "A leader is someone who steps back from the entire system and tries to build a more collaborative, more innovative system that will work over the long term." - Robert Reich

[Numpsy]

I'd suggest adding a finally block to dispose of the stream in case of error, but I don't have a repro of an 'EOF' exception to test it with.

In fact, it looks to me like there are some situations where it will create an output file of the original size, and then fill it with junk past the part where the data is truncated, when i would expect it to throw instead?

If i run the test

public void TarStreamReadTest()
{
	var tarfilename = Utils.GetDummyFileName();
	using (var tarfile = new FileStream(tarfilename, FileMode.OpenOrCreate, FileAccess.ReadWrite))
	{
		using (var tarOut = TarArchive.CreateOutputTarArchive(tarfile))
		using (var dummyFile = Utils.GetDummyFile(1024))
		{
			tarOut.IsStreamOwner = false;
			tarOut.WriteEntry(TarEntry.CreateEntryFromFile(dummyFile.Filename), false);
		}
				
		tarfile.SetLength(500); // corrupt archive
		tarfile.Seek(0, SeekOrigin.Begin);

		using (var tarIn = new TarInputStream(tarfile))
		{
			tarIn.IsStreamOwner = false;

			TarEntry entry = tarIn.GetNextEntry();
			byte[] buffer = new byte[2048];
			var result = tarIn.Read(buffer, 0, buffer.Length);

			Assert.That(result, Is.EqualTo(1024));
		}
	}
}

then it appears that call to TarInputStream.Read will report having read 1024 bytes, even though the input stream is only 500 - doesn't seem right?

[sslyle]

I think I can do something to give you a URL to a broken zip file. Or can I just attach them to this email?

…

On Sun, Feb 17, 2019, 17:18 Richard Webb ***@***.*** wrote: I'd suggest adding a finally block to dispose of the stream in case of error, but I don't have a repro of an 'EOF' exception to test it with. In fact, it looks to me like there are some situations where it will create an output file of the original size, and then fill it with junk past the part where the data is truncated, when i would expect it to throw instead? If i run the test public void TarStreamReadTest() { var tarfilename = Utils.GetDummyFileName(); using (var tarfile = new FileStream(tarfilename, FileMode.OpenOrCreate, FileAccess.ReadWrite)) { using (var tarOut = TarArchive.CreateOutputTarArchive(tarfile)) using (var dummyFile = Utils.GetDummyFile(1024)) { tarOut.IsStreamOwner = false; tarOut.WriteEntry(TarEntry.CreateEntryFromFile(dummyFile.Filename), false); } tarfile.SetLength(500); // corrupt archive tarfile.Seek(0, SeekOrigin.Begin); using (var tarIn = new TarInputStream(tarfile)) { tarIn.IsStreamOwner = false; TarEntry entry = tarIn.GetNextEntry(); byte[] buffer = new byte[2048]; var result = tarIn.Read(buffer, 0, buffer.Length); Assert.That(result, Is.EqualTo(1024)); } } } then it appears that call to TarInputStream.Read will report having read 1024 bytes, even though the input stream is only 500 - doesn't seem right? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#321 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFZh2s9DCvxd-7nWphHBRVBnU3e46Ab1ks5vOdVAgaJpZM4a8wdQ> .

[Numpsy]

If the file is less than a certain size (I forget what the limit is offhand), then you can attach it to the bug entry via the Github web interface.

[sslyle]

I will try that tomorrow

…

On Sun, Feb 17, 2019, 18:10 Richard Webb ***@***.*** wrote: If the file is less than a certain size (I forget what the limit is offhand), then you can attach it to the bug entry via the Github web interface. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#321 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFZh2nGCORsjFo8togo83hlaTVr4TP4hks5vOeFlgaJpZM4a8wdQ> .

[piksel]

The limit is 10MB.

[sslyle]

You just might recognize the content of the attached zip file

But again. All you have to do is 1) zip a file, 2) open the zipped file in Notepad++, 3) delete the second half of the file's content, 4) close and save.
This approximates what happens when you are streaming a file over then inet, ftp, ... and and somethign cuts the stream off short.

BrokenZipFile.zip

[sslyle]

Sent on GitHub

…

On Mon, Feb 18, 2019 at 1:17 PM nils måsén ***@***.***> wrote: The limit is 10MB. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#321 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFZh2uEYQhF-L8BsFZImDcq9GLmOmm-kks5vOu5PgaJpZM4a8wdQ> .

-- "In preparing for battle I have always found that plans are useless, but planning is indispensable." - General Dwight D. Eisenhower “For the things we have to learn before we can do them, we learn by doing them.” - Aristotle, The Nicomachean Ethics "A leader is someone who steps back from the entire system and tries to build a more collaborative, more innovative system that will work over the long term." - Robert Reich

[sslyle]

Sent on GitHub

…

On Sun, Feb 17, 2019 at 6:10 PM Richard Webb ***@***.***> wrote: If the file is less than a certain size (I forget what the limit is offhand), then you can attach it to the bug entry via the Github web interface. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#321 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFZh2nGCORsjFo8togo83hlaTVr4TP4hks5vOeFlgaJpZM4a8wdQ> .

-- "In preparing for battle I have always found that plans are useless, but planning is indispensable." - General Dwight D. Eisenhower “For the things we have to learn before we can do them, we learn by doing them.” - Aristotle, The Nicomachean Ethics "A leader is someone who steps back from the entire system and tries to build a more collaborative, more innovative system that will work over the long term." - Robert Reich