From 19274aeb473cab737d90469f35aef33fea98e1c6 Mon Sep 17 00:00:00 2001 From: Phillip Hoff Date: Sun, 11 Oct 2026 11:18:25 -0700 Subject: [PATCH] Validate update control ranges in ApplyChanges An FSPC, FFPC, VRPC or SGCC index or count outside the base record's array made ApplyChanges throw ArgumentOutOfRangeException from System.Collections.Immutable, with nothing to identify the record or field. The splice helpers now check the range (S-57 Part 3, 8.4.3) and throw InvalidDataException naming the control field and the record's RCNM/RCID. Throwing rather than logging and skipping: a skipped splice would return a record with silently wrong geometry, and ApplyChanges has no logger. Arrays the update leaves untouched (no update data, or a delete on an empty array) are still skipped without a check, because SGCC controls both SG2D and SG3D. --- src/EncDotNet.S57/S57Document.cs | 99 ++++++-- .../S57DocumentApplyChangesTests.cs | 216 ++++++++++++++++++ 2 files changed, 300 insertions(+), 15 deletions(-) diff --git a/src/EncDotNet.S57/S57Document.cs b/src/EncDotNet.S57/S57Document.cs index 69bb4f3..731a6b8 100644 --- a/src/EncDotNet.S57/S57Document.cs +++ b/src/EncDotNet.S57/S57Document.cs @@ -83,7 +83,17 @@ public IEnumerable GetFeaturesByObjectCode(S57ObjectCode objec /// (FSPC, FFPC, VRPC, SGCC) when present. /// /// + /// + /// A control field's 1-based index and count must address entries that exist in the + /// base record's array (S-57 Part 3, 8.4.3). An insert may target any position from 1 + /// to one past the last entry; a delete or modify must lie entirely within the array. + /// A control field that falls outside these bounds means the update does not match + /// the base cell, and no partial result is returned. + /// /// + /// An update control field (FSPC, FFPC, VRPC or SGCC) + /// has an index or count outside the bounds of the base record's array. The message names + /// the record (RCNM/RCID) and the control field. public S57Document ApplyChanges(S57Document update) { var features = ApplyRecordChanges( @@ -186,8 +196,8 @@ private static S57FeatureRecord ApplyFeatureModify(S57FeatureRecord baseRecord, UpdateInstruction = updateRecord.UpdateInstruction, Attributes = MergeAttributes(baseRecord.Attributes, updateRecord.Attributes), NationalAttributes = MergeAttributes(baseRecord.NationalAttributes, updateRecord.NationalAttributes), - SpatialPointers = ApplyArrayUpdate([.. baseRecord.SpatialPointers], [.. updateRecord.SpatialPointers], updateRecord.SpatialPointerControl), - FeaturePointers = ApplyArrayUpdate([.. baseRecord.FeaturePointers], [.. updateRecord.FeaturePointers], updateRecord.FeaturePointerControl), + SpatialPointers = ApplyArrayUpdate([.. baseRecord.SpatialPointers], [.. updateRecord.SpatialPointers], updateRecord.SpatialPointerControl, baseRecord.RecordName, S57FieldTags.FSPC), + FeaturePointers = ApplyArrayUpdate([.. baseRecord.FeaturePointers], [.. updateRecord.FeaturePointers], updateRecord.FeaturePointerControl, baseRecord.RecordName, S57FieldTags.FFPC), }; } @@ -199,9 +209,9 @@ private static S57VectorRecord ApplyVectorModify(S57VectorRecord baseRecord, S57 RecordVersion = updateRecord.RecordVersion, UpdateInstruction = updateRecord.UpdateInstruction, Attributes = MergeAttributes(baseRecord.Attributes, updateRecord.Attributes), - VectorPointers = ApplyArrayUpdate([.. baseRecord.VectorPointers], [.. updateRecord.VectorPointers], updateRecord.VectorPointerControl), - Coordinates2D = ApplyArrayUpdate([.. baseRecord.Coordinates2D], [.. updateRecord.Coordinates2D], updateRecord.CoordinateControl), - Soundings = ApplyArrayUpdate([.. baseRecord.Soundings], [.. updateRecord.Soundings], updateRecord.CoordinateControl), + VectorPointers = ApplyArrayUpdate([.. baseRecord.VectorPointers], [.. updateRecord.VectorPointers], updateRecord.VectorPointerControl, baseRecord.RecordName, S57FieldTags.VRPC), + Coordinates2D = ApplyArrayUpdate([.. baseRecord.Coordinates2D], [.. updateRecord.Coordinates2D], updateRecord.CoordinateControl, baseRecord.RecordName, S57FieldTags.SGCC), + Soundings = ApplyArrayUpdate([.. baseRecord.Soundings], [.. updateRecord.Soundings], updateRecord.CoordinateControl, baseRecord.RecordName, S57FieldTags.SGCC), }; } @@ -262,7 +272,9 @@ private static IReadOnlyList MergeAttributes( private static ImmutableArray ApplyArrayUpdate( ImmutableArray baseArray, ImmutableArray updateData, - S57FieldUpdateControl? control) + S57FieldUpdateControl? control, + S57RecordName recordName, + string controlFieldTag) where T : struct { if (control is null) @@ -275,16 +287,25 @@ private static ImmutableArray ApplyArrayUpdate( // Convert from 1-based S-57 index to 0-based. int index = ctrl.Index - 1; + // SGCC controls both SG2D and SG3D, so the splice helpers leave an array untouched when + // the update carries no data for it (or, for a delete, the base has none) before + // checking the range against it. return ctrl.UpdateInstruction switch { - S57UpdateInstruction.Insert => SpliceInsert(baseArray, updateData, index), - S57UpdateInstruction.Delete => SpliceDelete(baseArray, index, ctrl.Count), - S57UpdateInstruction.Modify => SpliceModify(baseArray, updateData, index, ctrl.Count), + S57UpdateInstruction.Insert => SpliceInsert(baseArray, updateData, index, ctrl, recordName, controlFieldTag), + S57UpdateInstruction.Delete => SpliceDelete(baseArray, index, ctrl, recordName, controlFieldTag), + S57UpdateInstruction.Modify => SpliceModify(baseArray, updateData, index, ctrl, recordName, controlFieldTag), _ => baseArray }; } - private static ImmutableArray SpliceInsert(ImmutableArray baseArray, ImmutableArray items, int index) + private static ImmutableArray SpliceInsert( + ImmutableArray baseArray, + ImmutableArray items, + int index, + S57FieldUpdateControl ctrl, + S57RecordName recordName, + string controlFieldTag) where T : struct { if (items.IsDefaultOrEmpty) @@ -292,21 +313,40 @@ private static ImmutableArray SpliceInsert(ImmutableArray baseArray, Im return baseArray; } + // An insert may target any existing position or one past the end (an append). + if (index < 0 || index > baseArray.Length) + { + throw OutOfRange(ctrl, baseArray.Length, recordName, controlFieldTag); + } + return baseArray.InsertRange(index, items); } - private static ImmutableArray SpliceDelete(ImmutableArray baseArray, int index, int count) + private static ImmutableArray SpliceDelete( + ImmutableArray baseArray, + int index, + S57FieldUpdateControl ctrl, + S57RecordName recordName, + string controlFieldTag) where T : struct { - if (count <= 0 || baseArray.IsDefaultOrEmpty) + if (ctrl.Count <= 0 || baseArray.IsDefaultOrEmpty) { return baseArray; } - return baseArray.RemoveRange(index, count); + EnsureRangeInBounds(baseArray.Length, index, ctrl, recordName, controlFieldTag); + + return baseArray.RemoveRange(index, ctrl.Count); } - private static ImmutableArray SpliceModify(ImmutableArray baseArray, ImmutableArray items, int index, int count) + private static ImmutableArray SpliceModify( + ImmutableArray baseArray, + ImmutableArray items, + int index, + S57FieldUpdateControl ctrl, + S57RecordName recordName, + string controlFieldTag) where T : struct { if (items.IsDefaultOrEmpty) @@ -314,8 +354,37 @@ private static ImmutableArray SpliceModify(ImmutableArray baseArray, Im return baseArray; } + EnsureRangeInBounds(baseArray.Length, index, ctrl, recordName, controlFieldTag); + // Modify = delete the existing range, then insert the new items at the same position. - var withRemoved = baseArray.RemoveRange(index, count); + var withRemoved = baseArray.RemoveRange(index, ctrl.Count); return withRemoved.InsertRange(index, items); } + + private static void EnsureRangeInBounds( + int length, + int index, + S57FieldUpdateControl ctrl, + S57RecordName recordName, + string controlFieldTag) + { + // Written as a subtraction so a large count cannot overflow index + count. + if (index < 0 || index > length || ctrl.Count < 0 || ctrl.Count > length - index) + { + throw OutOfRange(ctrl, length, recordName, controlFieldTag); + } + } + + private static InvalidDataException OutOfRange( + S57FieldUpdateControl ctrl, + int length, + S57RecordName recordName, + string controlFieldTag) + { + return new InvalidDataException( + $"Update control field {controlFieldTag} on record {recordName} " + + $"({ctrl.UpdateInstruction} at index {ctrl.Index}, count {ctrl.Count}) is outside " + + $"the base record's {length} entries (S-57 Part 3, 8.4.3). " + + "The update does not match the base cell."); + } } diff --git a/tests/EncDotNet.UnitTests/S57DocumentApplyChangesTests.cs b/tests/EncDotNet.UnitTests/S57DocumentApplyChangesTests.cs index b90a2c1..6c4a027 100644 --- a/tests/EncDotNet.UnitTests/S57DocumentApplyChangesTests.cs +++ b/tests/EncDotNet.UnitTests/S57DocumentApplyChangesTests.cs @@ -908,6 +908,222 @@ public void ApplyChanges_VrpcDelete_RemovesVectorPointersAtIndex() #endregion + #region Out-of-Range Update Controls + + [Fact] + public void ApplyChanges_SgccModifyBeyondBaseCoordinates_ThrowsNamingRecordAndField() + { + var baseDoc = new S57Document + { + FeatureRecords = ImmutableArray.Empty, + VectorRecords = + [ + CreateVector(1, coordinates: [Coord(10, 20), Coord(30, 40), Coord(50, 60)]) + ] + }; + var update = new S57Document + { + FeatureRecords = ImmutableArray.Empty, + VectorRecords = + [ + CreateVector(1, ruin: S57UpdateInstruction.Modify, + coordinateControl: new S57FieldUpdateControl + { + UpdateInstruction = S57UpdateInstruction.Modify, + Index = 2, + Count = 5 // only 2 coordinates from index 2 onward + }, + coordinates: [Coord(999, 888)]) + ] + }; + + var ex = Assert.Throws(() => baseDoc.ApplyChanges(update)); + + Assert.Contains(S57FieldTags.SGCC, ex.Message); + Assert.Contains($"RCNM={S57RecordNameCodes.Edge}, RCID=1", ex.Message); + } + + [Fact] + public void ApplyChanges_FspcDeleteBeyondBasePointers_ThrowsNamingRecordAndField() + { + var baseDoc = new S57Document + { + FeatureRecords = + [ + CreateFeature(7, + spatialPointers: + [ + SpatialPtr(S57RecordNameCodes.Edge, 1), + SpatialPtr(S57RecordNameCodes.Edge, 2) + ]) + ], + VectorRecords = ImmutableArray.Empty + }; + var update = new S57Document + { + FeatureRecords = + [ + CreateFeature(7, ruin: S57UpdateInstruction.Modify, + spatialPointerControl: new S57FieldUpdateControl + { + UpdateInstruction = S57UpdateInstruction.Delete, + Index = 3, // past the last of 2 pointers + Count = 1 + }) + ], + VectorRecords = ImmutableArray.Empty + }; + + var ex = Assert.Throws(() => baseDoc.ApplyChanges(update)); + + Assert.Contains(S57FieldTags.FSPC, ex.Message); + Assert.Contains($"RCNM={S57RecordNameCodes.Feature}, RCID=7", ex.Message); + } + + [Fact] + public void ApplyChanges_FfpcInsertBeyondAppendPosition_ThrowsNamingRecordAndField() + { + var basePtr = new S57FeaturePointer + { + Name = S57RecordName.FromRcnmRcid(S57RecordNameCodes.Feature, 100), + Relationship = S57RelationshipIndicator.Master, + Comment = "" + }; + var newPtr = basePtr with { Name = S57RecordName.FromRcnmRcid(S57RecordNameCodes.Feature, 200) }; + var baseDoc = new S57Document + { + FeatureRecords = [CreateFeature(1, featurePointers: [basePtr])], + VectorRecords = ImmutableArray.Empty + }; + var update = new S57Document + { + FeatureRecords = + [ + CreateFeature(1, ruin: S57UpdateInstruction.Modify, + featurePointerControl: new S57FieldUpdateControl + { + UpdateInstruction = S57UpdateInstruction.Insert, + Index = 3, // 1 pointer, so 2 (append) is the furthest valid position + Count = 1 + }, + featurePointers: [newPtr]) + ], + VectorRecords = ImmutableArray.Empty + }; + + var ex = Assert.Throws(() => baseDoc.ApplyChanges(update)); + + Assert.Contains(S57FieldTags.FFPC, ex.Message); + Assert.Contains($"RCNM={S57RecordNameCodes.Feature}, RCID=1", ex.Message); + } + + [Fact] + public void ApplyChanges_VrpcIndexZero_Throws() + { + var ptr = new S57VectorPointer + { + Name = S57RecordName.FromRcnmRcid(S57RecordNameCodes.ConnectedNode, 1), + Orientation = S57Orientation.Forward, + Usage = S57UsageIndicator.NotApplicable, + Topology = S57TopologyIndicator.Beginning, + Mask = S57MaskingIndicator.NotApplicable + }; + var baseDoc = new S57Document + { + FeatureRecords = ImmutableArray.Empty, + VectorRecords = [CreateVector(10, vectorPointers: [ptr])] + }; + var update = new S57Document + { + FeatureRecords = ImmutableArray.Empty, + VectorRecords = + [ + CreateVector(10, ruin: S57UpdateInstruction.Modify, + vectorPointerControl: new S57FieldUpdateControl + { + UpdateInstruction = S57UpdateInstruction.Delete, + Index = 0, // indices are 1-based + Count = 1 + }) + ] + }; + + var ex = Assert.Throws(() => baseDoc.ApplyChanges(update)); + + Assert.Contains(S57FieldTags.VRPC, ex.Message); + } + + [Fact] + public void ApplyChanges_SgccModifyCoveringLastCoordinate_Succeeds() + { + var baseDoc = new S57Document + { + FeatureRecords = ImmutableArray.Empty, + VectorRecords = + [ + CreateVector(1, coordinates: [Coord(10, 20), Coord(30, 40), Coord(50, 60)]) + ] + }; + var update = new S57Document + { + FeatureRecords = ImmutableArray.Empty, + VectorRecords = + [ + CreateVector(1, ruin: S57UpdateInstruction.Modify, + coordinateControl: new S57FieldUpdateControl + { + UpdateInstruction = S57UpdateInstruction.Modify, + Index = 2, + Count = 2 + }, + coordinates: [Coord(777, 0), Coord(888, 0)]) + ] + }; + + var result = baseDoc.ApplyChanges(update); + + var coords = result.VectorRecords[0].Coordinates2D; + Assert.Equal([10, 777, 888], coords.Select(c => c.X)); + } + + [Fact] + public void ApplyChanges_FfpcInsertAtAppendPosition_Succeeds() + { + var basePtr = new S57FeaturePointer + { + Name = S57RecordName.FromRcnmRcid(S57RecordNameCodes.Feature, 100), + Relationship = S57RelationshipIndicator.Master, + Comment = "" + }; + var newPtr = basePtr with { Name = S57RecordName.FromRcnmRcid(S57RecordNameCodes.Feature, 200) }; + var baseDoc = new S57Document + { + FeatureRecords = [CreateFeature(1, featurePointers: [basePtr])], + VectorRecords = ImmutableArray.Empty + }; + var update = new S57Document + { + FeatureRecords = + [ + CreateFeature(1, ruin: S57UpdateInstruction.Modify, + featurePointerControl: new S57FieldUpdateControl + { + UpdateInstruction = S57UpdateInstruction.Insert, + Index = 2, + Count = 1 + }, + featurePointers: [newPtr]) + ], + VectorRecords = ImmutableArray.Empty + }; + + var result = baseDoc.ApplyChanges(update); + + Assert.Equal([100, 200], result.FeatureRecords[0].FeaturePointers.Select(p => p.Name.RecordId)); + } + + #endregion + #region Empty Update Document [Fact]