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.
This commit is contained in:
@@ -79,8 +79,22 @@ landing after the format.
|
|||||||
version pair classifies `Readable`; for `TooNew` it stops at the semver —
|
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
|
don't "fix" it to read the manifest length there, a future structural format
|
||||||
may have moved it.
|
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
|
- The format carries no algorithm tag for `byteHash` — it is FNV-1a
|
||||||
(`capture::hashBytes`) implicitly. Changing the digest algorithm is a
|
(`capture::hashBytes`) implicitly. Changing the digest algorithm is a
|
||||||
`minReaderVersion` bump, not additive: an old reader would otherwise compare
|
`minReaderVersion` bump, not additive: an old reader would otherwise compare
|
||||||
a stored digest against bytes hashed the new way and silently misjudge
|
a stored digest against bytes hashed the new way and silently misjudge
|
||||||
corruption.
|
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).
|
||||||
|
|||||||
@@ -102,7 +102,13 @@ DecodedPackage decodePackage(const std::vector<std::uint8_t>& prefix,
|
|||||||
if (!r.ok) return dec;
|
if (!r.ok) return dec;
|
||||||
|
|
||||||
auto manifest = deserializeManifest(manifestJson);
|
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::uint64_t end = 0;
|
||||||
std::vector<PackageEntrySpan> layout;
|
std::vector<PackageEntrySpan> layout;
|
||||||
|
|||||||
@@ -43,8 +43,12 @@ bool PackageManifest::operator==(const PackageManifest& o) const {
|
|||||||
}
|
}
|
||||||
|
|
||||||
std::optional<std::string> serializeManifest(const PackageManifest& m) {
|
std::optional<std::string> serializeManifest(const PackageManifest& m) {
|
||||||
for (const auto& e : m.entries)
|
for (const auto& e : m.entries) {
|
||||||
if (!isValidEntryName(e.fileName)) return std::nullopt;
|
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;
|
if (duplicateName(m.entries)) return std::nullopt;
|
||||||
|
|
||||||
std::string out;
|
std::string out;
|
||||||
|
|||||||
@@ -41,9 +41,10 @@ struct PackageManifest {
|
|||||||
};
|
};
|
||||||
|
|
||||||
// Emits the manifest JSON. nullopt when the manifest cannot be represented:
|
// Emits the manifest JSON. nullopt when the manifest cannot be represented:
|
||||||
// an invalid or duplicate entry name, or a sample record BankModel itself would
|
// an invalid or duplicate entry name, a zero-length entry (see this
|
||||||
// reject (empty id, absolute path) — refusing on encode so an undecodable
|
// directory's CLAUDE.md — the shell's payload-append seam cannot round-trip
|
||||||
// package is never written.
|
// 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<std::string> serializeManifest(const PackageManifest& m);
|
std::optional<std::string> serializeManifest(const PackageManifest& m);
|
||||||
|
|
||||||
// Parses manifest JSON (nullopt on malformed input, never UB). Unknown keys are
|
// Parses manifest JSON (nullopt on malformed input, never UB). Unknown keys are
|
||||||
|
|||||||
@@ -110,7 +110,7 @@ static std::string handManifest(const std::string& name, int length,
|
|||||||
// --- encode / decode round trip ----------------------------------------------
|
// --- encode / decode round trip ----------------------------------------------
|
||||||
|
|
||||||
static void testEncodeDecodeRoundTrip() {
|
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);
|
auto enc = encodePackage(m);
|
||||||
CHECK(enc.has_value());
|
CHECK(enc.has_value());
|
||||||
|
|
||||||
@@ -120,8 +120,8 @@ static void testEncodeDecodeRoundTrip() {
|
|||||||
CHECK(enc->layout[0].offset == enc->prefix.size());
|
CHECK(enc->layout[0].offset == enc->prefix.size());
|
||||||
CHECK(enc->layout[0].length == 96000);
|
CHECK(enc->layout[0].length == 96000);
|
||||||
CHECK(enc->layout[1].offset == enc->prefix.size() + 96000);
|
CHECK(enc->layout[1].offset == enc->prefix.size() + 96000);
|
||||||
CHECK(enc->layout[1].length == 0);
|
CHECK(enc->layout[1].length == 48000);
|
||||||
CHECK(enc->totalSize == enc->prefix.size() + 96000);
|
CHECK(enc->totalSize == enc->prefix.size() + 96000 + 48000);
|
||||||
|
|
||||||
// decodePackage(encodePackage(x)) == x — payloads are never read by the
|
// decodePackage(encodePackage(x)) == x — payloads are never read by the
|
||||||
// codec, so the prefix plus the true total size is the whole input.
|
// 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);
|
PackageManifest m = fixture(1, 1);
|
||||||
m.entries[0].fileName = "../evil.wav";
|
m.entries[0].fileName = "../evil.wav";
|
||||||
CHECK(!encodePackage(m).has_value());
|
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 -------------------------------------------
|
// --- truncation: every byte offset -------------------------------------------
|
||||||
@@ -209,6 +216,30 @@ static void testTooNewProducesNoManifest() {
|
|||||||
CHECK(decodePackage(cut, cut.size()).status == PackageReadability::Malformed);
|
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<std::uint8_t> 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<std::uint8_t> sameVersion =
|
||||||
|
rawHeader(kPackageFormatVersion, kPackageMinReaderVersion, "1.0.0");
|
||||||
|
appendManifest(sameVersion, "not json");
|
||||||
|
CHECK(decodePackage(sameVersion, sameVersion.size()).status == PackageReadability::Malformed);
|
||||||
|
}
|
||||||
|
|
||||||
// --- version ladder: additive forward compatibility --------------------------
|
// --- version ladder: additive forward compatibility --------------------------
|
||||||
|
|
||||||
// The reason two integers exist: a NEWER formatVersion whose minReaderVersion
|
// The reason two integers exist: a NEWER formatVersion whose minReaderVersion
|
||||||
@@ -335,6 +366,7 @@ int main() {
|
|||||||
testEncodeRefusesWhatManifestRefuses();
|
testEncodeRefusesWhatManifestRefuses();
|
||||||
testTruncationAtEveryByteOffsetIsMalformed();
|
testTruncationAtEveryByteOffsetIsMalformed();
|
||||||
testTooNewProducesNoManifest();
|
testTooNewProducesNoManifest();
|
||||||
|
testAdditiveUnparseableManifestIsTooNew();
|
||||||
testNewerAdditiveFormatReads();
|
testNewerAdditiveFormatReads();
|
||||||
testHostileHeadersAreMalformed();
|
testHostileHeadersAreMalformed();
|
||||||
testRequiredPrefixSizeRefusals();
|
testRequiredPrefixSizeRefusals();
|
||||||
|
|||||||
@@ -64,7 +64,7 @@ static PackageManifest fixture() {
|
|||||||
m.bankDisplayName = "Drums \"live\""; // escaping exercised
|
m.bankDisplayName = "Drums \"live\""; // escaping exercised
|
||||||
m.exportTimestamp = 1754100000;
|
m.exportTimestamp = 1754100000;
|
||||||
m.entries.push_back({"kick.wav", 96000, "1111222233334444", fullSample()});
|
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-full");
|
||||||
m.slots.append("smp-bare");
|
m.slots.append("smp-bare");
|
||||||
m.slots.remove("smp-full"); // leaves a gap: slots round-trip must keep it
|
m.slots.remove("smp-full"); // leaves a gap: slots round-trip must keep it
|
||||||
@@ -173,6 +173,16 @@ static void testDuplicateEntriesKeyRejected() {
|
|||||||
|
|
||||||
// --- rejection: structural ---------------------------------------------------
|
// --- 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() {
|
static void testEncodeRejectsUnrepresentableSample() {
|
||||||
PackageManifest m = fixture();
|
PackageManifest m = fixture();
|
||||||
m.entries[0].sample.id.clear(); // BankModel::add rejects an empty id
|
m.entries[0].sample.id.clear(); // BankModel::add rejects an empty id
|
||||||
@@ -253,6 +263,7 @@ int main() {
|
|||||||
testDecodeRejectsBadEntryName();
|
testDecodeRejectsBadEntryName();
|
||||||
testDuplicateEntryNamesRejectedBothWays();
|
testDuplicateEntryNamesRejectedBothWays();
|
||||||
testDuplicateEntriesKeyRejected();
|
testDuplicateEntriesKeyRejected();
|
||||||
|
testEncodeRejectsZeroLengthEntry();
|
||||||
testEncodeRejectsUnrepresentableSample();
|
testEncodeRejectsUnrepresentableSample();
|
||||||
testDecodeRejectsMalformedShapes();
|
testDecodeRejectsMalformedShapes();
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user