You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
The zip and 7z backends read each file entirely into a Vec<u8> before handing
it to the writer, and read each entry entirely into a Vec<u8> before writing
it to disk. Peak memory therefore tracks the size of the largest file in the
tree, not the size of a buffer. tar does neither: it streams through append_file and unpack_in.
Measured with the release binary at v0.7.0 on macOS, one 200 MB file
(/usr/bin/time -l, maximum resident set size), level 1:
Operation
tar
zip
7z
compress a 200 MB file
2.3 MB
211.6 MB
220.9 MB
extract it again
2.4 MB
214.1 MB
274.3 MB
The archives were 208.7 MB (tar), 4.2 MB (zip) and 31 KB (7z), so the resident
memory is the input buffer, not the compressed output: the writers do stream
their output to the file. Only the reading side buffers.
Why it matters
Compressing a folder is the everyday case, and it is bounded by the largest
file in it rather than the total, because the directory backends loop over the
entries walk_tree collected and read one file at a time. So a photo library is
fine and a folder
holding one 8 GB disk image is not: that run needs 8 GB of RAM to produce an
archive, on a machine that may not have it. The desktop app is where this hurts
most, since it is a GUI a user is watching, and the failure is an allocation
error surfaced as a string.
On the server it multiplies. The request body is buffered as Bytes before it
is staged, and then the compression buffers the file again, so a single upload
peaks at roughly twice its size, and concurrent uploads add up even though the
worker compresses one at a time. docker-compose.yml:62 sets mem_limit: 2g
with a comment saying exactly this, and --max-upload-mb defaults to 500, which
is the real ceiling holding the arrangement together.
This is a scalability limit rather than a defect: it fails loudly, does not
corrupt anything, and most people will never notice.
Where it is
Compression, single file. apps/core/src/compression/zip.rs:32
Compression, per tree entry: apps/core/src/compression/zip.rs:74
(let bytes = fs::read(&entry.disk_path)?;) and apps/core/src/compression/sevenz.rs:72 (let content = fs::read(&entry.disk_path)?;).
Extraction, per entry. apps/core/src/compression/zip.rs:118
tar, for contrast: apps/core/src/compression/tar.rs appends with append_file and unpacks with unpack_in, both streaming, which is why the
column above reads 2.3 MB.
Why it is like that
It is inherited from the reference implementation and was never revisited; CLAUDE.md records it as "a known limitation inherited from the reference", and docs/threat_model.md lists it twice, under "Resource exhaustion" and in the
decompression-bomb bullet. The buffered form is also simply the shortest code
that works, and it is what the two writer APIs make most obvious: SevenZWriter::push_archive_entry takes an Option<impl Read>, which reads as
"give me the content".
Nothing depends on the buffering. There is no correctness argument for it.
#7 (decompression bombs) already owns the extraction half. Its acceptance
criteria say so explicitly: "Limits are enforced on bytes actually read, via
streaming (no whole-entry buffering of untrusted input)", and its implementation
notes call streaming extraction "the main piece of work". Do not fix extraction
here and there.
What #7 does not cover, and this issue is for, is the compression side: compress_zip, compress_zip_dir, compress_7z, compress_7z_dir. Those read
input the user owns, so there is no bomb and no security question, only peak
memory on a large local file. It is the half that affects the desktop app and
the CLI on their own machines, with no server and no untrusted input involved.
If both are done at once, the extraction work should still be tracked under #7
so its acceptance criteria stay honest.
What a fix looks like
zip, both directions: straightforward.ZipWriter implements Write, so
compression becomes std::io::copy(&mut source_file, &mut writer)?. Extraction
becomes std::io::copy(&mut entry, &mut File::create(&dest)?)? (the entry
implements Read). Both are smaller than what is there now. Note that compress_zip creates the output before opening the source, a separate known
sharp edge that leaves a zero byte .zip behind when the source is missing;
this is a good moment to reverse the order.
7z compression: probably as easy, worth checking.push_archive_entry
accepts any impl Read, so passing the File instead of a slice may be
enough. Whether sevenz-rust2 then streams internally or buffers anyway needs
measuring rather than assuming: solid-block compression has a legitimate reason
to hold data. The 220.9 MB measured above is the ceiling to beat, and it should
fall to roughly the LZMA2 dictionary size for the preset.
7z extraction:std::io::copy(reader, &mut File::create(&dest)?) inside the decompress_with_extract_fn closure. Keep sanitize_entry_path running before
the file is created, which it already does, and keep using decompress_with_extract_fn rather than decompress (the plain one writes
first and lets .. escape).
The server's upload buffering is a separate change in the same family: routes::compress_create takes the whole body as Bytes. Streaming it to <job>/input/upload as it arrives would halve the server's peak, and it
interacts with DefaultBodyLimit, so it wants its own issue rather than being
smuggled into this one.
How to prove it. A unit test cannot observe resident memory portably. The
options are: assert the archive still round-trips (necessary but not
sufficient), or measure out of band the way the table above was produced and
record the numbers in the pull request. A test that compresses a file larger
than a chosen ceiling would need a large temporary file and would be slow and
flaky in CI; the honest answer is to measure once, by hand, and say so.
How to know it is fixed
No read_to_end, fs::read or Vec<u8> of file content remains in apps/core/src/compression/zip.rs or apps/core/src/compression/sevenz.rs
for the compression paths.
apps/core/tests/zip.rs and apps/core/tests/sevenz.rs still pass unchanged:
the archives are byte-equivalent in content, and the round-trip tests are what
prove the rewrite is faithful.
A measurement in the pull request showing peak resident memory for a large
single file, compared against the table above.
docker-compose.yml's mem_limit comment and docs/server.md's
"Uploads and downloads are buffered whole in memory" are revisited once the
server side is done.
docs/threat_model.md, "Known limitations": resource exhaustion, and the
decompression-bomb bullet, both mention this buffering.
docs/server.md, "Known limitations": uploads and downloads buffered whole in
memory.
docker-compose.yml:62 and :140 (mem_limit: 2g) exist because of it.
The extraction endpoint issue in this batch depends on both halves: accepting
arbitrary archives from a browser is what turns this from a scalability limit
into an exposure.
What happens
The zip and 7z backends read each file entirely into a
Vec<u8>before handingit to the writer, and read each entry entirely into a
Vec<u8>before writingit to disk. Peak memory therefore tracks the size of the largest file in the
tree, not the size of a buffer. tar does neither: it streams through
append_fileandunpack_in.Measured with the release binary at v0.7.0 on macOS, one 200 MB file
(
/usr/bin/time -l, maximum resident set size), level 1:The archives were 208.7 MB (tar), 4.2 MB (zip) and 31 KB (7z), so the resident
memory is the input buffer, not the compressed output: the writers do stream
their output to the file. Only the reading side buffers.
Why it matters
Compressing a folder is the everyday case, and it is bounded by the largest
file in it rather than the total, because the directory backends loop over the
entries
walk_treecollected and read one file at a time. So a photo library isfine and a folder
holding one 8 GB disk image is not: that run needs 8 GB of RAM to produce an
archive, on a machine that may not have it. The desktop app is where this hurts
most, since it is a GUI a user is watching, and the failure is an allocation
error surfaced as a string.
On the server it multiplies. The request body is buffered as
Bytesbefore itis staged, and then the compression buffers the file again, so a single upload
peaks at roughly twice its size, and concurrent uploads add up even though the
worker compresses one at a time.
docker-compose.yml:62setsmem_limit: 2gwith a comment saying exactly this, and
--max-upload-mbdefaults to 500, whichis the real ceiling holding the arrangement together.
This is a scalability limit rather than a defect: it fails loudly, does not
corrupt anything, and most people will never notice.
Where it is
Compression, single file.
apps/core/src/compression/zip.rs:32apps/core/src/compression/sevenz.rs:20Compression, per tree entry:
apps/core/src/compression/zip.rs:74(
let bytes = fs::read(&entry.disk_path)?;) andapps/core/src/compression/sevenz.rs:72(let content = fs::read(&entry.disk_path)?;).Extraction, per entry.
apps/core/src/compression/zip.rs:118apps/core/src/compression/sevenz.rs:109tar, for contrast:
apps/core/src/compression/tar.rsappends withappend_fileand unpacks withunpack_in, both streaming, which is why thecolumn above reads 2.3 MB.
Why it is like that
It is inherited from the reference implementation and was never revisited;
CLAUDE.mdrecords it as "a known limitation inherited from the reference", anddocs/threat_model.mdlists it twice, under "Resource exhaustion" and in thedecompression-bomb bullet. The buffered form is also simply the shortest code
that works, and it is what the two writer APIs make most obvious:
SevenZWriter::push_archive_entrytakes anOption<impl Read>, which reads as"give me the content".
Nothing depends on the buffering. There is no correctness argument for it.
What this covers that #7 does not
#7 (decompression bombs) already owns the extraction half. Its acceptance
criteria say so explicitly: "Limits are enforced on bytes actually read, via
streaming (no whole-entry buffering of untrusted input)", and its implementation
notes call streaming extraction "the main piece of work". Do not fix extraction
here and there.
What #7 does not cover, and this issue is for, is the compression side:
compress_zip,compress_zip_dir,compress_7z,compress_7z_dir. Those readinput the user owns, so there is no bomb and no security question, only peak
memory on a large local file. It is the half that affects the desktop app and
the CLI on their own machines, with no server and no untrusted input involved.
If both are done at once, the extraction work should still be tracked under #7
so its acceptance criteria stay honest.
What a fix looks like
zip, both directions: straightforward.
ZipWriterimplementsWrite, socompression becomes
std::io::copy(&mut source_file, &mut writer)?. Extractionbecomes
std::io::copy(&mut entry, &mut File::create(&dest)?)?(the entryimplements
Read). Both are smaller than what is there now. Note thatcompress_zipcreates the output before opening the source, a separate knownsharp edge that leaves a zero byte
.zipbehind when the source is missing;this is a good moment to reverse the order.
7z compression: probably as easy, worth checking.
push_archive_entryaccepts any
impl Read, so passing theFileinstead of a slice may beenough. Whether
sevenz-rust2then streams internally or buffers anyway needsmeasuring rather than assuming: solid-block compression has a legitimate reason
to hold data. The 220.9 MB measured above is the ceiling to beat, and it should
fall to roughly the LZMA2 dictionary size for the preset.
7z extraction:
std::io::copy(reader, &mut File::create(&dest)?)inside thedecompress_with_extract_fnclosure. Keepsanitize_entry_pathrunning beforethe file is created, which it already does, and keep using
decompress_with_extract_fnrather thandecompress(the plain one writesfirst and lets
..escape).The server's upload buffering is a separate change in the same family:
routes::compress_createtakes the whole body asBytes. Streaming it to<job>/input/uploadas it arrives would halve the server's peak, and itinteracts with
DefaultBodyLimit, so it wants its own issue rather than beingsmuggled into this one.
How to prove it. A unit test cannot observe resident memory portably. The
options are: assert the archive still round-trips (necessary but not
sufficient), or measure out of band the way the table above was produced and
record the numbers in the pull request. A test that compresses a file larger
than a chosen ceiling would need a large temporary file and would be slow and
flaky in CI; the honest answer is to measure once, by hand, and say so.
How to know it is fixed
read_to_end,fs::readorVec<u8>of file content remains inapps/core/src/compression/zip.rsorapps/core/src/compression/sevenz.rsfor the compression paths.
apps/core/tests/zip.rsandapps/core/tests/sevenz.rsstill pass unchanged:the archives are byte-equivalent in content, and the round-trip tests are what
prove the rewrite is faithful.
single file, compared against the table above.
docs/threat_model.md's "Resource exhaustion" bullet is narrowed to what isstill true (extraction, until core: guard extraction against decompression bombs (size/ratio/count limits) #7 lands).
docker-compose.yml'smem_limitcomment anddocs/server.md's"Uploads and downloads are buffered whole in memory" are revisited once the
server side is done.
Related
is the compression half only.
docs/threat_model.md, "Known limitations": resource exhaustion, and thedecompression-bomb bullet, both mention this buffering.
docs/server.md, "Known limitations": uploads and downloads buffered whole inmemory.
docker-compose.yml:62and:140(mem_limit: 2g) exist because of it.arbitrary archives from a browser is what turns this from a scalability limit
into an exposure.