-
Notifications
You must be signed in to change notification settings - Fork 0
Fix adapter installer OOM: stream zip installs to disk instead of buffering in memory #129
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -43,25 +43,50 @@ public async Task<InstalledAdapter> InstallAsync(string adapterId) | |
| if (!Directory.Exists(directory)) | ||
| { | ||
| Directory.CreateDirectory(directory); | ||
|
|
||
| // ZipArchive needs a SEEKABLE stream to read the central directory. The | ||
| // cloud-storage read stream is not seekable, so handing it directly to | ||
| // ZipArchive silently makes .NET buffer the entire archive into one | ||
| // in-memory MemoryStream first - for a large adapter (DevExpress-sized, | ||
| // several hundred MB uncompressed) that one-time spike is big enough to | ||
| // OOM the whole host process on a memory-constrained container, well | ||
| // before the child adapter process itself even starts. Downloading to a | ||
| // temp FILE first keeps memory use to one bounded copy-buffer regardless | ||
| // of archive size, and a FileStream is seekable so ZipArchive reads | ||
| // straight off disk. | ||
| var tempZipPath = Path.Combine(Path.GetTempPath(), $"{Guid.NewGuid():N}.zip"); | ||
| try | ||
| { | ||
| using var stream = await cloudFilesService.OpenReadAsync( | ||
| $"{options.AdapterRemotePath}/{adapterId}".ToLower()); | ||
| using var archive = new ZipArchive(stream); | ||
| using (var remoteStream = await cloudFilesService.OpenReadAsync( | ||
| $"{options.AdapterRemotePath}/{adapterId}".ToLower())) | ||
| using (var tempFileStream = new FileStream(tempZipPath, FileMode.Create, | ||
| FileAccess.Write, FileShare.None)) | ||
| { | ||
| await remoteStream.CopyToAsync(tempFileStream); | ||
| } | ||
|
|
||
| foreach (var entry in archive.Entries) | ||
| using (var archiveStream = new FileStream(tempZipPath, FileMode.Open, | ||
| FileAccess.Read, FileShare.Read)) | ||
| using (var archive = new ZipArchive(archiveStream, ZipArchiveMode.Read)) | ||
| { | ||
| if (string.IsNullOrEmpty(entry.Name)) continue; | ||
| var path = $"{directory}/{entry.FullName.Replace("\\", "/")}"; | ||
| Directory.CreateDirectory(Path.GetDirectoryName(path)); | ||
| entry.ExtractToFile(path, overwrite: true); | ||
| foreach (var entry in archive.Entries) | ||
| { | ||
| if (string.IsNullOrEmpty(entry.Name)) continue; | ||
| var path = $"{directory}/{entry.FullName.Replace("\\", "/")}"; | ||
| Directory.CreateDirectory(Path.GetDirectoryName(path)); | ||
| entry.ExtractToFile(path, overwrite: true); | ||
|
Comment on lines
+75
to
+77
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. π Security & Privacy | π‘οΈ Analyzed with Security Review | π Major | β‘ Quick win π§© Analysis chainπ Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- AdapterInstaller.cs ---'
cat -n SW.Serverless/Services/AdapterInstaller.cs | sed -n '1,125p'
printf '%s\n' '--- related installer contracts/usages ---'
rg -n --glob '*.cs' 'AdapterInstaller|InstallAsync\(|IResidentAdapterLocator|ExtractToFile|entry\.FullName' SW.Serverless | head -120Repository: simplify9/SW-Serverless Length of output: 8867 π€ get_repo_knowledge executed:
Length of output: 5122 Path Traversal (CWE-22): Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal') Reject archive paths that escape the extraction root.
Resolve each entry against π€ Prompt for AI Agents |
||
| } | ||
| } | ||
| } | ||
| catch (Exception) | ||
| { | ||
| Directory.Delete(directory, true); | ||
| throw; | ||
| } | ||
| finally | ||
| { | ||
| try { File.Delete(tempZipPath); } catch { /* best-effort cleanup */ } | ||
| } | ||
| } | ||
| } | ||
| finally | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
π©Ί Stability & Availability | π‘ Minor | β‘ Quick win
π Supported by static analysis
π€ get_repo_knowledge executed:
get_repo_knowledge simplify9/SW-Serverless /tmp/coderabbit-repo-knowledge/simplify9-sw-serverless-d259e1ab/conventionsLength of output: 3505
π Script executed:
Repository: simplify9/SW-Serverless
Length of output: 9473
π Script executed:
Repository: simplify9/SW-Serverless
Length of output: 17603
Bound adapter storage usage.
remoteStream.CopyToAsync(tempFileStream)copies until EOF without a byte limit. The installer then extracts the archive without an extraction-size budget. A large archive or expansion can exhaust local storage and fail installation. Enforce configured download and extraction storage limits.π€ Prompt for AI Agents