Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

⚠⚠⚠ ZIP FILE CORRUPTION WHEN USING DIRECT MODE ⚠⚠⚠ #874

Open
Rackover opened this issue Aug 7, 2024 · 1 comment
Open

⚠⚠⚠ ZIP FILE CORRUPTION WHEN USING DIRECT MODE ⚠⚠⚠ #874

Rackover opened this issue Aug 7, 2024 · 1 comment
Labels
bug zip Related to ZIP file format

Comments

@Rackover
Copy link

Rackover commented Aug 7, 2024

Describe the bug

https://github.com/icsharpcode/SharpZipLib/blame/master/src/ICSharpCode.SharpZipLib/Zip/ZipFile.cs#L2861

This looks to me like a somewhat big oversight - it silently corrupts files and no warning nor error happens until it is too late.
After a few hours of debugging I feel like CopyEntryDirect is simply not functional if, for instance, one of the first few files of the ZipFile got deleted in a prior operation.

Let's put it this way - in direct mode, source stream and destination stream are the same.
That means writing on the destination is the same as writing on the source. You want to make extra sure you don't write over what you're going to read next. But if any file is deleted, this premise is broken - as deletions are treated, and copies are treated first, upon trying to copy the file SharpZipLib will happily overwrite the very file it's reading with itself.

Let's take an example, for instance the one I just spent a while looking into. A zip file with a 44 byte file, and then a 80kB file.
I decide to delete the first one. It has an offset of zero, and the next file has an offset of 44, because it's right after.

BeginUpdate => Delete => CommitUpdate

Upon commitment, updates are sorted, and the copy will now happen before the delete.
So this is what the library does - it takes the first update it has to handle, it's a copy! From <offset 44> to <wherever we are>. Since we have not done anything yet, <wherever we are> is offset 0. So, CopyEntryDirect copies byte 44<=>80044 to 0<=>80000.
There is no alignment, so just writing the "header" of that file we're copying, which totals 75 bytes, already overwrites the file itself. This causes massive, unrecoverable wreckage onto any zip file it touches!

This could maybe fixed with some minimum alignment the size of a "maximum zip header", or maybe by accepting some level of fragmentation in files and not moving stuff onto itself, or maybe temporarily copying the stuff to memory before committing it to file (not like the Safe mode, which renews the stream completely, something I cannot do in my project)

Sorry for the very long issue but I felt like this was important.

Reproduction Code

No response

Steps to reproduce

Add a few files in a zip using ZipFile, start with a tiny one (~40 bytes), give a long name to the others, then delete the first one you added.

Expected behavior

Something else than complete archive corurption

Operating System

Windows

Framework Version

.NET Framework 4.x, Unity

Tags

ZIP

Additional context

No response

@Rackover Rackover added the bug label Aug 7, 2024
@github-actions github-actions bot added the zip Related to ZIP file format label Aug 7, 2024
@SourceproStudio
Copy link

SourceproStudio commented Aug 7, 2024 via email

@Rackover Rackover changed the title ZipFile in direct mode does not support deleting any file but the last "Direct" mode corrupts file Aug 7, 2024
@Rackover Rackover changed the title "Direct" mode corrupts file "Direct" mode corrupts ZIP files Aug 8, 2024
@Rackover Rackover changed the title "Direct" mode corrupts ZIP files ⚠⚠⚠ ZIP FILE CORRUPTION WHEN USING DIRECT MODE ⚠⚠⚠ Sep 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug zip Related to ZIP file format
Projects
None yet
Development

No branches or pull requests

2 participants