From 857fe83459f462715be732ef8f08bd232d97a04d Mon Sep 17 00:00:00 2001 From: James Burton Date: Tue, 30 Jun 2026 21:01:45 +0100 Subject: [PATCH] fix(gguf): parse v2 counts/lengths as uint64 (spec-correct), not uint32 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GgufReader treated GGUF v2 as if it used uint32 tensor/metadata counts, string lengths and array lengths. Per the GGUF spec the uint32->uint64 switch happened at v1->v2: v2 and v3 are identical on the wire and both use uint64. (v1, the only uint32 form, is rejected by header validation.) Reading a uint32 (4 bytes) where a real v2 file writes a uint64 (8 bytes) misaligns the entire metadata + tensor-info parse. On TheBloke's Llama-2-13B-GGUF (a v2 file) this produced garbage tensor names and offsets, surfacing as a bogus "Tensor '' data extends beyond file boundary" in GgufFile.Open. The bug stayed latent because every model we had tested with was v3, and GgufTestData itself wrote v2 with uint32 — so the writer and reader shared the same wrong assumption and agreed with each other while disagreeing with the spec. Fix: read uint64 for counts, string lengths and array lengths (all supported versions). Fix GgufTestData to write spec-correct uint64 v2 so v2 tests actually exercise the real wire format. Add discriminating regression tests: a full v2 parse (header -> string-array metadata -> tensor infos, where misalignment compounds) and an end-to-end GgufFile.Open v2 case mirroring the Llama-2-13B failure. Verified against the gguf reference library, which parses the same file with uint64. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/DotLLM.Models/Gguf/GgufReader.cs | 28 +++++------ .../Models/Gguf/GgufFileTests.cs | 32 +++++++++++++ .../Models/Gguf/GgufReaderTests.cs | 48 +++++++++++++++++++ .../Models/Gguf/GgufTestData.cs | 21 ++------ 4 files changed, 96 insertions(+), 33 deletions(-) diff --git a/src/DotLLM.Models/Gguf/GgufReader.cs b/src/DotLLM.Models/Gguf/GgufReader.cs index 39f80c66..f0a0990a 100644 --- a/src/DotLLM.Models/Gguf/GgufReader.cs +++ b/src/DotLLM.Models/Gguf/GgufReader.cs @@ -5,7 +5,9 @@ namespace DotLLM.Models.Gguf; /// /// Static binary parser for the GGUF file format. Pure functions: bytes in, structs out. -/// Handles both GGUF v2 (uint32 counts) and v3 (uint64 counts). +/// Supports GGUF v2 and v3, which are identical on the wire: tensor/metadata counts, +/// string lengths and array lengths are all uint64. (The uint32 form belongs to +/// the obsolete v1, which the header validation rejects.) /// public static class GgufReader { @@ -30,19 +32,9 @@ public static GgufHeader ReadHeader(BinaryReader reader) throw new InvalidDataException( $"Unsupported GGUF version: {version}. Only versions 2 and 3 are supported."); - ulong tensorCount; - ulong metadataKvCount; - - if (version == 2) - { - tensorCount = reader.ReadUInt32(); - metadataKvCount = reader.ReadUInt32(); - } - else - { - tensorCount = reader.ReadUInt64(); - metadataKvCount = reader.ReadUInt64(); - } + // GGUF v2 and v3 both store these counts as uint64 (the uint32 form was v1 only). + ulong tensorCount = reader.ReadUInt64(); + ulong metadataKvCount = reader.ReadUInt64(); return new GgufHeader(version, tensorCount, metadataKvCount); } @@ -109,11 +101,12 @@ public static List ReadTensorInfos(BinaryReader reader, Gg } /// - /// Reads a GGUF length-prefixed UTF-8 string. V2 uses uint32 length, v3 uses uint64. + /// Reads a GGUF length-prefixed UTF-8 string. The length is uint64 in both supported + /// versions (v2 and v3); only the obsolete v1 used a uint32 length. /// internal static string ReadGgufString(BinaryReader reader, uint version) { - ulong length = version == 2 ? reader.ReadUInt32() : reader.ReadUInt64(); + ulong length = reader.ReadUInt64(); if (length == 0) return string.Empty; @@ -149,7 +142,8 @@ private static object ReadMetadataValue(BinaryReader reader, uint version, GgufV private static object ReadArray(BinaryReader reader, uint version) { var elementType = (GgufValueType)reader.ReadUInt32(); - ulong count = version == 2 ? reader.ReadUInt32() : reader.ReadUInt64(); + // Array length is uint64 in both supported versions (v2 and v3); v1 used uint32. + ulong count = reader.ReadUInt64(); if (count > int.MaxValue) throw new InvalidDataException($"GGUF array length {count} exceeds Int32.MaxValue."); diff --git a/tests/DotLLM.Tests.Unit/Models/Gguf/GgufFileTests.cs b/tests/DotLLM.Tests.Unit/Models/Gguf/GgufFileTests.cs index 1de928f7..82c4a57d 100644 --- a/tests/DotLLM.Tests.Unit/Models/Gguf/GgufFileTests.cs +++ b/tests/DotLLM.Tests.Unit/Models/Gguf/GgufFileTests.cs @@ -67,6 +67,38 @@ public void Open_WithTensors_ProvidesDataPointer() } } + [Fact] + public void Open_V2File_WithArrayMetadataAndTensors_Succeeds() + { + // Reproduces the real-world GGUF v2 failure (e.g. TheBloke/Llama-2-13B-GGUF): before the + // uint64 width fix, the reader mis-parsed v2 string/array lengths, producing garbage tensor + // offsets that tripped GgufFile.Open's "data extends beyond file boundary" guard (with an + // empty tensor name). A correctly parsed v2 file opens and exposes its tensors. + byte[] embd = new byte[8 * 16 * 4]; + for (int i = 0; i < embd.Length; i++) embd[i] = (byte)((i % 255) + 1); + + var data = new GgufTestData(version: 2) + .AddString("general.architecture", "llama") + .AddStringArray("tokenizer.ggml.tokens", ["", "", "hello", "world"]) + .AddTensor("token_embd.weight", [8, 16], 0, embd) // F32 + .AddTensor("output_norm.weight", [16], 0, new byte[16 * 4]); // F32 + string path = WriteTempGguf(data); + + using var file = GgufFile.Open(path); + + Assert.Equal(2u, file.Header.Version); + Assert.Equal(2ul, file.Header.TensorCount); + Assert.Equal("llama", file.Metadata.GetString("general.architecture")); + Assert.True(file.TensorsByName.ContainsKey("token_embd.weight")); + Assert.True(file.TensorsByName.ContainsKey("output_norm.weight")); + Assert.NotEqual(nint.Zero, file.DataBasePointer); + unsafe + { + byte* ptr = (byte*)file.DataBasePointer; + Assert.Equal(1, ptr[0]); // first byte of token_embd data + } + } + [Fact] public void Open_TensorsByName_LookupWorks() { diff --git a/tests/DotLLM.Tests.Unit/Models/Gguf/GgufReaderTests.cs b/tests/DotLLM.Tests.Unit/Models/Gguf/GgufReaderTests.cs index 03e83e7a..3584347b 100644 --- a/tests/DotLLM.Tests.Unit/Models/Gguf/GgufReaderTests.cs +++ b/tests/DotLLM.Tests.Unit/Models/Gguf/GgufReaderTests.cs @@ -193,6 +193,54 @@ public void ReadMetadata_V2_ScalarTypes() Assert.Equal(42u, metadata["count"].Value); } + /// + /// Regression for the GGUF v2 width bug: v2 stores counts, string lengths and array lengths + /// as uint64 (identical to v3 — the uint32 form was v1 only). The reader and the + /// in-memory test writer both previously treated v2 as uint32, so they agreed with each other + /// while disagreeing with the spec — and every real v2 file (e.g. TheBloke's Llama-2-13B-GGUF) + /// mis-parsed: a uint32 string length consumed 4 bytes where the file wrote 8, cascading through + /// the string-array metadata into garbage tensor names and offsets (the latter surfaced as a + /// bogus "data extends beyond file boundary" with an empty tensor name). A scalar-only v2 case + /// can't catch this — it needs a multi-element array followed by tensor infos so misalignment + /// compounds. This asserts a full v2 parse (header → array metadata → tensor infos) is exact. + /// + [Fact] + public void Reads_V2_FullFile_ArrayMetadataThenTensors_Exactly() + { + var data = new GgufTestData(version: 2) + .AddString("general.architecture", "llama") + .AddStringArray("tokenizer.ggml.tokens", ["", "", "hello", "world"]) + .AddUInt32("llama.block_count", 2) + .AddTensor("token_embd.weight", [8, 16], 0, new byte[8 * 16 * 4]) // F32 + .AddTensor("blk.0.attn_q.weight", [16, 16], 1, new byte[16 * 16 * 2]) // F16 + .AddTensor("output_norm.weight", [16], 0, new byte[16 * 4]); // F32 + byte[] bytes = data.Build(); + + using var stream = new MemoryStream(bytes); + using var reader = new BinaryReader(stream); + + var header = GgufReader.ReadHeader(reader); + Assert.Equal(2u, header.Version); + Assert.Equal(3ul, header.TensorCount); + Assert.Equal(3ul, header.MetadataKvCount); + + var metadata = GgufReader.ReadMetadata(reader, header); + Assert.Equal("llama", metadata["general.architecture"].Value); + Assert.Equal(["", "", "hello", "world"], + (string[])metadata["tokenizer.ggml.tokens"].Value); + Assert.Equal(2u, metadata["llama.block_count"].Value); + + var tensors = GgufReader.ReadTensorInfos(reader, header); + Assert.Equal(3, tensors.Count); + Assert.Equal("token_embd.weight", tensors[0].Name); + Assert.Equal("blk.0.attn_q.weight", tensors[1].Name); + Assert.Equal("output_norm.weight", tensors[2].Name); + // Offsets are sequential blobs (no alignment between them in the writer). + Assert.Equal(0ul, tensors[0].DataOffset); + Assert.Equal((ulong)(8 * 16 * 4), tensors[1].DataOffset); + Assert.Equal((ulong)(8 * 16 * 4 + 16 * 16 * 2), tensors[2].DataOffset); + } + #endregion #region Tensor Infos diff --git a/tests/DotLLM.Tests.Unit/Models/Gguf/GgufTestData.cs b/tests/DotLLM.Tests.Unit/Models/Gguf/GgufTestData.cs index 06a6f636..f9847749 100644 --- a/tests/DotLLM.Tests.Unit/Models/Gguf/GgufTestData.cs +++ b/tests/DotLLM.Tests.Unit/Models/Gguf/GgufTestData.cs @@ -144,20 +144,11 @@ public byte[] Build() _stream.SetLength(0); _stream.Position = 0; - // Header + // Header. GGUF v2 and v3 both store counts as uint64 (only the obsolete v1 used uint32). _writer.Write(GgufReader.GgufMagic); _writer.Write(_version); - - if (_version == 2) - { - _writer.Write((uint)_tensorWriters.Count); - _writer.Write((uint)_metadataWriters.Count); - } - else - { - _writer.Write((ulong)_tensorWriters.Count); - _writer.Write((ulong)_metadataWriters.Count); - } + _writer.Write((ulong)_tensorWriters.Count); + _writer.Write((ulong)_metadataWriters.Count); // Metadata foreach (var writeMetadata in _metadataWriters) @@ -200,10 +191,8 @@ private void WriteGgufString(BinaryWriter writer, string value) private void WriteLength(BinaryWriter writer, ulong length) { - if (_version == 2) - writer.Write((uint)length); - else - writer.Write(length); + // String and array lengths are uint64 in both supported versions (v2 and v3); v1 used uint32. + writer.Write(length); } private static long AlignUp(long value, uint alignment)