From a0e3868934a3d3e48a8ff9f2c19cafacba421ca1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Martin?= Date: Mon, 5 Oct 2026 19:13:25 +0200 Subject: [PATCH] Debug Console put: verify the card's copy, never zero-fill after a failed write Found by M3's shared-bus test: when the card refused a write, the retry closed the file (losing up to 3 KB of earlier chunks still in the write buffer), then truncate() extended it back with zeros. The checksum only covered the received bytes, so `put` reported success with 3 KB of zeros on the card. Now a retry gives up if the card lost data, and the finished file is read back and must hash the same before it's renamed. The card refuses a write about once in five 1.7 MB uploads, with the radio asleep as often as listening. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01EhqxQ49eCju4CzKYNjZzwT --- lib/ota/src/file_receiver.cpp | 5 ++++ lib/ota/src/file_receiver.h | 3 ++ src/services/debug_console.cpp | 21 +++++++++++++- .../test_file_receiver/test_file_receiver.cpp | 29 +++++++++++++++++++ 4 files changed, 57 insertions(+), 1 deletion(-) diff --git a/lib/ota/src/file_receiver.cpp b/lib/ota/src/file_receiver.cpp index ec28acb..ba0cc9e 100644 --- a/lib/ota/src/file_receiver.cpp +++ b/lib/ota/src/file_receiver.cpp @@ -94,6 +94,11 @@ void FileReceiver::chunkWritten(bool ok, uint32_t nowMs) { state_ = received_ == size_ ? State::Finishing : State::Receiving; } +void FileReceiver::cardChecked(const uint8_t digest[32]) { + if (state_ != State::Finishing) return; + if (std::memcmp(digest, expected_, sizeof expected_) != 0) fail("the copy on the card differs"); +} + void FileReceiver::finished(bool ok) { if (state_ != State::Finishing) return; if (!ok) return fail("rename failed"); diff --git a/lib/ota/src/file_receiver.h b/lib/ota/src/file_receiver.h index 49502d1..47f93b3 100644 --- a/lib/ota/src/file_receiver.h +++ b/lib/ota/src/file_receiver.h @@ -34,6 +34,9 @@ class FileReceiver { // Takes bytes for the current chunk; returns how many were used (none while a chunk waits). size_t feed(const uint8_t* data, size_t len, uint32_t nowMs); void chunkWritten(bool ok, uint32_t nowMs); + // While Finishing: the SHA-256 of the file as read back from the card. The checksum on the + // received bytes doesn't prove the card kept them (a failed write can lose buffered data). + void cardChecked(const uint8_t digest[32]); void finished(bool ok); void tick(uint32_t nowMs); void reset() { *this = FileReceiver(); } diff --git a/src/services/debug_console.cpp b/src/services/debug_console.cpp index d57afbb..dddce6b 100644 --- a/src/services/debug_console.cpp +++ b/src/services/debug_console.cpp @@ -15,6 +15,7 @@ #include #include "file_receiver.h" +#include "sha256.h" #include "platform/console.h" #include "version.h" @@ -233,11 +234,20 @@ void DebugConsole::put(NetworkClient& client, const std::string& args) { const auto& c = r.chunk(); bool ok = f.write(c.data(), c.size()) == c.size(); // A card can fail one write and take the next. FATFS keeps a failed file in error, - // so: close, cut back to the last good byte, reopen, try again. + // so: close, cut back to the last good byte, reopen, try again. Earlier chunks may + // have been lost with the write buffer (M3: 3 KB came back as zeros): never extend + // the file to cover them; give up instead, as the sender can't resend them. for (int retry = 1; !ok && retry <= 3; retry++) { console.printf("put: write failed at %u, retry %d\n", (unsigned)r.received(), retry); f.close(); delay(50 * retry); + f = SD.open(part.c_str(), FILE_READ); + size_t onCard = f ? f.size() : 0; + if (f) f.close(); + if (onCard < r.received()) { + console.printf("put: the card lost %u B written before\n", (unsigned)(r.received() - onCard)); + break; + } truncate(("/sd" + part).c_str(), r.received()); f = SD.open(part.c_str(), FILE_APPEND); ok = f && f.size() == r.received() && f.write(c.data(), c.size()) == c.size(); @@ -254,6 +264,15 @@ void DebugConsole::put(NetworkClient& client, const std::string& args) { } } f.close(); + if (r.state() == S::Finishing) { // read it back: the received checksum doesn't cover the card + Sha256 sha; + f = SD.open(part.c_str(), FILE_READ); + for (int n; f && (n = f.read(buf, sizeof buf)) > 0;) sha.update(buf, n); + if (f) f.close(); + uint8_t digest[32]; + sha.finish(digest); + r.cardChecked(digest); + } if (r.state() == S::Finishing) { if (SD.exists(r.path().c_str())) SD.remove(r.path().c_str()); r.finished(SD.rename(part.c_str(), r.path().c_str())); diff --git a/test/test_file_receiver/test_file_receiver.cpp b/test/test_file_receiver/test_file_receiver.cpp index 69208ce..a5c41cd 100644 --- a/test/test_file_receiver/test_file_receiver.cpp +++ b/test/test_file_receiver/test_file_receiver.cpp @@ -184,6 +184,34 @@ void test_reset_returns_to_idle() { TEST_ASSERT_FALSE(r.active()); } +// The received bytes can be right and the card's copy wrong: read back, it must hash the same. +void test_card_check() { + auto data = bytes(10); + FileReceiver r; + r.begin(args("/a.bin", data), 0); + r.feed(data.data(), data.size(), 0); + r.chunkWritten(true, 0); + TEST_ASSERT_TRUE(r.state() == FileReceiver::State::Finishing); + uint8_t good[32]; + Sha256::hash(data.data(), data.size(), good); + r.cardChecked(good); + TEST_ASSERT_TRUE(r.state() == FileReceiver::State::Finishing); + r.finished(true); + TEST_ASSERT_TRUE(r.state() == FileReceiver::State::Done); + + FileReceiver bad; + bad.begin(args("/a.bin", data), 0); + bad.feed(data.data(), data.size(), 0); + bad.chunkWritten(true, 0); + auto zeroed = data; + zeroed[4] = 0; + uint8_t wrong[32]; + Sha256::hash(zeroed.data(), zeroed.size(), wrong); + bad.cardChecked(wrong); + TEST_ASSERT_TRUE(bad.state() == FileReceiver::State::Failed); + TEST_ASSERT_EQUAL_STRING("the copy on the card differs", bad.error().c_str()); +} + int main() { UNITY_BEGIN(); RUN_TEST(test_begin_accepts_path_size_and_checksum); @@ -198,5 +226,6 @@ int main() { RUN_TEST(test_wanted_counts_down_within_a_chunk); RUN_TEST(test_a_rename_that_never_completes_times_out); RUN_TEST(test_reset_returns_to_idle); + RUN_TEST(test_card_check); return UNITY_END(); }