test(#176): phar fixtures written by PHP, covering every packaging variant - #460
Merged
Conversation
libarchive does not read the native phar format at all, so the archive extra
gains its own reader behind the same `Member` shape the libarchive reader
presents. A phar is a PHP script (the "stub") ending in `__HALT_COMPILER();`,
followed by a binary manifest and then the member data - a layout no generic
archive library recognises. The tar- and zip-based phar variants
(`.phar.tar`, `.phar.zip`) need none of this: they are ordinary tars and zips
that the libarchive reader already handles.
Entry points mirror the libarchive reader, in-memory and file-based:
`ListPharMembers`, `ListPharMembersOfFile`, `ReadPharMember`,
`ReadPharMemberOfFile`.
Details worth stating, each test-pinned:
- Neither entry point reads a whole container. Listing reads the stub scan
window plus exactly the manifest length the header declares; a content read
then seeks to that one member's byte range. The stub scan is bounded (1 MiB)
so handing the walk a large non-phar file cannot turn into a full read, and
the declared manifest length is capped so a corrupt or hostile length field
cannot become a huge allocation.
- Every declared length is honoured even when the value is unused (the API
version, the container alias, the per-member metadata). Skipping one does not
fail - it shifts every following field, which is why a non-empty alias and a
second member's data offset each have their own case.
- A DIRECTORY member is the trailing-slash spelling of a name; phar has no flag
bit for it. The reported path drops the slash, and a read of it reports
FailedPrecondition ("no content"), not NotFound.
- A per-member COMPRESSED entry (phar may deflate or bzip2 individual members)
is refused with Unimplemented rather than returning the stored bytes.
Returning them would make `-grep` silently miss a pattern the member does
contain. Decompression is a follow-up slice; listing such members already
works, since the manifest carries their metadata regardless.
- Errors keep the libarchive reader's split: InvalidArgument for "not a phar"
(the walk treats it as an ordinary file) versus DataLoss for a phar-shaped
container whose manifest is truncated, lies about its member count, or points
member data past the end.
Member-name normalization (`./x` and `dir/` spellings) moves into
`xff_extras_api`'s member-path library as `NormalizeMemberName`, since both
readers need exactly the same rule and that library owns member-path spelling.
There is no phar writer to lean on, so the test builds containers byte by byte:
the fixture is the format specification.
Conflict: #455 (archive VFS backend) and this branch each appended targets to extra_modules/archive/BUILD.bazel. Both sets are kept. Also does what this PR promised once #457 landed: `archive_reader.cc` drops its local copy of the member-name normalization and uses the shared `NormalizeMemberName` from the member-path library, so the rule exists once for both readers rather than being duplicated on main.
The merge commit resolving this branch's BUILD conflict slipped past the local trunk hook, which skips on a merge, so a dep inserted out of order only surfaced in CI.
…riant
The byte-level reader test builds containers by hand, so it pins what we BELIEVE
the format is - it cannot catch a misreading, only an inconsistency. These
fixtures come from the reference implementation: nine containers written by PHP
itself (~60 KiB total, each well under 10 KiB), committed under
extra_modules/archive/test_data/, generated by tools/make_phar_fixtures.php:
php -d phar.readonly=0 tools/make_phar_fixtures.php
The generator is a committed dev tool, never run by a test: the tests must not
need a PHP interpreter and CI must not install one, and the files are tiny enough
that checking them in costs less than the dependency would.
The variants exist because each is a different thing to get right - native
stored, native per-member DEFLATE, native per-member bzip2, SHA-256 signed,
whole-file gzip, whole-file bzip2, tar-based, tar-based + gzip, and zip-based.
All carry the same member set, so one set of expectations spans them.
They immediately earned their keep by finding a real bug. A tar- or zip-based
phar stores the stub as an ordinary member (`.phar/stub.php`), so
`__HALT_COMPILER();` appears inside a perfectly good tar. The reader committed on
the token alone and then reported that tar as a CORRUPT phar - worse than not
recognising it, because the walk reports an error instead of reading the archive
libarchive handles fine. Detection now:
- validates the manifest HEADER before committing (declared length in range, a
member count the declared manifest could actually hold, a non-zero version
major - tar's NUL padding fails all three), and
- tries EVERY halt-token occurrence, so a stub that merely mentions the token
(or an embedded stub) no longer hides the real manifest.
Only the fixed header must be present: a plausible header with the file ending
inside the declared manifest is still a truncated phar (DataLoss), not "some
other file". The byte-level test's lying-member-count case therefore moves from
DataLoss to InvalidArgument, with the reason recorded, and gains a
stray-token-before-the-real-one case.
Two current gaps are now pinned by tests rather than asserted in prose: a
per-member compressed entry lists but refuses to read (Unimplemented), and a
whole-file compressed container is not recognised at all (its halt token is
inside the compressed stream).
…xtures Retargeted onto main now that the phar reader landed. The three phar files conflicted add/add because the reader was squash-merged; this branch's copies are that content plus the detection fix, so they are kept whole. Two real problems surfaced while syncing: - The `end-of-file-fixer` and `trailing-whitespace` hooks APPENDED a newline to four of the containers. They begin with PHP source, so pre-commit's content sniff calls them text. The corruption was silent: a byte at EOF moves neither the manifest nor any member's data, so every functional test still passed - but PHP rejects the result, because the signature covers the whole file and its `GBMB` magic must be the last four bytes. The fixtures are regenerated (and verified with PHP: all nine open, 3 members each, signature intact). - The hooks now exclude archive EXTENSIONS rather than that one directory: the hazard is the file type, not where it lives, so it holds for any container added anywhere. The repo-policy pygrep hooks get the same exclusion, since they also scan whatever is sniffed as text. And a guard so this cannot recur quietly: phar_fixture_test asserts the `GBMB` trailer is still the last four bytes of each uncompressed native fixture. Only those - in the whole-file compressed and tar / zip variants the trailer is inside the compressed stream or behind the format's own end record, and those files are binary from byte 0, which is exactly why no text tool mistook them for editable.
helly25
enabled auto-merge (squash)
August 11, 2026 08:30
helly25
added a commit
that referenced
this pull request
Aug 11, 2026
Retargeted onto main now that #460 merged. The conflicts are all supersets of main's content (the hook exclusion list, the archive BUILD file, the fixture README), so this branch's copies are kept. Two problems found while doing it, both worse than the thing this PR set out to add: 1. SIX FIXTURES WERE NEVER COMMITTED. A global gitignore commonly ignores archive extensions, and this machine's does (`*.jar`, `*.zip`, `*.tar`, `*.gz`, ...). A global ignore makes `git add` skip the file silently, so the two new fixtures here AND FOUR of the phar fixtures already merged in #460 (tarbased.phar.tar, targz.phar.tar.gz, wholegz.phar.gz, zipbased.phar.zip) are missing from the repo, while every local test kept passing because the files are on disk. Fixed by a repo-level negation for the fixture directory, which outranks the global file, plus re-adding all six. 2. @xff_archive HAS HAD NO CI COVERAGE AT ALL. Every cell runs `bazel test //... @xff_pcre2//...`, and `//...` does NOT match another bazel module, so no archive test - reader, VFS backend, phar, fixtures - has ever run in CI. That is why #460 could merge green with four fixtures missing. Now spelled out alongside @xff_pcre2 in all three cells (test matrix, tsan, msan), which also gives the readers the asan/tsan coverage the comment there already claimed they had - they parse untrusted binary headers, so it matters. An extra's own tests need no --//xff:xff_<extra> flag: that flag gates whether the CORE links the extra, not whether the extra builds. Verified with the exact CI command: 105 tests pass (was 99 before, the difference being the archive extra that never ran).
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.
Stacked on #458 (base is
feat/176-phar-reader, notmain) - not armed for auto-merge; it gets rebased onto main once #458 lands.The byte-level reader test builds containers by hand, so it pins what we believe the format is - it cannot catch a misreading, only an inconsistency. These fixtures come from the reference implementation: nine containers written by PHP itself (~60 KiB total, each well under 10 KiB) under
extra_modules/archive/test_data/, generated bytools/make_phar_fixtures.php:The generator is a committed dev tool, never run by a test: the tests must not need a PHP interpreter and CI must not install one, and the files are tiny enough that checking them in costs less than the dependency would. Regeneration is idempotent (it clears the directory first, since Phar refuses to overwrite a conversion target).
plain.pharentrygz.pharentrybz2.pharsha256.pharGBMBtrail after the member datawholegz.phar.gzwholebz2.phar.bz2tarbased.phar.tartargz.phar.tar.gzzipbased.phar.zipAll carry the same member set, so one set of expectations spans them.
They immediately found a real bug
A tar- or zip-based phar stores the stub as an ordinary member (
.phar/stub.php), so__HALT_COMPILER();appears inside a perfectly good tar. The reader committed on the token alone and then reported that tar as a corrupt phar - worse than not recognising it, because the walk reports an error instead of reading the archive libarchive handles fine. Detection now:Only the fixed header must be present: a plausible header with the file ending inside the declared manifest is still a truncated phar (
DataLoss), not "some other file". The byte-level lying-member-count case therefore moves fromDataLosstoInvalidArgument, with the reason recorded, and gains a stray-token-before-the-real-one case.Two current gaps are now pinned by tests rather than asserted in prose: a per-member compressed entry lists but refuses to read (
Unimplemented), and a whole-file compressed container is not recognised at all.Note on the BUILD wiring: the fixture test lives in an external bazel module, whose runfiles sit under a canonical repo directory (
xff_archive+) that no test should hard-code, so theenvattribute hands the test one fixture's$(rootpath)and it derives the directory from that.