From 1aebf519386710abcf58e653a475106e6d12ffde Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Sun, 2 Aug 2026 08:14:55 -0400 Subject: [PATCH] Relabel post-manifest-parse failure as TooNew; refuse zero-length package entries at encode An additively-tagged newer package that fails to parse now reports TooNew (with writer semver) instead of unactionable Malformed. Format layer also refuses encoding a zero-length entry, honoring the shell's appendPayload contract; both test-covered. --- src/core/package/CLAUDE.md | 14 ++++++++++ src/core/package/bank_package.cpp | 8 +++++- src/core/package/package_manifest.cpp | 6 ++++- src/core/package/package_manifest.h | 7 ++--- tests/test_bank_package.cpp | 38 ++++++++++++++++++++++++--- tests/test_package_manifest.cpp | 13 ++++++++- 6 files changed, 77 insertions(+), 9 deletions(-) diff --git a/src/core/package/CLAUDE.md b/src/core/package/CLAUDE.md index a8bfe62..373b512 100644 --- a/src/core/package/CLAUDE.md +++ b/src/core/package/CLAUDE.md @@ -79,8 +79,22 @@ landing after the format. version pair classifies `Readable`; for `TooNew` it stops at the semver — don't "fix" it to read the manifest length there, a future structural format may have moved it. +- A package whose header classifies `Readable` (fv > ours, minReader still + within reach — the additive case) but whose manifest fails to parse is + reported `TooNew`, not `Malformed`: the header is valid and already carries + the writer's semver, so the refusal can still name what to install. This + widens `TooNew` to cover "read and failed" as well as "stopped at the frozen + region" — both refuse whole and write nothing, so the safety property is + unchanged, only the message. `classifyPackageVersion` and the frozen-region + `TooNew` path are unaffected; this is the post-manifest-parse branch only. - The format carries no algorithm tag for `byteHash` — it is FNV-1a (`capture::hashBytes`) implicitly. Changing the digest algorithm is a `minReaderVersion` bump, not additive: an old reader would otherwise compare a stored digest against bytes hashed the new way and silently misjudge corruption. +- **Cross-module contract with `src/shell/package`:** a genuinely zero-length + entry cannot round-trip through the filesystem seam there (`appendPayload` + refuses an empty payload — an empty buffer signals an upstream read failure, + not a real entry). `serializeManifest` refuses a zero-length `PackageEntry` + at encode so this layer never produces one; decode does not enforce it (a + hostile/older package declaring one is not this track's concern). diff --git a/src/core/package/bank_package.cpp b/src/core/package/bank_package.cpp index b5a9389..9b0edbe 100644 --- a/src/core/package/bank_package.cpp +++ b/src/core/package/bank_package.cpp @@ -102,7 +102,13 @@ DecodedPackage decodePackage(const std::vector& prefix, if (!r.ok) return dec; auto manifest = deserializeManifest(manifestJson); - if (!manifest) return dec; + if (!manifest) { + // A newer additive format's parse failure reports TooNew, not the + // unactionable Malformed — the header (with the writer semver) is + // already valid here. See this directory's CLAUDE.md for the tradeoff. + if (formatVersion > kPackageFormatVersion) dec.status = PackageReadability::TooNew; + return dec; + } std::uint64_t end = 0; std::vector layout; diff --git a/src/core/package/package_manifest.cpp b/src/core/package/package_manifest.cpp index a4d4ba9..4740b3e 100644 --- a/src/core/package/package_manifest.cpp +++ b/src/core/package/package_manifest.cpp @@ -43,8 +43,12 @@ bool PackageManifest::operator==(const PackageManifest& o) const { } std::optional serializeManifest(const PackageManifest& m) { - for (const auto& e : m.entries) + for (const auto& e : m.entries) { if (!isValidEntryName(e.fileName)) return std::nullopt; + // Cross-module contract with src/shell/package — see this directory's + // CLAUDE.md. + if (e.byteLength == 0) return std::nullopt; + } if (duplicateName(m.entries)) return std::nullopt; std::string out; diff --git a/src/core/package/package_manifest.h b/src/core/package/package_manifest.h index 3afe70d..0b31ce0 100644 --- a/src/core/package/package_manifest.h +++ b/src/core/package/package_manifest.h @@ -41,9 +41,10 @@ struct PackageManifest { }; // Emits the manifest JSON. nullopt when the manifest cannot be represented: -// an invalid or duplicate entry name, or a sample record BankModel itself would -// reject (empty id, absolute path) — refusing on encode so an undecodable -// package is never written. +// an invalid or duplicate entry name, a zero-length entry (see this +// directory's CLAUDE.md — the shell's payload-append seam cannot round-trip +// one), or a sample record BankModel itself would reject (empty id, absolute +// path) — refusing on encode so an undecodable package is never written. std::optional serializeManifest(const PackageManifest& m); // Parses manifest JSON (nullopt on malformed input, never UB). Unknown keys are diff --git a/tests/test_bank_package.cpp b/tests/test_bank_package.cpp index 565ab7c..27f16af 100644 --- a/tests/test_bank_package.cpp +++ b/tests/test_bank_package.cpp @@ -110,7 +110,7 @@ static std::string handManifest(const std::string& name, int length, // --- encode / decode round trip ---------------------------------------------- static void testEncodeDecodeRoundTrip() { - const PackageManifest m = fixture(96000, 0); // a zero-length payload is legal + const PackageManifest m = fixture(96000, 48000); auto enc = encodePackage(m); CHECK(enc.has_value()); @@ -120,8 +120,8 @@ static void testEncodeDecodeRoundTrip() { CHECK(enc->layout[0].offset == enc->prefix.size()); CHECK(enc->layout[0].length == 96000); CHECK(enc->layout[1].offset == enc->prefix.size() + 96000); - CHECK(enc->layout[1].length == 0); - CHECK(enc->totalSize == enc->prefix.size() + 96000); + CHECK(enc->layout[1].length == 48000); + CHECK(enc->totalSize == enc->prefix.size() + 96000 + 48000); // decodePackage(encodePackage(x)) == x — payloads are never read by the // codec, so the prefix plus the true total size is the whole input. @@ -141,6 +141,13 @@ static void testEncodeRefusesWhatManifestRefuses() { PackageManifest m = fixture(1, 1); m.entries[0].fileName = "../evil.wav"; CHECK(!encodePackage(m).has_value()); + + // A zero-length entry cannot round-trip through the shell's filesystem + // seam (src/shell/package's appendPayload refuses an empty payload) — the + // format layer must never produce one. + PackageManifest zeroLen = fixture(1, 1); + zeroLen.entries[1].byteLength = 0; + CHECK(!encodePackage(zeroLen).has_value()); } // --- truncation: every byte offset ------------------------------------------- @@ -209,6 +216,30 @@ static void testTooNewProducesNoManifest() { CHECK(decodePackage(cut, cut.size()).status == PackageReadability::Malformed); } +// An additively-tagged package (fv > ours, minReader still within reach) whose +// manifest fails to parse: the header classifies Readable, so decode reads +// into the manifest and fails there. That failure must still report TooNew — +// the header is valid and already carries the writer's semver — not the +// unactionable Malformed a genuinely corrupt header produces. +static void testAdditiveUnparseableManifestIsTooNew() { + std::vector bytes = rawHeader(kPackageFormatVersion + 1, kPackageMinReaderVersion, "1.9.0"); + appendManifest(bytes, "not json"); + const DecodedPackage dec = decodePackage(bytes, bytes.size()); + CHECK(dec.status == PackageReadability::TooNew); + CHECK(dec.header.formatVersion == kPackageFormatVersion + 1); + CHECK(dec.header.minReaderVersion == kPackageMinReaderVersion); + CHECK(dec.header.writerVersion == "1.9.0"); + CHECK(dec.manifest.entries.empty()); + CHECK(dec.layout.empty()); + + // Same-version unparseable manifest stays Malformed: nothing "newer" + // excuses it, so this is not a blanket "unparseable == TooNew" rule. + std::vector sameVersion = + rawHeader(kPackageFormatVersion, kPackageMinReaderVersion, "1.0.0"); + appendManifest(sameVersion, "not json"); + CHECK(decodePackage(sameVersion, sameVersion.size()).status == PackageReadability::Malformed); +} + // --- version ladder: additive forward compatibility -------------------------- // The reason two integers exist: a NEWER formatVersion whose minReaderVersion @@ -335,6 +366,7 @@ int main() { testEncodeRefusesWhatManifestRefuses(); testTruncationAtEveryByteOffsetIsMalformed(); testTooNewProducesNoManifest(); + testAdditiveUnparseableManifestIsTooNew(); testNewerAdditiveFormatReads(); testHostileHeadersAreMalformed(); testRequiredPrefixSizeRefusals(); diff --git a/tests/test_package_manifest.cpp b/tests/test_package_manifest.cpp index 8d35475..e418ed7 100644 --- a/tests/test_package_manifest.cpp +++ b/tests/test_package_manifest.cpp @@ -64,7 +64,7 @@ static PackageManifest fixture() { m.bankDisplayName = "Drums \"live\""; // escaping exercised m.exportTimestamp = 1754100000; m.entries.push_back({"kick.wav", 96000, "1111222233334444", fullSample()}); - m.entries.push_back({"snare.wav", 0, "5555666677778888", bareSample()}); // 0-length legal + m.entries.push_back({"snare.wav", 48000, "5555666677778888", bareSample()}); m.slots.append("smp-full"); m.slots.append("smp-bare"); m.slots.remove("smp-full"); // leaves a gap: slots round-trip must keep it @@ -173,6 +173,16 @@ static void testDuplicateEntriesKeyRejected() { // --- rejection: structural --------------------------------------------------- +// A zero-length entry cannot round-trip through the shell's filesystem seam +// (src/shell/package's appendPayload refuses an empty payload) — the format +// layer must never produce one, so encode refuses it. Decode does not enforce +// this (a hostile/older package declaring one is not this codec's concern). +static void testEncodeRejectsZeroLengthEntry() { + PackageManifest m = fixture(); + m.entries[0].byteLength = 0; + CHECK(!serializeManifest(m).has_value()); +} + static void testEncodeRejectsUnrepresentableSample() { PackageManifest m = fixture(); m.entries[0].sample.id.clear(); // BankModel::add rejects an empty id @@ -253,6 +263,7 @@ int main() { testDecodeRejectsBadEntryName(); testDuplicateEntryNamesRejectedBothWays(); testDuplicateEntriesKeyRejected(); + testEncodeRejectsZeroLengthEntry(); testEncodeRejectsUnrepresentableSample(); testDecodeRejectsMalformedShapes();