Conversation
…es (#1776) Closes #1775. A file with holes on disk went out as a GNU sparse entry, typeflag `S`, decimal 83, because `tar::Builder::new` enables sparse mode by default. Podman has no case for that type, so it drops the whole archive on account of one file. The build endpoint says `unhandled tar header type 83`; the container archive endpoint that `cp` and the watch sync PUT to says `unrecognized Typeflag S`. ## What I changed The four places that constructed a `tar::Builder` now go through one constructor, `engine::tar_stream::builder`, which turns the flag off. Turning it off four times would have fixed today and left the fifth site free to be written the old way, so `tests/tar_builder_single_site.rs` keeps it the only one. Those four sites sat in three unrelated files and were wrong the same way precisely because each was written on its own. `follow_symlinks` is untouched at all three sites that set it, which matters: two of them set it to `false` on purpose so a symlink in the tree is stored as a link instead of packing the bytes of its target. ## Verification Every row below is a real run on this machine, podman 5.7.0, released `v5.9.2` against this branch's binary, same input, and in every case the control is the same bytes with the hole filled in (`cp --sparse=never`). Fixtures were checked for a real hole (`stat -c '%s %b'`) before each run, on tmpfs and again on ext4. | Path | Case | v5.9.2 | This branch | | --- | --- | --- | --- | | build | context holding an 8 MiB sparse file | `unhandled tar header type 83`, exit 1 | builds, exit 0 | | cp | single file | `unrecognized Typeflag S`, exit 1 | copies, exit 0 | | cp | directory holding a sparse file and a plain one | `unrecognized Typeflag S`, exit 1, and neither file arrives | both arrive | | watch | initial sync | warning only, exit 0, `/app` empty | `synced`, both files present | | watch | live event | warning only, exit 0, `/app` empty | `synced`, file present | The `watch` rows are the reason this is worth a gate. A failed sync is a `warn!` and the loop is meant to survive one, so the exit code stays 0, the session keeps saying it is watching, and the file just never arrives. Verified by `ls` and `sha256sum` inside the container rather than by exit code. No corruption from expanding the holes: every destination `sha256sum` equals the host's, and every byte count matches, on all five transfers. Two things the runs turned up that were not in the issue. The rejection is per archive, so a sparse file takes its ordinary neighbours down with it. And `cp` of a directory creates the destination before the stream is refused, leaving an empty directory that reads as success to anything checking only for the path. ## Cost Measured through the same `GzEncoder` the real paths use, on 1 GiB of holes with four bytes of data: 125 gzipped bytes become 1043015, and peak RSS moves from 2832 KiB to 2852. `io::copy` writes through a fixed stack buffer into the encoder, so the streaming build path still never holds the archive. About 1 MiB on the wire per GiB of holes, and the reading and gzipping of that GiB, which is the real cost. ## Tests - `our_builder_never_writes_a_gnu_sparse_entry` packs a real sparse file through the constructor and asserts no entry is `GNUSparse`. Reverting `.sparse(false)` makes it fail; I checked, rather than assuming. - `the_crate_default_emits_the_type_podman_refuses_when_the_hole_is_real` is the positive control. Without it the test above is vacuous wherever the crate never looks for holes, and would stay green with the fix reverted while reporting that it had checked something. The crate only looks on Android, FreeBSD and Linux, so the control is gated on exactly that list rather than on `cfg(unix)`: macOS is Unix and APFS really does leave the range unallocated, so a block-count check would have failed the control on the macOS runner for a reason unrelated to this fix. - `the_expanded_entry_carries_every_byte_the_file_had` reads the archive back and checks the hole comes out as zeroes with the tail intact. - `tests/tar_builder_single_site.rs` refuses a second construction site. It rejects both spellings: the qualified `tar::Builder::new(`, and an import of `tar::Builder` that would let a bare `Builder::new(` slip past a literal search. I sabotaged it with each spelling and watched it go red for each. Full suite green: 2671 tests, 0 failures, plus `cargo fmt --check` and `cargo clippy --all-features --all-targets` with nothing emitted. ## Scope podup only. The five other Rust repositories do not carry the `tar` crate even transitively, and there is no `.sparse(` call anywhere else in the tree. --------- Signed-off-by: Jaro-c <75870284+Jaro-c@users.noreply.github.com>
One fix landed since 5.9.2 and no published binary carries it, so it goes out as a patch. #1776 stopped podup writing GNU sparse tar entries, which Podman refuses. A file with holes anywhere in a build context, a `cp` payload or a watched tree took the whole transfer down with it: `unhandled tar header type 83` on a build, `unrecognized Typeflag S` on a `cp`, and on a watch sync a warning and silent non-delivery. Every release from 0.3.0 onwards carried it on Linux, Android and FreeBSD hosts. All three files that stamp a version move together, `Cargo.toml`, `debian/changelog` and `Cargo.lock`, because the release workflow refuses to build when they disagree. The reason that check exists is 1.9.0, which shipped `podup_1.8.0_*.deb` when only `Cargo.toml` had been touched, so apt saw the version it already had and reported nothing to upgrade. The changelog entry names the three messages a user would have seen. The whole difficulty of this defect was that the failure read as arbitrary, so the wording a person will search for is the useful part of the entry. Signed-off-by: Jaro-c <75870284+Jaro-c@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Merge commit, no tag in this pull request. Two commits past 5.9.2: one fix and its version bump.
podup wrote GNU sparse tar entries, which Podman refuses, so one file with holes on disk took down the whole transfer it travelled in. A build died with
unhandled tar header type 83, acpwithunrecognized Typeflag S, and a watch sync logged a warning and silently never delivered the file, exit code 0 and all. The rejection is per archive rather than per entry, so ordinary files beside the sparse one were lost with it.Nobody puts a sparse file in a build context deliberately. They arrive as database files, disk images, virtual machine state or anything preallocated with
fallocate, andlsshows nothing unusual, so the failure read as arbitrary. Podman's own client packs in Go and never emits the type, which is whypodman buildaccepted a contextpodup buildrejected.Not a regression: every release from 0.3.0 onwards carried it, on Linux, Android and FreeBSD hosts, which are the three targets where the
tarcrate looks for holes at all.The fix routes all four
tar::Builderconstruction sites through one constructor that turns sparse off, with a test gate keeping it the only one, and four tests against a live Podman covering build,cpas a file and as a directory, and the watch sync. Each was verified red by reverting the fix.All three files that stamp a version read 5.9.3 on
origin/develop, checked immediately before this was opened. The tag goes onmainafter this merges, which is the only thing that triggers a release.