From 68f7a2eda6b2345e139887e8a72bf68e6e2de832 Mon Sep 17 00:00:00 2001 From: Andrei Errapart Date: Tue, 25 Aug 2026 20:36:09 +0200 Subject: [PATCH] PcbDocReader: length-frame shape-based records; fix trailing-NUL misdetection ParseShapeBasedRegion guessed whether the properties block carries a trailing NUL by testing the next byte for zero. That misfires when a record has no trailing NUL and its outline count is a multiple of 256 (low byte 0x00): the count byte is consumed as the NUL, every following field shifts by one, and a garbage hole count sends the parser off the end of the storage. An Agilex board (MicroModule) hits this with a 257-vertex mid-layer region - one record in 9403 - and because BitConverter reports the overrun as a plain ArgumentException, which the catch filter did not cover, the whole PcbDoc read crashed. The records are length-framed, so use it: the SubRecord-1 length (read and previously discarded) now bounds the parse. The trailing-NUL choice is disambiguated by checking which variant makes the outline plus the length-prefixed holes land exactly on the record end, preferring the NUL when both would (preserving the old behaviour for every unambiguous record). Each record resyncs to its recorded end afterwards, so a single odd record degrades to a per-record diagnostic instead of poisoning the walk, and the outer catch now covers ArgumentException/EndOfStream so a truly corrupt storage degrades to a warning. Verified on MicroModule.PcbDoc: full read (1509 components, 9403 regions, no diagnostics) and byte-identical round-trip of both shape-based storages; the existing suite passes (857 tests). --- .../Serialization/Readers/PcbDocReader.cs | 64 +++++++++++++++++-- 1 file changed, 58 insertions(+), 6 deletions(-) diff --git a/src/OriginalCircuit.Altium/Serialization/Readers/PcbDocReader.cs b/src/OriginalCircuit.Altium/Serialization/Readers/PcbDocReader.cs index fcbeff0..bd34693 100644 --- a/src/OriginalCircuit.Altium/Serialization/Readers/PcbDocReader.cs +++ b/src/OriginalCircuit.Altium/Serialization/Readers/PcbDocReader.cs @@ -964,20 +964,31 @@ private void ReadShapeBased(CompoundFileAccessor accessor, string storageName, b try { while (pos < data.Length && data[pos] == typeByte) - target.Add(ParseShapeBasedRegion(data, ref pos)); + { + target.Add(ParseShapeBasedRegion(data, ref pos, out var consumedExactly)); + if (!consumedExactly) + _diagnostics.Add(new AltiumDiagnostic(DiagnosticSeverity.Warning, + $"{storageName}: record {target.Count - 1} did not parse to its recorded " + + "length; skipped to the next record.", storageName)); + } } - catch (Exception ex) when (ex is IndexOutOfRangeException or ArgumentOutOfRangeException) + catch (Exception ex) when (ex is IndexOutOfRangeException or ArgumentException or EndOfStreamException) { + // BitConverter reports a short buffer as a plain ArgumentException, so the broad + // filter is deliberate: a truncated storage degrades to a diagnostic, not a crash. _diagnostics.Add(new AltiumDiagnostic(DiagnosticSeverity.Warning, $"Failed to fully parse {storageName}: {ex.Message}", storageName)); } } - private static PcbShapeBasedRegion ParseShapeBasedRegion(byte[] data, ref int pos) + private static PcbShapeBasedRegion ParseShapeBasedRegion(byte[] data, ref int pos, out bool consumedExactly) { var r = new PcbShapeBasedRegion { TypeByte = data[pos] }; pos += 1; // type byte (0x0B / 0x0C) - pos += 4; // SubRecord-1 length (derived on write = body length) + var bodyLength = BitConverter.ToInt32(data, pos); pos += 4; // SubRecord-1 length (derived on write = body length) + if (bodyLength < 0 || pos + bodyLength > data.Length) + throw new EndOfStreamException("Shape-based record length overruns the storage."); + var recordEnd = pos + bodyLength; r.Layer = data[pos]; pos += 1; r.Flags1 = data[pos]; pos += 1; r.Flags2 = data[pos]; pos += 1; @@ -999,7 +1010,19 @@ private static PcbShapeBasedRegion ParseShapeBasedRegion(byte[] data, ref int po ? new KeyValuePair(seg, null) : new KeyValuePair(seg[..eq], seg[(eq + 1)..])); } - if (pos < data.Length && data[pos] == 0) { r.PropsHasTrailingNull = true; pos += 1; } + + // The properties block may or may not carry a trailing NUL. A lone "next byte is 0" + // test misfires when there is no NUL and the outline count's low byte happens to be + // zero (count 256, 512, ...), which shifts the rest of the record by one byte. The + // recorded body length disambiguates: accept the variant whose outline and holes + // land exactly on the record end. + if (data[pos] == 0) + { + var withNull = LandsOnRecordEnd(data, pos + 1, holeCount, recordEnd); + var withoutNull = LandsOnRecordEnd(data, pos, holeCount, recordEnd); + r.PropsHasTrailingNull = withNull || !withoutNull; + } + if (r.PropsHasTrailingNull) pos += 1; var outlineCount = BitConverter.ToUInt32(data, pos); pos += 4; for (var i = 0; i <= outlineCount; i++) // disk count is N, but N+1 vertices are stored { @@ -1012,17 +1035,46 @@ private static PcbShapeBasedRegion ParseShapeBasedRegion(byte[] data, ref int po }; pos += 37; r.Outline.Add(v); + if (pos > recordEnd) + break; } - for (var h = 0; h < holeCount; h++) + for (var h = 0; h < holeCount && pos + 4 <= recordEnd; h++) { var hv = BitConverter.ToUInt32(data, pos); pos += 4; + if (pos + 16L * hv > recordEnd) + break; var hole = new List<(double, double)>((int)hv); for (var i = 0; i < hv; i++) { hole.Add((BitConverter.ToDouble(data, pos), BitConverter.ToDouble(data, pos + 8))); pos += 16; } r.Holes.Add(hole); } + consumedExactly = pos == recordEnd; + pos = recordEnd; // records are length-framed; always resync so one oddity cannot poison the walk return r; } + /// + /// Whether reading an outline count at followed by its + /// 37-byte vertices and length-prefixed holes ends exactly + /// at . + /// + private static bool LandsOnRecordEnd(byte[] data, int countPos, int holeCount, int recordEnd) + { + if (countPos + 4 > recordEnd) + return false; + var outlineCount = BitConverter.ToUInt32(data, countPos); + var p = countPos + 4 + 37L * (outlineCount + 1); + for (var h = 0; h < holeCount; h++) + { + if (p + 4 > recordEnd) + return false; + var hv = BitConverter.ToUInt32(data, (int)p); + p += 4 + 16L * hv; + if (p > recordEnd) + return false; + } + return p == recordEnd; + } + private void ReadComponentBodies(CompoundFileAccessor accessor, PcbDocument document, CancellationToken cancellationToken) { ReadPrimitiveStorage(accessor, "ComponentBodies6", reader =>