Skip to content

core: zip and 7z hold whole files in memory, on compression as well as extraction #76

Description

@otsobide

What happens

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

    let mut source_file = File::open(source)?;
    let mut buffer = Vec::new();
    source_file.read_to_end(&mut buffer)?;
    writer.write_all(&buffer)?;

apps/core/src/compression/sevenz.rs:20

    let content = fs::read(source)?;

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

            let mut buf = Vec::new();
            entry.read_to_end(&mut buf)?;
            fs::write(&dest, &buf)?;

apps/core/src/compression/sevenz.rs:109

            let mut buf = Vec::new();
            reader
                .read_to_end(&mut buf)
                .map_err(sevenz_rust2::Error::io)?;
            fs::write(&dest, &buf).map_err(sevenz_rust2::Error::io)?;

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.

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 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.
  • docs/threat_model.md's "Resource exhaustion" bullet is narrowed to what is
    still true (extraction, until core: guard extraction against decompression bombs (size/ratio/count limits) #7 lands).
  • 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.

Related

  • core: guard extraction against decompression bombs (size/ratio/count limits) #7 owns the extraction half and the size/ratio/entry-count caps; this issue
    is the compression half only.
  • 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.

Metadata

Metadata

Assignees

No one assigned

    Labels

    enhancementNew feature or request

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions