diff --git a/CHANGELOG.md b/CHANGELOG.md index f548a12f..e10ef62f 100644 Binary files a/CHANGELOG.md and b/CHANGELOG.md differ diff --git a/modules/core_geocaching/tests/CMakeLists.txt b/modules/core_geocaching/tests/CMakeLists.txt index a65987e4..c558e938 100644 --- a/modules/core_geocaching/tests/CMakeLists.txt +++ b/modules/core_geocaching/tests/CMakeLists.txt @@ -435,6 +435,7 @@ target_compile_features(geocaching_browse_runtime_test PRIVATE cxx_std_17) target_compile_definitions(geocaching_browse_runtime_test PRIVATE ESP_PLATFORM=1) target_link_libraries(geocaching_browse_runtime_test PRIVATE geocaching_native_hash) add_test(NAME geocaching_browse_runtime COMMAND geocaching_browse_runtime_test ${CMAKE_CURRENT_SOURCE_DIR}/fixtures) +add_test(NAME geocaching_browse_runtime_close_detail COMMAND geocaching_browse_runtime_test ${CMAKE_CURRENT_SOURCE_DIR}/fixtures save-close-detail) add_test(NAME geocaching_draft_only_map COMMAND geocaching_browse_runtime_test ${CMAKE_CURRENT_SOURCE_DIR}/fixtures local-draft-only) add_executable(geocaching_local_map_test test_local_map_overlay.cpp) diff --git a/modules/core_geocaching/tests/run_browse_runtime_asan.sh b/modules/core_geocaching/tests/run_browse_runtime_asan.sh index 63481156..61050b0e 100644 --- a/modules/core_geocaching/tests/run_browse_runtime_asan.sh +++ b/modules/core_geocaching/tests/run_browse_runtime_asan.sh @@ -34,3 +34,4 @@ g++ -std=c++17 -g -O1 -fsanitize=address -fno-omit-frame-pointer -DTRAIL_MATE_RE "$build_dir/ge.o" "$build_dir/sc.o" "$build_dir/sha512.o" \ -o "$build_dir/browse_runtime" ASAN_OPTIONS=detect_leaks=1:halt_on_error=1 "$build_dir/browse_runtime" modules/core_geocaching/tests/fixtures +ASAN_OPTIONS=detect_leaks=1:halt_on_error=1 "$build_dir/browse_runtime" modules/core_geocaching/tests/fixtures save-close-detail diff --git a/modules/core_geocaching/tests/test_browse_runtime.cpp b/modules/core_geocaching/tests/test_browse_runtime.cpp index 46afc204..3851d717 100644 --- a/modules/core_geocaching/tests/test_browse_runtime.cpp +++ b/modules/core_geocaching/tests/test_browse_runtime.cpp @@ -251,7 +251,8 @@ DetailText detailText(const ui::geocaching::Item& item) } int main(int argc, char** argv) { - require(argc == 2 || argc == 3, "expected fixture directory and optional local-draft-only scenario"); + require(argc == 2 || argc == 3, "expected fixture directory and optional runtime scenario"); + const bool close_on_save = argc == 3 && std::strcmp(argv[2], "save-close-detail") == 0; const std::string folder(argv[1]); const auto encoded_record = fixture(folder, "record-v1.bin"); geocaching::RecordView expected_record; @@ -260,7 +261,7 @@ int main(int argc, char** argv) chat::MeshAdapterRouter router; LoraBoard board; rt::configure(router, board); - if (argc == 3) + if (argc == 3 && !close_on_save) { require(std::strcmp(argv[2], "local-draft-only") == 0, "unknown runtime scenario"); router.ready = false; @@ -468,13 +469,48 @@ int main(int argc, char** argv) "initial publication projection did not load"); test::source->requestWindow(Section::Discover, 0, 1); generation = snapshot(Section::Discover).generation; + void* open_response = nullptr; + for (const auto& allocation : test::allocations) + if (allocation.second.owner == "geocaching.rx") open_response = allocation.first; + require(open_response != nullptr, "open detail response missing before save"); + test::memory_available = false; + require(!test::source->download(item, generation), "save ignored unavailable PSRAM"); + test::memory_available = true; + require(detailText(item).status == ui::geocaching::DetailStatus::Ready, + "failed save invalidated the open verified detail"); require(test::source->download(item, generation), "download not queued"); - test::source->closeDetail(); + require(test::allocated("geocaching.download.detail"), "verified download did not own a response copy"); + if (close_on_save) + { + // Release the original response before the installation's first tick. + // Every verified view must belong to the queued download's copy. + test::source->closeDetail(); + until([&] + { + const auto allocation = test::allocations.find(open_response); + return allocation == test::allocations.end() || allocation->second.owner != "geocaching.rx"; }, + "closing pending download retained the detail response"); + } until([&] - { return std::strstr(snapshot(Section::Discover).status.data(), "Shared caches"); }, + { + if (!close_on_save) + require(detailText(item).status == ui::geocaching::DetailStatus::Ready, + "saving invalidated the open verified detail"); + return std::strstr(snapshot(Section::Discover).status.data(), "Shared caches"); }, "download did not finish"); + if (!close_on_save) + { + require(test::allocations.count(open_response), "saving released the open detail response"); + require(detailText(item).description == expected_record.description && detailText(item).hint == expected_record.hint, + "saved open detail lost verified text"); + require(!test::source->archiveCache(item.id, item.revision_hash), "saved foreign detail became archivable"); + test::source->closeDetail(); + tick(); + } require(router.sends == 4, "saving verified detail sent a redundant network request"); - require(!test::allocated("geocaching.detail.rx"), "saved detail retained its response buffer"); + const auto released_response = test::allocations.find(open_response); + require(released_response == test::allocations.end() || released_response->second.owner != "geocaching.rx", + "saved detail retained its response buffer"); // A complete download writes requests, attempts, objects and installed // heads. None changes the publication projection we already loaded. require(std::strstr(snapshot(Section::Published).status.data(), "No local drafts"), @@ -486,6 +522,22 @@ int main(int argc, char** argv) return view.count == 1 && test::source->item(Section::Downloaded, 0, view.generation, item) && item.downloaded; }, "installed GPX missing from directory"); const auto saved_id = item.id; + if (close_on_save) + { + test::source->open(item, snapshot(Section::Downloaded).generation); + until([&] + { return detailText(item).status == ui::geocaching::DetailStatus::Ready; }, + "closed detail download lost installed details"); + require(detailText(item).description == expected_record.description && detailText(item).hint == expected_record.hint, + "closed detail download corrupted installed text"); + require(router.sends == 4, "installed detail required another network request"); + test::source->closeDetail(); + test::source->activate(false); + until([&] + { return test::allocations.empty(); }, + "closed detail installation leaked PSRAM"); + return 0; + } ui::geocaching::DraftInput draft; draft.name = "Published through device runtime"; draft.description = "A local draft with a durable signed revision"; diff --git a/platform/esp/arduino_common/src/geocaching/browse_runtime.cpp b/platform/esp/arduino_common/src/geocaching/browse_runtime.cpp index 7fe41399..abc85a76 100644 --- a/platform/esp/arduino_common/src/geocaching/browse_runtime.cpp +++ b/platform/esp/arduino_common/src/geocaching/browse_runtime.cpp @@ -2085,13 +2085,26 @@ class Facade final : public ::ui::geocaching::Source if (!session->response && detail && detail->state == CacheDetail::State::Ready && detail->response && detail->matches(item.id, item.revision_hash) && detail->remote.bytes == remote.bytes) { - job->verified.reset(::platform::memory::createPsram(detail->verified_record)); + job->verified.reset(::platform::memory::createPsram()); if (!job->verified) return false; job->request = detail->request; - job->response = detail->response; job->response_size = detail->response_size; - detail->response = nullptr; - detail->response_size = 0; + job->response = static_cast(mem::allocatePreferred("geocaching.download.detail", job->response_size, false)); + if (!job->response) return false; + std::memcpy(job->response, detail->response, job->response_size); + // Both owners may outlive the other. Reparse the immutable copy so + // every borrowed field belongs to the download's response, while + // retaining the already verified identity/hash without more crypto. + gc::protocol::GetResponseView parsed; + if (!gc::protocol::decodeGetResponse({job->response, job->response_size}, job->request, 8192, parsed) || parsed.has_conflict) return false; + gc::protocol::CmpReader reader(parsed.signed_cache); + size_t count = 0; + gc::ByteView encoded; + if (!reader.array(count, 2) || count != 2 || !reader.binary(encoded, gc::kMaxRecordBytes) || + !reader.binary(job->verified->signature, 64) || job->verified->signature.size != 64 || !reader.finished() || + !gc::protocol::decodeGeocacheRecord(encoded, job->verified->record)) return false; + job->verified->id = detail->verified_record.id; + job->verified->hash = detail->verified_record.hash; } session->storage_requested = true; session->download_start = std::move(job); @@ -2353,10 +2366,11 @@ void step() if (s.local_read_only && (draftSaveActive() || downloadActive() || publicationActive() || s.needsRecovery() || (s.saved && s.saved->error()) || (s.draft_catalog && s.draft_catalog->failed))) { - // The audit and local readers share a frame lease. Release every - // reader first; queued edits own their bytes independently. + // The audit and local readers share a frame lease. Ready details + // own their response independently; release only their reader. s.workspace_owner.release(s.detail.get()); - s.detail.reset(); + if (s.detail && s.detail->state == CacheDetail::State::Ready) s.detail->saved_read.reset(); + else s.detail.reset(); s.pending_detail.reset(); if (s.store) s.store->releaseDraftRead(); s.saved.reset();