From de5ba053b9818bb53f3cc5ffc1da9ee4bca2d340 Mon Sep 17 00:00:00 2001 From: "torlando-agent[bot]" <281092095+torlando-agent[bot]@users.noreply.github.com> Date: Wed, 29 Jul 2026 09:48:44 +0000 Subject: [PATCH] fix: verify durable SD tile writes --- .../Hardware/TDeck/MapTileStoreSD.cpp | 39 ++++++++++++------- lib/tdeck_ui/Hardware/TDeck/MapTileStoreSD.h | 1 + .../test_map_tile_store_contract.py | 10 +++++ tests/native/test_map_tile_store.cpp | 10 +++-- tests/native/test_map_tile_store.py | 2 +- 5 files changed, 43 insertions(+), 19 deletions(-) diff --git a/lib/tdeck_ui/Hardware/TDeck/MapTileStoreSD.cpp b/lib/tdeck_ui/Hardware/TDeck/MapTileStoreSD.cpp index 1e15490e..dcd39384 100644 --- a/lib/tdeck_ui/Hardware/TDeck/MapTileStoreSD.cpp +++ b/lib/tdeck_ui/Hardware/TDeck/MapTileStoreSD.cpp @@ -7,18 +7,22 @@ #include #include #include +#include #include +#include namespace Hardware { namespace TDeck { namespace { +bool makeMountedPath(const char* name, char* mounted, std::size_t capacity) { + if ((name == NULL) || (mounted == NULL)) return false; + const int written = std::snprintf(mounted, capacity, "/sd%s", name); + return (written >= 0) && (static_cast(written) < capacity); +} TileStoreResult statMountedPathLocked(const char* name, struct stat& info) { char mounted[MapTileStore::PATH_CAPACITY + 4U] = {}; - const int written = std::snprintf(mounted, sizeof(mounted), "/sd%s", name); - if ((written < 0) || (static_cast(written) >= sizeof(mounted))) { - return TileStoreResult::INVALID_ARGUMENT; - } + if (!makeMountedPath(name, mounted, sizeof(mounted))) return TileStoreResult::INVALID_ARGUMENT; errno = 0; if (::stat(mounted, &info) == 0) return TileStoreResult::OK; return (errno == ENOENT) ? TileStoreResult::MISS : TileStoreResult::IO_ERROR; @@ -26,7 +30,7 @@ TileStoreResult statMountedPathLocked(const char* name, struct stat& info) { } MapTileStoreSD::MapTileStoreSD() - : stream_(), list_root_(), list_zoom_(), list_x_(), writing_(false), healthy_(true) {} + : stream_(), list_root_(), list_zoom_(), list_x_(), write_fd_(-1), writing_(false), healthy_(true) {} MapTileStoreSD::~MapTileStoreSD() { abortWrite(); endRead(); endList(); } bool MapTileStoreSD::cardPresentLocked() { return SD.cardType() != CARD_NONE; } @@ -102,36 +106,41 @@ TileStoreResult MapTileStoreSD::beginWrite(const char* name) { if (!healthy_ || !SDAccess::is_ready() || !SDAccess::acquire_bus(500U)) return TileStoreResult::STORAGE_UNAVAILABLE; if (!cardPresentLocked()) { SDAccess::release_bus(); return TileStoreResult::STORAGE_UNAVAILABLE; } if (!makeParentDirectoriesLocked(name)) { SDAccess::release_bus(); return TileStoreResult::IO_ERROR; } - stream_ = SD.open(name, FILE_WRITE); - writing_ = static_cast(stream_); + char mounted[MapTileStore::PATH_CAPACITY + 4U] = {}; + if (!makeMountedPath(name, mounted, sizeof(mounted))) { SDAccess::release_bus(); return TileStoreResult::INVALID_ARGUMENT; } + write_fd_ = ::open(mounted, O_WRONLY | O_CREAT | O_TRUNC, 0666); + writing_ = (write_fd_ >= 0); SDAccess::release_bus(); return writing_ ? TileStoreResult::OK : TileStoreResult::IO_ERROR; } TileStoreResult MapTileStoreSD::writeChunk(const std::uint8_t* data, std::size_t size, std::size_t& written) { - if (!healthy_ || !stream_ || !writing_) return TileStoreResult::IO_ERROR; + if (!healthy_ || (write_fd_ < 0) || !writing_) return TileStoreResult::IO_ERROR; if (!SDAccess::acquire_bus(500U)) return TileStoreResult::STORAGE_UNAVAILABLE; if (!cardPresentLocked()) { SDAccess::release_bus(); return TileStoreResult::STORAGE_UNAVAILABLE; } - written = stream_.write(data, size); + const ssize_t result = ::write(write_fd_, data, size); + written = (result < 0) ? 0U : static_cast(result); SDAccess::release_bus(); return (written == size) ? TileStoreResult::OK : TileStoreResult::IO_ERROR; } TileStoreResult MapTileStoreSD::commitWrite() { - if (!healthy_ || !stream_ || !writing_) return TileStoreResult::IO_ERROR; + if (!healthy_ || (write_fd_ < 0) || !writing_) return TileStoreResult::IO_ERROR; if (!SDAccess::acquire_bus(500U)) return TileStoreResult::STORAGE_UNAVAILABLE; if (!cardPresentLocked()) { SDAccess::release_bus(); return TileStoreResult::STORAGE_UNAVAILABLE; } - stream_.flush(); - stream_.close(); + const bool synced = (::fsync(write_fd_) == 0); + const bool closed = (::close(write_fd_) == 0); + write_fd_ = -1; writing_ = false; SDAccess::release_bus(); - return TileStoreResult::OK; + return (synced && closed) ? TileStoreResult::OK : TileStoreResult::IO_ERROR; } void MapTileStoreSD::abortWrite() { - if (!stream_ || !writing_) return; + if ((write_fd_ < 0) || !writing_) return; if (SDAccess::acquire_bus(500U)) { - stream_.close(); + if (::close(write_fd_) != 0) healthy_ = false; + write_fd_ = -1; writing_ = false; SDAccess::release_bus(); } else { diff --git a/lib/tdeck_ui/Hardware/TDeck/MapTileStoreSD.h b/lib/tdeck_ui/Hardware/TDeck/MapTileStoreSD.h index 92d421f2..1a13e53d 100644 --- a/lib/tdeck_ui/Hardware/TDeck/MapTileStoreSD.h +++ b/lib/tdeck_ui/Hardware/TDeck/MapTileStoreSD.h @@ -46,6 +46,7 @@ private: fs::File list_root_; fs::File list_zoom_; fs::File list_x_; + int write_fd_; bool writing_; bool healthy_; diff --git a/tests/build_scripts/test_map_tile_store_contract.py b/tests/build_scripts/test_map_tile_store_contract.py index 90c5c25e..357bcc0d 100644 --- a/tests/build_scripts/test_map_tile_store_contract.py +++ b/tests/build_scripts/test_map_tile_store_contract.py @@ -66,3 +66,13 @@ def test_sd_adapter_distinguishes_missing_files_from_open_failures(): source.index("TileStoreResult MapTileStoreSD::nextList")] assert "present == TileStoreResult::MISS" in list_body assert "if (!list_root_)" in list_body and "TileStoreResult::IO_ERROR" in list_body + + +def test_sd_adapter_checks_sync_and_close_before_acknowledging_write(): + source = SD_CPP.read_text() + body = source[source.index("TileStoreResult MapTileStoreSD::commitWrite"): + source.index("void MapTileStoreSD::abortWrite")] + assert "::fsync(write_fd_) == 0" in body + assert "::close(write_fd_) == 0" in body + assert "synced && closed" in body + assert "stream_.flush()" not in body diff --git a/tests/native/test_map_tile_store.cpp b/tests/native/test_map_tile_store.cpp index 1aac9cdc..d7c07dc2 100644 --- a/tests/native/test_map_tile_store.cpp +++ b/tests/native/test_map_tile_store.cpp @@ -27,6 +27,7 @@ class FakeStorage : public MapTileStorage { public: bool available; bool short_write; + bool fail_commit; bool fail_remove; int fail_remove_call; int fail_rename_call; @@ -40,7 +41,7 @@ public: int rename_calls; int remove_calls; - FakeStorage() : available(true), short_write(false), fail_remove(false), fail_remove_call(0), + FakeStorage() : available(true), short_write(false), fail_commit(false), fail_remove(false), fail_remove_call(0), fail_rename_call(0), fail_rename_call2(0), power_cut_after_rename_call(0), position(0U), list_position(0U), rename_calls(0), remove_calls(0) {} @@ -79,7 +80,7 @@ public: files[static_cast(i)].bytes.insert(files[static_cast(i)].bytes.end(), data, data + written); return TileStoreResult::OK; } - virtual TileStoreResult commitWrite() { open_path.clear(); return available ? TileStoreResult::OK : TileStoreResult::STORAGE_UNAVAILABLE; } + virtual TileStoreResult commitWrite() { open_path.clear(); return !available ? TileStoreResult::STORAGE_UNAVAILABLE : (fail_commit ? TileStoreResult::IO_ERROR : TileStoreResult::OK); } virtual void abortWrite() { if (!open_path.empty()) remove(open_path.c_str()); open_path.clear(); } virtual TileStoreResult remove(const char* path) { if (!available) return TileStoreResult::STORAGE_UNAVAILABLE; @@ -161,6 +162,9 @@ void testMalformedPngs() { beginTest(); FakeStorage fs; MapTileStore s(fs,config void testShortWriteAbortsTemp() { beginTest(); FakeStorage fs; MapTileStore s(fs,config()); CHECK(s.initialize()==TileStoreResult::OK); fs.short_write=true; CHECK(put(s,TileKey{0U,0U,0U},png())==TileStoreResult::IO_ERROR); CHECK(fs.files.empty()); } +void testCommitFailureDoesNotAcknowledgeOrReplaceLive() { beginTest(); FakeStorage fs; MapTileStore s(fs,config()); CHECK(s.initialize()==TileStoreResult::OK); const TileKey key={0U,0U,0U}; CHECK(put(s,key,png())==TileStoreResult::OK); + fs.fail_commit=true; CHECK(put(s,key,png(50U))==TileStoreResult::IO_ERROR); fs.fail_commit=false; drain(s,key,40U); +} void testExactQuotaAndLruEviction() { beginTest(); FakeStorage fs; MapTileStore s(fs,config(3U,80U,80U)); CHECK(s.initialize()==TileStoreResult::OK); CHECK(put(s,TileKey{1U,0U,0U},png())==TileStoreResult::OK); CHECK(put(s,TileKey{1U,1U,0U},png())==TileStoreResult::OK); std::uint32_t n=0U; CHECK(s.beginGet(TileKey{1U,0U,0U},n)==TileStoreResult::OK); s.endGet(); @@ -280,4 +284,4 @@ void testDeterministicStress() { beginTest(); FakeStorage fs; MapTileStore s(fs, std::uint32_t size=0U; for(std::uint32_t i=0U;i<100000U;++i) { const TileKey k={2U,i&3U,(i>>2)&3U}; TileStoreResult r=s.beginGet(k,size); CHECK(r==TileStoreResult::OK||r==TileStoreResult::MISS); if(r==TileStoreResult::OK)s.endGet(); } } } -int main() { testKeyAndCanonicalPath(); testMissHitAndRemoval(); testMalformedPngs(); testShortWriteAbortsTemp(); testExactQuotaAndLruEviction(); testDuplicateAtomicReplacement(); testInterruptedFilesRecover(); testLiveWinsRecovery(); testCorruptLiveRecoversValidBackup(); testCorruptLiveWithoutBackupIsRemoved(); testStaleTempRemovalFailureAbortsPut(); testRecoveryRejectsMalformedAndExhaustion(); testRecoveryQuotaFailsClosed(); testRenameFailureRestoresDuplicate(); testDuplicateRollbackFailureInvalidatesStore(); testPromotionFailureDoesNotEvictVictims(); testEvictionPreflightFailurePreservesAllVictims(); testEvictionStageFailureRollsBackAllVictims(); testEvictionPowerCutsRestoreWholeOldGeneration(); testDuplicateEvictionPowerCutsRestoreOldCandidateAndVictim(); testStaleDuplicateBackupMustClearBeforeManifest(); testSemanticManifestValidationPrecedesMutation(); testMalformedEvictionManifestFailsClosed(); testPostCommitCleanupResidueKeepsWholeNewGeneration(); testHardMaxLivePlusCrashTempRecovers(); testUnboundedCommittedEvictionResidueCleansInBatches(); testDeterministicStress(); std::cout<<"map tile store: "< None: env["UBSAN_OPTIONS"] = "halt_on_error=1:print_stacktrace=1" ran = subprocess.run([str(binary)], capture_output=True, text=True, timeout=60, env=env) assert ran.returncode == 0, ran.stdout + ran.stderr - assert ran.stdout == "map tile store: 27 tests passed\n" + assert ran.stdout == "map tile store: 28 tests passed\n"