adamhathcock / sharpcompress

SharpCompress is a fully managed C# library to deal with many compression types and formats.
MIT License
2.29k stars 482 forks source link

OS specific invalid characters are causing extraction to corrupt file #858

Closed DineshSolanki closed 4 months ago

DineshSolanki commented 4 months ago

https://www.deviantart.com/zenoasis/art/Japanese-TV-Dorama-folder-icon-pack-162-1077192465 Download zip from there image

Extract using SharpCompress, we'll see that the files REAL 恋愛殺人捜査班 : Real - Renai Satsujin Sosa Han.png and あの子の子ども : My Girlfriend's Child.png are extracted as 0 byte file with following names REAL µüïµä¢µ«║Σ║║µì£µƒ╗τÅ¡ , πüéπü«σ¡Éπü«σ¡Éπü¿πéÖπéé

I also tried with IBM437 encoding but same result.

However when you extract using 7zip you can see that it extracts fine and 7zip makes some changes to file name - which seeems to be removing : character which might be either U+A789 or U+2236

filenames from 7zip extraction REAL 恋愛殺人捜査班 _ Real - Renai Satsujin Sosa Han.png, あの子の子ども _ My Girlfriend's Child.png

Morilli commented 4 months ago

There is no validation of the destination file name, so an attempt is made to write to a filestream with a filename containing :. Interestingly this seems to somewhat work and no library function throws an exception (not even Path.GetFullPath even though it's clearly documented to do so), but the output is kinda garbage.

While I'm actually kind of interested in figuring out what actually happens when this is attempted, the fix here is most likely to sanitize the output path before attempting to open any output:

diff --git a/src/SharpCompress/Common/ExtractionMethods.cs b/src/SharpCompress/Common/ExtractionMethods.cs
index 27d4164..80b8e9d 100644
--- a/src/SharpCompress/Common/ExtractionMethods.cs
+++ b/src/SharpCompress/Common/ExtractionMethods.cs
@@ -37,6 +37,7 @@ internal static class ExtractionMethods
         options ??= new ExtractionOptions() { Overwrite = true };

         var file = Path.GetFileName(entry.Key.NotNull("Entry Key is null")).NotNull("File is null");
+        file = string.Join("_", file.Split(Path.GetInvalidFileNameChars()));
         if (options.ExtractFullPath)
         {
             var folder = Path.GetDirectoryName(entry.Key.NotNull("Entry Key is null"))
DineshSolanki commented 4 months ago

Yes that seems to be the solution, I wonder if 7zip handles it in same way. to answer on what happens when we try it, it seems to truncate the filename and stream is consumed somewhere else as its definitely not writing to that file with truncated name. even windows explorer can't extract it

image

adamhathcock commented 4 months ago

Could it be as simple as put the string through the encoding?

DineshSolanki commented 4 months ago

Could it be as simple as put the string through the encoding?

didn't work, tried UTF 8 and IBM437

var file = Path.GetFileName(entry.Key.NotNull("Entry Key is null")).NotNull("File is null");
var encoding = Encoding.UTF8;
 file = encoding.GetString(encoding.GetBytes(file)) ?? "";

However a 7zip discussion also mentions the encoding https://sourceforge.net/p/sevenzip/discussion/45798/thread/82ae0f9c/ Maybe I'm encoding in wrong manner?

DineshSolanki commented 4 months ago

issue seems to be with invalid character itself instead of the encoding, I was wrong in saying that the colon was probably unicode U+A789.

the following code will run into same issue

File.WriteAllText("あの子の子ども : My Girlfriend's Child.txt", "test");

so we have to remove invalid characters as @Morilli suggested, I tried his solution and it works

7zip https://github.com/ip7z/7zip/blob/a7a1d4a241492e81f659a920f7379c193593ebc6/CPP/7zip/UI/Common/ExtractingFilePath.cpp#L23

and the SevenZipSharp also seems to does the same https://github.com/StevenBonePgh/SevenZipSharp/blob/d0bf5c4a3d65ea5e85ffe7ef94312354475f5714/SevenZip/ArchiveExtractCallback.cs#L624C13-L641C14

adamhathcock commented 4 months ago

Your fix names sense: we shouldn't put invalid path characters in....but seems like it wouldn't work for everything? I'm inclined to accept it as other things do it

DineshSolanki commented 4 months ago

@adamhathcock It surely work for the issue we are having, its the same logic that 7zip is using