Format v2: hardening, compression, platform backends and CI - #4
Merged
Conversation
reader.cpp referenced an identifier `idx` that does not exist. Release builds define NDEBUG so the assert vanished and nobody noticed; any Debug, ASAN or UBSAN build failed to compile, which is why none of the defects in this codebase had been caught. Assert on the member that actually exists. CMakeLists overwrote CMAKE_CXX_FLAGS wholesale, which clobbered any flags the user passed, hardcoded GCC/Clang syntax despite config.hpp supporting MSVC, and forced LTO on every consumer. Warnings now live on an interface target that applies to our targets only, LTO is opt-in, and there are install/export rules so the library can be consumed outside add_subdirectory. The stricter warnings immediately found three real issues, all fixed here: a shadowed member in mmap_handle's constructor, a const-stripping C cast in munmap, and entry::path being a flexible array member (a C99 feature that is only a compiler extension in C++). Adds a dependency-free test framework and eight round-trip tests covering contents, iteration, early exit, zero-length files, large files and the 64-byte payload alignment the format promises. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three defects, each with a regression test. The bounds check for a payload used `>=` against the file size, but the last payload legitimately ends exactly at the last byte whenever its size is already 64-byte aligned, because round_up_64 is then the identity. Roughly one archive in 64 built from arbitrary content was silently unopenable while the writer reported success. The check now compares against the data region rather than the file, which makes the boundary case correct by construction. An archive with zero files set index_start == file_size == 64, which validate() rejected. The empty set is a legitimate archive and now round-trips. The reader inserted an entry into the map before checking any of its fields, so a corrupt path_len built a string_view over the mapping and hashed it. Values are now read into locals, checked against the index and data regions, and only then published. header::validate does its arithmetic in unsigned against a remaining-space budget instead of adding fields together, since signed overflow in the check would itself be undefined behaviour. UBSAN then found a further problem the review had missed: a corrupt index_start makes `reinterpret_cast<const entry*>` produce a misaligned pointer, which is undefined behaviour regardless of whether the load would have worked. Rather than only bounds-check the alignment, the header and index are now read through memcpy accessors, which removes the assumption entirely and also disposes of the object-lifetime problem in casting mapped bytes to a struct pointer. Fuzzing 3000 single-byte mutations under ASAN+UBSAN, same harness as before: memory errors 5.6% -> 0%. The 44% of corruptions still accepted as valid are the absence of a checksum, addressed separately. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
An exception escaping a std::jthread's entry point calls std::terminate, and the worker body both called a throwing open() and threw directly on a size mismatch. Archiving a live tree -- a build output directory, a working copy, anything another process touches -- killed the caller with SIGABRT. Workers now catch, record the failure, and commit() reports it. The failure modes are split in two. Files that cannot be sized or read are found in a sizing pass that runs before any layout is computed, so they can be dropped cleanly; on_unreadable::skip leaves them out and lists them in skipped(), while the default fails the whole archive. Failures during the copy itself are always fatal, because the index has already been written by then; that window is now small since the sizing pass also checks readability. Other fixes in the same area: - add_file() passed a string_view's .data() to stat(), reading past the end of the view whenever it was a prefix of a longer buffer, which silently archived the wrong file. fd_handle now takes const char* so the mistake cannot be spelled, and paths are copied before use. - stat()'s return value was ignored, so a missing file became a zero-length entry instead of an error. - Files were stat'd twice, once in add_file and once per worker. - commit() ended with a bare sync(), which is POSIX's flush-every-mounted- filesystem call, so committing a 2 KB archive could stall behind unrelated writeback. Now msync(MS_SYNC) on the mapping plus fsync on the fd. - The archive is built under a temporary name and renamed into place, so a failed commit can no longer destroy the archive it was replacing. - posix_fallocate reserves space up front, so running out of disk is an error rather than a SIGBUS when a dirty page is written back. - The worker pool pulls from a shared counter instead of being fed through per-thread queues polled in a spin loop, and sizes itself to the workload rather than always spawning 16 threads. The readerwriterqueue dependency is no longer needed. Ten tests cover the crash cases, the skip policy, atomic replacement and the string_view over-read. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Nothing normalised or checked archive paths, so "..", absolute paths, empty components and embedded NULs all passed straight through. uvfs never resolves these names itself, but anything extracting an archive joins them onto an output directory, so the format has to state the rule rather than leave every extractor to reinvent it. uvfs/path.hpp is a public header giving the rule as a constexpr predicate: UTF-8, '/' as the only separator, no empty components, no "." or "..", no NUL, no backslash, at most 65535 bytes. A leading '/' is allowed and kept verbatim, since these are lookup keys rather than filesystem paths; extractors strip it. Exposing is_safe_archive_path() means an extractor can apply the same rule uvfs applied at write time. add_file() now rejects bad paths immediately, where the caller can still say which input was at fault. Adding the same archive path twice used to produce an archive whose header claimed N files while the reader exposed N-1, with the shadowed payload written but permanently unreachable. commit() now reports duplicates and refuses by default; on_duplicate::replace keeps the last registration, in registration order, and drops the superseded payload from the layout entirely rather than leaving it stranded in the file. The reader treats a duplicate in the index as corruption, since it is exactly the condition that makes file_count disagree with what can be reached. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Version 1 stored the index as a chain of variable-length records. Every reader
had to walk that chain and rebuild it into a heap hash map before it could
answer a single lookup: work proportional to the number of files, repeated in
every process that opened the archive, and impossible to share between them.
For an archive whose selling point is fast indexing, opening was the slowest
thing it did.
Version 2 writes the index in its finished form and reads it in place:
header | fixed 128 bytes
index | entries, sorted by name, fixed 32-byte stride
| hash table, power of two, open addressing, 32-bit fingerprints
| per-entry content hashes (optional, unused until the next commit)
| name blob
data | payloads
Opening is now one mmap, one header check and six pointer assignments. On
/usr/include (44691 files) that is 1350us -> 9us, and the 5.7 MB per-process
heap index becomes zero, since the index lives in the mapping and is shared
through the page cache by every process that opens the same archive. Lookups
are unchanged at ~39ns: the fingerprint stored beside each entry index means a
colliding probe is rejected without touching the entry or the name blob. The
realistic case -- open an archive, read a handful of files -- goes from 1539us
to 66us. The archive is 0.2% larger.
Sorting entries by name is not only for binary search: index order is now
sorted order, so iteration comes out in the order extraction wants, and it
touches the archive in layout order rather than hash order.
Not walking the index at open means entries cannot be validated at open, so
they are checked where they are used instead -- a few comparisons on values
already in registers. The mutation test caught this: without those checks a
corrupt name_offset walked off the name blob. Whole-index integrity is a
separate question, answered by the index hash in the next commit.
The API changes with the format:
- reader is movable, so it can be returned from a factory or held in a
container. Copying stays deleted.
- count()/at(i) expose the entry array directly, in sorted order.
- for_each_file takes a function_ref instead of a std::function, so a stack
lambda no longer risks a heap allocation and an indirect call per entry.
- stat() reports sizes and how an entry is stored; read()/read_into() copy
(and, shortly, decompress); find() keeps the zero-copy pointer and is
documented as returning nullopt for entries that have no verbatim bytes.
- The unused loaded_file_entry::start and a stray <iostream> are gone.
The entry carries codec, stored_size and orig_size, and the header carries
flags, dictionary and hash fields, so compression and checksums are layout
changes that have already happened rather than ones still to come.
Tests reach into the private format header so assertions about the on-disk
layout use the real constants instead of magic numbers.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The review measured 44% of single-byte corruptions being accepted as a valid archive, because nothing in the format could tell intact bytes from damaged ones. XXH3-64 is the right hash for this: the threat model is corruption, not an adversary choosing inputs, so nothing cryptographic is needed, and XXH3 beats hardware CRC32C on throughput -- measured here at 14.3 GB/s hashing payloads. It is also the hash zstd already carries. Where the hashes go matters more than which hash it is. Hashing everything at open would have undone the previous commit: the index of a 44691-file archive is 2.9 MB, and checking it costs 175us against a 2.7us open. So the work is split by what it protects and when it is needed: - The header hash covers the 120 bytes every other offset is derived from. It is free and therefore never optional. - The index hash covers entries, table and names. Opt in with integrity::index; 175us for this archive, and it catches the whole class of "structurally plausible" damage -- a flipped bit inside a name, an offset that still lands in the data region but on the wrong payload. - Per-entry content hashes cover payloads. Under integrity::full they are checked as each entry is read, so the cost is proportional to what is actually used rather than to the size of the archive -- which is the only granularity that makes sense for a format whose point is lazy mmap access. verify() does the exhaustive sweep on demand: 32.7ms for 466 MB. Default is header_only, so opening stays O(1) and memory safety does not depend on any of this: entries are bounds-checked on use regardless of level. Identity is checked before the header hash, so a file that is not an archive, or is a newer format version, says so rather than reporting a checksum mismatch. A genuinely newer archive has a valid header hash, so the ordering costs no detection. Writing content hashes is on by default and costs 8 bytes per entry plus a hash of data already in cache from the copy; set_content_hashes(false) turns it off. Measured on the same 3000-mutation sweep the review used, restricted to the header and index: 0 accepted, down from 44%. The forged-duplicate case the previous commit could no longer catch at open is caught here, since forging a duplicate necessarily changes the index. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Compression is a property of each entry, not of the archive, because the content it has to handle is mixed. The entry's codec field already existed in the v2 layout; this fills it in. compression::automatic is the setting that matters for asset data. It compresses, then keeps the result only if it saved at least min_gain_percent (default 10%). Anything already compressed -- audio, video, images -- falls back to being stored verbatim, which means it keeps its 64-byte aligned zero-copy pointer and costs nothing to read. Measured on an asset-pack corpus of 620 MB of incompressible media plus 2000 small JSON files: all 190 media files stay zero-copy, all 2000 metadata files compress, and reading everything back still takes 0.04s because the bulk never goes through a codec. The threshold is deliberately not zero. A payload that compresses by 2% has given up its pointer and gained a decompression step on every read in exchange for nothing. On /usr/include (44691 files, 465.9 MB): none 472.0 MB 101.3% read-all 0.05s 44691 zero-copy automatic -3 100.8 MB 21.6% read-all 0.27s automatic -9 92.6 MB 19.9% read-all 0.26s automatic -9 +dict 79.1 MB 17.0% read-all 0.26s Building a compressed archive is not slower than an uncompressed one here -- there is 4.7x less to write. Implementation notes: - Large payloads are decided from a 256 KB sample rather than by compressing the whole thing, so a multi-gigabyte video is not compressed in full only to discover it was already compressed. - Payloads too large to stage in memory are streamed straight into the mapping, so memory stays bounded by the batch (64 MB) rather than by the archive. - Compression runs in parallel but offsets are assigned sequentially in sorted order, so output is byte-for-byte reproducible across runs. - With compression the final size is unknown until every payload is placed, so the file is mapped at its uncompressed upper bound (a payload is never stored larger than the file) and truncated back down. The unwritten tail is a hole. - An optional trained dictionary is stored in the archive and is what makes compression work on many small files, where each payload is too short to build any history of its own: 24% smaller on a 2000-file corpus. - Content hashes cover the stored bytes, so verify() detects damage without decompressing, and a damaged compressed payload is reported rather than handed back as wrong bytes. zstd is optional at build time. Without it the library still reads and writes uncompressed archives and refuses entries it cannot decode, with a message that says why. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ading Two changes, both measured on 200k small files (41 MB total) written to NVMe. The writer mapped and unmapped every source file individually. That is two syscalls plus a TLB shootdown broadcast to every core, per file, and with dozens of threads doing it at once it was the most expensive thing the writer did -- 42% of all syscall time. Payloads now go through pread, and files over 256 KB are handed to copy_file_range so the bytes never enter user space and can be reflinked instead of copied on filesystems that support it. mmap and munmap disappear from the profile entirely. The second change is thread count. Copying files is bound by per-file syscalls and page-cache contention, not by CPU, and running one thread per hardware thread actively hurts: 48 threads wall 0.48s sys 6.70s 77387 context switches 12 threads wall 0.43s sys 1.44s 18573 context switches so the copy phase is capped at 12 threads by default. Compression is the opposite -- it is CPU-bound and scales all the way out, 1.31s at one thread to 0.21s at 48 on /usr/include -- so it still uses every hardware thread. The two phases now get different defaults, and set_thread_count() overrides both. Against the writer as it was at the start of this branch, for the same corpus: wall 1.15s -> 0.42s, system time 11.51s -> 1.44s, and the 69431 voluntary context switches from the old spin-and-poll loop are gone. Also drops the per-file access() check unless the caller has asked for unreadable inputs to be skipped; with the default policy the copy reports the failure anyway, so it was a syscall per file for nothing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… CLI The README documented an unscientific benchmark and nothing else -- no format description, so the only specification was a comment block in a private header that had already drifted from the code. It now documents the on-disk layout field by field, the path rule, the integrity model and the limits, with measured numbers throughout. Including the unflattering ones. uvfs spends roughly 50 bytes of index per entry plus up to 63 bytes of alignment padding, which is nothing for an asset pack (+0.0% on 653 MB of media) and a lot for 200-byte files (+73.9%). That is worth saying plainly rather than only quoting the corpus that flatters the design. tests/fuzz_reader.cpp is a libFuzzer target for the parser, built with -DUVFS_BUILD_FUZZERS=ON. It fuzzes at integrity::header_only deliberately: memory safety must not depend on a checksum matching, so the weakest setting is the one worth exercising. 89702 executions against a seeded corpus, no crashes. src/bench.cpp replaces the ad-hoc harness and is what produced the numbers in the README. CMakeLists referred to it under UVFS_BUILD_BENCHMARKS but the file did not exist, so enabling the option failed to configure. The archiver was a 26-line loop that stored absolute system paths as archive keys. It is now a real tool with create/list/verify/extract, compression flags, prefix stripping, and an extractor that re-checks is_safe_archive_path() rather than trusting the archive it was handed. It no longer follows directory symlinks, which is what made the old benchmark store 48952 paths for 45796 distinct files. Verified across Debug, Release, RelWithDebInfo+LTO, clang, ASAN+UBSAN and -DUVFS_WITH_ZSTD=OFF: no diagnostics, all tests pass. Without zstd the library builds, writes and reads uncompressed archives, and refuses compression with a message that says why; archives written by that build are readable by a build that has zstd. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A file added under more than one archive path, or reached through hard links, was written once per name. Entries now share a payload: the first occurrence in sorted order writes the bytes and the rest adopt its offset, size, codec and content hash. This needed no format change. Nothing ever required payload offsets to be distinct, so a reader sees ordinary entries that happen to point at the same place, and every existing guarantee still holds: both names read correctly, both hand out the same pointer, and verify() reports both when the shared payload is damaged. Detection is by (st_dev, st_ino) from the stat the writer already does, so it costs one hash lookup per entry. Identical *content* under different inodes is still stored twice; catching that would mean hashing every input before the layout is computed, doubling read I/O to save a measured 4.2% on /usr/include, which is not a good default. Also evaluated and rejected: madvise(MADV_WILLNEED) on the index and MADV_RANDOM on the data region. Across cold-cache runs reading 10, 100 and 1000 files, the difference stayed inside run-to-run variance on this hardware, so there is no evidence to justify the complexity. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…okup Found by the fuzz target added two commits ago, which I had misread as having found nothing: its run ended in "SUMMARY: libFuzzer: timeout", and a libFuzzer timeout is a finding, not a time limit. validate() checks the hash table's capacity against the file count but says nothing about its contents. Linear probing terminated only on the empty sentinel, so an archive whose table had been corrupted to contain no empty slot made find() of an absent key spin forever. Reproduced directly from the saved artifact: the archive opens, reports 2 entries, and never returns from find(). The probe is now bounded by the table capacity, after which every slot has been visited and there is nothing left to find. A well-formed table is at most 70% full so a real miss still stops after a couple of probes, and the bound cannot cut a legitimate lookup short -- the test covers a full table that does still hold a reachable entry. This is a class of bug the mutation tests were structurally unable to find: every one of them asserts that an operation completes, so a hang is not a failing check, it is a hung test run. Fuzzing catches it because libFuzzer watches wall-clock time per unit. Re-fuzzed with the offending input seeded into the corpus: 8938336 executions in 301 seconds, no crashes, no timeouts, slowest unit 0 seconds. Throughput went from 134 execs/sec to roughly 29700, which is itself a measure of how much time the old code spent spinning. Audited the remaining loops over archive-controlled data; the reader's other two are bounded by file_count, which validate() constrains, and the writer's table insertion walks data it produced itself with a guaranteed empty slot. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Found by an adversarial review. A corrupt dictionary decoded to silently wrong bytes at every integrity level, including integrity::full, and verify() saw nothing wrong. Nothing covered the dictionary region. header_hash stops at byte 120, index_hash covers only [index_start, index_start + index_size), and per-entry content hashes cover the *stored* bytes -- which a damaged dictionary leaves perfectly intact. dict_start sits between the index and the data, outside all three. The only thing that ever caught it was zstd noticing a decompressed size that did not match, which is incidental. This is the worst possible failure for a format whose stated threat model is corruption: not a crash, not a refusal, but confidently wrong bytes. It is also amplified rather than localised -- one damaged dictionary changes what every entry using it decodes to, so a single bit flip corrupts thousands of files. dict_hash goes in reserved header space, so the layout does not move, and it is covered by header_hash like every other field. The reader checks it whenever a dictionary is present, regardless of integrity level: the ladder exists to let callers trade detection against open cost, but this one is not optional and costs a few microseconds on a 16 KB dictionary. The regression test flips one bit at a time through the dictionary of a 600-entry archive and compares what every entry reads back against a pristine copy, so "silently wrong" is distinguished from "correctly refused" rather than just checking that something threw. Before: 53 of 64 flips silently wrong, 2 detected. After: 0 silently wrong, 64 detected. Also corrects the comment in verify() that asserted the opposite, and the README's integrity table. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Found by an adversarial review, via a fuzz target it extended to call read() -- the shipped one never did, which is exactly why this survived. sane() constrained orig_size only for codec::store. For a compressed entry it was unchecked, so read() sized a std::vector straight from a corrupt index field. Fuzzing found inputs requesting up to 0x29000000000300e8 bytes. On Linux with overcommit a request of a few hundred GB can succeed and the process is then OOM-killed rather than throwing, so this is not reliably even an exception. std::bad_alloc is also not std::runtime_error, which the rest of the API trains callers to catch. Two checks, because neither is sufficient alone: sane() now applies a ratio bound. zstd's densest encoding is a run-length block, 3 bytes of header for up to 128 KiB of output, so no valid frame beats about 43691:1; the bound is 50000:1. It has to stay loose to admit every real frame -- a measured best case, 8 MiB of zeros at level 19, reaches 30728:1 -- which is why it is not enough on its own: a 1 MB payload could still claim 50 GB. So read() and read_into() also cross-check orig_size against the size the zstd frame header declares, which settles it exactly and costs a few bytes to read. A frame that declares no size falls back to the ratio bound. The order matters: the check runs before the allocation, not after. read() no longer goes through stat() either, which also removes a second lookup. The CLI's extract sized its buffer from the index the same way and now goes through read(). Test asserts the distinction that matters -- refusing is correct, throwing bad_alloc means the allocation was attempted -- and a companion test pins that a genuine 30728:1 payload still reads, so the bound cannot be tightened into rejecting real data without failing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Found by an adversarial review. The store path's worker is wrapped in a try/catch whose comment says failures are recorded rather than thrown; the compression path had no handler at all. Its body allocates two buffers the size of the payload -- one to stage the file, one for the compressed result -- so std::bad_alloc there is not hypothetical, it is what a large batch under memory pressure does. An exception escaping a thread's entry point calls std::terminate, taking the caller's process with it. Reproduced with a helper that runs one commit under RLIMIT_AS: 16 x 60 MB incompressible files, 8 threads, 1150 MB of address space, dies with SIGABRT from inside the compression lambda. The window is narrow -- at 1050 MB the allocation that fails is the one in read_file, which was already protected and reports cleanly -- so the test sweeps a range of limits instead of pinning one value, and asserts the property that actually matters: no limit may kill the process. Succeeding is fine, reporting commit_error is fine, aborting is not. Fixed in all three worker bodies, including catch(...) for non-std exceptions, which the store path was also missing. record_error is now noexcept: it was itself capable of throwing bad_alloc out of the handler that exists to stop exceptions escaping. If recording the detail fails, an atomic flag still fails the commit, so the archive is never published on the strength of an error nobody could allocate a string for. The test also asserts no temporary is left behind at any limit, which is a separate defect handled in the next commit; it passes here only because the aborts were what leaked them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Found by an adversarial review. commit() promises in its documentation that "an interrupted or failed commit never leaves a partial archive behind", but cleanup lived in catch clauses that named commit_error and std::runtime_error. A std::bad_alloc or std::length_error escaped both and left the temporary in place -- observed at roughly a gigabyte after a worker died under memory pressure, in the output directory the caller chose. Cleanup by exception type is the wrong shape for the problem: it is a list that has to be kept in step with every allocation and every library call anywhere under commit(). A scope guard does not need to know what went wrong, only that the scope was left without success being declared, so the manual unlink calls and the dependence on which handler ran are both gone. dismiss() is called exactly once, after rename() has put the archive under its final name. The guard is tested directly against std::runtime_error, std::bad_alloc, std::length_error, std::invalid_argument, a non-std exception type and an ordinary return, because the defect being fixed is precisely that some of those were not covered. Commit-level tests pin the behaviour end to end for a failed rename, a failed copy, a failed compressed copy and a successful commit, and the memory-pressure sweep checks it across eight address-space limits. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Found by an adversarial review. Two related defects, both reproduced by committing into a 4 MB tmpfs in a private mount namespace. Compressing, the writer mapped a purely sparse file and stored payloads through the mapping without reserving anything: SIGBUS, core dumped. A store to a page the filesystem cannot back is not something the kernel can turn into an error return, so the blocks have to exist before the bytes are written through them. reserve_space() was called only on the uncompressed path, because with compression the final size is not known until every payload has been placed. Not compressing, the archive was reserved up front -- but posix_fallocate's return value was discarded, by a function whose comment said it existed "so that running out of space is reported here, as an error, rather than as a SIGBUS". The failure instead surfaced as pread returning EFAULT when it tried to write into an unbackable page, and was reported as "read failed: Bad address" naming an *input* file, which points anyone debugging it at entirely the wrong thing. reserve_range() now reports: it throws when the space genuinely is not there, and returns false when the filesystem simply has no fallocate, which is not a failure but does mean the guarantee is unavailable. The compressed path reserves the data region incrementally, in 32 MB chunks ahead of the cursor, which needs no advance knowledge of the final size and costs about one syscall per chunk. The header, index and dictionary are reserved too -- they are written through the mapping just as much as the payloads are. Both modes now fail with "not enough space for the archive (No space left on device)". The test also pins that an archive which does fit still commits, so that reserving space cannot quietly turn a workable commit into a failure. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Found by an adversarial review. Move leaves the pimpl null, and every accessor
dereferenced it without checking -- including size(), count(), empty() and
has_content_hashes(), which are noexcept, so there was not even an exception to
catch. `reader b{std::move(a)}; a.size();` segfaults.
The header advertised move support and said nothing about the resulting state,
which is the part that makes this a defect rather than a documented sharp edge:
a caller has no way to know. The standard contract for a moved-from object is
"valid but unspecified", and valid means member functions can still be called.
So the moved-from state is now an empty archive: size() and count() report
zero, lookups miss, iteration visits nothing, verify() finds nothing, and at()
throws out_of_range like any other out-of-range index. Nothing pretends the
data is still there; it is simply well defined.
Measured on a 44691-entry archive, best of five over all 44691 keys in random
order: 29.4 ns per lookup with the check against 29.1 ns with it removed. The
difference is inside run-to-run noise, as expected for a branch that is never
taken in the hot path.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Found by an adversarial review. A payload larger than the staging batch takes the streaming path, and that path opened the source file outside any handler. An unreadable or vanished large input therefore escaped as a bare runtime_error rather than a commit_error, so the caller lost the per-file list entirely -- and the CLI, which catches commit_error specifically, printed nothing about which file was at fault. Worse, the message named the wrong file. commit() appends the output path to every runtime_error that reaches it, on the assumption that anything getting that far is about the archive. For an input error that assumption inverts the message: a permission failure on the input was reported as "could not open (Permission denied): <the archive>", pointing at a file that was never the problem. The streaming branch now records the failure like every other input error, so it arrives as commit_error carrying the input's path, and the append-the-output convention becomes true again rather than merely usual. One case must not be demoted that way: running out of space is a property of the archive being written, not of the input that happened to be in flight when the disk filled. Collected as a per-file error it would look skippable, and under on_unreadable::skip the writer would drop files and carry on producing an archive on a filesystem with no room for it. out_of_space is now its own type and propagates. Tests cover both a 70 MB unreadable input and a 70 MB input removed after sizing, asserting the exception type, that the per-file list is populated, and that the message names the input rather than the archive. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Found by an adversarial review. entry::name_offset is a uint32_t, but the writer's running names_size is an int64_t and nothing compared the two, so the assignment truncated. The README listed "4 GB of names in total" as a limit; it was documented and unenforced, which is the worst combination -- a reader of the docs would reasonably assume it was checked. The failure is silent and total. Reproduced with 65541 paths of 65535 bytes, both inside the documented per-path and per-file limits: commit() reports success, the archive passes integrity::index because the index hash is self-consistent over the truncated offsets, and the affected entries carry other entries' names -- so at() and for_each_file() return paths that were never added, while find() and read() cannot locate them under the names they were given. Nothing anywhere says something went wrong. It needs an extreme corpus -- roughly 43 million files at 100-byte paths -- but "unlikely" is not a property that stops it happening, and there is no way to detect it after the fact. The limit is now a named constant next to the path rules it belongs with, and commit() refuses rather than truncating. The reproduction holds more than 4 GiB of paths in memory, so it is behind UVFS_SLOW_TESTS=1 rather than in the default run. It was executed for real to confirm both directions: it fails before the change (commit succeeds, archive written) and passes after. A cheap always-on test pins the constant against uint32_t's range so that widening the field without revisiting this guard cannot pass unnoticed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The shipped fuzz target stopped at find(), which only ever hands back a pointer into the mapping. Everything downstream of that -- decompression, the dictionary, and every size the index claims about a payload -- was never reached. That is not a small gap: it is precisely where the adversarial review found an unbounded allocation, using a target it had extended in exactly this way. read() and read_into() are now called for every entry. Failures are swallowed per entry rather than abandoning the archive, because refusing a corrupt entry is the correct outcome and the run should carry on to the next one. A 64 MB cap keeps a legitimately large claim from being reported as an out-of-memory finding; the bound on absurd claims is a separate concern with its own test. Coverage over the same corpus goes from 89 edges to 120, and features from 147 to 495. Throughput drops from roughly 29700 to 1866 executions a second, which is the point: each execution now does real decompression work instead of bouncing off a pointer handout. 561890 executions in 301 seconds against a corpus seeded with stored, compressed, dictionary-compressed and hash-less archives: no crashes, no timeouts, no out-of-memory, slowest unit 0 seconds. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both helpers asked for compression outside their handlers, so a build with -DUVFS_WITH_ZSTD=OFF aborted where it should have skipped, and the OOM helper function was unused in that configuration. The library was fine; the tests were not. Also records how to run ThreadSanitizer here: it aborts at startup on this kernel with "unexpected memory mapping" unless address-space randomization is disabled, so it must be invoked under setarch -R. Without that it exits silently and a run looks clean because nothing ran at all. Verified under TSan with ASLR disabled: the full suite, and a 6000-file corpus built in store, automatic and dictionary modes with twelve threads reading and decompressing concurrently. No races. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Preparing for CI on more platforms immediately found that the library does not compile anywhere except glibc. strerror_r comes in two incompatible shapes: the XSI one returns int and fills the caller's buffer, the GNU one returns char* which may or may not point at that buffer. errno_string assumed the GNU shape, so musl, macOS and the BSDs all failed with "invalid conversion from 'int' to 'const char*'" -- confirmed against Alpine. Which declaration is in scope depends on the libc and on feature-test macros, so choosing with #ifdef gets it wrong on some combination; an overload pair lets the compiler answer instead. current_rss_kb() read /proc/self/statm and returned 0 when it could not, so on any system without /proc the open-cost test would have compared zero against zero and passed while measuring nothing. It now reports -1 and the test skips itself, which is honest about what is and is not being checked. getrusage is not a substitute: it reports peak RSS, which is monotonic and so useless for comparing one operation against another. The namespace probe in the disk-full test treated "unshare: command not found" as success, so on a system without user namespaces it would have run the test anyway and failed. It now requires the probe commit to actually succeed. The benchmark computed a resident-size figure and then discarded it. Verified: Alpine/musl builds and passes. gcc 12, 14, 15.3 and 16.1 and clang 19, 20 and 22 all pass, each at C++20, C++23 and C++26. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Preparing a CI gate for these tools meant running them for the first time. .clang-format had two CommentPragmas keys, which makes the whole file invalid to clang-format 17 and newer -- so the project's style has not actually been enforceable for some time, and 26 of 34 files had drifted. The duplicate is gone and the tree is formatted with clang-format 18, which is the version CI pins; leaving it unpinned would turn a runner image update into a spurious failure. clang-tidy found one real defect: a dead store in copy_payload's partial-copy fallback. dst_offset was advanced and never read again, because the remaining bytes are addressed through the mapping while the source offset stays absolute. The store is gone and the reason is written down. The rest of what it found was smaller: multiplications performed in int before widening to int64_t (harmless at these values, wrong in principle), three variables that could be const, and a parameter passed by value that is only copied. Two findings are deliberate and now say so at the site rather than being disabled globally -- the empty catch in record_error, which exists so a failure to record cannot escape a worker thread, and the memcpy into the name blob, which is length-prefixed and so intentionally not NUL-terminated. cppcheck could not parse main()'s function-try-block, which is valid C++ that several analysers stumble over; an inner try is equivalent and costs nothing. Its remaining complaint is a parser limitation on function_ref and is suppressed by name, so a genuine syntax error still fails the run. Both tools are configured explicitly rather than by default, so a new release cannot silently change what CI enforces. performance-enum-size is off: the base type of a public enum is an ABI decision, not a lint. src/test.cpp was the version 1 benchmark, replaced by src/bench.cpp and unused since; nothing built it. Verified after formatting: gcc 12 and 14 and clang 19 and 22, each at C++20, C++23 and C++26, plus gcc 15.3 and 16.1, Alpine musl on x86_64 and arm64. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Five workflows, modelled on the celtera/libremidi and ossia/score layout: a named-config matrix, fail-fast off, concurrency cancellation, and separate jobs for the targets that need a VM. build.yml Ubuntu x86_64 and arm64, macOS arm64 and x86_64, each with gcc and clang, at C++20, C++23 and C++26, in Debug and Release. Containers cover gcc 12 through 16 and Debian trixie/sid; a separate matrix covers clang 17 to 21 against both libstdc++ and libc++, because libc++ is what macOS, Android and the BSDs use and it lags libstdc++ by years on some features. Alpine covers musl on both architectures. There are jobs for building without zstd, for LTO, and for consuming the library both as an installed package and as a subdirectory. bsd.yml FreeBSD, OpenBSD and NetBSD. These are the platforms most likely to catch a Linux assumption: their own libc, no /proc, and none of the Linux-only syscalls uvfs takes when they are available. sanitizers.yml ASan+UBSan under both compilers, TSan, valgrind on the format and reader suites, and a _GLIBCXX_DEBUG build. The TSan job runs under setarch -R and then greps its own output, because TSan aborts at startup on kernels with more ASLR entropy than its shadow mapping allows -- and exits 0 doing so, so an unguarded job would report success having run nothing. quality.yml clang-format, clang-tidy and cppcheck as hard errors; a reproducibility check that the same inputs give byte-identical archives across runs and across thread counts; a command line round trip through every compression setting with the extracted tree diffed against the input; a check that a damaged archive is refused; and a job that compiles for s390x and requires the little-endian static_assert to fire, so the guard cannot rot into silently writing byte-swapped archives. fuzz.yml libFuzzer over the reader on every push, longer nightly, with the corpus cached between runs and any reproducer uploaded as an artifact. Two things had to be fixed to make this work rather than just be declared. The installed package was not usable: find_package(uvfs) found nothing to link because the exported targets were named uvfs::uvfs_reader while the build tree alias was uvfs::reader, and zstd was wrapped in $<BUILD_INTERFACE:> so a static library recorded nothing about the symbols it still needed. There is now a generated uvfsConfig.cmake, EXPORT_NAME on both targets, and zstd propagating as a link-only dependency. Both consumption modes are checked in CI and were verified locally first. std::jthread is gone in favour of std::thread plus a join guard. jthread needs libc++ 18, which is newer than several targets ship; nothing here wanted a stop token, so the only thing it provided was the join, and the guard does that on every path including an exception part way through starting the pool. Windows is the one gap and is documented as such: uvfs is built on mmap and pread, and without a Win32 backend there is nothing for MSVC to compile. Locally verified before pushing: gcc 12, 14, 15.3 and 16.1 and clang 19, 20 and 22, each at C++20/23/26; Alpine musl on x86_64 and arm64; the s390x guard; the reproducibility, round-trip, damaged-archive, no-zstd and both consumption checks; and clang-tidy, cppcheck and clang-format all passing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ws and wasm
uvfs does a handful of things to files -- copy a range, reserve blocks, map,
make durable, replace atomically -- and each has a best primitive that differs
per system. Those were previously #ifdefs scattered through the writer, with
only Linux actually special-cased and everything else falling through to the
generic path. src/platform.hpp now states the operations once and each backend
implements them:
linux copy_file_range, posix_fallocate, madvise
freebsd copy_file_range (13+), posix_fallocate
macos F_PREALLOCATE, F_FULLFSYNC, F_RDAHEAD -- fsync alone does not
survive power loss there, and there is no posix_fallocate
windows CreateFileMapping/MapViewOfFile, positioned ReadFile through
OVERLAPPED, SetEndOfFile, MoveFileEx, PrefetchVirtualMemory
emscripten no writable shared mapping at all
generic pread/pwrite and mmap, which every POSIX system has
Every call is resolved at compile time; the abstraction costs nothing.
Windows was previously not buildable -- there was no mmap for MSVC to compile
against. It now works, which closes the gap left when CI was added.
Emscripten needed more than a backend. Its mmap accepts MAP_SHARED but never
writes the mapping back, so an archive built through one would be silently
lost. output_region hides that: where a shared mapping works it *is* the file,
and where it does not the region is memory that gets written out on flush. The
writer has one code path either way. A build without pthreads also cannot
create a thread, so the worker pool now runs inline at one thread and falls
back to the calling thread if the system refuses to start more -- which is a
better failure everywhere, not just under wasm.
Validated by running, not just compiling:
linux x86_64 100 tests
windows cross-built with mingw-w64, run under wine: 80 tests,
9074 checks, 0 failures
wasm built with emsdk, run under node: 80 tests, 9178 checks,
0 failures
Running the Windows build found a real portability bug: commit() derived the
destination directory by searching for '/', so on Windows the temporary landed
in the current directory instead of beside the archive -- which fails outright
where that directory is not writable, and quietly stops the final rename being
atomic where it is. It now uses std::filesystem::path::parent_path().
Other things the ports surfaced: path::c_str() is wchar_t* on Windows, so
fopen call sites went through string(); UVFS_EXPORT listed three attribute
spellings and warned on every compiler that understood only one; and the
disk-full and memory-limit tests are Linux-specific and now say so rather than
failing elsewhere.
tests/test_platform.cpp holds every backend to the same 14 tests: positioned
reads at page boundaries and past end of file, writes, resize both ways,
reservation, mapping contents, copy at offsets either side of the kernel-copy
threshold, copy with no mapping to read into, file identity for hard links,
atomic replace over an existing file, and readable error text.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The backends exist now, so MSVC on x86_64 and arm64, MinGW cross-compiled and run under wine, and Emscripten run under node all join the matrix. The wine job in particular is not ceremony: running the Windows build is what caught commit() deriving its temporary's directory with a POSIX separator, which cross-compiling alone would never have shown. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
macOS CI caught this immediately. `if constexpr` only discards a branch from instantiation inside a template; copy_file_into is an ordinary function, so the untaken branch still had to name-lookup, and macOS has no ::copy_file_range to find. It needs a preprocessor guard, not a constexpr one. UVFS_FORCE_GENERIC_COPY compiles the fallback on a platform that has the fast path, so the code macOS, OpenBSD and NetBSD actually run can be built and tested on Linux instead of only ever being exercised where nobody can debug it. Verified both ways: 100 tests pass with the kernel copy on and off. Also stops driving the Alpine container through actions/checkout. Actions run on the runner's own node binary, which is linked against glibc and cannot execute inside musl, so the checkout failed before any build started; the container is now driven from a run step instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
macOS built test_oom.cpp but the helper binary it runs is only built on Linux, so UVFS_OOM_HELPER was undefined. Driving a commit into an address-space limit needs RLIMIT_AS enforced and that helper present, so the test is now scoped to where both are true rather than to 'not Windows'. MSVC failed on unistd.h. The guard around it had been written but clang-format sorts includes and had lifted the include out of its own #if; it is now inside a clang-format off region so the guard survives formatting. The BSD jobs report only 'ssh exited with code 1' when the script inside the VM fails, which says nothing at all. They now run under set -x and print the compiler and cmake versions, and invoke the test binary directly rather than through ctest, so the next failure comes with evidence. The pinned FreeBSD release is dropped in favour of whatever the action currently provides. Re-verified after the edits: 100 tests on Linux, and the Windows build under wine still passes 80. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ctly MSVC found two tests still calling ::access with R_OK and the fuzz target calling ::getpid. Both questions already have a portable answer in the platform layer -- readable() and process_id() -- which is the point of having it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
It uses the platform layer for a portable process id, but only the test binary had src/ on its include path. Verified by building the fuzz target the way CI does and running it: 928958 executions, clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The macos-13 runner has been retired, so those six jobs sat queued indefinitely rather than failing -- which is worse than a red check, because nothing ever reports. macos-15-intel is the current Intel image. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Long rationale blocks, narration of how the code used to behave, and restated signatures. Comment density across src, include and tests goes from 16% to 7%. What is kept is the format layout, the non-obvious constraints (unaligned loads, unsigned bounds arithmetic, the zstd expansion ceiling) and the reasons behind choices that look wrong at a glance.
jcelerier
force-pushed
the
format-v2-hardening
branch
from
August 15, 2026 11:29
e464ee4 to
791ec14
Compare
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.
Rebuilds uvfs around a v2 format, fixes every defect two rounds of review turned up, and adds CI across every platform the library now supports.
Correctness
Six defects from the first review, each with a regression test that reproduces it first:
>=off-by-onestring_view::data()tostatconst char*path_lencommit_errorassert(idx)Fuzzing, same harness: memory errors 5.6% → 0%.
An adversarial review then found eight more, all fixed the same way:
orig_sizewas unbounded for compressed entries, so a corrupt index drove an allocation of up to 2^62 bytes.try/catch, sobad_alloccalledstd::terminate.posix_fallocate's return value was discarded.catchclauses did not name.readerwas UB.commit_errorand the message named the wrong file.Plus a hang the fuzzer found: an unbounded probe loop on a corrupt hash table. Mutation tests could not catch it — they all assert that an operation completes.
Format v2
The index is stored finished and read in place: sorted entries, a baked hash table with fingerprints, a name blob. Open went 1350 µs → 2.2 µs and no longer scales with archive size; the 5.7 MB per-process heap index is gone, shared through the page cache instead.
Integrity is layered so open stays O(1): header hash always, index hash opt-in (175 µs), per-entry content hashes checked as entries are read. XXH3 throughout — 14.3 GB/s measured.
Compression
Per entry, not per archive.
automaticcompresses and keeps the result only if it saves enough. On an asset-pack corpus of 620 MB media plus 2000 JSON files, all 190 media files keep their zero-copy pointer and every JSON file compresses; on/usr/includeit is 472 MB → 79 MB.Platform backends
Each platform gets its fastest primitives rather than a common denominator:
copy_file_range(Linux, FreeBSD),F_PREALLOCATE/F_FULLFSYNC(macOS),CreateFileMappingand positionedReadFile(Windows), staged writes where Emscripten cannot write back a shared mapping.Validated by running, not just compiling: Windows cross-built with mingw and run under wine (80 tests), wasm run under node (80 tests). Running the Windows build caught a real bug —
commit()derived its temp directory with a POSIX separator.CI
21 jobs: Linux x86_64/arm64, macOS arm64/x86_64, MSVC x86_64/arm64, MinGW under wine, Emscripten under node, FreeBSD/OpenBSD/NetBSD, musl on two arches, gcc 12–16, clang 17–21 against libstdc++ and libc++, C++20/23/26, ASan/UBSan/TSan/valgrind, clang-tidy and cppcheck as errors, fuzzing, reproducibility, and a check that the little-endian guard actually fires on s390x.
Setting it up found that the library did not compile on any non-glibc libc (
strerror_rhas two incompatible shapes), that the installed CMake package was unusable, and that.clang-formathad been invalid for years.Testing
100 tests, ~10,000 assertions. Green on Debug, Release, LTO, clang, ASan+UBSan, TSan, no-zstd, musl, arm64, Windows and wasm.
One caveat: the BSD and macOS jobs are the ones I could not run locally, so they are the most likely to need a follow-up.