From 9cb4bad1bdc4aac21700656a2f434867561e859a Mon Sep 17 00:00:00 2001 From: Mura Li <2606021+typeless@users.noreply.github.com> Date: Mon, 17 Aug 2026 10:28:19 +0800 Subject: [PATCH 1/2] Reject a recorded type byte that names no type MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A persisted entry type was cast to NodeType and a persisted edge type to LinkType without checking either against its enum. NodeType names 0-10, so a recorded byte of 11-255 became a well-defined value no enumerator names; it then fell through prior_paths' deliberately default-less switch into none of sources, generated, or unowned, and fed the overwrite guard as a path classified as nothing. The switch's totality comment was true of the switch and false of the function. The edge side was the same shape with a worse tail: LinkType names 1-5 and has no zero enumerator, so both 0 and anything above 5 produced LinkRole::Unknown, which joins no mask and therefore routes nothing — a recorded dependency silently ceasing to route is the failure class the incremental campaign exists to prevent. An unnameable type byte is damage, not a value from a future version. Tolerating it has no disposition a reader can implement: dropping the entry is the silent-nothing state itself, and substituting a classification is the substitution this area already forbids. Forward compatibility does not apply either, because the readable window's floor admits only versions whose type vocabulary is a prefix of this one's. Enforce at the cast site so no entry carrying an unnameable type is ever constructed: FileEntry::from_raw and EdgeEntry::from_raw now return Result and report IndexDamaged, which the announcement side already covers. Observers are untouched, no default: arm is added anywhere, and the totality comment stands unchanged because the fix makes it true. names_node_type switches exhaustively with no default, so adding a NodeType enumerator fails -Wswitch rather than silently widening what the reader accepts. names_link_type delegates to link_role instead of restating the valid range, keeping one source of truth for which link types exist. The recovery read (read_prior_paths, #291) reports the record as lost on a damaged byte rather than proceeding with a partial table — no prior knowledge is strictly safer than knowledge that omits entries. Verified: RED quoted for both legs before any production code. build, format, tidy, iwyu, spec-check all exit 0; [index] tag 0 with 7356 assertions. The full suite is green when carried onto the #397 base; on main it fails only in test_builder.cpp:87, which makes scratch under a read-only /tmp on this machine and is exactly what PR #406 fixes. This change does not touch that file. Fixes #399 --- include/pup/core/types.hpp | 27 ++++++ include/pup/index/entry.hpp | 7 +- spec/requirements/record-read.ears.md | 17 ++++ src/index/entry.cpp | 10 +- src/index/reader.cpp | 18 +++- test/unit/test_index.cpp | 126 +++++++++++++++++++++++--- 6 files changed, 185 insertions(+), 20 deletions(-) diff --git a/include/pup/core/types.hpp b/include/pup/core/types.hpp index 8a28f5bc..8004ad7c 100644 --- a/include/pup/core/types.hpp +++ b/include/pup/core/types.hpp @@ -161,6 +161,33 @@ constexpr auto link_role(LinkType type) -> LinkRole return LinkRole::Unknown; } +/// A persisted type byte outside its enum is damage, not a value from a future version (#399). +[[nodiscard]] +constexpr auto names_node_type(std::uint8_t value) -> bool +{ + switch (static_cast(value)) { + case NodeType::File: + case NodeType::Command: + case NodeType::Directory: + case NodeType::Variable: + case NodeType::Generated: + case NodeType::Ghost: + case NodeType::Group: + case NodeType::GeneratedDir: + case NodeType::Root: + case NodeType::Condition: + case NodeType::Phi: + return true; + } + return false; +} + +[[nodiscard]] +constexpr auto names_link_type(std::uint8_t value) -> bool +{ + return link_role(static_cast(value)) != LinkRole::Unknown; +} + /// One command that must run before another, on evidence no edge in the graph carries. A /// discovered dependency is recorded only in the index, so both the router that decides who runs /// and the scheduler that decides in what order have to be told about it separately (#276, #277). diff --git a/include/pup/index/entry.hpp b/include/pup/index/entry.hpp index cf26be7e..5705ab55 100644 --- a/include/pup/index/entry.hpp +++ b/include/pup/index/entry.hpp @@ -6,6 +6,7 @@ #include "format.hpp" #include "pup/core/arena.hpp" #include "pup/core/node_id_map.hpp" +#include "pup/core/result.hpp" #include "pup/core/string_id.hpp" #include "pup/core/types.hpp" #include "pup/core/vec.hpp" @@ -35,12 +36,13 @@ struct FileEntry { /// Create from raw format (path must be computed separately from parent chain) /// @param array_index 0-based position in file array (ID = array_index + 1) + /// @return IndexDamaged if the recorded type byte names no NodeType this build knows [[nodiscard]] static auto from_raw( RawFileEntry const& raw, std::string_view name_str, std::size_t array_index - ) -> FileEntry; + ) -> Result; }; /// In-memory command entry (v8) @@ -96,8 +98,9 @@ struct EdgeEntry { auto to_raw() const -> RawEdge; /// Create from raw format + /// @return IndexDamaged if the recorded type byte names no LinkType this build knows [[nodiscard]] - static auto from_raw(RawEdge const& raw) -> EdgeEntry; + static auto from_raw(RawEdge const& raw) -> Result; }; /// Complete in-memory index diff --git a/spec/requirements/record-read.ears.md b/spec/requirements/record-read.ears.md index c4fe8dda..dae3254c 100644 --- a/spec/requirements/record-read.ears.md +++ b/spec/requirements/record-read.ears.md @@ -17,6 +17,12 @@ downstream code acts on — the first feeds change detection, the second the pat so a validation failure makes the record unreadable rather than weaker. `DESIGN.md`'s "What a record claims" carries the rule and its display-side counterpart. +A recorded type byte that names no enumerator is the same class of failure. Tolerating it has no +disposition a reader can implement: dropping the entry is the silent-nothing state — an edge that +joins no mask routes nothing — and substituting a classification is the substitution this area +already forbids. Nor is it forward compatibility, because the readable window's floor admits only +versions whose type vocabulary is a prefix of this one's. + Two boundaries keep that from over-reaching. Rejection follows a failed check, never a value: the string table's offset 0 is a legitimate empty string and reads as one. And each reader validates what it reads, so the recovery read of the file table (`read_prior_paths`, issue #291) is unaffected @@ -42,6 +48,17 @@ What a failed validation does to the record. If a semantics-bearing field's declared position fails its bounds check, then putup shall report the record as unreadable rather than returning an empty value in that field's place. +### REQ-READ-REJECT-UNKNOWN-TYPE + +- conformance: putup-only +- discharge: test "A record whose entry carries a type this putup cannot name is unreadable" +- discharge: test "A record whose edge carries a link type this putup cannot name is unreadable" +- discharge: test "A record whose edge carries link type zero is unreadable" + +If a recorded entry or edge carries a type byte naming no node or link type this putup knows, then +putup shall report the record as unreadable rather than admitting an entry whose type no observer +classifies. + ### REQ-READ-REJECT-SELF-CONTRADICTION - conformance: putup-only diff --git a/src/index/entry.cpp b/src/index/entry.cpp index e35bc638..9e811948 100644 --- a/src/index/entry.cpp +++ b/src/index/entry.cpp @@ -40,8 +40,11 @@ auto FileEntry::from_raw( RawFileEntry const& raw, std::string_view name_str, std::size_t array_index -) -> FileEntry +) -> Result { + if (!names_node_type(raw.type)) { + return make_error(ErrorCode::IndexDamaged, "Recorded entry type names no node type"); + } return FileEntry { .id = static_cast(array_index + 1), .parent_id = raw.parent_id, @@ -107,8 +110,11 @@ auto EdgeEntry::to_raw() const -> RawEdge }; } -auto EdgeEntry::from_raw(RawEdge const& raw) -> EdgeEntry +auto EdgeEntry::from_raw(RawEdge const& raw) -> Result { + if (!names_link_type(raw.type)) { + return make_error(ErrorCode::IndexDamaged, "Recorded edge type names no link type"); + } return EdgeEntry { .from = raw.from_id, .to = raw.to_id, diff --git a/src/index/reader.cpp b/src/index/reader.cpp index 05e00670..33401ec3 100644 --- a/src/index/reader.cpp +++ b/src/index/reader.cpp @@ -146,7 +146,11 @@ auto read_index(IndexFile const& f) -> Result if (!name) { return pup::unexpected(name.error()); } - index.add_file(FileEntry::from_raw(raw, *name, i)); + auto entry = FileEntry::from_raw(raw, *name, i); + if (!entry) { + return pup::unexpected(entry.error()); + } + index.add_file(*entry); } // Compute paths from parent chain (after all files loaded) @@ -174,7 +178,11 @@ auto read_index(IndexFile const& f) -> Result // Read edges auto edges = index_raw_edges(f); for (auto const& raw : edges) { - index.add_edge(EdgeEntry::from_raw(raw)); + auto edge = EdgeEntry::from_raw(raw); + if (!edge) { + return pup::unexpected(edge.error()); + } + index.add_edge(*edge); } // Build edge indices for O(1) lookup @@ -282,7 +290,11 @@ auto read_prior_paths(std::string_view path) -> PriorPaths if (!name) { return lost; } - recorded.add_file(FileEntry::from_raw(raw[i], *name, i)); + auto entry = FileEntry::from_raw(raw[i], *name, i); + if (!entry) { + return lost; + } + recorded.add_file(*entry); } recorded.compute_paths(); diff --git a/test/unit/test_index.cpp b/test/unit/test_index.cpp index fa35c6a5..858e494f 100644 --- a/test/unit/test_index.cpp +++ b/test/unit/test_index.cpp @@ -153,14 +153,15 @@ TEST_CASE("FileEntry conversion", "[index]") // ID is computed from array index (41 + 1 = 42) auto restored = FileEntry::from_raw(raw, "main.cpp", 41); + REQUIRE(restored.has_value()); - REQUIRE(restored.id == 42); - REQUIRE(restored.parent_id == file.parent_id); - REQUIRE(restored.type == file.type); - REQUIRE(restored.flags == file.flags); - REQUIRE(restored.name == file.name); - REQUIRE(restored.size == file.size); - REQUIRE(restored.content_hash == file.content_hash); + REQUIRE(restored->id == 42); + REQUIRE(restored->parent_id == file.parent_id); + REQUIRE(restored->type == file.type); + REQUIRE(restored->flags == file.flags); + REQUIRE(restored->name == file.name); + REQUIRE(restored->size == file.size); + REQUIRE(restored->content_hash == file.content_hash); } TEST_CASE("CommandEntry conversion", "[index]") @@ -232,10 +233,11 @@ TEST_CASE("EdgeEntry conversion", "[index]") REQUIRE(raw.type == static_cast(LinkType::Sticky)); auto restored = EdgeEntry::from_raw(raw); + REQUIRE(restored.has_value()); - REQUIRE(restored.from == edge.from); - REQUIRE(restored.to == edge.to); - REQUIRE(restored.type == edge.type); + REQUIRE(restored->from == edge.from); + REQUIRE(restored->to == edge.to); + REQUIRE(restored->type == edge.type); } SECTION("Implicit edge (header dependency)") @@ -253,10 +255,11 @@ TEST_CASE("EdgeEntry conversion", "[index]") REQUIRE(raw.type == static_cast(LinkType::Implicit)); auto restored = EdgeEntry::from_raw(raw); + REQUIRE(restored.has_value()); - REQUIRE(restored.from == edge.from); - REQUIRE(restored.to == edge.to); - REQUIRE(restored.type == LinkType::Implicit); + REQUIRE(restored->from == edge.from); + REQUIRE(restored->to == edge.to); + REQUIRE(restored->type == LinkType::Implicit); } } @@ -1764,6 +1767,103 @@ TEST_CASE("A name offset that wraps makes the record unreadable", "[index]") std::filesystem::remove(path); } +TEST_CASE("A record whose entry carries a type this putup cannot name is unreadable", "[index]") +{ + auto index = Index {}; + index.add_file(FileEntry { .id = 1, .parent_id = 0, .type = NodeType::File, .name = intern("main.c") }); + + auto data = serialize_index(index); + REQUIRE(data.has_value()); + + auto bytes = *data; + // NOLINTNEXTLINE(cppcoreguidelines-pro-type-reinterpret-cast) + auto const* hdr = reinterpret_cast(bytes.data()); + REQUIRE(hdr->file_count > 0); + auto constexpr UNNAMEABLE_TYPE = static_cast(static_cast(NodeType::Phi) + 1); + bytes[hdr->file_offset + offsetof(RawFileEntry, type)] = std::byte { UNNAMEABLE_TYPE }; + stamp_version(bytes, INDEX_VERSION); + + auto const path = temp_index_path("pup_unnameable_node_type"); + write_bytes(path, bytes); + + auto opened = open_index(path); + REQUIRE(opened.has_value()); + auto restored = read_index(*opened); + REQUIRE_FALSE(restored.has_value()); + REQUIRE(restored.error().code == ErrorCode::IndexDamaged); + + // The recovery read builds the same entries, so it must not answer for this record either. + opened->file.close(); + REQUIRE(read_prior_paths(path).kind == PriorPaths::Kind::Lost); + + std::filesystem::remove(path); +} + +TEST_CASE("A record whose edge carries a link type this putup cannot name is unreadable", "[index]") +{ + auto index = Index {}; + index.add_file(FileEntry { .id = 1, .parent_id = 0, .type = NodeType::File, .name = intern("main.c") }); + index.add_file(FileEntry { .id = 2, .parent_id = 0, .type = NodeType::Generated, .name = intern("main.o") }); + index.add_edge(EdgeEntry { .from = 1, .to = 2, .type = LinkType::Normal }); + + auto data = serialize_index(index); + REQUIRE(data.has_value()); + + auto bytes = *data; + // NOLINTNEXTLINE(cppcoreguidelines-pro-type-reinterpret-cast) + auto const* hdr = reinterpret_cast(bytes.data()); + REQUIRE(hdr->edge_count > 0); + auto constexpr UNNAMEABLE_LINK = static_cast(static_cast(LinkType::OrderOnly) + 1); + bytes[hdr->edge_offset + offsetof(RawEdge, type)] = std::byte { UNNAMEABLE_LINK }; + stamp_version(bytes, INDEX_VERSION); + + auto const path = temp_index_path("pup_unnameable_link_type"); + write_bytes(path, bytes); + + auto opened = open_index(path); + REQUIRE(opened.has_value()); + auto restored = read_index(*opened); + REQUIRE_FALSE(restored.has_value()); + REQUIRE(restored.error().code == ErrorCode::IndexDamaged); + + // Damage in a section the recovery read never looks at leaves the recorded paths readable. + opened->file.close(); + REQUIRE(read_prior_paths(path).kind == PriorPaths::Kind::Known); + + std::filesystem::remove(path); +} + +TEST_CASE("A record whose edge carries link type zero is unreadable", "[index]") +{ + auto index = Index {}; + index.add_file(FileEntry { .id = 1, .parent_id = 0, .type = NodeType::File, .name = intern("main.c") }); + index.add_file(FileEntry { .id = 2, .parent_id = 0, .type = NodeType::Generated, .name = intern("main.o") }); + index.add_edge(EdgeEntry { .from = 1, .to = 2, .type = LinkType::Normal }); + + auto data = serialize_index(index); + REQUIRE(data.has_value()); + + auto bytes = *data; + // NOLINTNEXTLINE(cppcoreguidelines-pro-type-reinterpret-cast) + auto const* hdr = reinterpret_cast(bytes.data()); + REQUIRE(hdr->edge_count > 0); + // LinkType starts at 1, so zero is as unnameable as anything past the last enumerator. + bytes[hdr->edge_offset + offsetof(RawEdge, type)] = std::byte { 0 }; + stamp_version(bytes, INDEX_VERSION); + + auto const path = temp_index_path("pup_zero_link_type"); + write_bytes(path, bytes); + + auto opened = open_index(path); + REQUIRE(opened.has_value()); + auto restored = read_index(*opened); + REQUIRE_FALSE(restored.has_value()); + REQUIRE(restored.error().code == ErrorCode::IndexDamaged); + + opened->file.close(); + std::filesystem::remove(path); +} + /// Whole-record rejection is safe only because each reader's validation scope is its read scope: /// the recovery slice reads names, so operand damage elsewhere leaves it Known (#291). TEST_CASE("A corrupt operand record still leaves the recorded paths readable", "[index]") From a611bb42d5379394a6545c5ecfcd2d1c6f52de13 Mon Sep 17 00:00:00 2001 From: Mura Li <2606021+typeless@users.noreply.github.com> Date: Mon, 17 Aug 2026 12:31:57 +0800 Subject: [PATCH 2/2] Reject a recorded flags word carrying a bit no flag names The persisted node and command flags words were read back with no check against their vocabularies. NodeFlags names bits 0-4 and CommandFlag bit 0, but get_node_flags reconstructs the whole 16-bit word and set_node_flags writes the whole word back, neither masked -- so a garbage high bit read today is written into a fresh, re-checksummed record and thereafter looks authentic. An unnamed bit is not inert; it carries. Openness buys nothing to offset that: read_index opens at exactly INDEX_VERSION, so no record it reads can carry a legitimately newer flag vocabulary. Validate at the cast site, where the type bytes are already validated: FileEntry::from_raw rejects a node flags word with a bit outside the readable mask, and CommandEntry::from_raw -- now returning Result -- rejects a command flags word the same way. Neither mask is written out. RECORDED_NODE_FLAGS_MASK is derived by a constexpr bit sweep over an exhaustive is_recorded_node_flag switch with no default arm, so a new flag that forgets the mask is a -Wswitch build error rather than a reader that silently rejects a legitimate record. The command mask derives from the existing flag_category for the same reason, minting no second classifier; that made flag_category's "nothing calls this" comment false, so it is gone. The vocabulary a word is judged against spans every version the readable window admits, uniformly rather than per-version -- the check takes no version and branches on none. NodeFlags::Inactive held bit 5 through index version 13 and was retired without a format change, and read_prior_paths reads back to INDEX_LAYOUT_FLOOR, so bit 5 stays readable: validating against today's live mask alone turns a legitimate v9-v13 record into Lost. A read word is also carried whole into the next record written, so a current-version record holding a retired bit is an honest descendant of a pre-retirement one rather than damage. Bit 5 is now a hole, and a sixth flag would naturally take it -- at which point an old record's stale Inactive bit would silently mean the new flag, which is misinterpretation rather than the loud over-rejection this fix started from. A disjointness static_assert makes reassigning a retired bit a build error, and the value pin's message routes a future retirement into the retired mask instead of letting the bit vanish. Both from_raw doc headers now state their rejection contracts, which neither did for the failure modes this change gives them: FileEntry's named only the type byte, and CommandEntry's said nothing at all on a function that had just gained a failure mode. Verified: RED quoted for both legs before any production code; the retired bit is pinned by a test proved to fail against the live-mask-only shape, and the disjointness assert was proved to fire by reassigning a retired bit. build, format, tidy, iwyu, spec-check all exit 0; whole suite green in one process, 170265 assertions in 841 test cases. --- include/pup/core/types.hpp | 4 + include/pup/index/entry.hpp | 6 +- include/pup/index/format.hpp | 71 ++++++++++++++++- spec/requirements/record-read.ears.md | 38 +++++---- src/index/entry.cpp | 9 ++- src/index/reader.cpp | 8 +- test/unit/test_index.cpp | 106 +++++++++++++++++++++++--- 7 files changed, 212 insertions(+), 30 deletions(-) diff --git a/include/pup/core/types.hpp b/include/pup/core/types.hpp index 8004ad7c..01cd3a5d 100644 --- a/include/pup/core/types.hpp +++ b/include/pup/core/types.hpp @@ -144,6 +144,8 @@ enum class LinkRole : std::uint8_t { /// The one place a new LinkType must be classified: no `default`, so -Wswitch makes /// omitting one a build error here instead of a silent exclusion from every mask. +/// The recovery read consumes this across the whole readable window: retiring an enumerator without +/// a format bump would need the retired set NodeFlags has and this does not (#399). [[nodiscard]] constexpr auto link_role(LinkType type) -> LinkRole { @@ -162,6 +164,8 @@ constexpr auto link_role(LinkType type) -> LinkRole } /// A persisted type byte outside its enum is damage, not a value from a future version (#399). +/// The recovery read consumes this across the whole readable window: retiring an enumerator without +/// a format bump would need the retired set NodeFlags has and this does not (#399). [[nodiscard]] constexpr auto names_node_type(std::uint8_t value) -> bool { diff --git a/include/pup/index/entry.hpp b/include/pup/index/entry.hpp index 5705ab55..e9824361 100644 --- a/include/pup/index/entry.hpp +++ b/include/pup/index/entry.hpp @@ -36,7 +36,8 @@ struct FileEntry { /// Create from raw format (path must be computed separately from parent chain) /// @param array_index 0-based position in file array (ID = array_index + 1) - /// @return IndexDamaged if the recorded type byte names no NodeType this build knows + /// @return IndexDamaged if the recorded type byte names no NodeType this build knows, or the + /// recorded flags word carries a bit no version in the readable window names [[nodiscard]] static auto from_raw( RawFileEntry const& raw, @@ -75,6 +76,7 @@ struct CommandEntry { /// Create from raw format /// @param array_index 0-based position in command array (ID = node_id::make_command(array_index + 1)) + /// @return IndexDamaged if the recorded flags word carries a bit no CommandFlag names [[nodiscard]] static auto from_raw( RawCommandEntry const& raw, @@ -84,7 +86,7 @@ struct CommandEntry { Vec inputs, Vec outputs, std::size_t array_index - ) -> CommandEntry; + ) -> Result; }; /// In-memory edge diff --git a/include/pup/index/format.hpp b/include/pup/index/format.hpp index e334c683..008e76d9 100644 --- a/include/pup/index/format.hpp +++ b/include/pup/index/format.hpp @@ -180,9 +180,22 @@ constexpr auto flag_category(CommandFlag flag) -> std::string_view return {}; } -// Nothing calls flag_category — the switch is the point, so this is what keeps it compiled. static_assert(!flag_category(CommandFlag::MustRerun).empty(), "a flag names a category or it is not a category"); +/// Which bits a recorded command flags word may carry. Derived from `flag_category` rather than +/// written out, so a new flag widens what the reader accepts by the same answer that names it. +/// No retired term, unlike the node word: commands are read only at exact version (#399). +inline constexpr auto RECORDED_COMMAND_FLAGS_MASK = [] { + auto mask = std::uint32_t { 0 }; + for (auto bit = 0; bit < 32; ++bit) { + auto const flag = static_cast(std::uint32_t { 1 } << bit); + if (!flag_category(flag).empty()) { + mask |= static_cast(flag); + } + } + return mask; +}(); + [[nodiscard]] constexpr auto to_underlying(CommandFlag flag) -> std::uint32_t { @@ -228,6 +241,62 @@ struct alignas(8) RawFooter { static_assert(sizeof(RawFooter) == 32, "RawFooter must be 32 bytes"); +/// The one place a new NodeFlags bit must be answered for: -Wswitch makes an unlisted enumerator a +/// build error, and the reader's validity mask is derived from this switch, so a bit cannot reach +/// the record without joining it (#399). Single-bit enumerators only: a composite convenience +/// spelling, if one is ever added, classifies false. The mask derivation below is what keeps this +/// switch compiled, so it needs no keep-compiled static_assert of its own like `flag_category`'s. +[[nodiscard]] +constexpr auto is_recorded_node_flag(NodeFlags flag) -> bool +{ + switch (flag) { + case NodeFlags::Modified: + case NodeFlags::Created: + case NodeFlags::AbsenceRouted: + case NodeFlags::ConfigDep: + case NodeFlags::Transient: + return true; + case NodeFlags::None: + return false; + } + return false; +} + +inline constexpr auto RECORDED_NODE_FLAGS_MASK = [] { + auto mask = std::uint16_t { 0 }; + for (auto bit = 0; bit < 16; ++bit) { + auto const flag = static_cast(std::uint16_t { 1 } << bit); + if (is_recorded_node_flag(flag)) { + mask |= static_cast(flag); + } + } + return mask; +}(); + +static_assert( + RECORDED_NODE_FLAGS_MASK == 0x1F, + "update deliberately with the enum; this is the on-disk vocabulary, not a checksum — and a flag " + "retired without a format bump MOVES its bit to RETIRED_NODE_FLAGS_MASK rather than vanishing: " + "the recovery read reaches back to INDEX_LAYOUT_FLOOR, so records that carried the bit are still " + "readable (#399, fa5deb220)" +); + +/// Bit 5 was NodeFlags::Inactive until it was retired without a format change (fa5deb220, index +/// version 13), and the recovery read reaches back to INDEX_LAYOUT_FLOOR, so a record that carries +/// it is old rather than damaged. A retired bit is history and cannot grow by being forgotten. +inline constexpr auto RETIRED_NODE_FLAGS_MASK = std::uint16_t { 1 << 5 }; + +static_assert( + (RECORDED_NODE_FLAGS_MASK & RETIRED_NODE_FLAGS_MASK) == 0, + "a retired bit cannot be reassigned while INDEX_LAYOUT_FLOOR admits records that carried it: an " + "old record's stale bit would be read as the new flag's meaning. Reuse the bit only after raising " + "the floor past the retirement." +); + +/// Which bits a recorded node flags word may carry, over every version the readers accept. +inline constexpr auto READABLE_NODE_FLAGS_MASK + = static_cast(RECORDED_NODE_FLAGS_MASK | RETIRED_NODE_FLAGS_MASK); + /// Helper to get NodeFlags from entry [[nodiscard]] inline auto get_node_flags(RawFileEntry const& entry) -> NodeFlags diff --git a/spec/requirements/record-read.ears.md b/spec/requirements/record-read.ears.md index dae3254c..e4c3c7e9 100644 --- a/spec/requirements/record-read.ears.md +++ b/spec/requirements/record-read.ears.md @@ -17,17 +17,24 @@ downstream code acts on — the first feeds change detection, the second the pat so a validation failure makes the record unreadable rather than weaker. `DESIGN.md`'s "What a record claims" carries the rule and its display-side counterpart. -A recorded type byte that names no enumerator is the same class of failure. Tolerating it has no -disposition a reader can implement: dropping the entry is the silent-nothing state — an edge that -joins no mask routes nothing — and substituting a classification is the substitution this area -already forbids. Nor is it forward compatibility, because the readable window's floor admits only -versions whose type vocabulary is a prefix of this one's. - -Two boundaries keep that from over-reaching. Rejection follows a failed check, never a value: the -string table's offset 0 is a legitimate empty string and reads as one. And each reader validates -what it reads, so the recovery read of the file table (`read_prior_paths`, issue #291) is unaffected -by damage in sections it never looks at — which is what makes whole-record rejection safe rather -than a wider outage than the damage warrants. +A recorded value naming nothing in its enum or flag vocabulary — an entry or edge type byte, an +entry or command flags word — is the same class of failure. Tolerating it has no disposition a +reader can implement: dropping the entry is the silent-nothing state — an edge that joins no mask +routes nothing — and substituting a classification is the substitution this area already forbids. +Nor is a value the reader merely ignores harmless, because a read word is written back into a fresh +record and thereafter looks authentic. + +Three boundaries keep that from over-reaching. Rejection follows a failed check, never a value: the +string table's offset 0 is a legitimate empty string and reads as one. Each reader validates what it +reads, so the recovery read of the file table (`read_prior_paths`, issue #291) is unaffected by +damage in sections it never looks at — which is what makes whole-record rejection safe rather than a +wider outage than the damage warrants. And the vocabulary a value is judged against spans every +version the readable window admits, not today's alone: type vocabularies only ever grew, so the +floor admits only prefixes of this one's, but a flag bit retired without a format change +(`NodeFlags` bit 5, index version 13) was legitimate when written and stays readable. That span is +uniform rather than per-version, because a read word is carried whole into the record the next +build writes — so a current-version record holding a retired bit is an honest descendant of one +that predates the retirement, and judging it by today's vocabulary alone would reject its lineage. Upstream tup keeps its state in a database and delegates this class of decision to SQLite, so it has no counterpart and every requirement here is `putup-only`. @@ -48,15 +55,18 @@ What a failed validation does to the record. If a semantics-bearing field's declared position fails its bounds check, then putup shall report the record as unreadable rather than returning an empty value in that field's place. -### REQ-READ-REJECT-UNKNOWN-TYPE +### REQ-READ-REJECT-UNKNOWN-VALUE - conformance: putup-only - discharge: test "A record whose entry carries a type this putup cannot name is unreadable" - discharge: test "A record whose edge carries a link type this putup cannot name is unreadable" - discharge: test "A record whose edge carries link type zero is unreadable" +- discharge: test "A record whose entry carries a flag bit this putup cannot name is unreadable" +- discharge: test "A record whose command carries a flag bit this putup cannot name is unreadable" +- discharge: test "A record carrying a flag bit retired since it was written stays readable" -If a recorded entry or edge carries a type byte naming no node or link type this putup knows, then -putup shall report the record as unreadable rather than admitting an entry whose type no observer +If a recorded entry, edge, or command carries a value no version in the readable window defines, +then putup shall report the record as unreadable rather than admitting a value no observer classifies. ### REQ-READ-REJECT-SELF-CONTRADICTION diff --git a/src/index/entry.cpp b/src/index/entry.cpp index 9e811948..fb219d50 100644 --- a/src/index/entry.cpp +++ b/src/index/entry.cpp @@ -45,6 +45,10 @@ auto FileEntry::from_raw( if (!names_node_type(raw.type)) { return make_error(ErrorCode::IndexDamaged, "Recorded entry type names no node type"); } + auto const raw_flags = static_cast(get_node_flags(raw)); + if ((raw_flags & ~READABLE_NODE_FLAGS_MASK) != 0) { + return make_error(ErrorCode::IndexDamaged, "Recorded entry flags carry a bit no flag names"); + } return FileEntry { .id = static_cast(array_index + 1), .parent_id = raw.parent_id, @@ -84,8 +88,11 @@ auto CommandEntry::from_raw( Vec inputs, Vec outputs, std::size_t array_index -) -> CommandEntry +) -> Result { + if ((raw.flags & ~RECORDED_COMMAND_FLAGS_MASK) != 0U) { + return make_error(ErrorCode::IndexDamaged, "Recorded command flags carry a bit no flag names"); + } return CommandEntry { .id = node_id::make_command(array_index + 1), .dir_id = raw.dir_id, diff --git a/src/index/reader.cpp b/src/index/reader.cpp index 33401ec3..b63165d5 100644 --- a/src/index/reader.cpp +++ b/src/index/reader.cpp @@ -170,9 +170,13 @@ auto read_index(IndexFile const& f) -> Result if (!operands) { return pup::unexpected(operands.error()); } - index.add_command(CommandEntry::from_raw( + auto command = CommandEntry::from_raw( raw, instruction_pattern, display, *env, std::move(operands->first), std::move(operands->second), i - )); + ); + if (!command) { + return pup::unexpected(command.error()); + } + index.add_command(std::move(*command)); } // Read edges diff --git a/test/unit/test_index.cpp b/test/unit/test_index.cpp index 858e494f..5f688035 100644 --- a/test/unit/test_index.cpp +++ b/test/unit/test_index.cpp @@ -204,16 +204,18 @@ TEST_CASE("CommandEntry conversion", "[index]") pup::Vec { 10 }, pup::Vec { 20 }, 4 ); - REQUIRE(restored.id == node_id::make_command(5)); - REQUIRE(restored.dir_id == cmd.dir_id); - REQUIRE(restored.instruction_pattern == cmd.instruction_pattern); - REQUIRE(restored.display == cmd.display); - REQUIRE(restored.env == cmd.env); - REQUIRE(restored.key == key); - REQUIRE(restored.signature == signature); - REQUIRE(restored.must_rerun); - REQUIRE(restored.inputs == cmd.inputs); - REQUIRE(restored.outputs == cmd.outputs); + REQUIRE(restored.has_value()); + + REQUIRE(restored->id == node_id::make_command(5)); + REQUIRE(restored->dir_id == cmd.dir_id); + REQUIRE(restored->instruction_pattern == cmd.instruction_pattern); + REQUIRE(restored->display == cmd.display); + REQUIRE(restored->env == cmd.env); + REQUIRE(restored->key == key); + REQUIRE(restored->signature == signature); + REQUIRE(restored->must_rerun); + REQUIRE(restored->inputs == cmd.inputs); + REQUIRE(restored->outputs == cmd.outputs); } TEST_CASE("EdgeEntry conversion", "[index]") @@ -1864,6 +1866,90 @@ TEST_CASE("A record whose edge carries link type zero is unreadable", "[index]") std::filesystem::remove(path); } +TEST_CASE("A record whose entry carries a flag bit this putup cannot name is unreadable", "[index]") +{ + auto index = Index {}; + index.add_file(FileEntry { .id = 1, .parent_id = 0, .type = NodeType::File, .name = intern("main.c") }); + + auto data = serialize_index(index); + REQUIRE(data.has_value()); + + auto bytes = *data; + // NOLINTNEXTLINE(cppcoreguidelines-pro-type-reinterpret-cast) + auto const* hdr = reinterpret_cast(bytes.data()); + REQUIRE(hdr->file_count > 0); + bytes[hdr->file_offset + offsetof(RawFileEntry, flags_high)] = std::byte { 0x80 }; + stamp_version(bytes, INDEX_VERSION); + + auto const path = temp_index_path("pup_unnameable_node_flag"); + write_bytes(path, bytes); + + auto opened = open_index(path); + REQUIRE(opened.has_value()); + auto restored = read_index(*opened); + REQUIRE_FALSE(restored.has_value()); + REQUIRE(restored.error().code == ErrorCode::IndexDamaged); + + opened->file.close(); + std::filesystem::remove(path); +} + +/// NodeFlags::Inactive occupied bit 5 through index version 13 and was retired without a format +/// change, so records inside the recovery read's window still carry it. +TEST_CASE("A record carrying a flag bit retired since it was written stays readable", "[index]") +{ + auto index = Index {}; + index.add_file(FileEntry { .id = 1, .parent_id = 0, .type = NodeType::File, .name = intern("main.c") }); + + auto data = serialize_index(index); + REQUIRE(data.has_value()); + + auto bytes = *data; + // NOLINTNEXTLINE(cppcoreguidelines-pro-type-reinterpret-cast) + auto const* hdr = reinterpret_cast(bytes.data()); + REQUIRE(hdr->file_count > 0); + bytes[hdr->file_offset + offsetof(RawFileEntry, flags_low)] = std::byte { 1 << 5 }; + stamp_version(bytes, 13); + + auto const path = temp_index_path("pup_retired_node_flag"); + write_bytes(path, bytes); + + REQUIRE(read_prior_paths(path).kind == PriorPaths::Kind::Known); + + std::filesystem::remove(path); +} + +TEST_CASE("A record whose command carries a flag bit this putup cannot name is unreadable", "[index]") +{ + auto index = Index {}; + index.add_file(FileEntry { .id = 1, .parent_id = 0, .type = NodeType::File, .name = intern("main.c") }); + index.add_file(FileEntry { .id = 2, .parent_id = 0, .type = NodeType::Generated, .name = intern("main.o") }); + index.add_command(CommandEntry { + .id = node_id::make_command(1), .instruction_pattern = intern("cc %f"), .inputs = { 1 }, .outputs = { 2 } }); + + auto data = serialize_index(index); + REQUIRE(data.has_value()); + + auto bytes = *data; + // NOLINTNEXTLINE(cppcoreguidelines-pro-type-reinterpret-cast) + auto const* hdr = reinterpret_cast(bytes.data()); + REQUIRE(hdr->command_count > 0); + bytes[hdr->command_offset + offsetof(RawCommandEntry, flags) + 3] = std::byte { 0x80 }; + stamp_version(bytes, INDEX_VERSION); + + auto const path = temp_index_path("pup_unnameable_command_flag"); + write_bytes(path, bytes); + + auto opened = open_index(path); + REQUIRE(opened.has_value()); + auto restored = read_index(*opened); + REQUIRE_FALSE(restored.has_value()); + REQUIRE(restored.error().code == ErrorCode::IndexDamaged); + + opened->file.close(); + std::filesystem::remove(path); +} + /// Whole-record rejection is safe only because each reader's validation scope is its read scope: /// the recovery slice reads names, so operand damage elsewhere leaves it Known (#291). TEST_CASE("A corrupt operand record still leaves the recorded paths readable", "[index]")