Validate map tile archives before extracting and never publish a partial file - #138
Open
dirkpetersen wants to merge 1 commit into
Open
dirkpetersen wants to merge 1 commit into
dirkpetersen wants to merge 1 commit into
Conversation
…artial file
Fixes the failure that left a device unable to read its own map tiles
("could not unmarshal offline data: unexpected EOF") and silently re-downloading
~286 MB of OSM data over cellular to repair itself.
Four independent defects in the offline-tile download path, all of which end with
corrupt data at a LIVE tile path:
1. No validation before extraction. The archive was extracted straight over live
tiles; a truncated download was only discovered later, at read time, after it
had already overwritten good data. ValidateArchive now reads the whole stream
first and the extract is gated on it.
The subtle part: it is not enough to walk the tar. tar.Next returns io.EOF at
the tar end-of-archive marker, which sits BEFORE gzip's 8-byte CRC32+ISIZE
trailer -- so a tail truncation (the last bytes lost as a connection dies)
validates clean if you stop there. ValidateArchive drains the gzip stream to
its end, which is what actually checks the checksum and length. My first
version had this bug; the 99%-truncation case in the tests found it.
2. Downloads were written directly to their final path via os.Create. A process
killed mid-transfer -- power cut, reboot, OOM -- left a truncated file that is
indistinguishable from a complete one. Now staged as <path>.part and renamed
only after the transfer is verified, so the destination either does not exist
or is complete. TestDestinationDoesNotExistWhileTransferIsInFlight asserts
this from inside the server handler, mid-copy.
3. Extraction opened targets with os.O_CREATE|os.O_RDWR and NO os.O_TRUNC.
Writing a SHORTER file over a longer existing one left the old tail attached,
producing a corrupt hybrid that SURVIVED re-downloading -- which is why a bad
tile could not be repaired by fetching it again. Each file is now written via
.part + rename, replacing it wholesale.
4. Every extraction error was logged and ignored: a failed os.Open produced a nil
file that was handed to gzip.NewReader, a failed gzip parse produced a nil
reader handed to tar.NewReader, and a failed io.Copy left a half-written file
in place -- after all of which the tile was still counted as successfully
downloaded. Extraction now returns an error and the tile is not counted.
Also adds a tar-slip guard (safeJoin): entries were joined to the base path with
no check that they stay under it, so "../../.." in an archive could write
anywhere. The source is a trusted HTTPS host, so this is hardening, not an
observed exploit.
Honest scope notes, both verified by mutation rather than assumed:
* The Content-Length check is defence-in-depth, NOT the primary guard -- Go's
HTTP client already reports a short body against a declared length as
io.ErrUnexpectedEOF. Removing the check alone does not let truncation
through.
* O_TRUNC is likewise belt-and-braces. The RENAME is what fixes the stale-tail
bug; removing O_TRUNC does not fail the tests.
Tests: 15 new in settings/download_validate_test.go, covering intact/truncated
(4 truncation points)/garbage/empty/no-file archives, the mid-transfer crash
window, stale .part contamination, HTTP errors, the stale-tail overwrite, tar
slip, and that a failed validation leaves live tiles untouched. go build, go vet
and go test ./... all clean.
Built for arm64 via native CI and running on the owner's comma 3X since
2026-09-03.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FwdRXRST5H2Hoft9yDkyU3
Contributor
Author
|
I drove a few weeks with this validation feature and when i enable automated real time downloading ..... now it no longer attempts to download maps multiple times for me. |
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.
Fixes a data-corruption path in the offline tile download: a truncated or interrupted download could end up as a corrupt tile at a live path, after which mapd logged
could not unmarshal offline data: unexpected EOFfor that area on every load and could not repair it by downloading again.What happened
On a drive with the full WA/OR/ID region on disk (~286 MB), the device started re-downloading the whole region over cellular.
maps.ReadOfflinewas failing withunexpected EOFon several tiles that were shorter than they should have been. Tracing it back, the per-archive block insettings/download.gohas four independent ways of leaving corrupt bytes at a live tile path:os.Create). A process killed mid-transfer (power cut, reboot, OOM) leaves a short file that looks complete.O_CREATE|O_RDWRand noO_TRUNC. Writing a shorter tile over a longer one left the old tail attached — a hybrid that survived re-downloading, which is why fetching the region again did not fix it.os.Openhanded a nil file togzip.NewReader, a failed gzip parse handed a nil reader totar.NewReader, a failedio.Copyleft a half-written file), and the tile was still counted as downloaded.The change
All in
settings/download.go; no schema, settings, CLI or message changes.DownloadFilewrites to<dest>.partand renames into place only after the body is complete (and matchesContent-Lengthwhen the server declares one). Any failure removes the.part. The destination either does not exist or is complete.ValidateArchivereads the whole gzip+tar stream toio.Discardbefore anything is extracted. This has to drain the gzip stream, not just walk the tar:tar.Nextreturnsio.EOFat the tar end-of-archive marker, which sits before gzip's CRC32/ISIZE trailer, so a tail truncation of a few bytes validates clean if you stop at the tar level. (My first version had exactly that bug; the 99 %-truncation test case caught it.)extractArchivereplaces the inline loop and returns an error on the first failure instead of warning and continuing. Each regular file is written throughwriteFileAtomic(.part+Sync+Rename), which also fixes the stale-tail bug — the rename replaces the file wholesale, so length cannot carry over.downloadBounds, the per-archive flow is now download → validate → extract, and a tile that fails validation or extraction is discarded and not counted inDownloadedFiles, the same way a failed HTTP fetch already isn't.safeJoinrejects archive entries that would escape the destination (../). The tile host is trusted, so this is hardening rather than an observed problem — happy to drop it if you'd rather keep the PR to the bug.Tests
settings/download_validate_test.go(new; the package had no tests): intact archive accepted; truncation at 10/50/90/99 % rejected; garbage, empty and no-file archives rejected;DownloadFilepublishes a complete file and no.part, rejects a short body againstContent-Length, does not publish on HTTP error, ignores a stale.part, and — asserted from inside the test HTTP handler while the copy is in flight — the destination does not exist mid-transfer;writeFileAtomicreplaces a longer file wholesale and leaves no.part;safeJoinrejects..entries and allows normal ones; and a truncated archive leaves an existing live tile untouched.Two of the guards are deliberately weaker claims than they look, checked by mutation: Go's HTTP client already reports a short body against a declared
Content-Lengthasio.ErrUnexpectedEOF, so the explicit length check is defence-in-depth; andO_TRUNCon the.partis belt-and-braces — the rename is what fixes the stale tail.Also checked against real archives from
map-data.pfeifer.dev, on this exact branch:offline/46/-124.tar.gz(35.3 MB) andoffline/46/-122.tar.gz(8.6 MB) both validate, and each minus its last 3 bytes is rejected withunexpected EOF.go build,go vet ./...,go test ./...andgofumpt -lare clean. A native-arm64 build with this change has been running on a comma 3X since 2026-09-03 with a full region on disk; the device has not lost map data since.Compatibility
DownloadedFilesfor that location ends short ofTotalFiles, which is what a failed fetch already does today. No new progress fields.<tile>.partnext to a live tile. The reader opens tiles by exact path (FindWaysAroundPosition→os.ReadFile), so it is inert, and the next extract of that tile removes it.ReadOfflinestill returnsLoaded: falsewithout deleting it. Deleting an unreadable tile so it gets re-fetched would be a sensible follow-up; left out to keep this to the download path.🤖 Generated with Claude Code
https://claude.ai/code/session_01FwdRXRST5H2Hoft9yDkyU3