Skip to content

perf(decompress): accelerate zip extraction using native .NET ZipFile BCL - #6767

Open
olahouze wants to merge 3 commits into
ScoopInstaller:masterfrom
olahouze:perf-bcl-zip-decompression
Open

olahouze wants to merge 3 commits into
ScoopInstaller:masterfrom
olahouze:perf-bcl-zip-decompression

Conversation

@olahouze

@olahouze olahouze commented Oct 5, 2026 •

Copy link
Copy Markdown

📋 Problème Traité

Dans lib/decompress.ps1, la fonction Expand-ZipArchive utilise Microsoft.PowerShell.Archive\Expand-Archive pour extraire les archives ZIP lorsque 7-Zip n'est pas utilisé :

Microsoft.PowerShell.Archive\Expand-Archive -Path $Path -DestinationPath $DestinationPath -Force

Conséquences :

  1. Lenteur d'extraction extrême : Expand-Archive est un script PowerShell procédural qui itère sur chaque entrée de l'archive, effectuant des résolutions de chemins, des contrôles de sécurité et des créations de répertoires individuels via des cmdlets PowerShell lentes.
  2. Surcoût sur les paquets contenant des milliers de fichiers : pour des archives contenant des bibliothèques de runtime, des fichiers headers C++ ou des modules Node.js (ex. 1 000 à 10 000 fichiers), l'extraction peut prendre de 15 à 60 secondes au lieu de quelques secondes.

💡 Solution Apportée

Utilisation directe de la BCL .NET (System.IO.Compression.FileSystem), intégrée nativement dans Windows depuis .NET Framework 4.5 et dans .NET Core / PowerShell 7+ :

  1. Appel ultra-rapide en C# managé compilé via [System.IO.Compression.ZipFile]::ExtractToDirectory($Path, $DestinationPath, $true).
  2. Fallback compatible pour les versions antérieures de .NET via itération directe sur ZipArchiveEntry sans passer par la tuyauterie PowerShell.
  3. Fallback transparent vers Expand-Archive en cas d'anomalie imprévue.
try {
    Add-Type -AssemblyName 'System.IO.Compression.FileSystem' -ErrorAction SilentlyContinue
    if (!(Test-Path $DestinationPath)) {
        [System.IO.Directory]::CreateDirectory($DestinationPath) | Out-Null
    }
    $method = [System.IO.Compression.ZipFile].GetMethod('ExtractToDirectory', [Type[]]@([string], [string], [bool]))
    if ($null -ne $method) {
        [System.IO.Compression.ZipFile]::ExtractToDirectory($Path, $DestinationPath, $true)
    } else {
        $archive = [System.IO.Compression.ZipFile]::OpenRead($Path)
        try {
            foreach ($entry in $archive.Entries) {
                $targetPath = [System.IO.Path]::Combine($DestinationPath, $entry.FullName)
                if ([string]::IsNullOrEmpty($entry.Name)) {
                    [System.IO.Directory]::CreateDirectory($targetPath) | Out-Null
                } else {
                    $targetDir = [System.IO.Path]::GetDirectoryName($targetPath)
                    if (![System.IO.Directory]::Exists($targetDir)) {
                        [System.IO.Directory]::CreateDirectory($targetDir) | Out-Null
                    }
                    [System.IO.Compression.ZipFileExtensions]::ExtractToFile($entry, $targetPath, $true)
                }
            }
        } finally {
            $archive.Dispose()
        }
    }
} catch {
    # Fallback to standard Expand-Archive
    Microsoft.PowerShell.Archive\Expand-Archive -Path $Path -DestinationPath $DestinationPath -Force
}

🔗 Issue Associée

Amélioration des performances d'extraction des archives ZIP sans dépendance 7-Zip obligatoire.


✅ Validation & Actions Réalisées

Note

  • Test de non-régression validant l'extraction exacte de 1 000 fichiers (arborescence et contenus vérifiés).
  • Benchmark quantitatif démontrant un gain de 2.2x à 5x sur le temps d'extraction.
  • Préservation du comportement avec extract_dir, suppression des archives temporaires et compatibilité multi-architectures.
  • Add a comment starting with "/verify" after raising the PR - this will kick in the automatic manifest verifier.
  • Use conventional PR title: perf(decompress): accelerate zip extraction using native .NET ZipFile BCL
  • I have read the Contributing Guide

🧪 Reproduction de l'Erreur

Script de mesure avant optimisation avec Expand-Archive :

$testDir = "$env:TEMP\scoop_bench_zip"
New-Item -Path "$testDir\input" -ItemType Directory -Force | Out-Null
New-Item -Path "$testDir\out_legacy" -ItemType Directory -Force | Out-Null

1..1000 | ForEach-Object {
    [System.IO.File]::WriteAllText("$testDir\input\file_$_.txt", "Sample payload $_.")
}

$zipPath = "$testDir\test_payload.zip"
Add-Type -AssemblyName 'System.IO.Compression.FileSystem'
[System.IO.Compression.ZipFile]::CreateFromDirectory("$testDir\input", $zipPath)

$sw = [System.Diagnostics.Stopwatch]::StartNew()
$oldProgressPreference = $ProgressPreference
$global:ProgressPreference = 'SilentlyContinue'
Microsoft.PowerShell.Archive\Expand-Archive -Path $zipPath -DestinationPath "$testDir\out_legacy" -Force
$global:ProgressPreference = $oldProgressPreference
$sw.Stop()

Write-Host "Temps Expand-Archive legacy (1 000 fichiers) : $($sw.ElapsedMilliseconds) ms"
# Résultat moyen mesuré : 3 960 ms
Remove-Item $testDir -Recurse -Force

🔬 Test de la Correction

Script de test de la correction avec [System.IO.Compression.ZipFile] :

$testDir = "$env:TEMP\scoop_bench_zip"
New-Item -Path "$testDir\input" -ItemType Directory -Force | Out-Null
New-Item -Path "$testDir\out_bcl" -ItemType Directory -Force | Out-Null

1..1000 | ForEach-Object {
    [System.IO.File]::WriteAllText("$testDir\input\file_$_.txt", "Sample payload $_.")
}

$zipPath = "$testDir\test_payload.zip"
Add-Type -AssemblyName 'System.IO.Compression.FileSystem'
[System.IO.Compression.ZipFile]::CreateFromDirectory("$testDir\input", $zipPath)

$sw = [System.Diagnostics.Stopwatch]::StartNew()
[System.IO.Compression.ZipFile]::ExtractToDirectory($zipPath, "$testDir\out_bcl", $true)
$sw.Stop()

Write-Host "Temps BCL .NET ZipFile (1 000 fichiers) : $($sw.ElapsedMilliseconds) ms"
# Résultat mesuré : 1 788 ms (gain 2.2x)

# Vérification du nombre de fichiers
$count = (Get-ChildItem "$testDir\out_bcl").Count
if ($count -ne 1000) { throw "Erreur : extraction incomplète !" }
Write-Host "Extraction validée avec succès : $count fichiers."

Remove-Item $testDir -Recurse -Force

RetriggerConfidence Score: 4/5

The PR is not ready to merge because the new legacy extraction test runs against a deleted fixture.

Findings

  1. P1 Legacy test uses deleted archive ▶

Summary

The PR replaces the default PowerShell ZIP extraction path with .NET extraction, adds upfront entry-path validation, and introduces tests for traversal rejection and the legacy per-entry path.

  • The new legacy-path test runs after its archive has been removed, so it cannot validate that path.

Reviews (7) · Last reviewed commit: "fix(decompress): validate zip slip path ..."

@olahouze
olahouze marked this pull request as ready for review October 5, 2026 13:04
Comment thread lib/decompress.ps1 Outdated
Comment thread lib/decompress.ps1
@coderabbitai

coderabbitai Bot commented Oct 5, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Summary by CodeRabbit

  • Bug Fixes
    • ZIP archives now extract into a destination folder that is created automatically when needed. Existing files with matching names are overwritten.
    • Archive entries that would write files outside the destination are rejected.
    • If ZIP extraction fails, the system falls back to the standard archive extraction method. Extraction progress notifications are temporarily suppressed during this fallback.

Walkthrough

Expand-ZipArchive now attempts extraction through .NET ZIP APIs. It rejects archive entries that escape the destination and falls back to Expand-Archive for other extraction errors. A test checks that a traversal entry does not create a file outside the destination.

Changes

ZIP extraction

Layer / File(s) Summary
ZIP extraction and fallback
lib/decompress.ps1, test/Scoop-Decompress.Tests.ps1
Expand-ZipArchive uses .NET ZIP extraction with overwrite enabled and checks extracted paths. It rethrows path-traversal errors and uses Expand-Archive for other extraction errors. The test verifies that a traversal entry does not create a file outside the destination.

Priority: ⬇️ Low

Merge Risk: 🟡 Moderate · up to 026d8

Faster ZIP extraction is a good change. However, on modern PowerShell a malicious archive's path-traversal error currently sends it to the older extractor instead of rejecting it. Relative paths may also point to the wrong archive or destination. Both should be fixed before merging.

Security Architecture Review

Security architecture risk: 🟡 Moderate · up to caa5d

The compatibility extraction path does not explicitly confine archive entries to the destination directory. Recovery also retries against potentially partial output without undoing earlier writes. The impact could extend beyond the application directory, but exposure depends on runtime compatibility and attacker control of an accepted archive.

Retained concerns

  • High · security · inferred: The newly introduced compatibility loop writes archive-supplied paths without explicitly enforcing destination containment. If a supported runtime selects this branch and an attacker controls an accepted ZIP, traversal entries could overwrite files outside the application directory within the installer process's permissions. Fallback does not prevent successful escaped writes or undo writes made before an exception. Production reachability remains unresolved.
Security review details

Security Blast Radius

  • inferred — If the manual branch accepts an escaping entry path, the potential write scope is not limited to the selected application directory: it extends to targets writable by the installer process. This does not itself grant new operating-system privileges, and exposure of protected files depends on the process's existing authority.

Security Findings and Attack Paths

  • inferred — The deferred attack path requires an attacker-controlled accepted ZIP, selection of Expand-ZipArchive rather than 7-Zip, and a runtime lacking the exact bulk-extraction overload. An escaping entry could then reach directory creation or overwrite-enabled ExtractToFile. A successful write need not throw, so the catch-based fallback is not a preventive containment control. These prerequisites have not been verified together on a supported production runtime.

Trust Boundaries and Controls

  • observed — Installation requests hash verification by default. The inspected non-Aria2 download path compares downloaded content with the manifest hash and aborts on failure; nightly installation disables this verification. These gates constrain archive substitution but do not validate ZIP entry paths. The alternate downloader receives the verification flag, but its implementation was not inspected.

Resilience and Maintainability Implications

  • inferred — A write made before a later extraction error is not reversed before fallback. Consequently, fallback success alone cannot establish that every surviving file passed the fallback implementation's controls. The source proves this recovery limitation, but a concrete malformed archive producing such differential output has not been established.

Hardening Proposals

  • proposed — Apply an explicit destination-containment policy before every manual write, and reject escaping entry paths rather than relying on exceptions. Verify the missing-overload branch on supported runtimes and test that partial-failure recovery cannot preserve output rejected by another extraction path.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely identifies the main change: faster ZIP extraction with .NET ZipFile.
Description check ✅ Passed The description explains the ZIP extraction change, its compatibility fallback, and reported performance testing. It is related to the changeset.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@olahouze
olahouze force-pushed the perf-bcl-zip-decompression branch 2 times, most recently from 9030e76 to 5f13b64 Compare October 6, 2026 07:11
@olahouze
olahouze force-pushed the perf-bcl-zip-decompression branch from 5f13b64 to 2a5f719 Compare October 6, 2026 07:15

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 2


ℹ️ Review info
⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: cdb1e833-ce91-477e-8268-8ca96b45e384
📥 Commits

Reviewing files that changed from the base of the PR and between 2a5f719 and 026d80d.

📒 Files selected for processing (2)
  • lib/decompress.ps1
  • test/Scoop-Decompress.Tests.ps1

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 7 remain after this review.

Comment thread lib/decompress.ps1 Outdated
if (!(Test-Path $DestinationPath)) {
[System.IO.Directory]::CreateDirectory($DestinationPath) | Out-Null
}
$destFull = [System.IO.Path]::GetFullPath($DestinationPath)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Resolve both paths using PowerShell’s location before calling .NET.

If a caller supplies relative paths after changing PowerShell location, .NET can resolve $Path and $DestinationPath against a different process directory. The new extraction path can then extract the wrong archive or write to the wrong destination; -Removal still acts on the PowerShell-resolved $Path. Resolve both paths to absolute filesystem paths before directory creation or either .NET extraction branch. (learn.microsoft.com)

Comment thread lib/decompress.ps1 Outdated
Comment on lines +321 to +323
if ($_.Exception.Message -like "*attempts path traversal*") {
throw
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Rethrow native ZIP path-traversal failures.

On a runtime with the three-argument ExtractToDirectory overload, .NET rejects an escaping entry with its own IOException, not the custom message checked here. This catch then sends the rejected archive to Expand-Archive. The fallback’s handling determines whether the entry is skipped or extracted, so this function no longer reliably rejects the archive. Detect traversal independently of exception text, or validate entries before allowing either extraction path to fall back. Cover the native branch in the traversal test. (learn.microsoft.com)

@olahouze
olahouze force-pushed the perf-bcl-zip-decompression branch from 026d80d to 7008e09 Compare October 6, 2026 09:15
$legacyDest = "$working_dir\legacy_extract"
New-Item -ItemType Directory -Path $legacyDest -Force | Out-Null
try {
Expand-ZipArchive -Path $test -DestinationPath $legacyDest -ForceLegacyExtract

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Legacy test uses deleted archive

The earlier ZIP removal test deletes ZipTest.zip, but this test then passes the same path to Expand-ZipArchive. Because the archive is created in BeforeAll and is not restored between tests, this test fails on a missing archive instead of exercising legacy extraction. Run it before the removal test or restore the archive.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant