Clamp both bounds of slice to avoid trapping on truncated records - #6
Merged
Conversation
slice(_:) clamped only its upper bound, so a field whose range began past the end of a short line produced an inverted range and an uncatchable runtime failure. A 13-byte airport record was enough to take down the host app. Clamping both bounds makes an out-of-range field read as empty, which the record parsers already handle: their required-field guards throw CIFPError.missingRequiredField, so the bad record reaches errorCallback like any other parse failure instead of killing the process. This matches the short-line tolerance the single-byte field reads have had all along. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QZ3UWydnavm3g9Gs62Xirb
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug
RandomAccessCollection.slice(_:)(Sources/SwiftCIFP/Parser/ByteParsing.swift)clamped its upper bound but not its lower bound:
When
range.lowerBoundexceedscount,lower > upperand the subscript trapswith
Fatal error: Range requires lowerBound <= upperBound— an uncatchable_assertionFailure, not a Swift error.This is reachable from ordinary input.
CIFPByteParser.parseRecordonly checksbytes.count >= 12(and>= 13for theP/Hsubsections) before dispatching,but the record parsers then slice far past that — up to lower bound 117 in
+PathPoint.swift, 112 in+Heliport.swiftand+Procedure.swift, 98 in+Waypoint.swift, 95 in+Airport.swift, 93 in+Navaid.swiftand+Airspace.swift. A 13-byte airport record (SUSAP KLAXK2A) reachesparseAirport'sbytes.slice(27..<30)and takes down the process.The trap defeats the library's own error design:
CIFP.init(data:…)wrapsparseRecordindo/catchand routes failures toerrorCallback, deliberatelymaking a malformed record non-fatal for the host app. A trap bypasses that entirely.
This is a pre-existing bug in short-line handling. Its trigger is line
truncation — nothing to do with any particular NASR/CIFP cycle or with field
widening.
The fix
Clamp both bounds, so an out-of-range field reads as an empty subsequence:
Empty is the right contract here, verified against the callers rather than
assumed. Single-byte field reads in the same files already tolerate short lines
the same way —
bytes.count > 28 ? bytes[bytes.startIndex + 28] : ASCII.space(
+Navaid.swift:25,+Procedure.swift:58, and ~40 more) — so short lines wereanticipated for scalar reads and missed only for slices. And every record parser
except one guards a mandatory field that lives past column 12 (coordinate,
magnetic variation, elevation, sequence number). On a truncated line those
now-empty slices parse to
niland the parser throwsCIFPError.missingRequiredField, which reacheserrorCallbacklike any otherparse failure. Empty slices restore the intended error path rather than papering
over it.
One caller that does not report a truncated record — flagging, not fixing
parsePathPointPrimary(Sources/SwiftCIFP/Parser/RecordParsers/CIFPByteParser+PathPoint.swift)is the only primary-record parser with no required-field guard. Every field it
reads is optional in the
PathPointmodel, so a line truncated after column 10yields a record with a real
airportIdandnilfor everything else, whichbuildPathPointsfolds into that airport with no error reported. That is asilent partial record, not a misparse — no field takes a wrong value — and it is
strictly better than today's trap, but it is worth knowing about.
I left it alone deliberately: deciding which fields make a
PathPointvalid is achange to the record model's contract, not part of fixing the trap.
PathPoint.coordinateisCoordinate?by design, so a sparse path point isrepresentable on purpose and a guard would need a considered rule about what
"required" means here. Happy to follow up if you want one.
The two continuation parsers (
parsePathPointContinuation,parseApproachContinuation) are also unguarded, but their payloads areall-optional by construction — a continuation carrying nothing merges nothing.
Tests
Two tests, both in
Tests/SwiftCIFPTests/SwiftCIFPTests.swift:returns an empty slice for a range beginning past the end, added to theexisting
ByteParsing testssuite next toslices a byte range.reports a truncated record through the error callback, which drives thepublic
CIFP(data:errorCallback:)entry point with a truncated airport recordand asserts the error arrives on the callback for line 1. Without the fix this
test does not fail — it kills the test runner.
I also ran a throwaway fuzz over every section and subsection code at every
truncation length from 0 to 140 bytes: it trapped before the fix and passes
after. It is not included in the PR, since
slice(_:)being total is now whatmakes all those paths safe and a broad fuzz adds little over the targeted tests.
Verification
CHANGELOG.mdgains an## [Unreleased]section. No version bump, no tag.🤖 Generated with Claude Code
https://claude.ai/code/session_01QZ3UWydnavm3g9Gs62Xirb