From 463f6994593d7ce04be57c47bd41fd8529571374 Mon Sep 17 00:00:00 2001 From: Giulia Stocco <98900+gfs@users.noreply.github.com> Date: Fri, 31 Jul 2026 07:54:15 -0700 Subject: [PATCH 1/3] Add extraction benchmarks and fix per-entry temp file spilling Issue #208 claims RecursiveExtractor is slow and blames the quine check. Adds a BenchmarkDotNet project that measures this rather than guessing, and fixes the real problem the measurements exposed. Benchmarks (RecursiveExtractor.Benchmarks) compare RecursiveExtractor against raw SharpCompress on identical in-memory archives, isolate the quine check and type detection so their share of runtime can be read directly, and spread the same payload across zip/tar/tar.gz/tar.bz2/7z. Findings on a 1000 entry x 4 KiB deflate zip: - IsQuine over all entries costs ~20 us with zero allocations, ~0.06% of extraction. AreIdentical compares Length and Name before any bytes. - Type detection is ~1%. - RecursiveExtractor adds ~10-20% over a raw reader doing the same buffering; the format library is the rest. - The container spans two orders of magnitude for the same payload. The measurements also exposed a real bug. StreamFactory treats a stream whose Length throws as "assume large" and returns a delete-on-close FileStream. Archive entry streams cannot seek, so this fired on every entry, writing each one to a temporary file. ZipExtractor's async path hit it directly and 7z/Rar/Ace/Arj/Arc/Xz hit it through FileEntry. Fixed in two places: - ZipExtractor now passes the entry size it already has, matching what the synchronous path has always done. - The unknown-length fallback now returns a SpillOverStream that buffers in memory and moves to a delete-on-close FileStream only if the content actually exceeds MemoryStreamCutoff, preserving the memory guard. ExtractAsync on a 1000 entry zip goes from 930 ms to 38 ms (24.8x), and 7z extraction from 320 ms to 53 ms (6.1x). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8aea0ccd-2ed2-4d36-ba1e-b69a21b4a9ef --- .gitignore | 1 + .../ArchiveFormatBenchmarks.cs | 129 +++++++++ .../BenchmarkConfig.cs | 25 ++ .../ExtractToDirectoryBenchmarks.cs | 98 +++++++ .../FileTypeDetectionBenchmarks.cs | 80 ++++++ .../NestedArchiveBenchmarks.cs | 63 +++++ RecursiveExtractor.Benchmarks/Program.cs | 21 ++ .../QuineDetectionBenchmarks.cs | 131 ++++++++++ RecursiveExtractor.Benchmarks/README.md | 71 +++++ .../RecursiveExtractor.Benchmarks.csproj | 21 ++ .../StreamCopyExtensions.cs | 30 +++ .../SyntheticArchives.cs | 197 ++++++++++++++ .../ZipExtractionBenchmarks.cs | 246 ++++++++++++++++++ .../StreamFactoryTests.cs | 173 ++++++++++++ RecursiveExtractor.sln | 50 ++++ RecursiveExtractor/Extractors/ZipExtractor.cs | 6 +- RecursiveExtractor/SpillOverStream.cs | 140 ++++++++++ RecursiveExtractor/StreamFactory.cs | 13 +- 18 files changed, 1490 insertions(+), 5 deletions(-) create mode 100644 RecursiveExtractor.Benchmarks/ArchiveFormatBenchmarks.cs create mode 100644 RecursiveExtractor.Benchmarks/BenchmarkConfig.cs create mode 100644 RecursiveExtractor.Benchmarks/ExtractToDirectoryBenchmarks.cs create mode 100644 RecursiveExtractor.Benchmarks/FileTypeDetectionBenchmarks.cs create mode 100644 RecursiveExtractor.Benchmarks/NestedArchiveBenchmarks.cs create mode 100644 RecursiveExtractor.Benchmarks/Program.cs create mode 100644 RecursiveExtractor.Benchmarks/QuineDetectionBenchmarks.cs create mode 100644 RecursiveExtractor.Benchmarks/README.md create mode 100644 RecursiveExtractor.Benchmarks/RecursiveExtractor.Benchmarks.csproj create mode 100644 RecursiveExtractor.Benchmarks/StreamCopyExtensions.cs create mode 100644 RecursiveExtractor.Benchmarks/SyntheticArchives.cs create mode 100644 RecursiveExtractor.Benchmarks/ZipExtractionBenchmarks.cs create mode 100644 RecursiveExtractor.Tests/StreamFactoryTests.cs create mode 100644 RecursiveExtractor/SpillOverStream.cs diff --git a/.gitignore b/.gitignore index 38ea0479..91948b35 100644 --- a/.gitignore +++ b/.gitignore @@ -4,3 +4,4 @@ obj/ .vs RecursiveExtractor.sln.DotSettings.user .DS_Store +BenchmarkDotNet.Artifacts/ diff --git a/RecursiveExtractor.Benchmarks/ArchiveFormatBenchmarks.cs b/RecursiveExtractor.Benchmarks/ArchiveFormatBenchmarks.cs new file mode 100644 index 00000000..46a7aed2 --- /dev/null +++ b/RecursiveExtractor.Benchmarks/ArchiveFormatBenchmarks.cs @@ -0,0 +1,129 @@ +// Copyright (c) Microsoft Corporation. Licensed under the MIT License. + +using BenchmarkDotNet.Attributes; +using SharpCompress.Archives; +using SharpCompress.Readers; +using System; +using System.IO; + +namespace Microsoft.CST.RecursiveExtractor.Benchmarks +{ + /// + /// Extracts the same payload from every supported container so the cost attributable to the + /// underlying format implementation can be separated from RecursiveExtractor's own overhead. + /// + [MemoryDiagnoser] + public class ArchiveFormatBenchmarks + { + private readonly Extractor _extractor = new(); + private readonly ExtractorOptions _options = new(); + + private byte[] _archive = Array.Empty(); + private byte[] _copyBuffer = Array.Empty(); + private string _fileName = string.Empty; + + /// + /// The container and compression under test. + /// + [Params( + BenchmarkArchiveKind.ZipStore, + BenchmarkArchiveKind.ZipDeflate, + BenchmarkArchiveKind.Tar, + BenchmarkArchiveKind.TarGz, + BenchmarkArchiveKind.TarBz2, + BenchmarkArchiveKind.SevenZipLzma)] + public BenchmarkArchiveKind Kind { get; set; } + + /// + /// Builds the archive under test. + /// + [GlobalSetup] + public void Setup() + { + _archive = SyntheticArchives.Create(Kind, EntryCount, EntrySize); + _fileName = SyntheticArchives.FileNameFor(Kind); + _copyBuffer = new byte[81920]; + } + + /// + /// Number of entries in each generated archive. + /// + public const int EntryCount = 200; + + /// + /// Size in bytes of each entry in each generated archive. + /// + public const int EntrySize = 8192; + + /// + /// SharpCompress reading the archive and discarding the bytes. Forward-only readers handle + /// the outer compression layer of the tar variants transparently, so they stay comparable + /// with what RecursiveExtractor produces; 7z has no forward-only reader and is read through + /// the random access archive API instead. + /// + /// Bytes decompressed. + [Benchmark(Baseline = true, Description = "SharpCompress reader")] + public long SharpCompressReader() + { + using var source = new MemoryStream(_archive, false); + return Kind == BenchmarkArchiveKind.SevenZipLzma + ? ReadWithArchiveApi(source) + : ReadWithReaderApi(source); + } + + private long ReadWithReaderApi(Stream source) + { + long total = 0; + using var reader = ReaderFactory.OpenReader(source, new ReaderOptions { LeaveStreamOpen = true }); + while (reader.MoveToNextEntry()) + { + if (reader.Entry.IsDirectory) + { + continue; + } + + using var entryStream = reader.OpenEntryStream(); + total += entryStream.CopyToCounting(Stream.Null, _copyBuffer); + } + + return total; + } + + private long ReadWithArchiveApi(Stream source) + { + long total = 0; + using var archive = ArchiveFactory.OpenArchive(source, new ReaderOptions { LeaveStreamOpen = true }); + foreach (var entry in archive.Entries) + { + if (entry.IsDirectory) + { + continue; + } + + using var entryStream = entry.OpenEntryStream(); + total += entryStream.CopyToCounting(Stream.Null, _copyBuffer); + } + + return total; + } + + /// + /// RecursiveExtractor extracting the same archive. + /// + /// Bytes extracted. + [Benchmark(Description = "RecursiveExtractor.Extract")] + public long RecursiveExtractorExtract() + { + long total = 0; + using var source = new MemoryStream(_archive, false); + var root = new FileEntry(_fileName, source, passthroughStream: true); + foreach (var entry in _extractor.Extract(root, _options)) + { + total += entry.Content.Length; + entry.Content.Dispose(); + } + + return total; + } + } +} diff --git a/RecursiveExtractor.Benchmarks/BenchmarkConfig.cs b/RecursiveExtractor.Benchmarks/BenchmarkConfig.cs new file mode 100644 index 00000000..dc2453ee --- /dev/null +++ b/RecursiveExtractor.Benchmarks/BenchmarkConfig.cs @@ -0,0 +1,25 @@ +// Copyright (c) Microsoft Corporation. Licensed under the MIT License. + +using BenchmarkDotNet.Columns; +using BenchmarkDotNet.Configs; +using BenchmarkDotNet.Diagnosers; +using BenchmarkDotNet.Reports; + +namespace Microsoft.CST.RecursiveExtractor.Benchmarks +{ + /// + /// Shared BenchmarkDotNet configuration. + /// + public static class BenchmarkConfig + { + /// + /// Builds the configuration used by every benchmark in this assembly. + /// + /// The configuration. + public static IConfig Create() => + DefaultConfig.Instance + .AddDiagnoser(MemoryDiagnoser.Default) + .AddColumn(RankColumn.Arabic) + .WithSummaryStyle(SummaryStyle.Default.WithMaxParameterColumnWidth(40)); + } +} diff --git a/RecursiveExtractor.Benchmarks/ExtractToDirectoryBenchmarks.cs b/RecursiveExtractor.Benchmarks/ExtractToDirectoryBenchmarks.cs new file mode 100644 index 00000000..dc04a0ab --- /dev/null +++ b/RecursiveExtractor.Benchmarks/ExtractToDirectoryBenchmarks.cs @@ -0,0 +1,98 @@ +// Copyright (c) Microsoft Corporation. Licensed under the MIT License. + +using BenchmarkDotNet.Attributes; +using BenchmarkDotNet.Engines; +using System; +using System.IO; + +namespace Microsoft.CST.RecursiveExtractor.Benchmarks +{ + /// + /// Measures writing an extracted archive to disk with and without + /// . Only the write side is parallelized; the + /// extraction enumeration itself is sequential because entries are produced by a single + /// forward pass over the source archive. + /// + [MemoryDiagnoser] + [SimpleJob(RunStrategy.Monitoring, launchCount: 1, warmupCount: 1, iterationCount: 5)] + public class ExtractToDirectoryBenchmarks + { + private readonly Extractor _extractor = new(); + + private byte[] _archive = Array.Empty(); + private string _outputRoot = string.Empty; + private string _outputDirectory = string.Empty; + + /// + /// Whether entries are written to disk in parallel. + /// + [Params(false, true)] + public bool Parallel { get; set; } + + /// + /// Number of entries in the archive being written out. + /// + [Params(500)] + public int EntryCount { get; set; } + + /// + /// Builds the archive and the root output folder. + /// + [GlobalSetup] + public void Setup() + { + _archive = SyntheticArchives.Create(BenchmarkArchiveKind.ZipDeflate, EntryCount, 8192); + _outputRoot = Path.Combine(Path.GetTempPath(), $"re-bench-{Guid.NewGuid():N}"); + Directory.CreateDirectory(_outputRoot); + } + + /// + /// Removes the root output folder. + /// + [GlobalCleanup] + public void Cleanup() => TryDelete(_outputRoot); + + /// + /// Gives each iteration a clean output folder. + /// + [IterationSetup] + public void IterationSetup() + { + _outputDirectory = Path.Combine(_outputRoot, Guid.NewGuid().ToString("N")); + Directory.CreateDirectory(_outputDirectory); + } + + /// + /// Deletes the folder written by the previous iteration. + /// + [IterationCleanup] + public void IterationCleanup() => TryDelete(_outputDirectory); + + /// + /// Extracts the archive to disk. + /// + /// The extraction status. + [Benchmark(Description = "ExtractToDirectory")] + public ExtractionStatusCode ExtractToDirectory() + { + using var source = new MemoryStream(_archive, false); + var root = new FileEntry("benchmark.zip", source, passthroughStream: true); + return _extractor.ExtractToDirectory(_outputDirectory, root, new ExtractorOptions { Parallel = Parallel }); + } + + private static void TryDelete(string directory) + { + try + { + if (Directory.Exists(directory)) + { + Directory.Delete(directory, true); + } + } + catch (IOException) + { + // Best effort cleanup of temporary output. + } + } + } +} diff --git a/RecursiveExtractor.Benchmarks/FileTypeDetectionBenchmarks.cs b/RecursiveExtractor.Benchmarks/FileTypeDetectionBenchmarks.cs new file mode 100644 index 00000000..e9958fa2 --- /dev/null +++ b/RecursiveExtractor.Benchmarks/FileTypeDetectionBenchmarks.cs @@ -0,0 +1,80 @@ +// Copyright (c) Microsoft Corporation. Licensed under the MIT License. + +using BenchmarkDotNet.Attributes; +using System; +using System.IO; + +namespace Microsoft.CST.RecursiveExtractor.Benchmarks +{ + /// + /// Measures , which runs once per entry during + /// recursion via . It seeks to the end of the stream for the + /// DMG footer check, so the backing store matters. + /// + [MemoryDiagnoser] + public class FileTypeDetectionBenchmarks + { + private MemoryStream? _plainMemory; + private MemoryStream? _zipMemory; + private FileStream? _plainFile; + private string _tempFile = string.Empty; + + /// + /// Size in bytes of the content being sniffed. + /// + [Params(4096, 1048576)] + public int ContentSize { get; set; } + + /// + /// Creates the streams under test. + /// + [GlobalSetup] + public void Setup() + { + _plainMemory = new MemoryStream(SyntheticArchives.CreatePayload(ContentSize, true), false); + _zipMemory = new MemoryStream(SyntheticArchives.Create(BenchmarkArchiveKind.ZipDeflate, 4, 1024), false); + + _tempFile = Path.Combine(Path.GetTempPath(), $"re-bench-{Guid.NewGuid():N}.bin"); + File.WriteAllBytes(_tempFile, SyntheticArchives.CreatePayload(ContentSize, true)); + _plainFile = new FileStream(_tempFile, FileMode.Open, FileAccess.Read, FileShare.Read); + } + + /// + /// Disposes the streams and removes the temporary file. + /// + [GlobalCleanup] + public void Cleanup() + { + _plainMemory?.Dispose(); + _zipMemory?.Dispose(); + _plainFile?.Dispose(); + + if (File.Exists(_tempFile)) + { + File.Delete(_tempFile); + } + } + + /// + /// Detection against an in-memory non-archive, the common case for leaf files. + /// + /// The detected type. + [Benchmark(Baseline = true, Description = "MemoryStream, not an archive")] + public ArchiveFileType MemoryStreamPlain() => MiniMagic.DetectFileType(_plainMemory!); + + /// + /// Detection against an in-memory zip, which matches on the first four bytes. + /// + /// The detected type. + [Benchmark(Description = "MemoryStream, zip")] + public ArchiveFileType MemoryStreamZip() => MiniMagic.DetectFileType(_zipMemory!); + + /// + /// Detection against a FileStream, which is what backs entries larger than + /// . + /// + /// The detected type. + [Benchmark(Description = "FileStream, not an archive")] + public ArchiveFileType FileStreamPlain() => MiniMagic.DetectFileType(_plainFile!); + } +} diff --git a/RecursiveExtractor.Benchmarks/NestedArchiveBenchmarks.cs b/RecursiveExtractor.Benchmarks/NestedArchiveBenchmarks.cs new file mode 100644 index 00000000..392d4329 --- /dev/null +++ b/RecursiveExtractor.Benchmarks/NestedArchiveBenchmarks.cs @@ -0,0 +1,63 @@ +// Copyright (c) Microsoft Corporation. Licensed under the MIT License. + +using BenchmarkDotNet.Attributes; +using System; +using System.IO; + +namespace Microsoft.CST.RecursiveExtractor.Benchmarks +{ + /// + /// Extracts zips nested inside zips to show how the whole pipeline scales with nesting depth. + /// Every level re-buffers the inner archive and adds one more ancestor for the quine check to + /// walk, so this is the shape most likely to expose recursion overhead. + /// + [MemoryDiagnoser] + public class NestedArchiveBenchmarks + { + private readonly Extractor _extractor = new(); + private readonly ExtractorOptions _options = new(); + + private byte[] _archive = Array.Empty(); + + /// + /// Number of zip layers wrapped around the innermost archive. + /// + [Params(0, 4, 8)] + public int Depth { get; set; } + + /// + /// Builds the nested archive under test. + /// + [GlobalSetup] + public void Setup() => _archive = SyntheticArchives.CreateNestedZip(Depth, LeafEntryCount, LeafEntrySize); + + /// + /// Number of entries in the innermost archive. + /// + public const int LeafEntryCount = 200; + + /// + /// Size in bytes of each entry in the innermost archive. + /// + public const int LeafEntrySize = 4096; + + /// + /// Extracts every leaf file out of the nested archive. + /// + /// Bytes extracted. + [Benchmark(Description = "RecursiveExtractor.Extract")] + public long RecursiveExtractorExtract() + { + long total = 0; + using var source = new MemoryStream(_archive, false); + var root = new FileEntry("nested.zip", source, passthroughStream: true); + foreach (var entry in _extractor.Extract(root, _options)) + { + total += entry.Content.Length; + entry.Content.Dispose(); + } + + return total; + } + } +} diff --git a/RecursiveExtractor.Benchmarks/Program.cs b/RecursiveExtractor.Benchmarks/Program.cs new file mode 100644 index 00000000..f2363f26 --- /dev/null +++ b/RecursiveExtractor.Benchmarks/Program.cs @@ -0,0 +1,21 @@ +// Copyright (c) Microsoft Corporation. Licensed under the MIT License. + +using BenchmarkDotNet.Running; +using System.Reflection; + +namespace Microsoft.CST.RecursiveExtractor.Benchmarks +{ + /// + /// Entry point for the benchmark runner. + /// + public static class Program + { + /// + /// Runs the requested benchmarks. Pass --filter * to run everything, or + /// --list flat to see the available benchmarks. + /// + /// BenchmarkDotNet command line arguments. + public static void Main(string[] args) => + BenchmarkSwitcher.FromAssembly(Assembly.GetExecutingAssembly()).Run(args, BenchmarkConfig.Create()); + } +} diff --git a/RecursiveExtractor.Benchmarks/QuineDetectionBenchmarks.cs b/RecursiveExtractor.Benchmarks/QuineDetectionBenchmarks.cs new file mode 100644 index 00000000..9a33664f --- /dev/null +++ b/RecursiveExtractor.Benchmarks/QuineDetectionBenchmarks.cs @@ -0,0 +1,131 @@ +// Copyright (c) Microsoft Corporation. Licensed under the MIT License. + +using BenchmarkDotNet.Attributes; +using System; +using System.Collections.Generic; +using System.IO; + +namespace Microsoft.CST.RecursiveExtractor.Benchmarks +{ + /// + /// Isolates the cost of for one entry. The check walks the + /// parent chain, so cost scales with nesting depth rather than with the number of files in the + /// archive, and only reads content when an ancestor has + /// both the same length and the same name. + /// + [MemoryDiagnoser] + public class QuineDetectionBenchmarks + { + private readonly List _buffers = new(); + + private FileEntry? _differentSizes; + private FileEntry? _sameSizeDifferentNames; + private FileEntry? _sameSizeSameName; + private FileEntry? _actualQuine; + + /// + /// Size in bytes of the content at each level of the chain. + /// + [Params(4096, 1048576)] + public int ContentSize { get; set; } + + /// + /// How many ancestors the entry being checked has. + /// + [Params(1, 4)] + public int Depth { get; set; } + + /// + /// Builds the parent chains under test. + /// + [GlobalSetup] + public void Setup() + { + // Realistic case: an extracted file is never the same size as the archive it came from. + _differentSizes = BuildChain( + level => $"entry{level}.bin", + level => Track(SyntheticArchives.CreatePayload(ContentSize + (level * 512), true))); + + // Uniform payloads: lengths collide, but names still short circuit the content compare. + _sameSizeDifferentNames = BuildChain( + level => $"entry{level}.bin", + _ => Track(SyntheticArchives.CreatePayload(ContentSize, true))); + + // Worst case: same length and same name at every level forces a full byte compare + // that only fails on the final byte. + _sameSizeSameName = BuildChain( + _ => "entry.bin", + level => + { + var payload = SyntheticArchives.CreatePayload(ContentSize, true); + payload[payload.Length - 1] = (byte)level; + return Track(payload); + }); + + // A genuine quine: identical name and content, detected on the first comparison. + var quinePayload = Track(SyntheticArchives.CreatePayload(ContentSize, true)); + _actualQuine = BuildChain(_ => "entry.bin", _ => quinePayload); + } + + /// + /// Releases the streams held by the chains. + /// + [GlobalCleanup] + public void Cleanup() + { + _buffers.Clear(); + _differentSizes = null; + _sameSizeDifferentNames = null; + _sameSizeSameName = null; + _actualQuine = null; + } + + /// + /// The path taken by nearly every real entry: content lengths differ from the ancestors. + /// + /// Whether a quine was detected. + [Benchmark(Baseline = true, Description = "Ancestors differ in length (typical)")] + public bool DifferentSizes() => Extractor.IsQuine(_differentSizes!); + + /// + /// Lengths collide but names differ, so still no content comparison. + /// + /// Whether a quine was detected. + [Benchmark(Description = "Same length, different name")] + public bool SameSizeDifferentNames() => Extractor.IsQuine(_sameSizeDifferentNames!); + + /// + /// Same length and name at every level, so every ancestor is compared byte for byte. + /// + /// Whether a quine was detected. + [Benchmark(Description = "Same length + name (full compare, worst case)")] + public bool SameSizeSameName() => Extractor.IsQuine(_sameSizeSameName!); + + /// + /// An actual quine, which is detected on the first ancestor comparison. + /// + /// Whether a quine was detected. + [Benchmark(Description = "Actual quine (detected)")] + public bool ActualQuine() => Extractor.IsQuine(_actualQuine!); + + private FileEntry BuildChain(Func nameFor, Func contentFor) + { + FileEntry? parent = null; + FileEntry current = null!; + + for (var level = 0; level <= Depth; level++) + { + current = new FileEntry(nameFor(level), new MemoryStream(contentFor(level), false), parent, passthroughStream: true); + parent = current; + } + + return current; + } + + private byte[] Track(byte[] payload) + { + _buffers.Add(payload); + return payload; + } + } +} diff --git a/RecursiveExtractor.Benchmarks/README.md b/RecursiveExtractor.Benchmarks/README.md new file mode 100644 index 00000000..baf5d64b --- /dev/null +++ b/RecursiveExtractor.Benchmarks/README.md @@ -0,0 +1,71 @@ +# RecursiveExtractor.Benchmarks + +[BenchmarkDotNet](https://benchmarkdotnet.org/) harness for measuring RecursiveExtractor's +throughput and, more importantly, for separating RecursiveExtractor's own overhead from the cost +of the underlying format libraries (SharpCompress, DiscUtils). + +All archives are generated in memory at setup time by `SyntheticArchives`, so the benchmarks are +deterministic and do not depend on the test project's assets. + +## Running + +Benchmarks must be run against an optimized build: + +```bash +# Everything (slow) +dotnet run -c Release -f net8.0 --project RecursiveExtractor.Benchmarks -- --filter "*" + +# A single class +dotnet run -c Release -f net8.0 --project RecursiveExtractor.Benchmarks -- --filter "*ZipExtractionBenchmarks*" + +# Fewer iterations, good enough for triage +dotnet run -c Release -f net8.0 --project RecursiveExtractor.Benchmarks -- --filter "*" --job short + +# List what is available +dotnet run -c Release -f net8.0 --project RecursiveExtractor.Benchmarks -- --list flat +``` + +The project multi-targets `net8.0`, `net9.0` and `net10.0`, so `-f` is required. Results are +written to `BenchmarkDotNet.Artifacts/results` as markdown, CSV and HTML. + +## What each class measures + +| Class | Question it answers | +| --- | --- | +| `ZipExtractionBenchmarks` | How much does RecursiveExtractor add on top of raw SharpCompress for the same zip? Isolates quine detection and archive type detection so their share of total time can be read directly. | +| `ArchiveFormatBenchmarks` | How much of the cost is the container/compression itself? Same payload through zip (stored and deflate), tar, tar.gz, tar.bz2 and 7z, each against a raw SharpCompress reader baseline. | +| `QuineDetectionBenchmarks` | What does `Extractor.IsQuine` cost per entry — on the fast path, when lengths collide, and in the worst case where an ancestor has the same name and length and a full byte comparison is required. | +| `NestedArchiveBenchmarks` | How does the pipeline scale as archives nest inside archives? | +| `FileTypeDetectionBenchmarks` | What does `MiniMagic.DetectFileType` cost per entry, and how much worse is it when the entry is backed by a `FileStream` instead of a `MemoryStream`? | +| `ExtractToDirectoryBenchmarks` | What does `ExtractorOptions.Parallel` buy when writing extracted entries to disk? | + +## Interpreting the numbers + +- `SharpCompress: decompress only` is the floor. Nothing RecursiveExtractor does can go faster. +- `SharpCompress: decompress + buffer` adds the seekable copy of each entry that + `FileEntry.Content` requires, which is the price of type detection and quine detection. +- The gap between `SharpCompress: decompress + buffer` and `RE: Extract` is RecursiveExtractor's + actual overhead. +- `RE: Extract (no recurse)` skips the resource governor, quine check and type detection for each + entry, so the delta against `RE: Extract (recurse)` bounds the total cost of recursion bookkeeping. +- Absolute times are hardware and noise dependent; compare the `Ratio` column, not the means. + +## What these benchmarks found + +Measured on a 1000 entry x 4 KiB deflate zip, ShortRun, net8.0: + +- **Quine detection is not a bottleneck.** Running `IsQuine` over all 1000 entries costs ~20 us + with zero allocations: roughly 0.06% of the ~30 ms extraction, because `AreIdentical` compares + `Length` and `Name` before it ever compares bytes. The full content comparison only runs when an + ancestor has both the same size and the same name. +- **Archive type detection is ~1%** of extraction over the same entries. +- **RecursiveExtractor's own overhead is roughly 10-20%** over a raw SharpCompress reader that does + the same buffering. The remainder is the format library. +- **The format dominates everything else.** The same 200 x 8 KiB payload spans two orders of + magnitude between containers, from ~0.3 ms for stored zip to ~100 ms for tar.bz2. +- **`ExtractAsync` used to be ~28x slower than `Extract` for zip.** `ZipExtractor`'s async path + handed SharpCompress's non-seekable entry stream to `StreamFactory`, whose `Length` access threw + and fell back to a delete-on-close temporary file, one per entry. Fixed by passing the known + `zipEntry.Size` and by making the unknown-length fallback a `SpillOverStream` that only moves to + disk if the content actually exceeds `MemoryStreamCutoff`. Async is now within ~15% of sync, and + 7z (which hits the same fallback through `FileEntry`) got ~6x faster. diff --git a/RecursiveExtractor.Benchmarks/RecursiveExtractor.Benchmarks.csproj b/RecursiveExtractor.Benchmarks/RecursiveExtractor.Benchmarks.csproj new file mode 100644 index 00000000..333f97ab --- /dev/null +++ b/RecursiveExtractor.Benchmarks/RecursiveExtractor.Benchmarks.csproj @@ -0,0 +1,21 @@ + + + + Exe + net8.0;net9.0;net10.0 + Microsoft.CST.RecursiveExtractor.Benchmarks + false + enable + 10.0 + false + + + + + + + + + + + diff --git a/RecursiveExtractor.Benchmarks/StreamCopyExtensions.cs b/RecursiveExtractor.Benchmarks/StreamCopyExtensions.cs new file mode 100644 index 00000000..987d578b --- /dev/null +++ b/RecursiveExtractor.Benchmarks/StreamCopyExtensions.cs @@ -0,0 +1,30 @@ +// Copyright (c) Microsoft Corporation. Licensed under the MIT License. + +using System.IO; + +namespace Microsoft.CST.RecursiveExtractor.Benchmarks +{ + internal static class StreamCopyExtensions + { + /// + /// Copies a stream and reports how many bytes moved, without materializing them and + /// without allocating a fresh buffer per call. + /// + /// Stream to read from. + /// Stream to write to. + /// Scratch buffer reused across calls. + /// The number of bytes copied. + internal static long CopyToCounting(this Stream source, Stream destination, byte[] buffer) + { + long total = 0; + int read; + while ((read = source.Read(buffer, 0, buffer.Length)) > 0) + { + destination.Write(buffer, 0, read); + total += read; + } + + return total; + } + } +} diff --git a/RecursiveExtractor.Benchmarks/SyntheticArchives.cs b/RecursiveExtractor.Benchmarks/SyntheticArchives.cs new file mode 100644 index 00000000..d0aba92c --- /dev/null +++ b/RecursiveExtractor.Benchmarks/SyntheticArchives.cs @@ -0,0 +1,197 @@ +// Copyright (c) Microsoft Corporation. Licensed under the MIT License. + +using SharpCompress.Common; +using SharpCompress.Common.Tar.Headers; +using SharpCompress.Writers; +using SharpCompress.Writers.Tar; +using System; +using System.IO; + +namespace Microsoft.CST.RecursiveExtractor.Benchmarks +{ + /// + /// The kinds of archives the benchmarks build. Keeping this separate from + /// lets a single value describe both the container and the + /// compression applied to it. + /// + public enum BenchmarkArchiveKind + { + /// Zip container, deflate compressed entries. + ZipDeflate, + + /// Zip container, stored (uncompressed) entries. + ZipStore, + + /// Uncompressed tar container. + Tar, + + /// Tar container wrapped in gzip. + TarGz, + + /// Tar container wrapped in bzip2. + TarBz2, + + /// 7-Zip container using LZMA. + SevenZipLzma + } + + /// + /// Builds deterministic in-memory archives so the benchmarks do not depend on the test + /// project's assets and so payload size, entry count and compressibility can be varied. + /// + public static class SyntheticArchives + { + private const int PayloadSeed = 0x5EED; + + private static readonly string[] Words = + { + "lorem", "ipsum", "dolor", "sit", "amet", "consectetur", "adipiscing", "elit", + "sed", "eiusmod", "tempor", "incididunt", "labore", "magna", "aliqua", "enim", + "minim", "veniam", "quis", "nostrud", "exercitation", "ullamco", "laboris", + "aliquip", "commodo", "consequat", "duis", "aute", "irure", "reprehenderit" + }; + + /// + /// Creates a payload buffer of the requested size. + /// + /// Number of bytes to produce. + /// + /// When true the payload is prose-like text that compressors shrink by roughly 2-3x, which + /// is representative of real content. When false it is pseudo random data that compressors + /// cannot shrink. + /// + /// + /// Selects the pseudo random stream. The same variant always produces the same bytes, and + /// different variants produce content with no cross-entry redundancy. That matters because + /// repeating one buffer across every entry lets an outer archive layer compress the inner + /// archive enormously, which trips . + /// + /// The payload buffer. + public static byte[] CreatePayload(int size, bool compressible, int variant = 0) + { + var bytes = new byte[size]; + var random = new Random(PayloadSeed + variant); + + if (compressible) + { + var written = 0; + while (written < size) + { + var word = Words[random.Next(Words.Length)]; + for (var i = 0; i < word.Length && written < size; i++) + { + bytes[written++] = (byte)word[i]; + } + + if (written < size) + { + bytes[written++] = (byte)' '; + } + } + } + else + { + random.NextBytes(bytes); + } + + return bytes; + } + + /// + /// Builds an archive containing entries of + /// bytes each. + /// + /// The archive container and compression to use. + /// Number of entries to write. + /// Size in bytes of each entry. + /// Whether the entry payload should be compressible. + /// The bytes of the generated archive. + public static byte[] Create(BenchmarkArchiveKind kind, int entryCount, int entrySize, bool compressible = true) + { + using var output = new MemoryStream(); + + using (var writer = OpenWriter(kind, output)) + { + for (var i = 0; i < entryCount; i++) + { + // Distinct content per entry so no extractor can dedupe, and so an outer + // archive layer cannot collapse the inner one into a zip bomb ratio. + var payload = CreatePayload(entrySize, compressible, i); + using var entryStream = new MemoryStream(payload, false); + writer.Write($"dir{i % 8}/entry{i:D5}.bin", entryStream, DateTime.UnixEpoch); + } + } + + return output.ToArray(); + } + + /// + /// Builds a zip that nests another zip levels deep, with the + /// innermost archive holding the requested entries. Used to measure the cost of recursion + /// and of the quine check walking the parent chain. + /// + /// How many wrapping zip layers to add around the leaf archive. + /// Number of entries in the innermost archive. + /// Size in bytes of each innermost entry. + /// The bytes of the generated archive. + public static byte[] CreateNestedZip(int depth, int leafEntryCount, int leafEntrySize) + { + // Incompressible leaves keep the expansion ratio well under + // ExtractorOptions.MaxExtractedBytesRatio, so nesting does not trip the zip bomb guard. + var current = Create(BenchmarkArchiveKind.ZipDeflate, leafEntryCount, leafEntrySize, compressible: false); + + for (var level = 0; level < depth; level++) + { + using var output = new MemoryStream(); + using (var writer = OpenWriter(BenchmarkArchiveKind.ZipDeflate, output)) + { + using var entryStream = new MemoryStream(current, false); + writer.Write($"level{level}.zip", entryStream, DateTime.UnixEpoch); + } + + current = output.ToArray(); + } + + return current; + } + + /// + /// The file name (and therefore the extension used for type detection) that matches the + /// supplied . + /// + /// The archive kind. + /// A representative file name. + public static string FileNameFor(BenchmarkArchiveKind kind) => kind switch + { + BenchmarkArchiveKind.ZipDeflate => "benchmark.zip", + BenchmarkArchiveKind.ZipStore => "benchmark.zip", + BenchmarkArchiveKind.Tar => "benchmark.tar", + BenchmarkArchiveKind.TarGz => "benchmark.tar.gz", + BenchmarkArchiveKind.TarBz2 => "benchmark.tar.bz2", + BenchmarkArchiveKind.SevenZipLzma => "benchmark.7z", + _ => throw new ArgumentOutOfRangeException(nameof(kind)) + }; + + private static IWriter OpenWriter(BenchmarkArchiveKind kind, Stream output) => kind switch + { + // Tar is written directly rather than through WriterFactory so the ustar magic is + // emitted; MiniMagic looks for it at offset 257 and the older v7 header has no magic. + BenchmarkArchiveKind.Tar => new TarWriter(output, TarOptions(CompressionType.None)), + BenchmarkArchiveKind.TarGz => new TarWriter(output, TarOptions(CompressionType.GZip)), + BenchmarkArchiveKind.TarBz2 => new TarWriter(output, TarOptions(CompressionType.BZip2)), + BenchmarkArchiveKind.ZipDeflate => WriterFactory.OpenWriter(output, ArchiveType.Zip, ZipOptions(CompressionType.Deflate)), + BenchmarkArchiveKind.ZipStore => WriterFactory.OpenWriter(output, ArchiveType.Zip, ZipOptions(CompressionType.None)), + BenchmarkArchiveKind.SevenZipLzma => WriterFactory.OpenWriter(output, ArchiveType.SevenZip, ZipOptions(CompressionType.LZMA)), + _ => throw new ArgumentOutOfRangeException(nameof(kind)) + }; + + private static WriterOptions ZipOptions(CompressionType compressionType) => + new(compressionType) { LeaveStreamOpen = true }; + + private static TarWriterOptions TarOptions(CompressionType compressionType) => + // CompressionLevel defaults to 0 (stored) on TarWriterOptions, which would make + // tar.gz identical in size to plain tar and misrepresent the decompression cost. + new(compressionType, true, TarHeaderWriteFormat.USTAR) { LeaveStreamOpen = true, CompressionLevel = 6 }; + + } +} diff --git a/RecursiveExtractor.Benchmarks/ZipExtractionBenchmarks.cs b/RecursiveExtractor.Benchmarks/ZipExtractionBenchmarks.cs new file mode 100644 index 00000000..4a6ee9ff --- /dev/null +++ b/RecursiveExtractor.Benchmarks/ZipExtractionBenchmarks.cs @@ -0,0 +1,246 @@ +// Copyright (c) Microsoft Corporation. Licensed under the MIT License. + +using BenchmarkDotNet.Attributes; +using SharpCompress.Archives; +using SharpCompress.Archives.Zip; +using SharpCompress.Readers; +using System; +using System.Collections.Generic; +using System.IO; +using System.Threading.Tasks; + +namespace Microsoft.CST.RecursiveExtractor.Benchmarks +{ + /// + /// Measures how much time RecursiveExtractor adds on top of the underlying archive library + /// for a plain zip, and isolates the cost of the pieces that only RecursiveExtractor does + /// (quine detection, per-entry type detection, per-entry stream buffering). + /// + [MemoryDiagnoser] + public class ZipExtractionBenchmarks + { + private readonly Extractor _extractor = new(); + private readonly ExtractorOptions _options = new(); + private readonly ExtractorOptions _noRecurseOptions = new() { Recurse = false }; + + private byte[] _archive = Array.Empty(); + private byte[] _copyBuffer = Array.Empty(); + private FileEntry? _retainedRoot; + private List _retainedEntries = new(); + + /// + /// Number of entries in the benchmark archive. + /// + [Params(100, 1000)] + public int EntryCount { get; set; } + + /// + /// Size in bytes of each entry in the benchmark archive. + /// + [Params(4096)] + public int EntrySize { get; set; } + + /// + /// Builds the archive under test and materializes one extraction so the quine check can be + /// timed in isolation against exactly the same entries. + /// + [GlobalSetup] + public void Setup() + { + _archive = SyntheticArchives.Create(BenchmarkArchiveKind.ZipDeflate, EntryCount, EntrySize); + _copyBuffer = new byte[81920]; + + // Kept alive for the whole run: AreIdentical needs the parent's stream to still be readable. + _retainedRoot = new FileEntry("benchmark.zip", new MemoryStream(_archive, false), passthroughStream: true); + _retainedEntries = new List(EntryCount); + foreach (var entry in _extractor.Extract(_retainedRoot, _options)) + { + _retainedEntries.Add(entry); + } + } + + /// + /// Releases the entries retained for the quine benchmark. + /// + [GlobalCleanup] + public void Cleanup() + { + foreach (var entry in _retainedEntries) + { + entry.Content.Dispose(); + } + + _retainedEntries.Clear(); + _retainedRoot?.Content.Dispose(); + } + + /// + /// The floor: SharpCompress decompressing every entry, discarding the bytes. + /// + /// Bytes decompressed. + [Benchmark(Baseline = true, Description = "SharpCompress: decompress only")] + public long SharpCompressDecompressOnly() + { + long total = 0; + using var source = new MemoryStream(_archive, false); + using var archive = ZipArchive.OpenArchive(source, new ReaderOptions { LeaveStreamOpen = true }); + foreach (var entry in archive.Entries) + { + if (entry.IsDirectory) + { + continue; + } + + using var entryStream = entry.OpenEntryStream(); + total += entryStream.CopyToCounting(Stream.Null, _copyBuffer); + } + + return total; + } + + /// + /// SharpCompress decompressing every entry into a seekable MemoryStream, which is what + /// RecursiveExtractor has to do to hand back a seekable . + /// + /// Bytes buffered. + [Benchmark(Description = "SharpCompress: decompress + buffer")] + public long SharpCompressWithBuffering() + { + long total = 0; + using var source = new MemoryStream(_archive, false); + using var archive = ZipArchive.OpenArchive(source, new ReaderOptions { LeaveStreamOpen = true }); + foreach (var entry in archive.Entries) + { + if (entry.IsDirectory) + { + continue; + } + + using var entryStream = entry.OpenEntryStream(); + using var buffer = new MemoryStream(); + entryStream.CopyTo(buffer); + total += buffer.Length; + } + + return total; + } + + /// + /// RecursiveExtractor, synchronous, recursing into entries (the default). + /// + /// Bytes extracted. + [Benchmark(Description = "RE: Extract (recurse)")] + public long RecursiveExtractorExtract() + { + long total = 0; + using var source = new MemoryStream(_archive, false); + var root = new FileEntry("benchmark.zip", source, passthroughStream: true); + foreach (var entry in _extractor.Extract(root, _options)) + { + total += entry.Content.Length; + entry.Content.Dispose(); + } + + return total; + } + + /// + /// RecursiveExtractor with recursion disabled. The difference against + /// is the per-entry recursion overhead: + /// resource governor accounting, quine detection and archive type detection. + /// + /// Bytes extracted. + [Benchmark(Description = "RE: Extract (no recurse)")] + public long RecursiveExtractorExtractNoRecurse() + { + long total = 0; + using var source = new MemoryStream(_archive, false); + var root = new FileEntry("benchmark.zip", source, passthroughStream: true); + foreach (var entry in _extractor.Extract(root, _noRecurseOptions)) + { + total += entry.Content.Length; + entry.Content.Dispose(); + } + + return total; + } + + /// + /// RecursiveExtractor, asynchronous. + /// + /// Bytes extracted. + [Benchmark(Description = "RE: ExtractAsync (recurse)")] + public async Task RecursiveExtractorExtractAsync() + { + long total = 0; + using var source = new MemoryStream(_archive, false); + var root = new FileEntry("benchmark.zip", source, passthroughStream: true); + await foreach (var entry in _extractor.ExtractAsync(root, _options)) + { + total += entry.Content.Length; + entry.Content.Dispose(); + } + + return total; + } + + /// + /// The convenience overload that takes a Stream. It builds its own + /// without passthrough, so the whole archive is copied into a new backing stream first. + /// + /// Bytes extracted. + [Benchmark(Description = "RE: Extract(name, Stream) overload")] + public long RecursiveExtractorExtractStreamOverload() + { + long total = 0; + using var source = new MemoryStream(_archive, false); + foreach (var entry in _extractor.Extract("benchmark.zip", source, _options)) + { + total += entry.Content.Length; + entry.Content.Dispose(); + } + + return total; + } + + /// + /// The quine check alone, run over exactly the entries produced by extracting this archive. + /// Compare against to see its share of total time. + /// + /// Number of quines found (always zero here). + [Benchmark(Description = "RE: IsQuine over all entries only")] + public int QuineCheckOnly() + { + var found = 0; + for (var i = 0; i < _retainedEntries.Count; i++) + { + if (Extractor.IsQuine(_retainedEntries[i])) + { + found++; + } + } + + return found; + } + + /// + /// Archive type detection alone, run over the same entries. This is what + /// costs per entry during recursion. + /// + /// Number of entries detected as an archive. + [Benchmark(Description = "RE: DetectFileType over all entries only")] + public int TypeDetectionOnly() + { + var archives = 0; + for (var i = 0; i < _retainedEntries.Count; i++) + { + if (_retainedEntries[i].ArchiveType != ArchiveFileType.UNKNOWN) + { + archives++; + } + } + + return archives; + } + } +} diff --git a/RecursiveExtractor.Tests/StreamFactoryTests.cs b/RecursiveExtractor.Tests/StreamFactoryTests.cs new file mode 100644 index 00000000..f527577d --- /dev/null +++ b/RecursiveExtractor.Tests/StreamFactoryTests.cs @@ -0,0 +1,173 @@ +using Microsoft.CST.RecursiveExtractor; +using System; +using System.IO; +using System.Linq; +using System.Text; +using System.Threading.Tasks; +using Xunit; + +namespace RecursiveExtractor.Tests; + +public class StreamFactoryTests +{ + private const int Cutoff = 1024; + + /// + /// Wraps a stream so it reports the same characteristics as an archive library's entry + /// stream: readable, but not seekable and unable to report its length. + /// + private sealed class NonSeekableStream : Stream + { + private readonly Stream _inner; + + public NonSeekableStream(Stream inner) => _inner = inner; + + public override bool CanRead => true; + public override bool CanSeek => false; + public override bool CanWrite => false; + public override long Length => throw new NotSupportedException(); + + public override long Position + { + get => throw new NotSupportedException(); + set => throw new NotSupportedException(); + } + + public override void Flush() => _inner.Flush(); + public override int Read(byte[] buffer, int offset, int count) => _inner.Read(buffer, offset, count); + public override long Seek(long offset, SeekOrigin origin) => throw new NotSupportedException(); + public override void SetLength(long value) => throw new NotSupportedException(); + public override void Write(byte[] buffer, int offset, int count) => throw new NotSupportedException(); + + protected override void Dispose(bool disposing) + { + if (disposing) + { + _inner.Dispose(); + } + + base.Dispose(disposing); + } + } + + private static byte[] Payload(int size) + { + var bytes = new byte[size]; + new Random(42).NextBytes(bytes); + return bytes; + } + + [Fact] + public void StreamOfKnownLengthUnderCutoffIsBackedInMemory() + { + using var source = new MemoryStream(Payload(Cutoff / 2)); + using var backing = StreamFactory.GenerateAppropriateBackingStream(Cutoff, source, 4096); + Assert.IsType(backing); + } + + [Fact] + public void StreamOfKnownLengthOverCutoffIsBackedOnDisk() + { + using var source = new MemoryStream(Payload(Cutoff * 2)); + using var backing = StreamFactory.GenerateAppropriateBackingStream(Cutoff, source, 4096); + Assert.IsType(backing); + } + + /// + /// The regression guard for the temporary file per entry problem: an entry stream that cannot + /// report its length must not be assumed to be large. + /// + [Fact] + public void StreamOfUnknownLengthStaysInMemoryWhileSmall() + { + var content = Payload(Cutoff / 2); + using var source = new NonSeekableStream(new MemoryStream(content)); + using var backing = StreamFactory.GenerateAppropriateBackingStream(Cutoff, source, 4096); + + var spillOver = Assert.IsType(backing); + source.CopyTo(spillOver); + + Assert.False(spillOver.HasSpilledToDisk); + Assert.Equal(content.Length, spillOver.Length); + + spillOver.Position = 0; + using var readBack = new MemoryStream(); + spillOver.CopyTo(readBack); + Assert.Equal(content, readBack.ToArray()); + } + + [Fact] + public void StreamOfUnknownLengthMovesToDiskWhenItGrowsPastTheCutoff() + { + var content = Payload(Cutoff * 4); + using var source = new NonSeekableStream(new MemoryStream(content)); + using var backing = StreamFactory.GenerateAppropriateBackingStream(Cutoff, source, 4096); + + var spillOver = Assert.IsType(backing); + source.CopyTo(spillOver); + + Assert.True(spillOver.HasSpilledToDisk); + Assert.Equal(content.Length, spillOver.Length); + + spillOver.Position = 0; + using var readBack = new MemoryStream(); + spillOver.CopyTo(readBack); + Assert.Equal(content, readBack.ToArray()); + } + + [Fact] + public async Task SpillOverStreamPreservesContentWrittenAsynchronously() + { + var content = Payload(Cutoff * 4); + using var source = new NonSeekableStream(new MemoryStream(content)); + using var spillOver = new SpillOverStream(Cutoff, 4096); + + await source.CopyToAsync(spillOver); + + Assert.True(spillOver.HasSpilledToDisk); + + spillOver.Position = 0; + using var readBack = new MemoryStream(); + await spillOver.CopyToAsync(readBack); + Assert.Equal(content, readBack.ToArray()); + } + + [Fact] + public void SpillOverStreamPreservesContentWrittenOneByteAtATime() + { + var content = Encoding.ASCII.GetBytes(new string('a', Cutoff * 2)); + using var spillOver = new SpillOverStream(Cutoff, 4096); + + foreach (var b in content) + { + spillOver.WriteByte(b); + } + + Assert.True(spillOver.HasSpilledToDisk); + Assert.Equal(content.Length, spillOver.Length); + + spillOver.Position = 0; + Assert.Equal(content, Enumerable.Range(0, content.Length).Select(_ => (byte)spillOver.ReadByte()).ToArray()); + } + + [Fact] + public void SpillOverStreamSpillsWhenLengthIsSetPastTheCutoff() + { + using var spillOver = new SpillOverStream(Cutoff, 4096); + spillOver.SetLength(Cutoff * 2); + + Assert.True(spillOver.HasSpilledToDisk); + Assert.Equal(Cutoff * 2, spillOver.Length); + } + + /// + /// A null cutoff means the caller has opted out of the memory limit entirely. + /// + [Fact] + public void NoCutoffAlwaysUsesMemory() + { + using var source = new NonSeekableStream(new MemoryStream(Payload(Cutoff * 4))); + using var backing = StreamFactory.GenerateAppropriateBackingStream(null, source, 4096); + Assert.IsType(backing); + } +} diff --git a/RecursiveExtractor.sln b/RecursiveExtractor.sln index fe1ccb9e..9b33adee 100644 --- a/RecursiveExtractor.sln +++ b/RecursiveExtractor.sln @@ -17,28 +17,78 @@ Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "RecursiveExtractor.Cli", "R EndProject Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "RecursiveExtractor.Cli.Tests", "RecursiveExtractor.Cli.Tests\RecursiveExtractor.Cli.Tests.csproj", "{F37B314B-F641-4336-BCD6-BC5B85BEC5DB}" EndProject +Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "RecursiveExtractor.Benchmarks", "RecursiveExtractor.Benchmarks\RecursiveExtractor.Benchmarks.csproj", "{2F4511E3-7EB6-4131-9298-4D675F537904}" +EndProject Global GlobalSection(SolutionConfigurationPlatforms) = preSolution Debug|Any CPU = Debug|Any CPU + Debug|x64 = Debug|x64 + Debug|x86 = Debug|x86 Release|Any CPU = Release|Any CPU + Release|x64 = Release|x64 + Release|x86 = Release|x86 EndGlobalSection GlobalSection(ProjectConfigurationPlatforms) = postSolution {A7F7492B-60E0-468C-B267-BA60EC131E86}.Debug|Any CPU.ActiveCfg = Debug|Any CPU {A7F7492B-60E0-468C-B267-BA60EC131E86}.Debug|Any CPU.Build.0 = Debug|Any CPU + {A7F7492B-60E0-468C-B267-BA60EC131E86}.Debug|x64.ActiveCfg = Debug|Any CPU + {A7F7492B-60E0-468C-B267-BA60EC131E86}.Debug|x64.Build.0 = Debug|Any CPU + {A7F7492B-60E0-468C-B267-BA60EC131E86}.Debug|x86.ActiveCfg = Debug|Any CPU + {A7F7492B-60E0-468C-B267-BA60EC131E86}.Debug|x86.Build.0 = Debug|Any CPU {A7F7492B-60E0-468C-B267-BA60EC131E86}.Release|Any CPU.ActiveCfg = Release|Any CPU {A7F7492B-60E0-468C-B267-BA60EC131E86}.Release|Any CPU.Build.0 = Release|Any CPU + {A7F7492B-60E0-468C-B267-BA60EC131E86}.Release|x64.ActiveCfg = Release|Any CPU + {A7F7492B-60E0-468C-B267-BA60EC131E86}.Release|x64.Build.0 = Release|Any CPU + {A7F7492B-60E0-468C-B267-BA60EC131E86}.Release|x86.ActiveCfg = Release|Any CPU + {A7F7492B-60E0-468C-B267-BA60EC131E86}.Release|x86.Build.0 = Release|Any CPU {BB4A44C9-47E4-4BF5-A04A-D3A65E46D115}.Debug|Any CPU.ActiveCfg = Debug|Any CPU {BB4A44C9-47E4-4BF5-A04A-D3A65E46D115}.Debug|Any CPU.Build.0 = Debug|Any CPU + {BB4A44C9-47E4-4BF5-A04A-D3A65E46D115}.Debug|x64.ActiveCfg = Debug|Any CPU + {BB4A44C9-47E4-4BF5-A04A-D3A65E46D115}.Debug|x64.Build.0 = Debug|Any CPU + {BB4A44C9-47E4-4BF5-A04A-D3A65E46D115}.Debug|x86.ActiveCfg = Debug|Any CPU + {BB4A44C9-47E4-4BF5-A04A-D3A65E46D115}.Debug|x86.Build.0 = Debug|Any CPU {BB4A44C9-47E4-4BF5-A04A-D3A65E46D115}.Release|Any CPU.ActiveCfg = Release|Any CPU {BB4A44C9-47E4-4BF5-A04A-D3A65E46D115}.Release|Any CPU.Build.0 = Release|Any CPU + {BB4A44C9-47E4-4BF5-A04A-D3A65E46D115}.Release|x64.ActiveCfg = Release|Any CPU + {BB4A44C9-47E4-4BF5-A04A-D3A65E46D115}.Release|x64.Build.0 = Release|Any CPU + {BB4A44C9-47E4-4BF5-A04A-D3A65E46D115}.Release|x86.ActiveCfg = Release|Any CPU + {BB4A44C9-47E4-4BF5-A04A-D3A65E46D115}.Release|x86.Build.0 = Release|Any CPU {443B4E50-9AAF-436E-B3DF-644F782AF9B6}.Debug|Any CPU.ActiveCfg = Debug|Any CPU {443B4E50-9AAF-436E-B3DF-644F782AF9B6}.Debug|Any CPU.Build.0 = Debug|Any CPU + {443B4E50-9AAF-436E-B3DF-644F782AF9B6}.Debug|x64.ActiveCfg = Debug|Any CPU + {443B4E50-9AAF-436E-B3DF-644F782AF9B6}.Debug|x64.Build.0 = Debug|Any CPU + {443B4E50-9AAF-436E-B3DF-644F782AF9B6}.Debug|x86.ActiveCfg = Debug|Any CPU + {443B4E50-9AAF-436E-B3DF-644F782AF9B6}.Debug|x86.Build.0 = Debug|Any CPU {443B4E50-9AAF-436E-B3DF-644F782AF9B6}.Release|Any CPU.ActiveCfg = Release|Any CPU {443B4E50-9AAF-436E-B3DF-644F782AF9B6}.Release|Any CPU.Build.0 = Release|Any CPU + {443B4E50-9AAF-436E-B3DF-644F782AF9B6}.Release|x64.ActiveCfg = Release|Any CPU + {443B4E50-9AAF-436E-B3DF-644F782AF9B6}.Release|x64.Build.0 = Release|Any CPU + {443B4E50-9AAF-436E-B3DF-644F782AF9B6}.Release|x86.ActiveCfg = Release|Any CPU + {443B4E50-9AAF-436E-B3DF-644F782AF9B6}.Release|x86.Build.0 = Release|Any CPU {F37B314B-F641-4336-BCD6-BC5B85BEC5DB}.Debug|Any CPU.ActiveCfg = Debug|Any CPU {F37B314B-F641-4336-BCD6-BC5B85BEC5DB}.Debug|Any CPU.Build.0 = Debug|Any CPU + {F37B314B-F641-4336-BCD6-BC5B85BEC5DB}.Debug|x64.ActiveCfg = Debug|Any CPU + {F37B314B-F641-4336-BCD6-BC5B85BEC5DB}.Debug|x64.Build.0 = Debug|Any CPU + {F37B314B-F641-4336-BCD6-BC5B85BEC5DB}.Debug|x86.ActiveCfg = Debug|Any CPU + {F37B314B-F641-4336-BCD6-BC5B85BEC5DB}.Debug|x86.Build.0 = Debug|Any CPU {F37B314B-F641-4336-BCD6-BC5B85BEC5DB}.Release|Any CPU.ActiveCfg = Release|Any CPU {F37B314B-F641-4336-BCD6-BC5B85BEC5DB}.Release|Any CPU.Build.0 = Release|Any CPU + {F37B314B-F641-4336-BCD6-BC5B85BEC5DB}.Release|x64.ActiveCfg = Release|Any CPU + {F37B314B-F641-4336-BCD6-BC5B85BEC5DB}.Release|x64.Build.0 = Release|Any CPU + {F37B314B-F641-4336-BCD6-BC5B85BEC5DB}.Release|x86.ActiveCfg = Release|Any CPU + {F37B314B-F641-4336-BCD6-BC5B85BEC5DB}.Release|x86.Build.0 = Release|Any CPU + {2F4511E3-7EB6-4131-9298-4D675F537904}.Debug|Any CPU.ActiveCfg = Debug|Any CPU + {2F4511E3-7EB6-4131-9298-4D675F537904}.Debug|Any CPU.Build.0 = Debug|Any CPU + {2F4511E3-7EB6-4131-9298-4D675F537904}.Debug|x64.ActiveCfg = Debug|Any CPU + {2F4511E3-7EB6-4131-9298-4D675F537904}.Debug|x64.Build.0 = Debug|Any CPU + {2F4511E3-7EB6-4131-9298-4D675F537904}.Debug|x86.ActiveCfg = Debug|Any CPU + {2F4511E3-7EB6-4131-9298-4D675F537904}.Debug|x86.Build.0 = Debug|Any CPU + {2F4511E3-7EB6-4131-9298-4D675F537904}.Release|Any CPU.ActiveCfg = Release|Any CPU + {2F4511E3-7EB6-4131-9298-4D675F537904}.Release|Any CPU.Build.0 = Release|Any CPU + {2F4511E3-7EB6-4131-9298-4D675F537904}.Release|x64.ActiveCfg = Release|Any CPU + {2F4511E3-7EB6-4131-9298-4D675F537904}.Release|x64.Build.0 = Release|Any CPU + {2F4511E3-7EB6-4131-9298-4D675F537904}.Release|x86.ActiveCfg = Release|Any CPU + {2F4511E3-7EB6-4131-9298-4D675F537904}.Release|x86.Build.0 = Release|Any CPU EndGlobalSection GlobalSection(SolutionProperties) = preSolution HideSolutionNode = FALSE diff --git a/RecursiveExtractor/Extractors/ZipExtractor.cs b/RecursiveExtractor/Extractors/ZipExtractor.cs index 5140a60d..3ebf2665 100644 --- a/RecursiveExtractor/Extractors/ZipExtractor.cs +++ b/RecursiveExtractor/Extractors/ZipExtractor.cs @@ -135,7 +135,7 @@ public async IAsyncEnumerable ExtractAsync(FileEntry fileEntry, Extra try { using var zipStream = zipEntry.OpenEntryStream(); - target = StreamFactory.GenerateAppropriateBackingStream(options, zipStream); + target = StreamFactory.GenerateAppropriateBackingStream(options, zipEntry.Size); await zipStream.CopyToAsync(target); } catch (Exception e) @@ -369,7 +369,7 @@ private async IAsyncEnumerable YieldNonIndexedEntriesAsync( { using var readerStream = forwardReader.OpenEntryStream(); governor.CheckResourceGovernor(forwardReader.Entry.Size); - payload = StreamFactory.GenerateAppropriateBackingStream(options, readerStream); + payload = StreamFactory.GenerateAppropriateBackingStream(options, forwardReader.Entry.Size); await readerStream.CopyToAsync(payload); } catch (Exception ex) @@ -452,7 +452,7 @@ private IEnumerable YieldNonIndexedEntries( { using var readerStream = forwardReader.OpenEntryStream(); governor.CheckResourceGovernor(forwardReader.Entry.Size); - payload = StreamFactory.GenerateAppropriateBackingStream(options, readerStream); + payload = StreamFactory.GenerateAppropriateBackingStream(options, forwardReader.Entry.Size); readerStream.CopyTo(payload); } catch (Exception ex) diff --git a/RecursiveExtractor/SpillOverStream.cs b/RecursiveExtractor/SpillOverStream.cs new file mode 100644 index 00000000..447dea5e --- /dev/null +++ b/RecursiveExtractor/SpillOverStream.cs @@ -0,0 +1,140 @@ +using System; +using System.IO; +using System.Threading; +using System.Threading.Tasks; + +namespace Microsoft.CST.RecursiveExtractor; + +/// +/// A seekable, read-write backing stream that starts in memory and moves itself to a +/// delete-on-close once the content written to it exceeds a cutoff. +/// +/// +/// This exists for the case where the size of the content being buffered is not knowable up +/// front. Most archive libraries expose entry streams that cannot seek and throw when +/// is read, so choosing a backing store before the copy means +/// either assuming everything is small (risking memory exhaustion) or assuming everything is +/// large (paying for a temporary file per entry). Deferring the decision until the content +/// actually crosses the cutoff avoids both. +/// +internal sealed class SpillOverStream : Stream +{ + private readonly long _memoryStreamCutoff; + private readonly int _fileStreamBufferSize; + private Stream _backingStream; + + /// + /// Creates a stream backed by memory until bytes are + /// written to it. + /// + /// Largest size in bytes to keep in memory. + /// Buffer size for the FileStream spilled to. + internal SpillOverStream(long memoryStreamCutoff, int fileStreamBufferSize) + { + _memoryStreamCutoff = memoryStreamCutoff; + _fileStreamBufferSize = fileStreamBufferSize; + _backingStream = new MemoryStream(); + } + + /// + /// True once the content has grown past the cutoff and moved to a file on disk. + /// + internal bool HasSpilledToDisk { get; private set; } + + /// + public override bool CanRead => _backingStream.CanRead; + + /// + public override bool CanSeek => _backingStream.CanSeek; + + /// + public override bool CanWrite => _backingStream.CanWrite; + + /// + public override long Length => _backingStream.Length; + + /// + public override long Position + { + get => _backingStream.Position; + set => _backingStream.Position = value; + } + + /// + public override void Flush() => _backingStream.Flush(); + + /// + public override Task FlushAsync(CancellationToken cancellationToken) => _backingStream.FlushAsync(cancellationToken); + + /// + public override int Read(byte[] buffer, int offset, int count) => _backingStream.Read(buffer, offset, count); + + /// + public override Task ReadAsync(byte[] buffer, int offset, int count, CancellationToken cancellationToken) => + _backingStream.ReadAsync(buffer, offset, count, cancellationToken); + + /// + public override int ReadByte() => _backingStream.ReadByte(); + + /// + public override long Seek(long offset, SeekOrigin origin) => _backingStream.Seek(offset, origin); + + /// + public override void SetLength(long value) + { + SpillIfNeeded(value); + _backingStream.SetLength(value); + } + + /// + public override void Write(byte[] buffer, int offset, int count) + { + SpillIfNeeded(_backingStream.Position + count); + _backingStream.Write(buffer, offset, count); + } + + /// + public override async Task WriteAsync(byte[] buffer, int offset, int count, CancellationToken cancellationToken) + { + SpillIfNeeded(_backingStream.Position + count); + await _backingStream.WriteAsync(buffer, offset, count, cancellationToken).ConfigureAwait(false); + } + + /// + public override void WriteByte(byte value) + { + SpillIfNeeded(_backingStream.Position + 1); + _backingStream.WriteByte(value); + } + + /// + protected override void Dispose(bool disposing) + { + if (disposing) + { + _backingStream.Dispose(); + } + + base.Dispose(disposing); + } + + private void SpillIfNeeded(long requiredLength) + { + if (HasSpilledToDisk || requiredLength <= _memoryStreamCutoff) + { + return; + } + + var memoryStream = _backingStream; + var fileStream = StreamFactory.GenerateDeleteOnCloseFileStream(_fileStreamBufferSize); + + var position = memoryStream.Position; + memoryStream.Position = 0; + memoryStream.CopyTo(fileStream); + fileStream.Position = position; + + _backingStream = fileStream; + HasSpilledToDisk = true; + memoryStream.Dispose(); + } +} diff --git a/RecursiveExtractor/StreamFactory.cs b/RecursiveExtractor/StreamFactory.cs index dbd02c02..ee2d6dc6 100644 --- a/RecursiveExtractor/StreamFactory.cs +++ b/RecursiveExtractor/StreamFactory.cs @@ -39,9 +39,14 @@ public static Stream GenerateAppropriateBackingStream(ExtractorOptions opts, lon /// internal static Stream GenerateAppropriateBackingStream(int? memoryStreamCutoff, Stream targetStream, int fileStreamBufferSize) { + if (memoryStreamCutoff is not int cutoff) + { + return new MemoryStream(); + } + try { - if (targetStream.Length > memoryStreamCutoff) + if (targetStream.Length > cutoff) { return GenerateDeleteOnCloseFileStream(fileStreamBufferSize); } @@ -49,7 +54,11 @@ internal static Stream GenerateAppropriateBackingStream(int? memoryStreamCutoff, } catch (Exception) { - return GenerateDeleteOnCloseFileStream(fileStreamBufferSize); + // The length isn't knowable without draining the stream, which is the normal case for + // archive entry streams. Rather than assume the content is large and pay for a + // temporary file on every entry, buffer in memory and move to disk only if the content + // turns out to exceed the cutoff. + return new SpillOverStream(cutoff, fileStreamBufferSize); } } From 123afe1529cbbc65d2730fcbfcdefcfcfcd7dc35 Mon Sep 17 00:00:00 2001 From: Giulia Stocco <98900+gfs@users.noreply.github.com> Date: Fri, 31 Jul 2026 11:02:06 -0700 Subject: [PATCH 2/3] Restore the memory cutoff on the non-indexed ZIP scan The non-indexed entry scan walks local file headers with a forward-only reader. For entries written with a data descriptor the local header reports an uncompressed size of 0, so sizing the backing store from Entry.Size buffered arbitrarily large hidden content in memory no matter what MemoryStreamCutoff was set to. Measured: a 4 MB hidden entry landed in a MemoryStream under a 64 KB cutoff, on both the sync and async paths. Pass the entry stream instead and let SpillOverStream make the decision from the bytes actually observed. That keeps the guard intact without reintroducing a temporary file per entry, which is what the stream overload used to cost before SpillOverStream existed. The two cataloged ZipArchive call sites read from the central directory and keep using Entry.Size. Also: - Open the entry stream before reading Size on the sync cataloged path. SharpCompress refreshes Size from the local file header on open, so reading it first used the stale central directory value; the async path already had the safer ordering. - Dispose the spill target in SpillOverStream if the copy into it throws, and use MemoryStream.WriteTo rather than a chunked CopyTo. - Override the span and memory overloads on SpillOverStream. These are what CopyToAsync actually calls on modern targets; the base implementations routed to the byte[] overloads correctly but rented and copied through an ArrayPool buffer on every write. - Document that entries of unknown size are buffered in memory up to the cutoff, and that Parallel bounds peak usage at BatchSize * cutoff. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 991fa3eb-2417-4a56-80b3-d56a7c87b9c0 --- README.md | 5 + .../ExtractorTests/NonIndexedZipEntryTests.cs | 116 ++++++++++++++++++ .../StreamFactoryTests.cs | 69 +++++++++++ RecursiveExtractor/ExtractorOptions.cs | 10 ++ RecursiveExtractor/Extractors/ZipExtractor.cs | 17 ++- RecursiveExtractor/SpillOverStream.cs | 41 ++++++- 6 files changed, 250 insertions(+), 8 deletions(-) diff --git a/README.md b/README.md index 65cccf52..52e549cf 100644 --- a/README.md +++ b/README.md @@ -303,6 +303,11 @@ foreach(var file in results) } ``` +### Controlling Memory Usage +The `Content` stream of each `FileEntry` is held in memory while it is smaller than `ExtractorOptions.MemoryStreamCutoff` (100MB by default) and backed by a temporary file once it grows past it. Entries whose size is not known before extraction begins start in memory and move to disk only if they actually exceed the cutoff. + +Because entries are held concurrently when `ExtractorOptions.Parallel` is set, peak memory is bounded by `BatchSize * MemoryStreamCutoff`. Lower `MemoryStreamCutoff` if you are extracting in a memory constrained environment, or raise it to trade memory for fewer temporary files. + # Feedback If you have any issues or feature requests (for example, supporting other formats) you can open a new [Issue](https://github.com/microsoft/RecursiveExtractor/issues/new). diff --git a/RecursiveExtractor.Tests/ExtractorTests/NonIndexedZipEntryTests.cs b/RecursiveExtractor.Tests/ExtractorTests/NonIndexedZipEntryTests.cs index b210fc07..c9d17537 100644 --- a/RecursiveExtractor.Tests/ExtractorTests/NonIndexedZipEntryTests.cs +++ b/RecursiveExtractor.Tests/ExtractorTests/NonIndexedZipEntryTests.cs @@ -189,4 +189,120 @@ public void NormalZip_WithOptionOn_ReturnsNoNonIndexedEntries() Assert.True(results.Count > 0, "Expected at least one entry from TestData.zip"); Assert.DoesNotContain(results, r => r.EntryStatus == FileEntryStatus.NonIndexedEntry); } + + /// + /// Builds a tampered ZIP whose hidden entry mimics one written to a non-seekable stream: the + /// local file header declares an uncompressed size of 0 and the real size would trail the + /// payload in a data descriptor. The forward-only reader used by the non-indexed scan only + /// ever sees that local header. + /// + private static byte[] CraftZipWithLargeStreamingHiddenEntry(int hiddenSize) + { + byte[] zipWithVisible; + using (var ms = new MemoryStream()) + { + using (var arc = new ZipArchive(ms, ZipArchiveMode.Create, leaveOpen: true)) + { + var ve = arc.CreateEntry("visible.txt", CompressionLevel.NoCompression); + using var vw = new StreamWriter(ve.Open()); + vw.Write("VISIBLE_PAYLOAD"); + } + zipWithVisible = ms.ToArray(); + } + + byte[] zipWithHidden; + using (var ms = new MemoryStream()) + { + using (var arc = new ZipArchive(ms, ZipArchiveMode.Create, leaveOpen: true)) + { + var he = arc.CreateEntry("hidden.bin", CompressionLevel.Optimal); + using var hs = he.Open(); + hs.Write(new byte[hiddenSize], 0, hiddenSize); + } + zipWithHidden = ms.ToArray(); + } + + // hidden.bin's local file header sits at offset 0; its uncompressed size field is at +22. + Assert.True(zipWithHidden[0] == 0x50 && zipWithHidden[1] == 0x4B && zipWithHidden[2] == 0x03 && zipWithHidden[3] == 0x04, + "Expected a local file header at the start of the hidden ZIP"); + Array.Clear(zipWithHidden, 22, 4); + + int eocdVisible = ScanBackwardsForEocd(zipWithVisible); + int eocdHidden = ScanBackwardsForEocd(zipWithHidden); + uint cdOffsetVisible = BitConverter.ToUInt32(zipWithVisible, eocdVisible + 16); + uint cdOffsetHidden = BitConverter.ToUInt32(zipWithHidden, eocdHidden + 16); + + using var output = new MemoryStream(); + output.Write(zipWithVisible, 0, (int)cdOffsetVisible); + output.Write(zipWithHidden, 0, (int)cdOffsetHidden); + + int tailLength = zipWithVisible.Length - (int)cdOffsetVisible; + var tail = new byte[tailLength]; + Array.Copy(zipWithVisible, (int)cdOffsetVisible, tail, 0, tailLength); + BitConverter.GetBytes(cdOffsetVisible + cdOffsetHidden).CopyTo(tail, eocdVisible - (int)cdOffsetVisible + 16); + output.Write(tail, 0, tail.Length); + + return output.ToArray(); + } + + private static void AssertNotHeldEntirelyInMemory(Stream content) + { + if (content is SpillOverStream spillOver) + { + Assert.True(spillOver.HasSpilledToDisk, "Content exceeding the cutoff should have spilled to disk."); + return; + } + + Assert.IsType(content); + } + + /// + /// Regression guard: the non-indexed scan must not size its backing store from the declared + /// entry size. A forward-only reader reports 0 for streaming entries, which would silently + /// buffer arbitrarily large hidden content in memory regardless of MemoryStreamCutoff. + /// + [Theory] + [InlineData(true)] + [InlineData(false)] + public async Task LargeStreamingHiddenEntry_RespectsMemoryStreamCutoff(bool useAsync) + { + const int hiddenSize = 4 * 1024 * 1024; + var tamperedBytes = CraftZipWithLargeStreamingHiddenEntry(hiddenSize); + + var options = new ExtractorOptions + { + ExtractNonIndexedEntries = true, + Recurse = false, + MemoryStreamCutoff = 64 * 1024, + // Isolate the backing store decision from the zip bomb guard. + MaxExtractedBytesRatio = 0, + MaxExtractedBytes = long.MaxValue, + }; + + var extractor = new Extractor(); + var fe = new FileEntry("tampered.zip", new MemoryStream(tamperedBytes), passthroughStream: true); + + var results = new List(); + if (useAsync) + { + await foreach (var entry in extractor.ExtractAsync(fe, options)) + { + results.Add(entry); + } + } + else + { + results.AddRange(extractor.Extract(fe, options)); + } + + var hiddenResult = results.First(r => r.Name == "hidden.bin"); + Assert.Equal(FileEntryStatus.NonIndexedEntry, hiddenResult.EntryStatus); + Assert.Equal(hiddenSize, hiddenResult.Content.Length); + AssertNotHeldEntirelyInMemory(hiddenResult.Content); + + foreach (var result in results) + { + result.Content.Dispose(); + } + } } diff --git a/RecursiveExtractor.Tests/StreamFactoryTests.cs b/RecursiveExtractor.Tests/StreamFactoryTests.cs index f527577d..a79ddcf7 100644 --- a/RecursiveExtractor.Tests/StreamFactoryTests.cs +++ b/RecursiveExtractor.Tests/StreamFactoryTests.cs @@ -170,4 +170,73 @@ public void NoCutoffAlwaysUsesMemory() using var backing = StreamFactory.GenerateAppropriateBackingStream(null, source, 4096); Assert.IsType(backing); } + +#if !NETFRAMEWORK + /// + /// The span and memory overloads are what actually + /// calls on modern targets, so the spill decision has to be made there too. + /// + [Fact] + public void SpillOverStreamSpillsWhenWrittenThroughSpanOverload() + { + var content = Payload(Cutoff * 4); + using var spillOver = new SpillOverStream(Cutoff, 4096); + + spillOver.Write(new ReadOnlySpan(content)); + + Assert.True(spillOver.HasSpilledToDisk); + Assert.Equal(content.Length, spillOver.Length); + + spillOver.Position = 0; + var readBack = new byte[content.Length]; + Assert.Equal(content.Length, spillOver.Read(new Span(readBack))); + Assert.Equal(content, readBack); + } + + [Fact] + public async Task SpillOverStreamSpillsWhenWrittenThroughMemoryOverload() + { + var content = Payload(Cutoff * 4); + using var spillOver = new SpillOverStream(Cutoff, 4096); + + await spillOver.WriteAsync(new ReadOnlyMemory(content)); + + Assert.True(spillOver.HasSpilledToDisk); + + spillOver.Position = 0; + var readBack = new byte[content.Length]; + Assert.Equal(content.Length, await spillOver.ReadAsync(new Memory(readBack))); + Assert.Equal(content, readBack); + } +#endif + + /// + /// Content written before the spill has to survive it, including bytes that were overwritten + /// in place while the stream was still in memory. + /// + [Fact] + public void SpillOverStreamPreservesRewrittenContentAcrossTheSpill() + { + var content = Payload(Cutoff / 2); + using var spillOver = new SpillOverStream(Cutoff, 4096); + + spillOver.Write(content, 0, content.Length); + spillOver.Position = 4; + spillOver.Write(new byte[] { 1, 2, 3 }, 0, 3); + Assert.False(spillOver.HasSpilledToDisk); + + spillOver.Position = spillOver.Length; + var tail = Payload(Cutoff * 2); + spillOver.Write(tail, 0, tail.Length); + Assert.True(spillOver.HasSpilledToDisk); + + content[4] = 1; + content[5] = 2; + content[6] = 3; + + spillOver.Position = 0; + using var readBack = new MemoryStream(); + spillOver.CopyTo(readBack); + Assert.Equal(content.Concat(tail).ToArray(), readBack.ToArray()); + } } diff --git a/RecursiveExtractor/ExtractorOptions.cs b/RecursiveExtractor/ExtractorOptions.cs index 7852cd8d..f5f15d89 100644 --- a/RecursiveExtractor/ExtractorOptions.cs +++ b/RecursiveExtractor/ExtractorOptions.cs @@ -19,6 +19,16 @@ public class ExtractorOptions /// /// Maximum number of bytes before using a FileStream. Default 100MB /// + /// + /// When the size of an entry is known up front the backing store is chosen before the + /// copy. When it is not — which is the normal case for archive libraries that expose + /// non-seekable entry streams — the content is buffered in memory and moved to a + /// temporary file only once it grows past this value, so a single entry never occupies + /// more than the cutoff in memory. Note that entries are held concurrently when + /// is set, so the peak is bounded by + /// * ; lower this value if you + /// are extracting in a memory constrained environment. + /// public int MemoryStreamCutoff { get; set; } = 1024 * 1024 * 100; /// diff --git a/RecursiveExtractor/Extractors/ZipExtractor.cs b/RecursiveExtractor/Extractors/ZipExtractor.cs index 3ebf2665..198baa97 100644 --- a/RecursiveExtractor/Extractors/ZipExtractor.cs +++ b/RecursiveExtractor/Extractors/ZipExtractor.cs @@ -135,6 +135,8 @@ public async IAsyncEnumerable ExtractAsync(FileEntry fileEntry, Extra try { using var zipStream = zipEntry.OpenEntryStream(); + // Opened first so Size reflects the local file header, which SharpCompress + // refreshes on open and which a tampered central directory cannot fake. target = StreamFactory.GenerateAppropriateBackingStream(options, zipEntry.Size); await zipStream.CopyToAsync(target); } @@ -255,11 +257,14 @@ public IEnumerable Extract(FileEntry fileEntry, ExtractorOptions opti governor.CheckResourceGovernor(zipEntry.Size); - var fs = StreamFactory.GenerateAppropriateBackingStream(options, zipEntry.Size); + Stream? fs = null; try { using var zipStream = zipEntry.OpenEntryStream(); + // Opened first so Size reflects the local file header, which SharpCompress + // refreshes on open and which a tampered central directory cannot fake. + fs = StreamFactory.GenerateAppropriateBackingStream(options, zipEntry.Size); zipStream.CopyTo(fs); } catch (Exception e) @@ -369,7 +374,11 @@ private async IAsyncEnumerable YieldNonIndexedEntriesAsync( { using var readerStream = forwardReader.OpenEntryStream(); governor.CheckResourceGovernor(forwardReader.Entry.Size); - payload = StreamFactory.GenerateAppropriateBackingStream(options, forwardReader.Entry.Size); + // Deliberately size the backing store from the stream rather than + // Entry.Size: a forward-only reader sees only the local file header, which + // reports 0 for entries written with a data descriptor. Trusting it would + // buffer arbitrarily large content in memory regardless of MemoryStreamCutoff. + payload = StreamFactory.GenerateAppropriateBackingStream(options, readerStream); await readerStream.CopyToAsync(payload); } catch (Exception ex) @@ -452,7 +461,9 @@ private IEnumerable YieldNonIndexedEntries( { using var readerStream = forwardReader.OpenEntryStream(); governor.CheckResourceGovernor(forwardReader.Entry.Size); - payload = StreamFactory.GenerateAppropriateBackingStream(options, forwardReader.Entry.Size); + // See the note in YieldNonIndexedEntriesAsync: Entry.Size is not trustworthy + // for a forward-only reader, so let the stream drive the backing decision. + payload = StreamFactory.GenerateAppropriateBackingStream(options, readerStream); readerStream.CopyTo(payload); } catch (Exception ex) diff --git a/RecursiveExtractor/SpillOverStream.cs b/RecursiveExtractor/SpillOverStream.cs index 447dea5e..c6339c19 100644 --- a/RecursiveExtractor/SpillOverStream.cs +++ b/RecursiveExtractor/SpillOverStream.cs @@ -107,6 +107,29 @@ public override void WriteByte(byte value) _backingStream.WriteByte(value); } +#if !NETSTANDARD2_0 + /// + public override int Read(Span buffer) => _backingStream.Read(buffer); + + /// + public override ValueTask ReadAsync(Memory buffer, CancellationToken cancellationToken = default) => + _backingStream.ReadAsync(buffer, cancellationToken); + + /// + public override void Write(ReadOnlySpan buffer) + { + SpillIfNeeded(_backingStream.Position + buffer.Length); + _backingStream.Write(buffer); + } + + /// + public override async ValueTask WriteAsync(ReadOnlyMemory buffer, CancellationToken cancellationToken = default) + { + SpillIfNeeded(_backingStream.Position + buffer.Length); + await _backingStream.WriteAsync(buffer, cancellationToken).ConfigureAwait(false); + } +#endif + /// protected override void Dispose(bool disposing) { @@ -125,13 +148,21 @@ private void SpillIfNeeded(long requiredLength) return; } - var memoryStream = _backingStream; + // Only reachable while the backing store is still the MemoryStream created in the + // constructor, since HasSpilledToDisk flips at the same time the field is replaced. + var memoryStream = (MemoryStream)_backingStream; var fileStream = StreamFactory.GenerateDeleteOnCloseFileStream(_fileStreamBufferSize); - var position = memoryStream.Position; - memoryStream.Position = 0; - memoryStream.CopyTo(fileStream); - fileStream.Position = position; + try + { + memoryStream.WriteTo(fileStream); + fileStream.Position = memoryStream.Position; + } + catch + { + fileStream.Dispose(); + throw; + } _backingStream = fileStream; HasSpilledToDisk = true; From befc63b68392001fbb6563bc0392447dc622e298 Mon Sep 17 00:00:00 2001 From: Giulia Stocco <98900+gfs@users.noreply.github.com> Date: Fri, 31 Jul 2026 16:45:21 -0700 Subject: [PATCH 3/3] Dispose SharpCompress entry streams in the 7z and rar extractors SevenZipExtractor and RarExtractor never disposed the stream returned by entry.OpenEntryStream(). 7z is a solid format, so SharpCompress builds a fresh decoder chain for every entry, and each chain holds a 1 MiB LZMA dictionary plus a 128 KiB read cache. Holding those open for the life of the enumeration retained roughly 1.15 MiB per entry, nearly all of it on the large object heap. FileEntry copies the entry content into its own backing stream before it is returned, so the source stream can be disposed before yielding. Ace, Arc, Arj, Tar and Zip already did this. Extracting a 200 entry 7z: 320 ms -> 31 ms and 240 MB -> 10 MB allocated, which is 1.10x the raw SharpCompress baseline on both, down from 26.35x. Adds EntryStreamDisposalTests as a regression guard. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8aea0ccd-2ed2-4d36-ba1e-b69a21b4a9ef --- RecursiveExtractor.Benchmarks/README.md | 9 ++ .../EntryStreamDisposalTests.cs | 105 ++++++++++++++++++ RecursiveExtractor/Extractors/RarExtractor.cs | 11 +- .../Extractors/SevenZipExtractor.cs | 18 ++- 4 files changed, 139 insertions(+), 4 deletions(-) create mode 100644 RecursiveExtractor.Tests/EntryStreamDisposalTests.cs diff --git a/RecursiveExtractor.Benchmarks/README.md b/RecursiveExtractor.Benchmarks/README.md index baf5d64b..f78b4730 100644 --- a/RecursiveExtractor.Benchmarks/README.md +++ b/RecursiveExtractor.Benchmarks/README.md @@ -69,3 +69,12 @@ Measured on a 1000 entry x 4 KiB deflate zip, ShortRun, net8.0: `zipEntry.Size` and by making the unknown-length fallback a `SpillOverStream` that only moves to disk if the content actually exceeds `MemoryStreamCutoff`. Async is now within ~15% of sync, and 7z (which hits the same fallback through `FileEntry`) got ~6x faster. +- **7z and rar used to allocate ~26x the baseline.** `SevenZipExtractor` and `RarExtractor` never + disposed the stream returned by `entry.OpenEntryStream()`. Because 7z is a solid format, + SharpCompress builds a fresh decoder chain per entry, and each one holds a 1 MiB LZMA dictionary + plus a 128 KiB read cache. At 200 entries that is ~230 MiB retained, nearly all of it on the large + object heap. The equivalent baseline in `ArchiveFormatBenchmarks.ReadWithArchiveApi` used `using`, + which is what made the gap visible. `FileEntry` copies the content into its own backing stream, so + the source can be disposed before yielding. 7z is now 1.10x the baseline on both time and + allocations, down from 26.35x: 320 ms -> 31 ms and 240 MB -> 10 MB. + `EntryStreamDisposalTests` guards against a regression. diff --git a/RecursiveExtractor.Tests/EntryStreamDisposalTests.cs b/RecursiveExtractor.Tests/EntryStreamDisposalTests.cs new file mode 100644 index 00000000..6e3c9dca --- /dev/null +++ b/RecursiveExtractor.Tests/EntryStreamDisposalTests.cs @@ -0,0 +1,105 @@ +using Microsoft.CST.RecursiveExtractor; +using SharpCompress.Common; +using SharpCompress.Writers; +using System; +using System.IO; +using System.Linq; +using Xunit; + +namespace RecursiveExtractor.Tests; + +/// +/// Guards against re-introducing the per-entry decoder leak, where an extractor passed +/// entry.OpenEntryStream() into a without disposing it. +/// +/// +/// 7-Zip is solid, so SharpCompress builds a fresh LZMA decoder chain for every entry stream that +/// is opened. Each chain holds a 1 MiB dictionary window plus a 128 KiB read cache, and both are +/// only released when the entry stream is disposed. Leaving them open therefore costs roughly +/// 1.1 MiB per entry and pushes the dictionary onto the large object heap. +/// +public class EntryStreamDisposalTests +{ + private const int EntryCount = 40; + private const int EntrySize = 8192; + + /// + /// Comfortably above what extraction actually allocates per entry (~56 KiB) and comfortably + /// below the ~1.1 MiB per entry that leaking a decoder chain would cost. + /// + private const long MaxBytesAllocatedPerEntry = 300 * 1024; + + private static byte[] CreateSevenZip() + { + using var output = new MemoryStream(); + using (var writer = WriterFactory.OpenWriter(output, ArchiveType.SevenZip, new WriterOptions(CompressionType.LZMA) { LeaveStreamOpen = true })) + { + var random = new Random(1234); + for (var i = 0; i < EntryCount; i++) + { + var payload = new byte[EntrySize]; + random.NextBytes(payload); + using var entryStream = new MemoryStream(payload, false); + writer.Write($"entry{i:D4}.bin", entryStream, new DateTime(1970, 1, 1, 0, 0, 0, DateTimeKind.Utc)); + } + } + + return output.ToArray(); + } + + private static long Extract(byte[] archive) + { + long total = 0; + var extractor = new Extractor(); + using var source = new MemoryStream(archive, false); + var root = new FileEntry("test.7z", source, passthroughStream: true); + foreach (var entry in extractor.Extract(root, new ExtractorOptions())) + { + total += entry.Content.Length; + entry.Content.Dispose(); + } + + return total; + } + + [Fact] + public void SevenZipExtractionDoesNotRetainADecoderPerEntry() + { + var archive = CreateSevenZip(); + + // Warm up so JIT and one time initialisation are not attributed to the measured run. + Assert.Equal(EntryCount * (long)EntrySize, Extract(archive)); + +#if !NETFRAMEWORK + // Per thread rather than per process, so tests running in parallel cannot skew the result. + // Extract is synchronous, so everything it allocates lands on this thread. + var before = GC.GetAllocatedBytesForCurrentThread(); + var extracted = Extract(archive); + var allocatedPerEntry = (GC.GetAllocatedBytesForCurrentThread() - before) / EntryCount; + + Assert.Equal(EntryCount * (long)EntrySize, extracted); + Assert.True( + allocatedPerEntry < MaxBytesAllocatedPerEntry, + $"Extraction allocated {allocatedPerEntry} bytes per entry, over the {MaxBytesAllocatedPerEntry} byte budget. " + + "This usually means an entry stream is no longer being disposed, leaking a decoder and its dictionary per entry."); +#endif + } + + [Fact] + public void SevenZipExtractionStillReturnsEveryEntryIntact() + { + var archive = CreateSevenZip(); + var extractor = new Extractor(); + using var source = new MemoryStream(archive, false); + var root = new FileEntry("test.7z", source, passthroughStream: true); + + var entries = extractor.Extract(root, new ExtractorOptions()).ToList(); + + Assert.Equal(EntryCount, entries.Count); + foreach (var entry in entries) + { + Assert.Equal(EntrySize, entry.Content.Length); + entry.Content.Dispose(); + } + } +} diff --git a/RecursiveExtractor/Extractors/RarExtractor.cs b/RecursiveExtractor/Extractors/RarExtractor.cs index 85d7ff80..9f022499 100644 --- a/RecursiveExtractor/Extractors/RarExtractor.cs +++ b/RecursiveExtractor/Extractors/RarExtractor.cs @@ -105,7 +105,14 @@ public async IAsyncEnumerable ExtractAsync(FileEntry fileEntry, Extra { governor.CheckResourceGovernor(entry.Size); var name = (entry.Key ?? string.Empty).Replace('/', Path.DirectorySeparatorChar); - var newFileEntry = await FileEntry.FromStreamAsync(name, entry.OpenEntryStream(), fileEntry, entry.CreatedTime, entry.LastModifiedTime, entry.LastAccessedTime, memoryStreamCutoff: options.MemoryStreamCutoff).ConfigureAwait(false); + FileEntry newFileEntry; + // Dispose the entry stream before yielding. SharpCompress keeps a decoder and + // its dictionary buffer alive per open entry stream, and FileEntry has already + // taken its own copy of the content by this point. + using (var entryStream = entry.OpenEntryStream()) + { + newFileEntry = await FileEntry.FromStreamAsync(name, entryStream, fileEntry, entry.CreatedTime, entry.LastModifiedTime, entry.LastAccessedTime, memoryStreamCutoff: options.MemoryStreamCutoff).ConfigureAwait(false); + } if (newFileEntry != null) { try @@ -156,7 +163,7 @@ public IEnumerable Extract(FileEntry fileEntry, ExtractorOptions opti FileEntry? newFileEntry = null; try { - var stream = entry.OpenEntryStream(); + using var stream = entry.OpenEntryStream(); var name = (entry.Key ?? string.Empty).Replace('/', Path.DirectorySeparatorChar); newFileEntry = new FileEntry(name, stream, fileEntry, false, entry.CreatedTime, entry.LastModifiedTime, entry.LastAccessedTime, memoryStreamCutoff: options.MemoryStreamCutoff); } diff --git a/RecursiveExtractor/Extractors/SevenZipExtractor.cs b/RecursiveExtractor/Extractors/SevenZipExtractor.cs index 869da19d..809b5d71 100644 --- a/RecursiveExtractor/Extractors/SevenZipExtractor.cs +++ b/RecursiveExtractor/Extractors/SevenZipExtractor.cs @@ -42,7 +42,14 @@ public async IAsyncEnumerable ExtractAsync(FileEntry fileEntry, Extra { governor.CheckResourceGovernor(entry.Size); var name = (entry.Key ?? string.Empty).Replace('/', Path.DirectorySeparatorChar); - var newFileEntry = await FileEntry.FromStreamAsync(name, entry.OpenEntryStream(), fileEntry, entry.CreatedTime, entry.LastModifiedTime, entry.LastAccessedTime, memoryStreamCutoff: options.MemoryStreamCutoff).ConfigureAwait(false); + FileEntry newFileEntry; + // Dispose the entry stream before yielding. SharpCompress keeps a decoder and + // its dictionary buffer alive per open entry stream, and FileEntry has already + // taken its own copy of the content by this point. + using (var entryStream = entry.OpenEntryStream()) + { + newFileEntry = await FileEntry.FromStreamAsync(name, entryStream, fileEntry, entry.CreatedTime, entry.LastModifiedTime, entry.LastAccessedTime, memoryStreamCutoff: options.MemoryStreamCutoff).ConfigureAwait(false); + } if (newFileEntry != null) { @@ -164,7 +171,14 @@ public IEnumerable Extract(FileEntry fileEntry, ExtractorOptions opti { governor.CheckResourceGovernor(entry.Size); var name = (entry.Key ?? string.Empty).Replace('/', Path.DirectorySeparatorChar); - var newFileEntry = new FileEntry(name, entry.OpenEntryStream(), fileEntry, createTime: entry.CreatedTime, modifyTime: entry.LastModifiedTime, accessTime: entry.LastAccessedTime, memoryStreamCutoff: options.MemoryStreamCutoff); + FileEntry newFileEntry; + // Dispose the entry stream before yielding. SharpCompress keeps a decoder and + // its dictionary buffer alive per open entry stream, and FileEntry has already + // taken its own copy of the content by this point. + using (var entryStream = entry.OpenEntryStream()) + { + newFileEntry = new FileEntry(name, entryStream, fileEntry, createTime: entry.CreatedTime, modifyTime: entry.LastModifiedTime, accessTime: entry.LastAccessedTime, memoryStreamCutoff: options.MemoryStreamCutoff); + } try {