diff --git a/src/AcDream.Content/Pak/ObjectMeshDataSerializer.cs b/src/AcDream.Content/Pak/ObjectMeshDataSerializer.cs index 8236ca24..622f64cd 100644 --- a/src/AcDream.Content/Pak/ObjectMeshDataSerializer.cs +++ b/src/AcDream.Content/Pak/ObjectMeshDataSerializer.cs @@ -39,12 +39,15 @@ public static class ObjectMeshDataSerializer { WriteObjectMeshData(bw, data); } - public static ObjectMeshData Read(ReadOnlySpan bytes) { - using var ms = new MemoryStream(bytes.ToArray(), writable: false); + /// Deserializes directly over — no defensive copy (PakReader's single-pass read reuses its CRC buffer here). + 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 bytes) => Read(bytes.ToArray()); + // ---- ObjectMeshData ----------------------------------------------------- private static void WriteObjectMeshData(BinaryWriter w, ObjectMeshData data) { diff --git a/src/AcDream.Content/Pak/PakFormat.cs b/src/AcDream.Content/Pak/PakFormat.cs index f871d451..bf8b85bd 100644 --- a/src/AcDream.Content/Pak/PakFormat.cs +++ b/src/AcDream.Content/Pak/PakFormat.cs @@ -4,6 +4,19 @@ using System.IO; namespace AcDream.Content.Pak; +/// +/// Pak format constants shared by writer and reader. +/// +public static class PakFormat { + /// + /// 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. + /// + public const uint CurrentFormatVersion = 1; +} + /// /// Fixed 64-byte pak file header. Layout (all integers little-endian): /// diff --git a/src/AcDream.Content/Pak/PakReader.cs b/src/AcDream.Content/Pak/PakReader.cs index 02b87183..9a7549d5 100644 --- a/src/AcDream.Content/Pak/PakReader.cs +++ b/src/AcDream.Content/Pak/PakReader.cs @@ -7,17 +7,33 @@ using System.IO.MemoryMappedFiles; namespace AcDream.Content.Pak; /// -/// Zero-copy mmap reader for an acdream pak file. One +/// mmap reader for an acdream pak file. One /// 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. +/// Corrupt = missing (documented contract): 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. +/// +/// Thread safety: 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. +/// +/// Perf note: 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. /// 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 - /// -1 = not yet checked, 0 = crc mismatch (corrupt), 1 = crc ok. - private readonly ConcurrentDictionary _crcVerifiedByTocIndex = new(); + /// Lazy per-entry verdict: absent = not yet judged, 0 = bad (bounds/crc/structure), 1 = ok. + private readonly ConcurrentDictionary _entryVerdictByTocIndex = new(); private readonly ConcurrentDictionary _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)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)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})"); + } } } - /// True if is present AND its blob's CRC verifies. + /// True if is present AND its blob verifies (bounds + CRC). public bool ContainsKey(ulong key) { int index = BinarySearch(key); if (index < 0) return false; - return VerifyCrc(index); + return VerdictFor(index) == 1; } /// /// Reads and deserializes the stored under - /// . Returns false (and null via ) - /// if the key is absent OR the blob's CRC fails verification (corruption - /// tripwire — logged once per blob). + /// . 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. /// 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; + } } /// Returns the blob's file offset for alignment assertions in tests. @@ -97,13 +173,14 @@ public sealed class PakReader : IDisposable { /// O(n) linear scan, used only to cross-check the binary search in tests. 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; + /// Resolves (and caches) the entry's verdict, reading + CRC-checking the blob if not yet judged. + 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) { diff --git a/src/AcDream.Content/Pak/PakWriter.cs b/src/AcDream.Content/Pak/PakWriter.cs index fe6e33bd..2a745669 100644 --- a/src/AcDream.Content/Pak/PakWriter.cs +++ b/src/AcDream.Content/Pak/PakWriter.cs @@ -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(); } } diff --git a/tests/AcDream.Content.Tests/Crc32Tests.cs b/tests/AcDream.Content.Tests/Crc32Tests.cs new file mode 100644 index 00000000..3ab7897c --- /dev/null +++ b/tests/AcDream.Content.Tests/Crc32Tests.cs @@ -0,0 +1,31 @@ +using System.Text; +using AcDream.Content.Pak; + +namespace AcDream.Content.Tests; + +/// +/// Known-answer vectors for the hand-rolled CRC-32 (IEEE 802.3 / zip / PNG +/// variant). Without these the suite is blind to a self-consistent-but-wrong +/// implementation: writer and reader would agree with each other while +/// producing values no external tool could reproduce (review finding 6). +/// +public class Crc32Tests { + [Fact] + public void KnownAnswer_123456789_IsCBF43926() { + // THE standard CRC-32 check value: CRC of the ASCII string + // "123456789" is 0xCBF43926 for the reflected 0xEDB88320 polynomial. + var input = Encoding.ASCII.GetBytes("123456789"); + Assert.Equal(0xCBF43926u, Crc32.Compute(input)); + } + + [Fact] + public void KnownAnswer_EmptyInput_IsZero() { + Assert.Equal(0x00000000u, Crc32.Compute(ReadOnlySpan.Empty)); + } + + [Fact] + public void KnownAnswer_SingleZeroByte() { + // CRC-32 of a single 0x00 byte is 0xD202EF8D (standard vector). + Assert.Equal(0xD202EF8Du, Crc32.Compute(new byte[] { 0x00 })); + } +} diff --git a/tests/AcDream.Content.Tests/ObjectMeshDataEquality.cs b/tests/AcDream.Content.Tests/ObjectMeshDataEquality.cs index 9879a323..717f048c 100644 --- a/tests/AcDream.Content.Tests/ObjectMeshDataEquality.cs +++ b/tests/AcDream.Content.Tests/ObjectMeshDataEquality.cs @@ -62,7 +62,7 @@ public static class ObjectMeshDataEquality { for (int i = 0; i < expected.Length; i++) { AssertVector3Equal(expected[i].Position, actual[i].Position, $"{path}[{i}].Position"); AssertVector3Equal(expected[i].Normal, actual[i].Normal, $"{path}[{i}].Normal"); - Assert.True(expected[i].UV == actual[i].UV, $"{path}[{i}].UV: expected {expected[i].UV}, got {actual[i].UV}"); + Assert.True(BitEqual(expected[i].UV, actual[i].UV), $"{path}[{i}].UV: expected {expected[i].UV}, got {actual[i].UV}"); } } @@ -133,31 +133,31 @@ public static class ObjectMeshDataEquality { Assert.True(expected.ParticleType == actual.ParticleType, $"{path}.ParticleType: expected {expected.ParticleType}, got {actual.ParticleType}"); Assert.True(expected.GfxObjId.DataId == actual.GfxObjId.DataId, $"{path}.GfxObjId: expected 0x{expected.GfxObjId.DataId:X8}, got 0x{actual.GfxObjId.DataId:X8}"); Assert.True(expected.HwGfxObjId.DataId == actual.HwGfxObjId.DataId, $"{path}.HwGfxObjId: expected 0x{expected.HwGfxObjId.DataId:X8}, got 0x{actual.HwGfxObjId.DataId:X8}"); - Assert.True(expected.Birthrate == actual.Birthrate, $"{path}.Birthrate: expected {expected.Birthrate}, got {actual.Birthrate}"); + Assert.True(BitEqual(expected.Birthrate, actual.Birthrate), $"{path}.Birthrate: expected {expected.Birthrate}, got {actual.Birthrate}"); Assert.True(expected.MaxParticles == actual.MaxParticles, $"{path}.MaxParticles: expected {expected.MaxParticles}, got {actual.MaxParticles}"); Assert.True(expected.InitialParticles == actual.InitialParticles, $"{path}.InitialParticles: expected {expected.InitialParticles}, got {actual.InitialParticles}"); Assert.True(expected.TotalParticles == actual.TotalParticles, $"{path}.TotalParticles: expected {expected.TotalParticles}, got {actual.TotalParticles}"); - Assert.True(expected.TotalSeconds == actual.TotalSeconds, $"{path}.TotalSeconds: expected {expected.TotalSeconds}, got {actual.TotalSeconds}"); - Assert.True(expected.Lifespan == actual.Lifespan, $"{path}.Lifespan: expected {expected.Lifespan}, got {actual.Lifespan}"); - Assert.True(expected.LifespanRand == actual.LifespanRand, $"{path}.LifespanRand: expected {expected.LifespanRand}, got {actual.LifespanRand}"); + Assert.True(BitEqual(expected.TotalSeconds, actual.TotalSeconds), $"{path}.TotalSeconds: expected {expected.TotalSeconds}, got {actual.TotalSeconds}"); + Assert.True(BitEqual(expected.Lifespan, actual.Lifespan), $"{path}.Lifespan: expected {expected.Lifespan}, got {actual.Lifespan}"); + Assert.True(BitEqual(expected.LifespanRand, actual.LifespanRand), $"{path}.LifespanRand: expected {expected.LifespanRand}, got {actual.LifespanRand}"); AssertVector3Equal(expected.OffsetDir, actual.OffsetDir, $"{path}.OffsetDir"); - Assert.True(expected.MinOffset == actual.MinOffset, $"{path}.MinOffset: expected {expected.MinOffset}, got {actual.MinOffset}"); - Assert.True(expected.MaxOffset == actual.MaxOffset, $"{path}.MaxOffset: expected {expected.MaxOffset}, got {actual.MaxOffset}"); + Assert.True(BitEqual(expected.MinOffset, actual.MinOffset), $"{path}.MinOffset: expected {expected.MinOffset}, got {actual.MinOffset}"); + Assert.True(BitEqual(expected.MaxOffset, actual.MaxOffset), $"{path}.MaxOffset: expected {expected.MaxOffset}, got {actual.MaxOffset}"); AssertVector3Equal(expected.A, actual.A, $"{path}.A"); - Assert.True(expected.MinA == actual.MinA, $"{path}.MinA: expected {expected.MinA}, got {actual.MinA}"); - Assert.True(expected.MaxA == actual.MaxA, $"{path}.MaxA: expected {expected.MaxA}, got {actual.MaxA}"); + Assert.True(BitEqual(expected.MinA, actual.MinA), $"{path}.MinA: expected {expected.MinA}, got {actual.MinA}"); + Assert.True(BitEqual(expected.MaxA, actual.MaxA), $"{path}.MaxA: expected {expected.MaxA}, got {actual.MaxA}"); AssertVector3Equal(expected.B, actual.B, $"{path}.B"); - Assert.True(expected.MinB == actual.MinB, $"{path}.MinB: expected {expected.MinB}, got {actual.MinB}"); - Assert.True(expected.MaxB == actual.MaxB, $"{path}.MaxB: expected {expected.MaxB}, got {actual.MaxB}"); + Assert.True(BitEqual(expected.MinB, actual.MinB), $"{path}.MinB: expected {expected.MinB}, got {actual.MinB}"); + Assert.True(BitEqual(expected.MaxB, actual.MaxB), $"{path}.MaxB: expected {expected.MaxB}, got {actual.MaxB}"); AssertVector3Equal(expected.C, actual.C, $"{path}.C"); - Assert.True(expected.MinC == actual.MinC, $"{path}.MinC: expected {expected.MinC}, got {actual.MinC}"); - Assert.True(expected.MaxC == actual.MaxC, $"{path}.MaxC: expected {expected.MaxC}, got {actual.MaxC}"); - Assert.True(expected.StartScale == actual.StartScale, $"{path}.StartScale: expected {expected.StartScale}, got {actual.StartScale}"); - Assert.True(expected.FinalScale == actual.FinalScale, $"{path}.FinalScale: expected {expected.FinalScale}, got {actual.FinalScale}"); - Assert.True(expected.ScaleRand == actual.ScaleRand, $"{path}.ScaleRand: expected {expected.ScaleRand}, got {actual.ScaleRand}"); - Assert.True(expected.StartTrans == actual.StartTrans, $"{path}.StartTrans: expected {expected.StartTrans}, got {actual.StartTrans}"); - Assert.True(expected.FinalTrans == actual.FinalTrans, $"{path}.FinalTrans: expected {expected.FinalTrans}, got {actual.FinalTrans}"); - Assert.True(expected.TransRand == actual.TransRand, $"{path}.TransRand: expected {expected.TransRand}, got {actual.TransRand}"); + Assert.True(BitEqual(expected.MinC, actual.MinC), $"{path}.MinC: expected {expected.MinC}, got {actual.MinC}"); + Assert.True(BitEqual(expected.MaxC, actual.MaxC), $"{path}.MaxC: expected {expected.MaxC}, got {actual.MaxC}"); + Assert.True(BitEqual(expected.StartScale, actual.StartScale), $"{path}.StartScale: expected {expected.StartScale}, got {actual.StartScale}"); + Assert.True(BitEqual(expected.FinalScale, actual.FinalScale), $"{path}.FinalScale: expected {expected.FinalScale}, got {actual.FinalScale}"); + Assert.True(BitEqual(expected.ScaleRand, actual.ScaleRand), $"{path}.ScaleRand: expected {expected.ScaleRand}, got {actual.ScaleRand}"); + Assert.True(BitEqual(expected.StartTrans, actual.StartTrans), $"{path}.StartTrans: expected {expected.StartTrans}, got {actual.StartTrans}"); + Assert.True(BitEqual(expected.FinalTrans, actual.FinalTrans), $"{path}.FinalTrans: expected {expected.FinalTrans}, got {actual.FinalTrans}"); + Assert.True(BitEqual(expected.TransRand, actual.TransRand), $"{path}.TransRand: expected {expected.TransRand}, got {actual.TransRand}"); Assert.True(expected.IsParentLocal == actual.IsParentLocal, $"{path}.IsParentLocal: expected {expected.IsParentLocal}, got {actual.IsParentLocal}"); } @@ -177,11 +177,11 @@ public static class ObjectMeshDataEquality { Assert.True(expected is not null, $"{path}: expected null but actual was non-null"); Assert.True(actual is not null, $"{path}: expected non-null but actual was null"); AssertVector3Equal(expected.Origin, actual.Origin, $"{path}.Origin"); - Assert.True(expected.Radius == actual.Radius, $"{path}.Radius: expected {expected.Radius}, got {actual.Radius}"); + Assert.True(BitEqual(expected.Radius, actual.Radius), $"{path}.Radius: expected {expected.Radius}, got {actual.Radius}"); } private static void AssertVector3Equal(System.Numerics.Vector3 expected, System.Numerics.Vector3 actual, string path) { - Assert.True(expected == actual, $"{path}: expected {expected}, got {actual}"); + Assert.True(BitEqual(expected, actual), $"{path}: expected {expected}, got {actual}"); } private static void AssertVector3ArrayEqual(System.Numerics.Vector3[] expected, System.Numerics.Vector3[] actual, string path) { @@ -191,6 +191,31 @@ public static class ObjectMeshDataEquality { } private static void AssertMatrixEqual(System.Numerics.Matrix4x4 expected, System.Numerics.Matrix4x4 actual, string path) { - Assert.True(expected == actual, $"{path}: expected {expected}, got {actual}"); + Assert.True(BitEqual(expected, actual), $"{path}: expected {expected}, got {actual}"); } + + // ---- bitwise float equality ---------------------------------------------- + // The pak stores raw IEEE-754 bits, so "round-trip preserved the field" + // means BIT equality, not `==` semantics: `==` is false for NaN==NaN + // (a NaN payload surviving the round-trip would FAIL a correct + // serializer) and true for -0.0==+0.0 (a sign-bit flip would silently + // PASS). Review finding 9. + + private static bool BitEqual(float a, float b) => + BitConverter.SingleToInt32Bits(a) == BitConverter.SingleToInt32Bits(b); + + private static bool BitEqual(double a, double b) => + BitConverter.DoubleToInt64Bits(a) == BitConverter.DoubleToInt64Bits(b); + + private static bool BitEqual(System.Numerics.Vector2 a, System.Numerics.Vector2 b) => + BitEqual(a.X, b.X) && BitEqual(a.Y, b.Y); + + private static bool BitEqual(System.Numerics.Vector3 a, System.Numerics.Vector3 b) => + BitEqual(a.X, b.X) && BitEqual(a.Y, b.Y) && BitEqual(a.Z, b.Z); + + private static bool BitEqual(System.Numerics.Matrix4x4 a, System.Numerics.Matrix4x4 b) => + BitEqual(a.M11, b.M11) && BitEqual(a.M12, b.M12) && BitEqual(a.M13, b.M13) && BitEqual(a.M14, b.M14) && + BitEqual(a.M21, b.M21) && BitEqual(a.M22, b.M22) && BitEqual(a.M23, b.M23) && BitEqual(a.M24, b.M24) && + BitEqual(a.M31, b.M31) && BitEqual(a.M32, b.M32) && BitEqual(a.M33, b.M33) && BitEqual(a.M34, b.M34) && + BitEqual(a.M41, b.M41) && BitEqual(a.M42, b.M42) && BitEqual(a.M43, b.M43) && BitEqual(a.M44, b.M44); } diff --git a/tests/AcDream.Content.Tests/PakRoundTripTests.cs b/tests/AcDream.Content.Tests/PakRoundTripTests.cs index b46ff594..20f6db43 100644 --- a/tests/AcDream.Content.Tests/PakRoundTripTests.cs +++ b/tests/AcDream.Content.Tests/PakRoundTripTests.cs @@ -212,4 +212,194 @@ public class PakRoundTripTests : IDisposable { Assert.Equal(0u, reader.Header.TocCount); Assert.False(reader.ContainsKey(PakKey.Compose(PakAssetType.GfxObjMesh, 1))); } + + // ---- format-version enforcement (review finding 2) --------------------- + + [Fact] + public void Writer_StampsFormatVersion_IgnoringTemplate() { + // The default-0 footgun: a caller building a header template without + // setting FormatVersion must still produce a CURRENT-version pak. + var path = NewTempPakPath(); + using (var writer = new PakWriter(path, new PakHeader { BakeToolVersion = 1 })) { + writer.AddBlob(PakKey.Compose(PakAssetType.GfxObjMesh, 1), MakeSyntheticData(1, 1)); + writer.Finish(); + } + + using var reader = new PakReader(path); + Assert.Equal(PakFormat.CurrentFormatVersion, reader.Header.FormatVersion); + } + + [Theory] + [InlineData(0u)] + [InlineData(2u)] + public void Reader_RejectsWrongFormatVersion(uint wrongVersion) { + var path = NewTempPakPath(); + WritePak(path, MakeBlobSet(2)); + + // Patch the header's formatVersion dword (offset 4) in place. + using (var fs = new FileStream(path, FileMode.Open, FileAccess.ReadWrite)) { + fs.Position = 4; + Span buf = stackalloc byte[4]; + System.Buffers.Binary.BinaryPrimitives.WriteUInt32LittleEndian(buf, wrongVersion); + fs.Write(buf); + } + + var ex = Assert.Throws(() => new PakReader(path)); + Assert.Contains($"version {wrongVersion}", ex.Message); + Assert.Contains($"version {PakFormat.CurrentFormatVersion}", ex.Message); + } + + // ---- reader robustness (review finding 3) ------------------------------- + + [Fact] + public void CorruptTocEntry_TreatedAsMissing_SiblingsUnaffected_NoThrow() { + var path = NewTempPakPath(); + var blobs = MakeBlobSet(4); + WritePak(path, blobs); + + // Patch the FIRST victim entry's length field (entry offset +16) to a + // value that runs past the file end. + var victimKey = PakKey.Compose(blobs[0].Type, blobs[0].FileId); + long entryPos = FindTocEntryPosition(path, victimKey); + using (var fs = new FileStream(path, FileMode.Open, FileAccess.ReadWrite)) { + fs.Position = entryPos + 16; + Span buf = stackalloc byte[4]; + System.Buffers.Binary.BinaryPrimitives.WriteUInt32LittleEndian(buf, 0x7FFF_FFF0u); + fs.Write(buf); + } + + using var reader = new PakReader(path); // must NOT throw — corrupt entry = missing + Assert.False(reader.ContainsKey(victimKey)); + Assert.False(reader.TryReadObjectMeshData(victimKey, out var data)); + Assert.Null(data); + + for (int i = 1; i < blobs.Length; i++) { + var key = PakKey.Compose(blobs[i].Type, blobs[i].FileId); + Assert.True(reader.TryReadObjectMeshData(key, out var sibling), $"sibling blob {i} should be unaffected"); + ObjectMeshDataEquality.AssertEqual(blobs[i].Data, sibling); + } + } + + [Fact] + public void TruncatedPak_RefusedAtOpen() { + var path = NewTempPakPath(); + WritePak(path, MakeBlobSet(4)); + + // Chop the file mid-TOC: the header claims 4 entries the file no + // longer holds — a structural fault, refused at open. + using (var fs = new FileStream(path, FileMode.Open, FileAccess.ReadWrite)) { + fs.SetLength(fs.Length - PakTocEntry.Size - 4); + } + + var ex = Assert.Throws(() => new PakReader(path)); + Assert.Contains("truncated", ex.Message); + } + + [Fact] + public void HalfWrittenPak_UnfinalizedHeader_RefusedAtOpen() { + var path = NewTempPakPath(); + // Simulate a bake crash between the placeholder-header write and + // Finish(): current-version header with TocOffset still 0, blob bytes + // behind it. + using (var fs = new FileStream(path, FileMode.Create, FileAccess.Write)) { + new PakHeader { FormatVersion = PakFormat.CurrentFormatVersion }.WriteTo(fs); + var junk = new byte[256]; + new Random(42).NextBytes(junk); + fs.Write(junk); + } + + var ex = Assert.Throws(() => new PakReader(path)); + Assert.Contains("unfinalized", ex.Message); + } + + [Fact] + public void MalformedBlobBehindValidCrc_TreatedAsMissing_SiblingsUnaffected() { + var path = NewTempPakPath(); + var blobs = MakeBlobSet(3); + WritePak(path, blobs); + + // Corrupt the FIRST blob's STRUCTURE (vertices count:i32 at blob + // offset 9 -> -1) and then RE-COMPUTE the CRC over the tampered bytes + // so the CRC tripwire passes — only the deserialization catch can + // save the read now. + var victimKey = PakKey.Compose(blobs[0].Type, blobs[0].FileId); + long entryPos = FindTocEntryPosition(path, victimKey); + using (var fs = new FileStream(path, FileMode.Open, FileAccess.ReadWrite)) { + // Read the victim entry to get offset+length. + fs.Position = entryPos; + var entryBuf = new byte[PakTocEntry.Size]; + fs.ReadExactly(entryBuf); + var entry = PakTocEntry.ReadFrom((ReadOnlySpan)entryBuf); + + // Tamper: vertices count -> -1 (ObjectId u64 + IsSetup byte = offset 9). + fs.Position = (long)entry.Offset + 9; + Span negOne = stackalloc byte[4]; + System.Buffers.Binary.BinaryPrimitives.WriteInt32LittleEndian(negOne, -1); + fs.Write(negOne); + + // Recompute CRC over the tampered blob region. + fs.Position = (long)entry.Offset; + var blobBytes = new byte[entry.Length]; + fs.ReadExactly(blobBytes); + uint newCrc = Crc32.Compute(blobBytes); + + // Patch the TOC entry's crc32 field (entry offset +20). + fs.Position = entryPos + 20; + Span crcBuf = stackalloc byte[4]; + System.Buffers.Binary.BinaryPrimitives.WriteUInt32LittleEndian(crcBuf, newCrc); + fs.Write(crcBuf); + } + + using var reader = new PakReader(path); + Assert.False(reader.TryReadObjectMeshData(victimKey, out var data), + "a structurally-malformed blob behind a matching CRC must be treated as missing"); + Assert.Null(data); + // Second read: still missing, no throw (verdict cached). + Assert.False(reader.TryReadObjectMeshData(victimKey, out _)); + + for (int i = 1; i < blobs.Length; i++) { + var key = PakKey.Compose(blobs[i].Type, blobs[i].FileId); + Assert.True(reader.TryReadObjectMeshData(key, out var sibling), $"sibling blob {i} should be unaffected"); + ObjectMeshDataEquality.AssertEqual(blobs[i].Data, sibling); + } + } + + // ---- writer disposal safety (review finding 5) --------------------------- + + [Fact] + public void WriterDisposedAfterAddBlobException_ReleasesFileHandle() { + var path = NewTempPakPath(); + var data = MakeSyntheticData(1, 1); + var key = PakKey.Compose(PakAssetType.GfxObjMesh, 1); + + var writer = new PakWriter(path, new PakHeader { BakeToolVersion = 1 }); + writer.AddBlob(key, data); + Assert.Throws(() => writer.AddBlob(key, data)); // duplicate key mid-AddBlob + writer.Dispose(); + + // The stream must be closed even after the AddBlob exception: the + // file is deletable (no leaked/locked handle). + File.Delete(path); + Assert.False(File.Exists(path)); + } + + // ---- helpers ------------------------------------------------------------- + + /// Locates the on-disk file position of the TOC entry for by raw parsing. + private static long FindTocEntryPosition(string path, ulong key) { + using var fs = new FileStream(path, FileMode.Open, FileAccess.Read); + var headerBuf = new byte[PakHeader.Size]; + fs.ReadExactly(headerBuf); + var header = PakHeader.ReadFrom((ReadOnlySpan)headerBuf); + + var entryBuf = new byte[PakTocEntry.Size]; + for (uint i = 0; i < header.TocCount; i++) { + long pos = (long)header.TocOffset + i * PakTocEntry.Size; + fs.Position = pos; + fs.ReadExactly(entryBuf); + var entry = PakTocEntry.ReadFrom((ReadOnlySpan)entryBuf); + if (entry.Key == key) return pos; + } + throw new InvalidOperationException($"TOC entry for key 0x{key:X16} not found"); + } }