Cover the DEFLATE path and stop malformed zips raising #5

2026-09-10T23:28:47+10:00

unkin-agent commented

2026-09-10 23:28:47 +10:00

The suite never drove a byte through the vendored inflate, leaving the riskiest part of the extractor untested, and several malformed-archive paths raised a raw Lua error where every other failure returns nil, err.

Add a small DEFLATE encoder so compressed fixtures build in-process, without a zip binary
Cover zip and tar.gz round-trips for both huffman block types
Assert CRC, size, truncation, corruption and traversal failures write nothing
Bound the zip central directory and local headers against the buffer
Return nil, err for a directory colliding with a file and for a zero-length pax record
Fail any test that overruns its wall-clock budget instead of hanging the run

The suite never drove a byte through the vendored inflate, leaving the riskiest part of the extractor untested, and several malformed-archive paths raised a raw Lua error where every other failure returns `nil, err`. - Add a small DEFLATE encoder so compressed fixtures build in-process, without a `zip` binary - Cover zip and tar.gz round-trips for both huffman block types - Assert CRC, size, truncation, corruption and traversal failures write nothing - Bound the zip central directory and local headers against the buffer - Return `nil, err` for a directory colliding with a file and for a zero-length pax record - Fail any test that overruns its wall-clock budget instead of hanging the run

unkin-agent added 3 commits 2026-09-10 23:28:47 +10:00

Report malformed zip headers instead of raising 407ad8093f

A central directory record or local header cut short of its length fields
made zip_entries and zip_data do arithmetic on nil, so the parse raised
where every other failure returns nil, err. Bound both against the buffer
first. A pax record claiming a zero length no longer spins forever.

Test the DEFLATE path with real compressed fixtures 78a5df6cf3

The fixture builder only produced stored entries, so the vendored inflate
never ran under test. Add a small DEFLATE encoder and drive zip and
tar.gz entries through it: round-trips for fixed and dynamic huffman,
empty and large payloads, plus wrong CRC, wrong stated size, truncated
and corrupted streams, and a traversal name on a compressed entry.

Also cover the central directory and local header truncations that used
to raise.

Aim the truncated-directory test at the raising path

ci/woodpecker/pr/lint Pipeline was successful

Details

ci/woodpecker/pr/test Pipeline was successful

Details

ci/woodpecker/pr/pre-commit Pipeline was successful

Details

11c8fbf485

A record whose name is cut short already reported; the raise needed a
zero-length name and a directory offset pointing into the trailing
comment. Build that case instead.

unkin-agent added 1 commit 2026-09-10 23:31:05 +10:00

Report a directory that collides with a file

ci/woodpecker/pr/lint Pipeline was successful

Details

ci/woodpecker/pr/pre-commit Pipeline was successful

Details

ci/woodpecker/pr/test Pipeline was successful

Details

27b182f1c3

vim.fn.mkdir raises when a path component is an existing file, and an
archive can arrange that by shipping a file and then a path under it.
Return nil, err from mkdirp and propagate it.

unkin-agent commented

2026-09-10 23:47:02 +10:00

Reviewed the diff, ran make test/lint/fmt-check/pre-commit (all green, 146/146 locally with tar/zip/unzip/gzip present, 129/146 with them stripped from PATH — matches the body's claim, and the 17 gap is entirely pre-existing installer/init cases untouched by this PR). Verified tests/deflate.lua independently: decompressed its fixed- and dynamic-Huffman, empty, 400KB, and overlapping-copy streams with Python's zlib.decompressobj(-15) and gzip — all byte-exact and standards-compliant. Mutation-tested the hardening: reverting each of the four nil,err conversions (CD/local-header bounds, mkdir-collision, size check) makes the corresponding new regression test fail with the original raise/mismatch, as expected. Scope matches the body (tests + archive.lua error paths only, no vendored-library edits).

Two things to fix before merge:

1. tests/archive_spec.lua:640 ("stops a pax record that claims a zero length") — the regression test hangs indefinitely rather than failing.
Confirmed by removing the guard at lua/arti-pack/archive.lua:286-288 and running just make test: the nvim --headless process pins at 100% CPU and never returns (killed it manually after 2+ minutes). tests/harness.lua's M.it is a bare pcall with no time bound, and none of the .woodpecker/*.yaml steps set a timeout:. A future regression of this guard won't show up as a failing test — it'll silently stall the whole test step (blocking every other test's result along with it) until Woodpecker's global job timeout eventually kills it. Please bound this specific test (e.g. debug.sethook with an instruction-count limit to force a controlled error if the loop runs away, or drive pax_records directly with an iteration cap) so a regression here fails fast instead of hanging CI.

2. PR description violates the repo's PR-body convention.
Current body is 12 lines / 1636 characters; the cap is 10 lines and 800 characters. It also closes with a verification-narrative paragraph ("The suite still passes with zip, tar and unzip off PATH: 129 of 146 run there...") which the convention explicitly excludes. Please trim to one short why-paragraph plus present-tense bullets, and drop the trailing pass/skip-count paragraph (that belongs in the PR conversation, not the description).

Everything else — the DEFLATE encoder, the error-path hardening, and archive_root/extraction behavior for well-formed archives (unchanged, not touched by this diff) — looks correct.

Reviewed the diff, ran `make test`/`lint`/`fmt-check`/`pre-commit` (all green, 146/146 locally with tar/zip/unzip/gzip present, 129/146 with them stripped from PATH — matches the body's claim, and the 17 gap is entirely pre-existing installer/init cases untouched by this PR). Verified `tests/deflate.lua` independently: decompressed its fixed- and dynamic-Huffman, empty, 400KB, and overlapping-copy streams with Python's `zlib.decompressobj(-15)` and `gzip` — all byte-exact and standards-compliant. Mutation-tested the hardening: reverting each of the four `nil,err` conversions (CD/local-header bounds, mkdir-collision, size check) makes the corresponding new regression test fail with the original raise/mismatch, as expected. Scope matches the body (tests + archive.lua error paths only, no vendored-library edits). Two things to fix before merge: **1. `tests/archive_spec.lua:640` ("stops a pax record that claims a zero length") — the regression test hangs indefinitely rather than failing.** Confirmed by removing the guard at `lua/arti-pack/archive.lua:286-288` and running just `make test`: the `nvim --headless` process pins at 100% CPU and never returns (killed it manually after 2+ minutes). `tests/harness.lua`'s `M.it` is a bare `pcall` with no time bound, and none of the `.woodpecker/*.yaml` steps set a `timeout:`. A future regression of this guard won't show up as a failing test — it'll silently stall the whole `test` step (blocking every other test's result along with it) until Woodpecker's global job timeout eventually kills it. Please bound this specific test (e.g. `debug.sethook` with an instruction-count limit to force a controlled error if the loop runs away, or drive `pax_records` directly with an iteration cap) so a regression here fails fast instead of hanging CI. **2. PR description violates the repo's PR-body convention.** Current body is 12 lines / 1636 characters; the cap is 10 lines and 800 characters. It also closes with a verification-narrative paragraph ("The suite still passes with `zip`, `tar` and `unzip` off PATH: 129 of 146 run there...") which the convention explicitly excludes. Please trim to one short why-paragraph plus present-tense bullets, and drop the trailing pass/skip-count paragraph (that belongs in the PR conversation, not the description). Everything else — the DEFLATE encoder, the error-path hardening, and `archive_root`/extraction behavior for well-formed archives (unchanged, not touched by this diff) — looks correct.

unkin-agent added 1 commit 2026-09-11 00:00:23 +10:00

Bound every test with a wall-clock timeout

ci/woodpecker/pr/lint Pipeline was successful

Details

ci/woodpecker/pr/test Pipeline was successful

Details

ci/woodpecker/pr/pre-commit Pipeline was successful

Details

15383b8cce

A runaway test spins at full CPU instead of failing, stalling the whole
run. A debug hook enforces a per-test deadline; the JIT stays off so the
hook is reachable from compiled code.

benvin merged commit 3aa9a617b4 into main

2026-09-11 22:31:01 +10:00

benvin deleted branch benvin/deflate-tests

2026-09-11 22:31:01 +10:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: unkin/arti-pack#5