Skip to content

Code Quality: Use temporary files to extract zip entries #17461

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Code Quality: Use temporary files to extract zip entries #17461

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
156 changes: 122 additions & 34 deletions src/Files.App/Utils/Storage/StorageItems/ZipStorageFile.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,13 +122,32 @@ public override IAsyncOperation<IRandomAccessStream> OpenAsync(FileAccessMode ac

if (entry.FileName is not null)
{
var ms = new MemoryStream();
await zipFile.ExtractFileAsync(entry.Index, ms);
ms.Position = 0;
return new NonSeekableRandomAccessStreamForRead(ms, entry.Size)
// Use a temporary file to avoid memory issues with large files
var tempFile = IO.Path.GetTempFileName();
try
{
DisposeCallback = () => zipFile.Dispose()
};
await using (var tempStream = new FileStream(tempFile, FileMode.Create, FileAccess.Write))
{
await zipFile.ExtractFileAsync(entry.Index, tempStream);
}

var fileStream = new FileStream(tempFile, FileMode.Open, FileAccess.Read, FileShare.Read, 4096, FileOptions.DeleteOnClose);
return new NonSeekableRandomAccessStreamForRead(fileStream, entry.Size)
{
DisposeCallback = () =>
{
fileStream.Dispose();
Copy link
Preview

Copilot AI Aug 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Calling fileStream.Dispose() in the DisposeCallback is redundant and potentially harmful. The NonSeekableRandomAccessStreamForRead should handle disposal of its underlying stream, and calling Dispose() twice could cause ObjectDisposedException.

Suggested change
fileStream.Dispose();

Copilot uses AI. Check for mistakes.

zipFile.Dispose();
}
};
}
catch
{
// Clean up temp file if extraction failed
try { IO.File.Delete(tempFile); } catch { }
Copy link
Preview

Copilot AI Aug 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The empty catch block silently swallows all exceptions when trying to delete the temporary file. Consider logging the exception or at least catching specific exceptions like FileNotFoundException or UnauthorizedAccessException to avoid hiding unexpected errors.

Suggested change
try { IO.File.Delete(tempFile); } catch { }
try { IO.File.Delete(tempFile); } catch (Exception ex) { System.Diagnostics.Debug.WriteLine($"Failed to delete temp file '{tempFile}': {ex}"); }

Copilot uses AI. Check for mistakes.

zipFile.Dispose();
throw;
}
}
return null;
}
Expand Down Expand Up @@ -167,14 +186,33 @@ public override IAsyncOperation<IRandomAccessStreamWithContentType> OpenReadAsyn
return null;
}

var ms = new MemoryStream();
await zipFile.ExtractFileAsync(entry.Index, ms);
ms.Position = 0;
var nsStream = new NonSeekableRandomAccessStreamForRead(ms, entry.Size)
// Use a temporary file to avoid memory issues with large files
var tempFile = IO.Path.GetTempFileName();
try
{
DisposeCallback = () => zipFile.Dispose()
};
return new StreamWithContentType(nsStream);
await using (var tempStream = new FileStream(tempFile, FileMode.Create, FileAccess.Write))
{
await zipFile.ExtractFileAsync(entry.Index, tempStream);
}

var fileStream = new FileStream(tempFile, FileMode.Open, FileAccess.Read, FileShare.Read, 4096, FileOptions.DeleteOnClose);
var nsStream = new NonSeekableRandomAccessStreamForRead(fileStream, entry.Size)
{
DisposeCallback = () =>
{
fileStream.Dispose();
Copy link
Preview

Copilot AI Aug 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Calling fileStream.Dispose() in the DisposeCallback is redundant and potentially harmful. The NonSeekableRandomAccessStreamForRead should handle disposal of its underlying stream, and calling Dispose() twice could cause ObjectDisposedException.

Suggested change
fileStream.Dispose();

Copilot uses AI. Check for mistakes.

zipFile.Dispose();
}
};
return new StreamWithContentType(nsStream);
}
catch
{
// Clean up temp file if extraction failed
try { IO.File.Delete(tempFile); } catch { }
zipFile.Dispose();
throw;
}
}, ((IPasswordProtectedItem)this).RetryWithCredentialsAsync));
}

Expand Down Expand Up @@ -205,13 +243,32 @@ public override IAsyncOperation<IInputStream> OpenSequentialReadAsync()
return null;
}

var ms = new MemoryStream();
await zipFile.ExtractFileAsync(entry.Index, ms);
ms.Position = 0;
return new NonSeekableRandomAccessStreamForRead(ms, entry.Size)
// Use a temporary file to avoid memory issues with large files
var tempFile = IO.Path.GetTempFileName();
try
{
DisposeCallback = () => zipFile.Dispose()
};
await using (var tempStream = new FileStream(tempFile, FileMode.Create, FileAccess.Write))
{
await zipFile.ExtractFileAsync(entry.Index, tempStream);
}

var fileStream = new FileStream(tempFile, FileMode.Open, FileAccess.Read, FileShare.Read, 4096, FileOptions.DeleteOnClose);
return new NonSeekableRandomAccessStreamForRead(fileStream, entry.Size)
{
DisposeCallback = () =>
{
fileStream.Dispose();
Copy link
Preview

Copilot AI Aug 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Calling fileStream.Dispose() in the DisposeCallback is redundant and potentially harmful. The NonSeekableRandomAccessStreamForRead should handle disposal of its underlying stream, and calling Dispose() twice could cause ObjectDisposedException.

Suggested change
fileStream.Dispose();

Copilot uses AI. Check for mistakes.

zipFile.Dispose();
}
};
}
catch
{
// Clean up temp file if extraction failed
try { IO.File.Delete(tempFile); } catch { }
zipFile.Dispose();
throw;
}
}, ((IPasswordProtectedItem)this).RetryWithCredentialsAsync));
}

Expand Down Expand Up @@ -245,11 +302,25 @@ public override IAsyncOperation<BaseStorageFile> CopyAsync(IStorageFolder destin

if (destFolder is ICreateFileWithStream cwsf)
{
var ms = new MemoryStream();
await zipFile.ExtractFileAsync(entry.Index, ms);
ms.Position = 0;
using var inStream = new NonSeekableRandomAccessStreamForRead(ms, entry.Size);
return await cwsf.CreateFileAsync(inStream.AsStreamForRead(), desiredNewName, option.Convert());
// Use a temporary file to avoid memory issues with large files
var tempFile = IO.Path.GetTempFileName();
try
{
await using (var tempStream = new FileStream(tempFile, FileMode.Create, FileAccess.Write))
{
await zipFile.ExtractFileAsync(entry.Index, tempStream);
}

using var fileStream = new FileStream(tempFile, FileMode.Open, FileAccess.Read, FileShare.Read, 4096, FileOptions.DeleteOnClose);
Copy link
Preview

Copilot AI Aug 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using FileOptions.DeleteOnClose with a 'using' statement creates a race condition. The file will be deleted when the FileStream is disposed at the end of the using block, but the NonSeekableRandomAccessStreamForRead may still need access to it after this method returns.

Copilot uses AI. Check for mistakes.

using var inStream = new NonSeekableRandomAccessStreamForRead(fileStream, entry.Size);
return await cwsf.CreateFileAsync(inStream.AsStreamForRead(), desiredNewName, option.Convert());
}
catch
{
// Clean up temp file if operation failed
try { IO.File.Delete(tempFile); } catch { }
throw;
}
}
else
{
Expand Down Expand Up @@ -321,25 +392,33 @@ public override IAsyncAction RenameAsync(string desiredName, NameCollisionOption
{
return;
}
using (var ms = new MemoryStream())
// Use a temporary file to avoid memory issues with large archives
var tempFile = IO.Path.GetTempFileName();
try
{
await using (var tempStream = new FileStream(tempFile, FileMode.Create, FileAccess.Write))
await using (var archiveStream = await OpenZipFileAsync(FileAccessMode.Read))
{
SevenZipCompressor compressor = new SevenZipCompressor() { CompressionMode = CompressionMode.Append };
compressor.CustomParameters.Add("cu", "on");
compressor.SetFormatFromExistingArchive(archiveStream);
var fileName = IO.Path.GetRelativePath(containerPath, IO.Path.Combine(IO.Path.GetDirectoryName(Path), desiredName));
await compressor.ModifyArchiveAsync(archiveStream, new Dictionary<int, string>() { { index, fileName } }, Credentials.Password, ms);
await compressor.ModifyArchiveAsync(archiveStream, new Dictionary<int, string>() { { index, fileName } }, Credentials.Password, tempStream);
}

await using (var archiveStream = await OpenZipFileAsync(FileAccessMode.ReadWrite))
await using (var tempReadStream = new FileStream(tempFile, FileMode.Open, FileAccess.Read))
{
ms.Position = 0;
await ms.CopyToAsync(archiveStream);
await ms.FlushAsync();
await tempReadStream.CopyToAsync(archiveStream);
await archiveStream.FlushAsync();
archiveStream.SetLength(archiveStream.Position);
}
}
finally
{
// Clean up temp file
try { IO.File.Delete(tempFile); } catch { }
}
}
}, ((IPasswordProtectedItem)this).RetryWithCredentialsAsync));
}
Expand Down Expand Up @@ -371,23 +450,32 @@ public override IAsyncAction DeleteAsync(StorageDeleteOption option)
{
return;
}
using (var ms = new MemoryStream())
// Use a temporary file to avoid memory issues with large archives
var tempFile = IO.Path.GetTempFileName();
try
{
await using (var tempStream = new FileStream(tempFile, FileMode.Create, FileAccess.Write))
await using (var archiveStream = await OpenZipFileAsync(FileAccessMode.Read))
{
SevenZipCompressor compressor = new SevenZipCompressor() { CompressionMode = CompressionMode.Append };
compressor.CustomParameters.Add("cu", "on");
compressor.SetFormatFromExistingArchive(archiveStream);
await compressor.ModifyArchiveAsync(archiveStream, new Dictionary<int, string>() { { index, null } }, Credentials.Password, ms);
await compressor.ModifyArchiveAsync(archiveStream, new Dictionary<int, string>() { { index, null } }, Credentials.Password, tempStream);
}

await using (var archiveStream = await OpenZipFileAsync(FileAccessMode.ReadWrite))
await using (var tempReadStream = new FileStream(tempFile, FileMode.Open, FileAccess.Read))
{
ms.Position = 0;
await ms.CopyToAsync(archiveStream);
await ms.FlushAsync();
await tempReadStream.CopyToAsync(archiveStream);
await archiveStream.FlushAsync();
archiveStream.SetLength(archiveStream.Position);
}
}
finally
{
// Clean up temp file
try { IO.File.Delete(tempFile); } catch { }
}
}
}, ((IPasswordProtectedItem)this).RetryWithCredentialsAsync));
}
Expand Down
Loading