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/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 7c0f0ae..aafccc9 100644 --- a/src/Serialization.cc +++ b/src/Serialization.cc @@ -9,6 +9,7 @@ // For Windows #define _SCL_SECURE_NO_WARNINGS +#include #include #include #include @@ -96,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) { @@ -108,6 +108,41 @@ 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; + } + + 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 +163,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..0daa6bf 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,47 @@ 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; + + // 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 + // 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 +98,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..191b309 100644 --- a/src/TestSerialization.cc +++ b/src/TestSerialization.cc @@ -6,11 +6,14 @@ // Copyright (c) 2014 Derek Jones. All rights reserved. // +#include #include +#include #include "TestSerialization.hh" #include "TestRegistry.hh" #include "Serialization.hh" +#include "StringIdIndex.hh" #include using namespace std; @@ -43,5 +46,144 @@ namespace archetype { threw = true; } ARCHETYPE_TEST(threw); + + 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_() { + 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..43028f1 100644 --- a/src/TestSerialization.hh +++ b/src/TestSerialization.hh @@ -17,6 +17,9 @@ namespace archetype { class TestSerialization : public ITestSuite { protected: 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 5d4b314..0539a3f 100644 --- a/src/Universe.cc +++ b/src/Universe.cc @@ -13,6 +13,7 @@ #include #include #include +#include using namespace std; @@ -404,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; @@ -491,7 +494,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 +508,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/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; } diff --git a/tests/golden/bare.acx b/tests/golden/bare.acx index 8bf0d5c..84aa72c 100644 Binary files a/tests/golden/bare.acx and b/tests/golden/bare.acx differ diff --git a/tests/golden/cherry.acx b/tests/golden/cherry.acx index 0e78030..8c3abbb 100644 Binary files a/tests/golden/cherry.acx and b/tests/golden/cherry.acx differ