erik/acdream
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions 1

fix(pipeline): MP1b review - pak format/reader hardening

Browse source 
Adversarially-verified review findings 2,3,4,5,6,9:

(2) PakFormat.CurrentFormatVersion=1 constant; PakWriter stamps it
unconditionally (caller header templates can no longer produce a
version-0 pak — the default-0 footgun); PakReader refuses any other
version with a message naming found/expected. Tests: versions 0 and 2
both rejected; writer stamps even when the template omits the field.

(3) Reader robustness, all under the documented corrupt-=-missing
CONTRACT (external-file input — surface loudly once, then behave as
absent; never garbage, never throw from a lookup): (a) per-TOC-entry
bounds validation at open (offset/length outside [header, toc) or
past EOF -> logged once, entry missing, siblings unaffected); (b)
TryReadObjectMeshData catches deserialization failures (malformed
structure behind a matching CRC) -> logged once per entry, false; (c)
structurally unopenable files throw at OPEN with a clear message:
unfinalized header (tocOffset < header size — the placeholder-header
crash signature) and TOC-past-EOF truncation. Tests: corrupt TOC
entry, truncated pak, half-written pak, malformed-blob-behind-valid-
CRC (tamper + CRC recompute) — each verifying sibling blobs still read.

(4) Single-pass read: TryReadObjectMeshData now does ONE ReadArray out
of the map; CRC and deserialization run over the same buffer (was: a
separate VerifyCrc traversal + a second ReadArray + a ToArray copy
inside Serializer.Read). New Serializer.Read(byte[]) overload avoids
the defensive copy. Verdict set stays a ConcurrentDictionary (MP1c
calls this from 4 decode workers). True span-over-mmap zero-copy is
deferred to MP1c profiling per the class doc comment.

(5) PakWriter.Dispose restored to try/finally: Finish() does real I/O
and can throw (disk full) — the stream must ALWAYS close so no file
handle leaks mid-unwind. (Reverts the a5926ebc simplification, which
was wrong about this.) Test: dispose after an AddBlob exception leaves
the file deletable.

(6) CRC-32 known-answer vectors: "123456789" -> 0xCBF43926, empty ->
0x00000000, single zero byte -> 0xD202EF8D. The suite was previously
blind to a self-consistent-but-wrong CRC.

(9) Equality comparator floats switched to bit-equality
(SingleToInt32Bits/DoubleToInt64Bits, incl. Vector2/3 + Matrix4x4
components): for byte-identity round-trip purposes `==` was both too
strict (NaN==NaN false — a surviving NaN payload would wrongly FAIL)
and too lax (-0.0==+0.0 true — a sign-bit flip would wrongly PASS).

Content.Tests: 54/54 green (was 43).
This commit is contained in:
Erik 2026-07-05 22:08:59 +02:00
parent 8814a50f52
commit 84d1956d84
7 changed files with 416 additions and 57 deletions
Download patch file Download diff file Expand all files Collapse all files

7
src/AcDream.Content/Pak/ObjectMeshDataSerializer.cs
View file

@ -39,12 +39,15 @@ public static class ObjectMeshDataSerializer {
WriteObjectMeshData(bw, data);
}
public static ObjectMeshData Read(ReadOnlySpan<byte> bytes) {
using var ms = new MemoryStream(bytes.ToArray(), writable: false);
/// <summary>Deserializes directly over <paramref name="bytes"/> — no defensive copy (PakReader's single-pass read reuses its CRC buffer here).</summary>
public static ObjectMeshData Read(byte[] bytes) {
using var ms = new MemoryStream(bytes, writable: false);
using var br = new BinaryReader(ms, System.Text.Encoding.UTF8, leaveOpen: true);
return ReadObjectMeshData(br);
}
public static ObjectMeshData Read(ReadOnlySpan<byte> bytes) => Read(bytes.ToArray());
// ---- ObjectMeshData -----------------------------------------------------
private static void WriteObjectMeshData(BinaryWriter w, ObjectMeshData data) {

13
src/AcDream.Content/Pak/PakFormat.cs
View file

@ -4,6 +4,19 @@ using System.IO;
namespace AcDream.Content.Pak;
/// <summary>
/// Pak format constants shared by writer and reader.
/// </summary>
public static class PakFormat {
/// <summary>
/// The one format version this build writes and reads. PakWriter stamps
/// it unconditionally (callers cannot override it via the header
/// template); PakReader refuses to open any other version. Bump ONLY
/// with an accompanying reader migration path.
/// </summary>
public const uint CurrentFormatVersion = 1;
}
/// <summary>
/// Fixed 64-byte pak file header. Layout (all integers little-endian):
/// <code>

139
src/AcDream.Content/Pak/PakReader.cs
View file

@ -7,17 +7,33 @@ using System.IO.MemoryMappedFiles;
namespace AcDream.Content.Pak;
/// <summary>
/// Zero-copy mmap reader for an acdream pak file. One <see cref="MemoryMappedFile"/>
/// mmap reader for an acdream pak file. One <see cref="MemoryMappedFile"/>
/// for the whole file's lifetime; TOC is loaded once at construction into a
/// key-sorted array for O(log n) binary-search lookup. Blob CRCs are
/// verified LAZILY on first access of each blob (not eagerly at open) and
/// cached so a corrupt blob is only logged once; a corrupted blob is
/// thereafter always treated as "missing" rather than returning garbage.
/// key-sorted array for O(log n) binary-search lookup.
///
/// No locks anywhere: the underlying mapped memory is immutable for the
/// lifetime of this reader (acdream never mutates a pak file while it's
/// open for reading — MP1c's cutover writes a NEW pak and swaps it in), so
/// concurrent reads from multiple threads are safe by construction.
/// <para><b>Corrupt = missing (documented contract):</b> a pak is an external
/// file — the reader's job is to surface corruption loudly ONCE and then
/// behave as if the damaged entry were absent, never to hand back garbage or
/// throw from a lookup. Three tripwires feed the same "missing" state:
/// (1) structurally invalid TOC entries (offset/length outside the file) are
/// detected at open; (2) blob CRC-32 is verified LAZILY on first access and
/// cached; (3) a blob whose bytes fail deserialization despite a matching
/// CRC (malformed structure) fails at read. Each is logged once per entry.
/// Structurally unopenable files (bad magic, wrong format version,
/// unfinalized header, TOC past EOF) throw at OPEN — before any lookups —
/// with a message naming the problem.</para>
///
/// <para><b>Thread safety:</b> no locks on the read path — the mapped memory
/// is immutable for the lifetime of this reader (acdream never mutates a pak
/// while it's open for reading; MP1c's cutover writes a NEW pak and swaps it
/// in). The lazy per-entry verdict set is a ConcurrentDictionary because
/// MP1c calls TryReadObjectMeshData from up to 4 decode workers concurrently.</para>
///
/// <para><b>Perf note:</b> a first read costs ONE exact-size byte[] copy out
/// of the map — CRC and deserialization both run over that same buffer in a
/// single pass. True span-over-mmap zero-copy (deserializing straight out of
/// the mapped view with no copy at all) is deferred to MP1c profiling — do
/// not add it speculatively.</para>
/// </summary>
public sealed class PakReader : IDisposable {
private readonly MemoryMappedFile _mmf;
@ -25,8 +41,8 @@ public sealed class PakReader : IDisposable {
private readonly long _fileLength;
private readonly PakTocEntry[] _toc; // sorted ascending by Key
/// <summary>-1 = not yet checked, 0 = crc mismatch (corrupt), 1 = crc ok.</summary>
private readonly ConcurrentDictionary<int, int> _crcVerifiedByTocIndex = new();
/// <summary>Lazy per-entry verdict: absent = not yet judged, 0 = bad (bounds/crc/structure), 1 = ok.</summary>
private readonly ConcurrentDictionary<int, int> _entryVerdictByTocIndex = new();
private readonly ConcurrentDictionary<int, bool> _loggedCorruption = new();
public PakHeader Header { get; }
@ -44,6 +60,23 @@ public sealed class PakReader : IDisposable {
_accessor.ReadArray(0, headerBytes, 0, PakHeader.Size);
Header = PakHeader.ReadFrom((ReadOnlySpan<byte>)headerBytes);
if (Header.FormatVersion != PakFormat.CurrentFormatVersion) {
throw new InvalidDataException(
$"pak file '{path}' has format version {Header.FormatVersion}; this build reads only " +
$"version {PakFormat.CurrentFormatVersion}. Re-bake with the matching acdream-bake.");
}
// Half-written pak: PakWriter writes a placeholder header (TocOffset 0)
// first and only seeks back to finalize it in Finish(). A crash between
// those leaves TocOffset below the header size — structurally
// unfinalized; refuse loudly rather than serving an empty TOC over a
// file that has blob bytes in it.
if (Header.TocOffset < PakHeader.Size) {
throw new InvalidDataException(
$"pak file '{path}' has an unfinalized header (tocOffset={Header.TocOffset}) — " +
"the bake was interrupted before Finish(); re-bake.");
}
long tocBytesTotal = (long)Header.TocCount * PakTocEntry.Size;
if ((long)Header.TocOffset + tocBytesTotal > _fileLength) {
throw new InvalidDataException(
@ -58,33 +91,76 @@ public sealed class PakReader : IDisposable {
_accessor.ReadArray(tocPos, tocBytes, 0, PakTocEntry.Size);
_toc[i] = PakTocEntry.ReadFrom((ReadOnlySpan<byte>)tocBytes);
tocPos += PakTocEntry.Size;
// Per-entry bounds validation (corrupt = missing, not throw): a
// blob region must sit fully between the header and the TOC, and
// its offset must survive the (long) cast the accessor needs.
ref readonly var entry = ref _toc[i];
bool invalid =
entry.Offset < PakHeader.Size ||
entry.Offset > long.MaxValue ||
entry.Offset + entry.Length > Header.TocOffset ||
entry.Offset + entry.Length > (ulong)_fileLength;
if (invalid) {
_entryVerdictByTocIndex[i] = 0;
LogCorruptionOnce(i, $"TOC entry out of bounds (offset={entry.Offset}, length={entry.Length}, " +
$"file={_fileLength}, toc@{Header.TocOffset})");
}
}
}
/// <summary>True if <paramref name="key"/> is present AND its blob's CRC verifies.</summary>
/// <summary>True if <paramref name="key"/> is present AND its blob verifies (bounds + CRC).</summary>
public bool ContainsKey(ulong key) {
int index = BinarySearch(key);
if (index < 0) return false;
return VerifyCrc(index);
return VerdictFor(index) == 1;
}
/// <summary>
/// Reads and deserializes the <see cref="ObjectMeshData"/> stored under
/// <paramref name="key"/>. Returns false (and null via <paramref name="data"/>)
/// if the key is absent OR the blob's CRC fails verification (corruption
/// tripwire — logged once per blob).
/// <paramref name="key"/>. Returns false (data null) if the key is absent
/// OR the blob fails any tripwire (bounds, CRC, structural deserialization
/// failure) — logged once per entry, per the corrupt-=-missing contract.
/// </summary>
public bool TryReadObjectMeshData(ulong key, out ObjectMeshData? data) {
data = null;
int index = BinarySearch(key);
if (index < 0) return false;
if (!VerifyCrc(index)) return false;
// Previously judged bad (bounds at open, or an earlier CRC/structure
// failure): missing, no re-read, no re-log.
bool judged = _entryVerdictByTocIndex.TryGetValue(index, out var verdict);
if (judged && verdict == 0) return false;
// Single pass (review finding 4): ONE copy out of the map; CRC and
// deserialization both run over this same buffer.
ref readonly var entry = ref _toc[index];
var bytes = new byte[entry.Length];
_accessor.ReadArray((long)entry.Offset, bytes, 0, (int)entry.Length);
data = ObjectMeshDataSerializer.Read(bytes);
return true;
if (!judged) {
uint actualCrc = Crc32.Compute(bytes);
if (actualCrc != entry.Crc32) {
_entryVerdictByTocIndex[index] = 0;
LogCorruptionOnce(index, $"crc mismatch (expected 0x{entry.Crc32:X8}, got 0x{actualCrc:X8})");
return false;
}
_entryVerdictByTocIndex[index] = 1;
}
try {
data = ObjectMeshDataSerializer.Read(bytes);
return true;
}
catch (Exception ex) {
// Structurally malformed blob behind a valid CRC (bake-side bug or
// a tamper that recomputed the CRC). External-file input: demote
// to missing, log once — never propagate from a lookup.
data = null;
_entryVerdictByTocIndex[index] = 0;
LogCorruptionOnce(index, $"deserialization failed despite matching CRC: {ex.GetType().Name}: {ex.Message}");
return false;
}
}
/// <summary>Returns the blob's file offset for alignment assertions in tests.</summary>
@ -97,13 +173,14 @@ public sealed class PakReader : IDisposable {
/// <summary>O(n) linear scan, used only to cross-check the binary search in tests.</summary>
public bool DebugLinearScanContainsKey(ulong key) {
for (int i = 0; i < _toc.Length; i++) {
if (_toc[i].Key == key) return VerifyCrc(i);
if (_toc[i].Key == key) return VerdictFor(i) == 1;
}
return false;
}
private bool VerifyCrc(int tocIndex) {
if (_crcVerifiedByTocIndex.TryGetValue(tocIndex, out var cached)) return cached == 1;
/// <summary>Resolves (and caches) the entry's verdict, reading + CRC-checking the blob if not yet judged.</summary>
private int VerdictFor(int tocIndex) {
if (_entryVerdictByTocIndex.TryGetValue(tocIndex, out var cached)) return cached;
ref readonly var entry = ref _toc[tocIndex];
var bytes = new byte[entry.Length];
@ -111,13 +188,19 @@ public sealed class PakReader : IDisposable {
uint actualCrc = Crc32.Compute(bytes);
bool ok = actualCrc == entry.Crc32;
_crcVerifiedByTocIndex[tocIndex] = ok ? 1 : 0;
if (!ok && _loggedCorruption.TryAdd(tocIndex, true)) {
Console.Error.WriteLine(
$"[pak-corrupt] key 0x{entry.Key:X16} at offset {entry.Offset} (length {entry.Length}): " +
$"crc mismatch (expected 0x{entry.Crc32:X8}, got 0x{actualCrc:X8}) — treating as missing");
_entryVerdictByTocIndex[tocIndex] = ok ? 1 : 0;
if (!ok) {
LogCorruptionOnce(tocIndex, $"crc mismatch (expected 0x{entry.Crc32:X8}, got 0x{actualCrc:X8})");
}
return ok;
return ok ? 1 : 0;
}
private void LogCorruptionOnce(int tocIndex, string reason) {
if (!_loggedCorruption.TryAdd(tocIndex, true)) return;
ref readonly var entry = ref _toc[tocIndex];
Console.Error.WriteLine(
$"[pak-corrupt] key 0x{entry.Key:X16} at offset {entry.Offset} (length {entry.Length}): " +
$"{reason} — treating as missing");
}
private int BinarySearch(ulong key) {

24
src/AcDream.Content/Pak/PakWriter.cs
View file

@ -23,6 +23,12 @@ public sealed class PakWriter : IDisposable {
public PakWriter(string path, PakHeader headerTemplate) {
_stream = new FileStream(path, FileMode.Create, FileAccess.ReadWrite, FileShare.None);
_headerTemplate = headerTemplate;
// The format version is the WRITER's identity, not caller input —
// stamp it unconditionally so a caller-populated header template can
// never produce a pak claiming a version this code doesn't write
// (the default-0 footgun; PakReader rejects any version but
// PakFormat.CurrentFormatVersion).
_headerTemplate.FormatVersion = PakFormat.CurrentFormatVersion;
// Write a header placeholder now; finalized in Finish() once TocOffset/TocCount are known.
_headerTemplate.WriteTo(_stream);
@ -95,11 +101,19 @@ public sealed class PakWriter : IDisposable {
public void Dispose() {
// Best-effort finalize so a forgotten Finish() call doesn't leave a
// half-written file with a placeholder header claiming 0 entries
// while blobs are on disk. No try/catch needed: Finish() only throws
// when _finished is already true, which this guard already excludes.
if (!_finished) {
Finish();
// while blobs are on disk. try/finally, NOT a bare sequence: Finish()
// performs real I/O (TOC write, header seek-back, flush) and can
// throw for environmental reasons (disk full, I/O error) — the
// stream must ALWAYS be closed so the file handle is never leaked
// mid-unwind (review finding 5 on the a5926ebc cleanup, which had
// removed exactly this guarantee).
try {
if (!_finished) {
Finish();
}
}
finally {
_stream.Dispose();
}
_stream.Dispose();
}
}