From 4c6cb86112f5c7c4744b5177d6bf1e4b4f88839c Mon Sep 17 00:00:00 2001 From: "Derek T. Jones" Date: Tue, 11 Aug 2026 21:14:04 -0700 Subject: [PATCH 1/2] Give the .acx format a cookie and a version number Nothing at the head of an .acx said what it was, so the only thing separating a game binary from any other file was whether deserialization happened to trip over something. Often it didn't: feeding the interpreter a text file got as far as "No 'main' object", meaning a whole universe had been read out of Archetype source and believed. The web driver hands the deserializer a file the player picked off their own disk, so that is not a hypothetical. Files now begin with 7F 41 43 58 and a format version. The version is the layout's, not the interpreter's: a release that moves no bytes must leave every .acx as it was, or the goldens churn and comparing bytes stops being an oracle. Reading an older headerless file still works. That layout opens with ended_, a bool written as a one-byte varint, so its first byte can only be 0 or 2 -- enough of a gate to turn away an image or an archive while letting every real pre-header file through. When there are none of those left to care about, the branch can go. Deciding between the two needs a look at bytes that may belong to either, so Storage grows a peek: a read that can be taken back. The goldens are five bytes longer apiece and their Turtle is untouched, which is the whole claim -- the container changed and the contents did not. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_013Ui8UMgev1U8LW5iyQdSsJ --- CLAUDE.md | 1 + src/FileStorage.cc | 15 +++++++++ src/FileStorage.hh | 2 ++ src/Serialization.cc | 29 +++++++++++++++++ src/Serialization.hh | 37 ++++++++++++++++++++++ src/TestSerialization.cc | 65 +++++++++++++++++++++++++++++++++++++++ src/TestSerialization.hh | 2 ++ src/Universe.cc | 22 +++++++++++++ tests/golden/bare.acx | Bin 14693 -> 14698 bytes tests/golden/cherry.acx | Bin 11468 -> 11473 bytes 10 files changed, 173 insertions(+) diff --git a/CLAUDE.md b/CLAUDE.md index 8c7889a..e148c5b 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -55,4 +55,5 @@ echo "look" | ./build/archetype --source=games/gorreven.arch --include=games - `include "file"` in Archetype source uses implicit `.arch` extension; do not include the extension explicitly. - Compiled `.acx` files are fully resumable — a save file is a mutated copy of the original binary. +- An `.acx` begins with the four bytes `7F 41 43 58` and a format version. The version describes the byte layout only; it is not the interpreter's version, and it must not move when a release changes no layout. Files written before the header still load. - The `--include=games` flag is needed when compiling from source so the compiler can find `standard.arch` and other shared library files. diff --git a/src/FileStorage.cc b/src/FileStorage.cc index c25afdc..5011fa0 100644 --- a/src/FileStorage.cc +++ b/src/FileStorage.cc @@ -46,6 +46,17 @@ namespace archetype { void InFileStorage::write(span) { } + int InFileStorage::peek(span buf) { + int bytes_read = read(buf); + // A short read at the end of the file sets eofbit, and seekg does + // nothing at all while a stream is in a failed state, so the flags have + // to come off before the position can be put back. + stream_.clear(); + stream_.seekg(-bytes_read, ios::cur); + remaining_ += bytes_read; + return bytes_read; + } + OutFileStorage::OutFileStorage(const filesystem::path& filename): failed_{false} { @@ -78,6 +89,10 @@ namespace archetype { return 0; } + int OutFileStorage::peek(span) { + return 0; + } + void OutFileStorage::write(span buf) { stream_.write(reinterpret_cast(buf.data()), static_cast(buf.size())); if (not stream_) { diff --git a/src/FileStorage.hh b/src/FileStorage.hh index 113c554..2881df9 100644 --- a/src/FileStorage.hh +++ b/src/FileStorage.hh @@ -29,6 +29,7 @@ namespace archetype { virtual int remaining() const override; virtual int read(std::span buf) override; virtual void write(std::span buf) override; + virtual int peek(std::span buf) override; private: std::ifstream stream_; int remaining_; @@ -53,6 +54,7 @@ namespace archetype { virtual int remaining() const override; virtual int read(std::span buf) override; virtual void write(std::span buf) override; + virtual int peek(std::span buf) override; private: std::ofstream stream_; bool failed_; diff --git a/src/Serialization.cc b/src/Serialization.cc index 7c0f0ae..4a01fe7 100644 --- a/src/Serialization.cc +++ b/src/Serialization.cc @@ -9,6 +9,7 @@ // For Windows #define _SCL_SECURE_NO_WARNINGS +#include #include #include #include @@ -108,6 +109,28 @@ namespace archetype { return in; } + void writeFormatHeader(Storage& out) { + out.write(FormatCookie); + out << CurrentFormatVersion; + } + + int readFormatHeader(Storage& in) { + array cookie{}; + if (in.peek(cookie) != static_cast(cookie.size()) or + not ranges::equal(cookie, FormatCookie)) { + return UnversionedFormat; + } + in.read(cookie); + int version = in.readInteger(); + if (version <= UnversionedFormat or version > CurrentFormatVersion) { + throw invalid_argument( + format("Archetype format version {} is not one this interpreter " + "understands; it reads up to version {}", + version, CurrentFormatVersion)); + } + return version; + } + MemoryStorage::MemoryStorage(): seekIndex_{0} { } @@ -128,4 +151,10 @@ namespace archetype { ranges::copy(buf, back_inserter(bytes_)); } + int MemoryStorage::peek(span buf) { + int bytes_read = read(buf); + seekIndex_ -= bytes_read; + return bytes_read; + } + } diff --git a/src/Serialization.hh b/src/Serialization.hh index 3bf53ef..370d930 100644 --- a/src/Serialization.hh +++ b/src/Serialization.hh @@ -9,6 +9,7 @@ #ifndef __archetype__Serialization__ #define __archetype__Serialization__ +#include #include #include #include @@ -28,6 +29,12 @@ namespace archetype { virtual int read(std::span buf) = 0; virtual void write(std::span buf) = 0; + // Look at what read would return, without consuming it. Deciding + // whether a stream carries a format header means examining its first + // bytes before knowing whether they belong to the header or to the + // universe, and only a look that can be taken back can do that. + virtual int peek(std::span buf) = 0; + int readInteger(); void writeInteger(int value); }; @@ -40,6 +47,35 @@ namespace archetype { Storage& operator<<(Storage& out, std::string_view value); Storage& operator>>(Storage& in, std::string& value); + // The four bytes an Archetype binary begins with. 0x7F is neither + // printable nor a legal start to a UTF-8 sequence, so a text file mistaken + // for a game is rejected on its first byte. + inline constexpr std::array FormatCookie{0x7F, 'A', 'C', 'X'}; + + // The version of the *layout* that follows the cookie, which is a separate + // thing from the interpreter's own version and deliberately so: a release + // that moves no bytes around must leave every .acx exactly as it was, or + // the goldens churn and byte comparison stops being an oracle. Bump this + // only when what operator<< writes actually changes shape. + inline constexpr int CurrentFormatVersion = 1; + + // Version 0 is the unversioned layout that predates the header, still + // written by no one and still readable by everyone. + inline constexpr int UnversionedFormat = 0; + + void writeFormatHeader(Storage& out); + + // The format version the stream declares, with the header consumed if there + // was one. A stream that does not begin with the cookie is left untouched + // and reported as UnversionedFormat; deciding whether that is good enough + // is the caller's business, since only the caller knows what the + // unversioned layout was supposed to start with. + // + // Throws if the cookie is there but the version is one this interpreter + // does not know, which is the case worth a precise complaint: the file is + // definitely ours and definitely from the future. + int readFormatHeader(Storage& in); + class MemoryStorage : public Storage { size_t seekIndex_; std::vector bytes_; @@ -50,6 +86,7 @@ namespace archetype { virtual int remaining() const override; virtual int read(std::span buf) override; virtual void write(std::span buf) override; + virtual int peek(std::span buf) override; }; } diff --git a/src/TestSerialization.cc b/src/TestSerialization.cc index f5f3b27..d4168b7 100644 --- a/src/TestSerialization.cc +++ b/src/TestSerialization.cc @@ -6,7 +6,9 @@ // Copyright (c) 2014 Derek Jones. All rights reserved. // +#include #include +#include #include "TestSerialization.hh" #include "TestRegistry.hh" @@ -43,5 +45,68 @@ namespace archetype { threw = true; } ARCHETYPE_TEST(threw); + + testPeek_(); + testFormatHeader_(); + } + + void TestSerialization::testPeek_() { + MemoryStorage mem; + mem.writeInteger(4242); + int written = mem.remaining(); + + array looked{}; + ARCHETYPE_TEST_EQUAL(mem.peek(looked), 2); + // A look that cost nothing: the bytes are still all there, and the + // integer still reads back whole. + ARCHETYPE_TEST_EQUAL(mem.remaining(), written); + ARCHETYPE_TEST_EQUAL(mem.readInteger(), 4242); + ARCHETYPE_TEST_EQUAL(mem.remaining(), 0); + + // Peeking past the end reports what was actually available, and still + // leaves the stream where it found it. + MemoryStorage tiny; + tiny.write(span{FormatCookie.data(), 1}); + array too_many{}; + ARCHETYPE_TEST_EQUAL(tiny.peek(too_many), 1); + ARCHETYPE_TEST_EQUAL(tiny.remaining(), 1); + } + + void TestSerialization::testFormatHeader_() { + MemoryStorage mem; + writeFormatHeader(mem); + mem.writeInteger(99); + ARCHETYPE_TEST_EQUAL(readFormatHeader(mem), CurrentFormatVersion); + // The header is consumed, so what follows it is next. + ARCHETYPE_TEST_EQUAL(mem.readInteger(), 99); + + // A stream with no cookie reports the unversioned format and is left + // exactly as it was, which is what lets an older .acx be read on. + MemoryStorage headerless; + headerless.writeInteger(0); + headerless.writeInteger(77); + ARCHETYPE_TEST_EQUAL(readFormatHeader(headerless), UnversionedFormat); + ARCHETYPE_TEST_EQUAL(headerless.readInteger(), 0); + ARCHETYPE_TEST_EQUAL(headerless.readInteger(), 77); + + // Too short to hold a cookie is not a cookie, and must not be mistaken + // for one on the strength of the bytes that are there. + MemoryStorage truncated; + truncated.write(span{FormatCookie.data(), 2}); + ARCHETYPE_TEST_EQUAL(readFormatHeader(truncated), UnversionedFormat); + ARCHETYPE_TEST_EQUAL(truncated.remaining(), 2); + + // Ours, but from the future: the one case that earns a precise + // complaint rather than a shrug. + MemoryStorage from_the_future; + from_the_future.write(FormatCookie); + from_the_future.writeInteger(CurrentFormatVersion + 1); + bool threw = false; + try { + readFormatHeader(from_the_future); + } catch (const invalid_argument&) { + threw = true; + } + ARCHETYPE_TEST(threw); } } diff --git a/src/TestSerialization.hh b/src/TestSerialization.hh index 97956a2..6fe3526 100644 --- a/src/TestSerialization.hh +++ b/src/TestSerialization.hh @@ -17,6 +17,8 @@ namespace archetype { class TestSerialization : public ITestSuite { protected: virtual void runTests_() override; + void testPeek_(); + void testFormatHeader_(); public: TestSerialization(std::string name): ITestSuite(name) { } }; diff --git a/src/Universe.cc b/src/Universe.cc index 5d4b314..d732ca9 100644 --- a/src/Universe.cc +++ b/src/Universe.cc @@ -13,6 +13,7 @@ #include #include #include +#include using namespace std; @@ -491,7 +492,13 @@ namespace archetype { return drawn; } + // Every path that writes a universe -- --create, a save, an autosave, the + // snapshot a mid-turn rollback stands on -- comes through here, and every + // path that reads one comes through its opposite, so the header needs + // writing and checking in exactly these two places. + Storage& operator<<(Storage& out, const Universe& u) { + writeFormatHeader(out); out << static_cast(u.ended_); out << u.Messages << u.TextLiterals << u.Identifiers << u.ObjectIdentifiers; out << u.objects_; @@ -499,6 +506,21 @@ namespace archetype { } Storage& operator>>(Storage& in, Universe& u) { + if (readFormatHeader(in) == UnversionedFormat) { + // No cookie, so this is either an .acx from before the header or + // not an .acx at all. The unversioned layout opens with ended_, a + // bool written as a one-byte varint, so its first byte can only be + // 0 or 2 -- enough to turn away a text file, an image, or an + // archive at the door instead of letting it in to fail later on + // some absurd string length. When there are no more headerless + // files left to care about, this whole branch can go. + Storage::Byte first{}; + if (in.peek({&first, 1}) != 1 or (first != 0x00 and first != 0x02)) { + throw std::invalid_argument( + "Not an Archetype binary: no format header, and what is " + "there does not look like one of the older headerless ones"); + } + } int ended; in >> ended; u.ended_ = static_cast(ended); diff --git a/tests/golden/bare.acx b/tests/golden/bare.acx index 8bf0d5c5bd14a4838255e1141b4b4524985f2015..84aa72cc7152efd5748ad17e2600cb54db41067b 100644 GIT binary patch delta 13 UcmaD_^s0!p-qAUNX+x?d04v!AWB>pF delta 8 PcmaD=^t5OrYpNvx732g6 diff --git a/tests/golden/cherry.acx b/tests/golden/cherry.acx index 0e780303cd5ca1c86132782e3272888410f9f37a..8c3abbb3c8862f312fff53756a8addaf11462f67 100644 GIT binary patch delta 13 UcmX>Tc`=f;-qAUNX~P*E04N*lqyY6dMEn From 2d5b3356b57c2b400e486e8de33e4aa22a58f92d Mon Sep 17 00:00:00 2001 From: "Derek T. Jones" Date: Tue, 11 Aug 2026 21:27:51 -0700 Subject: [PATCH 2/2] Check every count before believing it A count in an .acx is a promise about bytes that follow, and the reader believed all of them on sight. Four places: a string's length, a string value's length, a registry's slot count, and an identifier map's size. The registry was the one that mattered. Its records each name the slot they belong in, and that number went into a deque subscript unchecked, so thirteen crafted bytes -- a header, one slot, one record naming slot fifty million -- wrote out of bounds and took the process down with SIGSEGV. The slot count itself was believed too: twelve bytes could ask for a deque of four hundred million strings, and the machine would go and try. No stream can back a count larger than the bytes left in it, since every element costs at least one byte to encode, so readCount refuses one that cannot be honoured. That is exact for strings and records. For a registry's slot count it is slightly stricter than the format demands -- a free slot costs nothing to write -- but free slots are reused rather than accumulated, and if a game ever does hold more objects than its save has bytes, the answer is to write the holes down rather than to stop checking. The string value in Value.cc had no check at all on what it managed to read, so a truncated literal came back as a run of NUL bytes instead of as an error. It has one now. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_013Ui8UMgev1U8LW5iyQdSsJ --- src/IdIndex.hh | 34 ++++++++++++++++-- src/Serialization.cc | 16 +++++++-- src/Serialization.hh | 12 +++++++ src/TestSerialization.cc | 77 ++++++++++++++++++++++++++++++++++++++++ src/TestSerialization.hh | 1 + src/Universe.cc | 6 ++-- src/Value.cc | 15 ++++++-- 7 files changed, 151 insertions(+), 10 deletions(-) diff --git a/src/IdIndex.hh b/src/IdIndex.hh index 9e7ac4c..87f2776 100644 --- a/src/IdIndex.hh +++ b/src/IdIndex.hh @@ -11,10 +11,12 @@ #include #include +#include #include #include #include #include +#include #include #include @@ -153,9 +155,26 @@ namespace archetype { } void read(Storage& in) { - int total_entries; - int indexed_entries; - in >> total_entries >> indexed_entries; + // Both of these arrive from the file, and both used to be believed + // on sight. A registry cannot hold more records than the stream + // has bytes to encode them in, and cannot hold more records than it + // has slots. + // + // Bounding the slot count by the bytes remaining is the one place + // this is stricter than the format strictly requires: a free slot + // costs nothing to write, so a registry whose holes outnumbered the + // whole file would now be refused. Free slots are reused rather + // than accumulated, so that registry would have to come from a game + // that had once held more objects than its save has bytes -- and if + // one ever does, the answer is to write the holes down rather than + // to stop checking. + int total_entries = readCount(in, "registry size"); + int indexed_entries = readCount(in, "registry record count"); + if (indexed_entries > total_entries) { + throw std::invalid_argument( + std::format("Registry of {} slots cannot hold {} records", + total_entries, indexed_entries)); + } registry_.resize(total_entries, T{}); // Nothing about the holes is written down, and nothing needs to be. // Each record names the slot it belongs in, so the occupied slots @@ -171,6 +190,15 @@ namespace archetype { for (int ii = 0; ii < indexed_entries; ++ii) { int value_index; in >> value_index; + // A record naming a slot the registry does not have was an + // out-of-bounds write straight into the deque, reachable from + // any file at all: thirteen crafted bytes segfaulted the + // interpreter here. + if (value_index < 0 or value_index >= total_entries) { + throw std::invalid_argument( + std::format("Record names slot {} in a registry of {}", + value_index, total_entries)); + } in >> registry_[value_index]; index_[registry_[value_index]] = value_index; occupied[value_index] = true; diff --git a/src/Serialization.cc b/src/Serialization.cc index 4a01fe7..aafccc9 100644 --- a/src/Serialization.cc +++ b/src/Serialization.cc @@ -97,8 +97,7 @@ namespace archetype { } Storage& operator>>(Storage& in, std::string& value) { - int size; - in >> size; + int size = readCount(in, "string length"); value.resize(size); int bytes_read = in.read({reinterpret_cast(value.data()), value.size()}); if (bytes_read != size) { @@ -109,6 +108,19 @@ namespace archetype { return in; } + int readCount(Storage& in, string_view what) { + int count = in.readInteger(); + if (count < 0) { + throw invalid_argument(format("A {} cannot be negative: {}", what, count)); + } + if (count > in.remaining()) { + throw invalid_argument( + format("A {} of {} is more than the {} bytes remaining can supply", + what, count, in.remaining())); + } + return count; + } + void writeFormatHeader(Storage& out) { out.write(FormatCookie); out << CurrentFormatVersion; diff --git a/src/Serialization.hh b/src/Serialization.hh index 370d930..0daa6bf 100644 --- a/src/Serialization.hh +++ b/src/Serialization.hh @@ -63,6 +63,18 @@ namespace archetype { // written by no one and still readable by everyone. inline constexpr int UnversionedFormat = 0; + // Read a number that counts something the stream still has to supply. + // + // Such a count is a promise about bytes that follow, and no stream can make + // good on a promise bigger than what is left in it: every element, however + // small, costs at least one byte to encode. Checking that before believing + // the number is the whole point -- believing it first is how a file of + // thirteen bytes used to ask for a vector of fifty million. + // + // "what" names the thing being counted, for the message thrown if the count + // is negative or larger than the stream can back. + int readCount(Storage& in, std::string_view what); + void writeFormatHeader(Storage& out); // The format version the stream declares, with the header consumed if there diff --git a/src/TestSerialization.cc b/src/TestSerialization.cc index d4168b7..191b309 100644 --- a/src/TestSerialization.cc +++ b/src/TestSerialization.cc @@ -13,6 +13,7 @@ #include "TestSerialization.hh" #include "TestRegistry.hh" #include "Serialization.hh" +#include "StringIdIndex.hh" #include using namespace std; @@ -48,6 +49,82 @@ namespace archetype { testPeek_(); testFormatHeader_(); + testCountsAreBounded_(); + } + + // A count in the stream is a promise about bytes that follow, and every one + // of these used to be believed without checking whether the stream could + // keep it. The registry case is the one that mattered most: it reached an + // out-of-bounds write and took the process down with it. + void TestSerialization::testCountsAreBounded_() { + auto rejects = [](MemoryStorage& mem) { + try { + readCount(mem, "test count"); + } catch (const invalid_argument&) { + return true; + } + return false; + }; + + MemoryStorage negative; + negative.writeInteger(-99); + ARCHETYPE_TEST(rejects(negative)); + + // Three bytes cannot be followed by a million of anything. + MemoryStorage overlong; + overlong.writeInteger(1000000); + ARCHETYPE_TEST(rejects(overlong)); + + // A count the stream can actually back is none of this function's + // business, and comes through untouched. + MemoryStorage honest; + honest.writeInteger(3); + honest.write(array{'a', 'b', 'c'}); + ARCHETYPE_TEST_EQUAL(readCount(honest, "test count"), 3); + + // A string is the commonest promise of all, and the one an .acx makes + // hundreds of times. + MemoryStorage lying; + lying.writeInteger(500); + lying.write(array{'h', 'i'}); + string scratch; + bool threw = false; + try { + lying >> scratch; + } catch (const invalid_argument&) { + threw = true; + } + ARCHETYPE_TEST(threw); + + // The registry: two slots claimed, one record, and that record naming a + // slot fifty million past the end. This is the shape that segfaulted. + MemoryStorage out_of_range; + out_of_range.writeInteger(2); + out_of_range.writeInteger(1); + out_of_range.writeInteger(50000000); + out_of_range.writeInteger(0); + StringIdIndex index; + threw = false; + try { + index.read(out_of_range); + } catch (const invalid_argument&) { + threw = true; + } + ARCHETYPE_TEST(threw); + + // A registry cannot hold more records than it has slots either. + MemoryStorage overfull; + overfull.writeInteger(1); + overfull.writeInteger(2); + overfull.writeInteger(0); + overfull.writeInteger(0); + threw = false; + try { + index.read(overfull); + } catch (const invalid_argument&) { + threw = true; + } + ARCHETYPE_TEST(threw); } void TestSerialization::testPeek_() { diff --git a/src/TestSerialization.hh b/src/TestSerialization.hh index 6fe3526..43028f1 100644 --- a/src/TestSerialization.hh +++ b/src/TestSerialization.hh @@ -19,6 +19,7 @@ namespace archetype { virtual void runTests_() override; void testPeek_(); void testFormatHeader_(); + void testCountsAreBounded_(); public: TestSerialization(std::string name): ITestSuite(name) { } }; diff --git a/src/Universe.cc b/src/Universe.cc index d732ca9..0539a3f 100644 --- a/src/Universe.cc +++ b/src/Universe.cc @@ -405,8 +405,10 @@ namespace archetype { Storage& operator>>(Storage&in, IdentifierMap& m) { m.clear(); - int entries; - in >> entries; + // The loop below would have run out of stream and thrown eventually, so + // this buys a sensible complaint rather than safety -- but a count is a + // count, and they are all checked the same way now. + int entries = readCount(in, "identifier map size"); for (int i = 0; i < entries; ++i) { int first, second; in >> first >> second; diff --git a/src/Value.cc b/src/Value.cc index 696431e..691515a 100644 --- a/src/Value.cc +++ b/src/Value.cc @@ -575,11 +575,20 @@ namespace archetype { break; } case STRING: { - int text_size; - in >> text_size; + // Not operator>>(string&) only because the value has to be + // built from the characters; the care it takes is the same, and + // going without it here left a truncated literal reading back + // as a run of NUL bytes rather than as an error. + int text_size = readCount(in, "string value length"); string text; text.resize(text_size); - in.read({reinterpret_cast(text.data()), text.size()}); + int bytes_read = + in.read({reinterpret_cast(text.data()), text.size()}); + if (bytes_read != text_size) { + throw invalid_argument( + format("Could not fully read string value declared as {} bytes; only read {}", + text_size, bytes_read)); + } v = make_unique(text); break; }