Bound the flat-ring map draw walk so an odd-length ring can't overrun#582
Open
patrickrb wants to merge 1 commit into
Open
Bound the flat-ring map draw walk so an odd-length ring can't overrun#582patrickrb wants to merge 1 commit into
patrickrb wants to merge 1 commit into
Conversation
The equirectangular land renderer (`MapScreen.drawWorldLand`), the azimuthal
land renderer (`MapScreen.drawAzimuthalLand`), and the POTA / QSO-path map
renderers (`PotaActivationMap.buildRingPath`, `QsoSheet.buildRingPath`) all
walked a flattened `[lon, lat, lon, lat, ...]` polygon ring with
`while (i < ring.size) { ...ring[i + 1]...; i += 2 }`. That reads one element
past the end of an odd-length ring, throwing ArrayIndexOutOfBoundsException on
the Compose draw thread (uncaught → whole-app crash). Two siblings in the same
codebase already avoid this by bounding to `ring.size / 2`
(`WorldOutlines.pointInRing`, `CarMapSurfaceRenderer.buildPaths`); the four draw
loops were the outliers.
Root cause: the pair walk trusted `ring.size` to be even. Today every ring comes
from `WorldOutlines.ringToFlat`, which allocates `FloatArray(n * 2)` (always
even), so the overrun is latent — but the four live draw paths have no guard
against a malformed/regenerated basemap ring, unlike their bounded siblings.
Fix: extract a single `forEachRingVertex` helper (next to the flat-ring format
docs and `pointInRing` in WorldOutlines) that walks exactly `ring.size / 2`
complete vertices, dropping a trailing unpaired coordinate, and skips rings with
fewer than 3 vertices (equivalent to the old `if (ring.size < 6) continue`). All
four draw loops now call it. `inline` keeps the per-vertex callback
allocation-free on the draw hot path. Byte-identical output for every
even-length ring.
Test: RingVertexWalkTest (pure JVM, JUnit4 + Truth) covers even/odd/short/empty
rings, the first-vertex flag, and a reproduction of the pre-fix unbounded walk
that asserts it overruns an odd ring while the bounded walk does not.
Verified: :app:testDebugUnitTest and :app:assembleDebug (4 ABIs) both green.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## dev #582 +/- ##
============================================
+ Coverage 29.36% 29.42% +0.05%
Complexity 197 197
============================================
Files 179 179
Lines 24068 24087 +19
Branches 3263 3270 +7
============================================
+ Hits 7067 7087 +20
+ Misses 16780 16777 -3
- Partials 221 223 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR prevents Compose land-rendering code paths from crashing on malformed odd-length flattened polygon rings by routing all ring-walks through a single bounded helper in WorldOutlines.kt, and adds unit tests to lock in the new behavior.
Changes:
- Added
forEachRingVertex(...)helper to iteratering.size / 2complete(lon, lat)pairs and ignore any trailing unpaired coordinate. - Refactored four live renderers (MapScreen equirectangular + azimuthal, POTA map, QSO-path map) to use the bounded helper and only
close()when a ring was actually plotted. - Added
RingVertexWalkTestto cover even/odd/short rings and a regression reproduction of the prior overrun.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| ft8af/app/src/main/kotlin/radio/ks3ckc/ft8af/ui/map/WorldOutlines.kt | Introduces the shared bounded ring-vertex walker used by multiple renderers. |
| ft8af/app/src/main/kotlin/radio/ks3ckc/ft8af/ui/map/MapScreen.kt | Refactors both land renderers to use the bounded helper (prevents draw-thread OOB). |
| ft8af/app/src/main/kotlin/radio/ks3ckc/ft8af/ui/pota/PotaActivationMap.kt | Updates POTA ring path building to use the bounded helper and conditional close. |
| ft8af/app/src/main/kotlin/radio/ks3ckc/ft8af/ui/decode/QsoSheet.kt | Updates QSO-path ring path building to use the bounded helper and conditional close. |
| ft8af/app/src/test/kotlin/radio/ks3ckc/ft8af/ui/map/RingVertexWalkTest.kt | Adds JVM unit tests asserting bounded behavior and guarding against regressions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+70
to
+72
| val vertices = ring.size / 2 | ||
| if (vertices < minVertices) return false | ||
| for (v in 0 until vertices) { |
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.
Summary
Four live Compose land renderers walked a flattened
[lon, lat, lon, lat, ...]polygon ring with
while (i < ring.size) { ...ring[i + 1]...; i += 2 }, whichreads one element past the end of an odd-length ring →
ArrayIndexOutOfBoundsExceptionon the Compose draw thread (uncaught → whole-appcrash). This PR routes all four through one bounded, unit-tested helper, matching
the two siblings that already do it safely.
Affected loops (all live, hosted in
ComposeMainActivity):ui/map/MapScreen.kt—drawWorldLand(equirectangular basemap)ui/map/MapScreen.kt—drawAzimuthalLand(azimuthal basemap)ui/pota/PotaActivationMap.kt—buildRingPath(POTA activation map)ui/decode/QsoSheet.kt—buildRingPath(QSO-path map)Root cause
The pair walk trusted
ring.sizeto be even. Today every ring is produced byWorldOutlines.ringToFlat, which allocatesFloatArray(n * 2)(always even), sothe overrun is latent with the current bundled Natural Earth basemap. But the
four live draw paths carry no guard against a malformed/regenerated ring — unlike
their bounded siblings
WorldOutlines.pointInRingandCarMapSurfaceRenderer.buildPaths, which both iterate0 until ring.size / 2.This is the same "one view has a guard its siblings lack" asymmetry fixed before
for the waterfall/spectrum views.
Fix
Extract a single
forEachRingVertexhelper inWorldOutlines.kt(next to theflat-ring format docs and
pointInRing) that walks exactlyring.size / 2complete vertices, ignores a trailing unpaired coordinate, and skips rings with
fewer than 3 vertices (equivalent to the old
if (ring.size < 6) continue). Allfour draw loops become thin callers.
inlinekeeps the per-vertex callbackallocation-free on the draw hot path (runs for every land ring, every frame).
Byte-identical output for every even-length ring — the only behavioral change
is that a hypothetical odd-length ring no longer crashes (its stray trailing
coordinate is dropped, as
CarMapSurfaceRendereralready does).Testing
RingVertexWalkTest(pure JVM, JUnit4 + Truth): even / odd / larger-odd /short / five-float / empty rings, the first-vertex flag, and a reproduction of
the pre-fix unbounded walk asserting it overruns an odd ring while the bounded
walk does not.
:app:testDebugUnitTest— green (full suite).:app:assembleDebug— green (all 4 ABIs, native + hamlib).Risk
Low. Localized to map/POTA/QSO land rendering; no protocol, DSP, decoder, or
timing behavior changes; happy path (all real rings) is byte-identical.
🤖 Generated with Claude Code