From d0ad88d61f7084c55a10c7fc3c26c870b6402d38 Mon Sep 17 00:00:00 2001 From: sanerdemirel <300525555+sanerdemirel@users.noreply.github.com> Date: Mon, 9 Nov 2026 16:29:30 +0100 Subject: [PATCH 1/6] host tests: let a test set the mocked free heap --- tests/host/common/MockEsp.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/host/common/MockEsp.cpp b/tests/host/common/MockEsp.cpp index 18e7be6c83..ec5df941df 100644 --- a/tests/host/common/MockEsp.cpp +++ b/tests/host/common/MockEsp.cpp @@ -115,9 +115,12 @@ uint32_t EspClass::getSketchSize() return 400000; } +// Tests can change this to simulate a low heap. +uint32_t mockFreeHeap = 30000; + uint32_t EspClass::getFreeHeap() { - return 30000; + return mockFreeHeap; } uint32_t EspClass::getMaxFreeBlockSize() From 0416e07a789425a87d10cb034ec9959f9e35f6bb Mon Sep 17 00:00:00 2001 From: sanerdemirel <300525555+sanerdemirel@users.noreply.github.com> Date: Mon, 9 Nov 2026 16:30:32 +0100 Subject: [PATCH 2/6] Updater: host tests for the buffer size choice and the fallback --- tests/host/core/test_Updater.cpp | 103 +++++++++++++++++++++++++++++++ 1 file changed, 103 insertions(+) diff --git a/tests/host/core/test_Updater.cpp b/tests/host/core/test_Updater.cpp index 8a1ec92bd8..f63e6b3bc2 100644 --- a/tests/host/core/test_Updater.cpp +++ b/tests/host/core/test_Updater.cpp @@ -15,6 +15,8 @@ #include #include +#include +#include // Use a SPIFFS file because we can't instantiate a virtual class like Print TEST_CASE("Updater fails when writes overflow requested size", "[core][Updater]") @@ -53,3 +55,104 @@ TEST_CASE("Updater fails when writes overflow requested size", "[core][Updater]" REQUIRE(!u->write(buff, 2048)); delete u; } + +// Count and, on request, refuse array allocations of given sizes, so a test +// can see which buffer size UpdaterClass::begin() asks for. +// UpdaterClass::begin() uses new (std::nothrow) uint8_t[], so replacing +// this one operator is enough. Everything else is passed to malloc(). +namespace +{ + constexpr size_t maxLog = 16; + size_t allocLog[maxLog]; + size_t allocCount = 0; + size_t refuseA = 0; // 0: refuse nothing + size_t refuseB = 0; + bool logging = false; +} + +void* operator new[](std::size_t size, const std::nothrow_t&) noexcept +{ + if (logging && allocCount < maxLog) + { + allocLog[allocCount++] = size; + } + if (size == refuseA || size == refuseB) + { + return nullptr; + } + return malloc(size); +} + +extern uint32_t mockFreeHeap; + +namespace +{ + // Run begin() with logging on. Sizes refuseA/refuseB are refused, + // heap is the mocked free heap. + bool beginWith(UpdaterClass& u, size_t a, size_t b, uint32_t heap) + { + allocCount = 0; + refuseA = a; + refuseB = b; + mockFreeHeap = heap; + logging = true; + const bool ok = u.begin(6000); + logging = false; + refuseA = 0; + refuseB = 0; + mockFreeHeap = 30000; + return ok; + } +} + +TEST_CASE("Updater uses a sector sized buffer when there is room", "[core][Updater]") +{ + UpdaterClass u; + REQUIRE(beginWith(u, 0, 0, 30000)); + REQUIRE(allocCount == 1); + REQUIRE(allocLog[0] == FLASH_SECTOR_SIZE); + u.end(true); // no data written: fails, and frees the buffer +} + +TEST_CASE("Updater falls back to a small buffer when the big one cannot be allocated", "[core][Updater]") +{ + UpdaterClass u; + uint8_t buff[6000]; + memset(buff, 0, sizeof(buff)); + // Plenty of free heap in total, but no block of 4 KB (fragmentation). + REQUIRE(beginWith(u, FLASH_SECTOR_SIZE, 0, 30000)); + REQUIRE(allocCount == 2); + REQUIRE(allocLog[0] == FLASH_SECTOR_SIZE); + REQUIRE(allocLog[1] == 256); + // And the update still works with the small buffer. + REQUIRE(u.write(buff, 6000)); + REQUIRE(u.remaining() == 0); + REQUIRE(!u.hasError()); +} + +TEST_CASE("Updater starts with the small buffer when the heap is low", "[core][Updater]") +{ + UpdaterClass u; + REQUIRE(beginWith(u, 0, 0, 2 * FLASH_SECTOR_SIZE)); + REQUIRE(allocCount == 1); + REQUIRE(allocLog[0] == 256); + u.end(true); +} + +TEST_CASE("Updater reports OOM when no buffer can be allocated", "[core][Updater]") +{ + UpdaterClass u; + // Low heap: only the small buffer is tried. + REQUIRE(!beginWith(u, 256, 0, 2 * FLASH_SECTOR_SIZE)); + REQUIRE(allocCount == 1); + REQUIRE(u.getError() == UPDATE_ERROR_OOM); + u.clearError(); + // Enough heap: both sizes are tried, both fail. + REQUIRE(!beginWith(u, FLASH_SECTOR_SIZE, 256, 30000)); + REQUIRE(allocCount == 2); + REQUIRE(u.getError() == UPDATE_ERROR_OOM); + // A failed begin() leaves the updater usable. + u.clearError(); + REQUIRE(beginWith(u, 0, 0, 30000)); + u.end(true); +} From f005cb937ab12139f199501a899f82333e836a5c Mon Sep 17 00:00:00 2001 From: sanerdemirel <300525555+sanerdemirel@users.noreply.github.com> Date: Mon, 9 Nov 2026 16:30:38 +0100 Subject: [PATCH 3/6] Updater: try the sector buffer, then 256 bytes, instead of one allocation --- cores/esp8266/Updater.cpp | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/cores/esp8266/Updater.cpp b/cores/esp8266/Updater.cpp index ef79a5cbf3..0d37bfafa1 100644 --- a/cores/esp8266/Updater.cpp +++ b/cores/esp8266/Updater.cpp @@ -171,12 +171,14 @@ bool UpdaterClass::begin(size_t size, int command, int ledPin, uint8_t ledOn) { _startAddress = updateStartAddress; _currentAddress = _startAddress; _size = size; - if (ESP.getFreeHeap() > 2 * FLASH_SECTOR_SIZE) { - _bufferSize = FLASH_SECTOR_SIZE; - } else { - _bufferSize = 256; + // Try the big buffer first, then the small one. The big one is skipped + // when free heap is low, and it can also fail on a fragmented heap. + static constexpr size_t bufferSizes[] { FLASH_SECTOR_SIZE, 256 }; + size_t first = (ESP.getFreeHeap() > 2 * FLASH_SECTOR_SIZE) ? 0 : 1; + for (size_t i = first; !_buffer && i < sizeof(bufferSizes) / sizeof(bufferSizes[0]); ++i) { + _bufferSize = bufferSizes[i]; + _buffer = new (std::nothrow) uint8_t[_bufferSize]; } - _buffer = new (std::nothrow) uint8_t[_bufferSize]; if (!_buffer) { _setError(UPDATE_ERROR_OOM); _reset(false); From d2fc6d5149390fd4437659730261d4a9083d3e9e Mon Sep 17 00:00:00 2001 From: sanerdemirel <300525555+sanerdemirel@users.noreply.github.com> Date: Mon, 9 Nov 2026 21:02:34 +0100 Subject: [PATCH 4/6] Updater host tests: skip under valgrind, free every buffer --- tests/host/core/test_Updater.cpp | 42 ++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/tests/host/core/test_Updater.cpp b/tests/host/core/test_Updater.cpp index f63e6b3bc2..a7ab7d9de2 100644 --- a/tests/host/core/test_Updater.cpp +++ b/tests/host/core/test_Updater.cpp @@ -103,10 +103,39 @@ namespace mockFreeHeap = 30000; return ok; } + + // Under valgrind, operator new is replaced by valgrind's own, whatever the + // test binary defines, so the hook above is never called. Check that + // once, and let the tests that need the hook skip themselves in that case. + bool allocHookWorks() + { + static const bool works = []() { + allocCount = 0; + logging = true; + // volatile: the compiler may not drop the new/delete pair + volatile uint8_t* probe = new (std::nothrow) uint8_t[1]; + logging = false; + if (probe) + { + *probe = 1; + } + delete[] probe; + return allocCount == 1; + }(); + if (!works) + { + WARN("operator new[] cannot be replaced here (valgrind?), test skipped"); + } + return works; + } } TEST_CASE("Updater uses a sector sized buffer when there is room", "[core][Updater]") { + if (!allocHookWorks()) + { + return; + } UpdaterClass u; REQUIRE(beginWith(u, 0, 0, 30000)); REQUIRE(allocCount == 1); @@ -116,6 +145,10 @@ TEST_CASE("Updater uses a sector sized buffer when there is room", "[core][Updat TEST_CASE("Updater falls back to a small buffer when the big one cannot be allocated", "[core][Updater]") { + if (!allocHookWorks()) + { + return; + } UpdaterClass u; uint8_t buff[6000]; memset(buff, 0, sizeof(buff)); @@ -128,10 +161,15 @@ TEST_CASE("Updater falls back to a small buffer when the big one cannot be alloc REQUIRE(u.write(buff, 6000)); REQUIRE(u.remaining() == 0); REQUIRE(!u.hasError()); + u.end(true); // frees the buffer } TEST_CASE("Updater starts with the small buffer when the heap is low", "[core][Updater]") { + if (!allocHookWorks()) + { + return; + } UpdaterClass u; REQUIRE(beginWith(u, 0, 0, 2 * FLASH_SECTOR_SIZE)); REQUIRE(allocCount == 1); @@ -141,6 +179,10 @@ TEST_CASE("Updater starts with the small buffer when the heap is low", "[core][U TEST_CASE("Updater reports OOM when no buffer can be allocated", "[core][Updater]") { + if (!allocHookWorks()) + { + return; + } UpdaterClass u; // Low heap: only the small buffer is tried. REQUIRE(!beginWith(u, 256, 0, 2 * FLASH_SECTOR_SIZE)); From 3ae4cdd98b5e5a728fb91a41241bb5ef933abcbd Mon Sep 17 00:00:00 2001 From: sanerdemirel <300525555+sanerdemirel@users.noreply.github.com> Date: Mon, 9 Nov 2026 21:15:51 +0100 Subject: [PATCH 5/6] Updater host tests: clang-format --- tests/host/core/test_Updater.cpp | 90 ++++++++++++++++---------------- 1 file changed, 46 insertions(+), 44 deletions(-) diff --git a/tests/host/core/test_Updater.cpp b/tests/host/core/test_Updater.cpp index a7ab7d9de2..9d00bb0fd8 100644 --- a/tests/host/core/test_Updater.cpp +++ b/tests/host/core/test_Updater.cpp @@ -62,13 +62,13 @@ TEST_CASE("Updater fails when writes overflow requested size", "[core][Updater]" // this one operator is enough. Everything else is passed to malloc(). namespace { - constexpr size_t maxLog = 16; - size_t allocLog[maxLog]; - size_t allocCount = 0; - size_t refuseA = 0; // 0: refuse nothing - size_t refuseB = 0; - bool logging = false; -} +constexpr size_t maxLog = 16; +size_t allocLog[maxLog]; +size_t allocCount = 0; +size_t refuseA = 0; // 0: refuse nothing +size_t refuseB = 0; +bool logging = false; +} // namespace void* operator new[](std::size_t size, const std::nothrow_t&) noexcept { @@ -87,48 +87,49 @@ extern uint32_t mockFreeHeap; namespace { - // Run begin() with logging on. Sizes refuseA/refuseB are refused, - // heap is the mocked free heap. - bool beginWith(UpdaterClass& u, size_t a, size_t b, uint32_t heap) - { - allocCount = 0; - refuseA = a; - refuseB = b; - mockFreeHeap = heap; - logging = true; - const bool ok = u.begin(6000); - logging = false; - refuseA = 0; - refuseB = 0; - mockFreeHeap = 30000; - return ok; - } +// Run begin() with logging on. Sizes refuseA/refuseB are refused, +// heap is the mocked free heap. +bool beginWith(UpdaterClass& u, size_t a, size_t b, uint32_t heap) +{ + allocCount = 0; + refuseA = a; + refuseB = b; + mockFreeHeap = heap; + logging = true; + const bool ok = u.begin(6000); + logging = false; + refuseA = 0; + refuseB = 0; + mockFreeHeap = 30000; + return ok; +} - // Under valgrind, operator new is replaced by valgrind's own, whatever the - // test binary defines, so the hook above is never called. Check that - // once, and let the tests that need the hook skip themselves in that case. - bool allocHookWorks() +// Under valgrind, operator new is replaced by valgrind's own, whatever the +// test binary defines, so the hook above is never called. Check that +// once, and let the tests that need the hook skip themselves in that case. +bool allocHookWorks() +{ + static const bool works = []() { - static const bool works = []() { - allocCount = 0; - logging = true; - // volatile: the compiler may not drop the new/delete pair - volatile uint8_t* probe = new (std::nothrow) uint8_t[1]; - logging = false; - if (probe) - { - *probe = 1; - } - delete[] probe; - return allocCount == 1; - }(); - if (!works) + allocCount = 0; + logging = true; + // volatile: the compiler may not drop the new/delete pair + volatile uint8_t* probe = new (std::nothrow) uint8_t[1]; + logging = false; + if (probe) { - WARN("operator new[] cannot be replaced here (valgrind?), test skipped"); + *probe = 1; } - return works; + delete[] probe; + return allocCount == 1; + }(); + if (!works) + { + WARN("operator new[] cannot be replaced here (valgrind?), test skipped"); } + return works; } +} // namespace TEST_CASE("Updater uses a sector sized buffer when there is room", "[core][Updater]") { @@ -143,7 +144,8 @@ TEST_CASE("Updater uses a sector sized buffer when there is room", "[core][Updat u.end(true); // no data written: fails, and frees the buffer } -TEST_CASE("Updater falls back to a small buffer when the big one cannot be allocated", "[core][Updater]") +TEST_CASE("Updater falls back to a small buffer when the big one cannot be allocated", + "[core][Updater]") { if (!allocHookWorks()) { From 312b717cf47b386a86fbbb55c7a129c7cefed876 Mon Sep 17 00:00:00 2001 From: sanerdemirel <300525555+sanerdemirel@users.noreply.github.com> Date: Tue, 10 Nov 2026 22:13:34 +0100 Subject: [PATCH 6/6] Updater host tests: drop the operator new[] hook tests Replacing operator new[] leaks into the whole test runtime and does not work under valgrind. The allocation tests and the mockFreeHeap seam go; only the change in Updater.cpp stays. --- tests/host/common/MockEsp.cpp | 5 +- tests/host/core/test_Updater.cpp | 147 ------------------------------- 2 files changed, 1 insertion(+), 151 deletions(-) diff --git a/tests/host/common/MockEsp.cpp b/tests/host/common/MockEsp.cpp index ec5df941df..18e7be6c83 100644 --- a/tests/host/common/MockEsp.cpp +++ b/tests/host/common/MockEsp.cpp @@ -115,12 +115,9 @@ uint32_t EspClass::getSketchSize() return 400000; } -// Tests can change this to simulate a low heap. -uint32_t mockFreeHeap = 30000; - uint32_t EspClass::getFreeHeap() { - return mockFreeHeap; + return 30000; } uint32_t EspClass::getMaxFreeBlockSize() diff --git a/tests/host/core/test_Updater.cpp b/tests/host/core/test_Updater.cpp index 9d00bb0fd8..8a1ec92bd8 100644 --- a/tests/host/core/test_Updater.cpp +++ b/tests/host/core/test_Updater.cpp @@ -15,8 +15,6 @@ #include #include -#include -#include // Use a SPIFFS file because we can't instantiate a virtual class like Print TEST_CASE("Updater fails when writes overflow requested size", "[core][Updater]") @@ -55,148 +53,3 @@ TEST_CASE("Updater fails when writes overflow requested size", "[core][Updater]" REQUIRE(!u->write(buff, 2048)); delete u; } - -// Count and, on request, refuse array allocations of given sizes, so a test -// can see which buffer size UpdaterClass::begin() asks for. -// UpdaterClass::begin() uses new (std::nothrow) uint8_t[], so replacing -// this one operator is enough. Everything else is passed to malloc(). -namespace -{ -constexpr size_t maxLog = 16; -size_t allocLog[maxLog]; -size_t allocCount = 0; -size_t refuseA = 0; // 0: refuse nothing -size_t refuseB = 0; -bool logging = false; -} // namespace - -void* operator new[](std::size_t size, const std::nothrow_t&) noexcept -{ - if (logging && allocCount < maxLog) - { - allocLog[allocCount++] = size; - } - if (size == refuseA || size == refuseB) - { - return nullptr; - } - return malloc(size); -} - -extern uint32_t mockFreeHeap; - -namespace -{ -// Run begin() with logging on. Sizes refuseA/refuseB are refused, -// heap is the mocked free heap. -bool beginWith(UpdaterClass& u, size_t a, size_t b, uint32_t heap) -{ - allocCount = 0; - refuseA = a; - refuseB = b; - mockFreeHeap = heap; - logging = true; - const bool ok = u.begin(6000); - logging = false; - refuseA = 0; - refuseB = 0; - mockFreeHeap = 30000; - return ok; -} - -// Under valgrind, operator new is replaced by valgrind's own, whatever the -// test binary defines, so the hook above is never called. Check that -// once, and let the tests that need the hook skip themselves in that case. -bool allocHookWorks() -{ - static const bool works = []() - { - allocCount = 0; - logging = true; - // volatile: the compiler may not drop the new/delete pair - volatile uint8_t* probe = new (std::nothrow) uint8_t[1]; - logging = false; - if (probe) - { - *probe = 1; - } - delete[] probe; - return allocCount == 1; - }(); - if (!works) - { - WARN("operator new[] cannot be replaced here (valgrind?), test skipped"); - } - return works; -} -} // namespace - -TEST_CASE("Updater uses a sector sized buffer when there is room", "[core][Updater]") -{ - if (!allocHookWorks()) - { - return; - } - UpdaterClass u; - REQUIRE(beginWith(u, 0, 0, 30000)); - REQUIRE(allocCount == 1); - REQUIRE(allocLog[0] == FLASH_SECTOR_SIZE); - u.end(true); // no data written: fails, and frees the buffer -} - -TEST_CASE("Updater falls back to a small buffer when the big one cannot be allocated", - "[core][Updater]") -{ - if (!allocHookWorks()) - { - return; - } - UpdaterClass u; - uint8_t buff[6000]; - memset(buff, 0, sizeof(buff)); - // Plenty of free heap in total, but no block of 4 KB (fragmentation). - REQUIRE(beginWith(u, FLASH_SECTOR_SIZE, 0, 30000)); - REQUIRE(allocCount == 2); - REQUIRE(allocLog[0] == FLASH_SECTOR_SIZE); - REQUIRE(allocLog[1] == 256); - // And the update still works with the small buffer. - REQUIRE(u.write(buff, 6000)); - REQUIRE(u.remaining() == 0); - REQUIRE(!u.hasError()); - u.end(true); // frees the buffer -} - -TEST_CASE("Updater starts with the small buffer when the heap is low", "[core][Updater]") -{ - if (!allocHookWorks()) - { - return; - } - UpdaterClass u; - REQUIRE(beginWith(u, 0, 0, 2 * FLASH_SECTOR_SIZE)); - REQUIRE(allocCount == 1); - REQUIRE(allocLog[0] == 256); - u.end(true); -} - -TEST_CASE("Updater reports OOM when no buffer can be allocated", "[core][Updater]") -{ - if (!allocHookWorks()) - { - return; - } - UpdaterClass u; - // Low heap: only the small buffer is tried. - REQUIRE(!beginWith(u, 256, 0, 2 * FLASH_SECTOR_SIZE)); - REQUIRE(allocCount == 1); - REQUIRE(u.getError() == UPDATE_ERROR_OOM); - u.clearError(); - // Enough heap: both sizes are tried, both fail. - REQUIRE(!beginWith(u, FLASH_SECTOR_SIZE, 256, 30000)); - REQUIRE(allocCount == 2); - REQUIRE(u.getError() == UPDATE_ERROR_OOM); - // A failed begin() leaves the updater usable. - u.clearError(); - REQUIRE(beginWith(u, 0, 0, 30000)); - u.end(true); -}