refactor: Added utils for non-throwing memory allocation and scoped cleanup (#1832)

## Summary Pared down version of #1418. Following up on b5df6cb2b5. Added lib/Memory/Memory.h with: - `makeUniqueNoThrow<T>` a `nothrow` wrapper for `std::make_unique` that return `nullptr` on OOM instead of calling `abort()` (the behavior of bare `new` with `-fno-exceptions`) - `ScopedCleanup` a helper to call a cleanup lambda on scope exit. These utilities help to write code that handles OOM scenarios gracefully, and consistently cleans up resources on scope exit. JpegToBmpConverter.cpp has been converted to use these utilities. Other files can be converted later. This will simplify some of the SD card font resource management in #1327. --- ### AI Usage While CrossPoint doesn't have restrictions on AI tools in contributing, please be transparent about their usage as it helps set the right context for reviewers. Did you use AI tools to help write this code? _**NO**_
2026-05-13 21:19:47 -05:00
parent 71518b3336
commit 8377ac9e31
3 changed files with 156 additions and 72 deletions
@@ -58,6 +58,7 @@ find src -name "*.cpp" -o -name "*.h" | xargs clang-format -i
 6. `constexpr` First: Compile-time constants and lookup tables must be `constexpr`, not just `static const`. This moves computation to compile time, enables dead-branch elimination, and guarantees flash placement. Use `static constexpr` for class-level constants.
 7. `std::vector` Pre-allocation: Always call `.reserve(N)` before any `push_back()` loop. Each growth event allocates a new block (2×), copies all elements, then frees the old one — three heap operations that fragment DRAM. When the final size is unknown, estimate conservatively.
 8. SPIFFS Write Throttling: Never write a settings file on every user interaction. Guard all writes with a value-change check (`if (newVal == _current) return;`). Progress saves during reading must be debounced — write on activity exit or every N page turns, not on every turn. SPIFFS sectors have a finite erase cycle limit.
+9. `new` is not nothrow on ESP32: With `-fno-exceptions`, bare `new` that fails calls `abort()` — it does NOT return `nullptr`. Always use `new (std::nothrow)` and null-check the result, or use `makeUniqueNoThrow<T>()` from `lib/Memory/Memory.h`. Never write bare `new` for any fallible allocation.

 ---

@@ -266,43 +267,78 @@ When a template is necessary, limit instantiations: use explicit template instan

 **Rules**: NO exceptions, NO abort(), ALWAYS log before error return

-### Acceptable malloc/free Patterns
+### Heap Buffer Allocation

-**Source**: [src/activities/home/HomeActivity.cpp:166](../src/activities/home/HomeActivity.cpp), [lib/GfxRenderer/GfxRenderer.cpp:439-440](../lib/GfxRenderer/GfxRenderer.cpp)
+**Prefer `makeUniqueNoThrow` over `malloc`.** Both are nothrow (return `nullptr` on OOM rather than calling `abort()`), but `malloc` requires a manual `free` on every return path — a common source of leaks. `makeUniqueNoThrow<uint8_t[]>(size)` from `lib/Memory/Memory.h` frees automatically when it goes out of scope.

-Despite "prefer stack allocation," malloc is acceptable for:
-1. **Large temporary buffers** (> 256 bytes, won't fit on stack)
-2. **One-time allocations** during activity initialization
-3. **Bitmap rendering buffers** (variable size, used briefly)
-
-**Pattern**:
+**Preferred pattern**:
 ```cpp
-// Allocate
-auto* buffer = static_cast<uint8_t*>(malloc(bufferSize));
+#include <Memory.h>
+
+auto buffer = makeUniqueNoThrow<uint8_t[]>(bufferSize);
 if (!buffer) {
-  LOG_ERR("MODULE", "malloc failed: %d bytes", bufferSize);
-  return false;  // Handle allocation failure
+  LOG_ERR("MODULE", "OOM: %d bytes", bufferSize);
+  return false;
 }

-// Use buffer
-processData(buffer, bufferSize);
+processData(buffer.get(), bufferSize);
+// freed automatically — no manual free needed, no leak on early return
+```

-// Free immediately after use
-free(buffer);
-buffer = nullptr;
+**`malloc` or `new (std::nothrow)` are still acceptable** when the buffer must be passed to a C API that takes ownership and frees it itself (e.g., certain SDK callbacks). In that case follow the manual pattern:
+```cpp
+auto* buffer = static_cast<uint8_t*>(malloc(bufferSize));  // or new (std::nothrow) uint8_t[bufferSize]
+if (!buffer) {
+  LOG_ERR("MODULE", "OOM: %d bytes", bufferSize);
+  return false;
+}
+sdkApiThatTakesOwnership(buffer, bufferSize);  // SDK calls free() / delete[]
 ```

 **Rules**:
- **ALWAYS check for nullptr** after malloc
- **Free immediately** after use (don't hold across multiple operations)
- **Set to nullptr** after free (avoid use-after-free)
- **Document size**: Comment why stack allocation was rejected
+- **Prefer `makeUniqueNoThrow`** — automatic cleanup eliminates leak risk on error paths
+- **ALWAYS check for nullptr** after any allocation and `LOG_ERR` before returning false
+- **Raw allocation only** when a C API takes ownership; document why in a comment

 **Examples in codebase**:
+- Memory utilities: [Memory.h](../lib/Memory/Memory.h) (`makeUniqueNoThrow`)
 - Cover image buffers: [HomeActivity.cpp:166](../src/activities/home/HomeActivity.cpp)
- Text chunk buffers: [TxtReaderActivity.cpp:259](../src/activities/reader/TxtReaderActivity.cpp)
 - Bitmap rendering: [GfxRenderer.cpp:439-440](../lib/GfxRenderer/GfxRenderer.cpp)
- OTA update buffer: [OtaUpdater.cpp:40](../src/network/OtaUpdater.cpp)
+
+### Heap Allocation with `new`: Always Use `makeUniqueNoThrow`
+
+**CRITICAL**: With `-fno-exceptions`, bare `new` on OOM calls `abort()` — it does NOT return `nullptr`. Always use `makeUniqueNoThrow` from `lib/Memory/Memory.h`, which wraps `new (std::nothrow)` and returns a `std::unique_ptr` that is null on OOM and automatically frees on scope exit.
+
+**Preferred pattern**:
+```cpp
+#include <Memory.h>
+
+auto obj = makeUniqueNoThrow<MyClass>(args);
+if (!obj) { LOG_ERR("MOD", "OOM: MyClass"); return false; }
+
+auto buf = makeUniqueNoThrow<uint8_t[]>(size);
+if (!buf) { LOG_ERR("MOD", "OOM: %d bytes", size); return false; }
+
+// Pass to C APIs via .get(); unique_ptr frees automatically on return
+someApi(buf.get(), size);
+```
+
+**`new (std::nothrow)` directly is acceptable** when the object must be passed to a C API that takes ownership and calls `delete` itself:
+```cpp
+auto* obj = new (std::nothrow) MyClass(args);
+if (!obj) { LOG_ERR("MOD", "OOM: MyClass"); return false; }
+sdkApiThatTakesOwnership(obj);  // SDK calls delete
+```
+
+**Rules**:
+- **Prefer `makeUniqueNoThrow`** — automatic cleanup eliminates leak risk on error paths
+- **NEVER use bare `new`** — always `makeUniqueNoThrow` or `new (std::nothrow)`
+- **ALWAYS `LOG_ERR` before returning false** on OOM
+- **Use `.get()`** to pass the raw pointer to C-style APIs; ownership stays with the `unique_ptr`
+- **`new (std::nothrow)` directly only** when a C API takes ownership; document why in a comment
+
+**Examples in codebase**:
+- Memory utilities: [Memory.h](../lib/Memory/Memory.h) (`makeUniqueNoThrow`)

 ---

@@ -4,10 +4,10 @@
 #include <HalStorage.h>
 #include <JPEGDEC.h>
 #include <Logging.h>
+#include <Memory.h>

 #include <cstdio>
 #include <cstring>
-#include <new>

 #include "BitmapHelpers.h"

@@ -211,26 +211,26 @@ struct BmpConvertCtx {

  // Accumulates one MCU row (up to MAX_MCU_HEIGHT source rows × srcWidth pixels)
  // Filled column-by-column as JPEGDEC callbacks arrive for the same MCU row
-  uint8_t* mcuBuf;
+  std::unique_ptr<uint8_t[]> mcuBuf;

  // Y-axis area averaging accumulators (needsScaling only)
  int currentOutY;
  uint32_t nextOutY_srcStart;  // 16.16 fixed-point boundary for the next output row
-  uint32_t* rowAccum;
-  uint32_t* rowCount;
+  std::unique_ptr<uint32_t[]> rowAccum;
+  std::unique_ptr<uint32_t[]> rowCount;

-  uint8_t* bmpRow;
+  std::unique_ptr<uint8_t[]> bmpRow;

-  AtkinsonDitherer* atkinsonDitherer;
-  FloydSteinbergDitherer* fsDitherer;
-  Atkinson1BitDitherer* atkinson1BitDitherer;
+  std::unique_ptr<AtkinsonDitherer> atkinsonDitherer;
+  std::unique_ptr<FloydSteinbergDitherer> fsDitherer;
+  std::unique_ptr<Atkinson1BitDitherer> atkinson1BitDitherer;

  bool error;
 };

 // Write a fully-assembled output row (grayscale bytes, length outWidth) to BMP
 static void writeOutputRow(BmpConvertCtx* ctx, const uint8_t* srcRow, int outY) {
-  memset(ctx->bmpRow, 0, ctx->bytesPerRow);
+  memset(ctx->bmpRow.get(), 0, ctx->bytesPerRow);

  if (USE_8BIT_OUTPUT && !ctx->oneBit) {
    for (int x = 0; x < ctx->outWidth; x++) {
@@ -262,12 +262,12 @@ static void writeOutputRow(BmpConvertCtx* ctx, const uint8_t* srcRow, int outY)
      ctx->fsDitherer->nextRow();
  }

-  ctx->bmpOut->write(ctx->bmpRow, ctx->bytesPerRow);
+  ctx->bmpOut->write(ctx->bmpRow.get(), ctx->bytesPerRow);
 }

 // Flush one scaled output row from Y-axis accumulators and advance currentOutY
 static void flushScaledRow(BmpConvertCtx* ctx) {
-  memset(ctx->bmpRow, 0, ctx->bytesPerRow);
+  memset(ctx->bmpRow.get(), 0, ctx->bytesPerRow);

  if (USE_8BIT_OUTPUT && !ctx->oneBit) {
    for (int x = 0; x < ctx->outWidth; x++) {
@@ -301,7 +301,7 @@ static void flushScaledRow(BmpConvertCtx* ctx) {
      ctx->fsDitherer->nextRow();
  }

-  ctx->bmpOut->write(ctx->bmpRow, ctx->bytesPerRow);
+  ctx->bmpOut->write(ctx->bmpRow.get(), ctx->bytesPerRow);
  ctx->currentOutY++;
 }

@@ -324,7 +324,7 @@ int bmpDrawCallback(JPEGDRAW* pDraw) {
  for (int r = 0; r < blockH && r < MAX_MCU_HEIGHT; r++) {
    const int copyW = (blockX + validW <= ctx->srcWidth) ? validW : (ctx->srcWidth - blockX);
    if (copyW <= 0) continue;
-    memcpy(ctx->mcuBuf + r * ctx->srcWidth + blockX, pixels + r * stride, copyW);
+    memcpy(ctx->mcuBuf.get() + r * ctx->srcWidth + blockX, pixels + r * stride, copyW);
  }

  // Wait for the last MCU column before processing any rows
@@ -334,7 +334,7 @@ int bmpDrawCallback(JPEGDRAW* pDraw) {
  const int endRow = blockY + blockH;

  for (int y = blockY; y < endRow && y < ctx->srcHeight; y++) {
-    const uint8_t* srcRow = ctx->mcuBuf + (y - blockY) * ctx->srcWidth;
+    const uint8_t* srcRow = ctx->mcuBuf.get() + (y - blockY) * ctx->srcWidth;

    if (!ctx->needsScaling) {
      // 1:1 — outWidth == srcWidth, write directly
@@ -364,8 +364,8 @@ int bmpDrawCallback(JPEGDRAW* pDraw) {
        flushScaledRow(ctx);
        ctx->nextOutY_srcStart = static_cast<uint32_t>(ctx->currentOutY + 1) * ctx->scaleY_fp;
        if (srcY_fp >= ctx->nextOutY_srcStart) continue;
-        memset(ctx->rowAccum, 0, ctx->outWidth * sizeof(uint32_t));
-        memset(ctx->rowCount, 0, ctx->outWidth * sizeof(uint32_t));
+        memset(ctx->rowAccum.get(), 0, ctx->outWidth * sizeof(uint32_t));
+        memset(ctx->rowCount.get(), 0, ctx->outWidth * sizeof(uint32_t));
      }
    }
  }
@@ -387,19 +387,20 @@ bool JpegToBmpConverter::jpegFileToBmpStreamInternal(FsFile& jpegFile, Print& bm

  s_jpegFile = &jpegFile;

-  JPEGDEC* jpeg = new (std::nothrow) JPEGDEC();
+  const auto jpeg = makeUniqueNoThrow<JPEGDEC>();
  if (!jpeg) {
-    LOG_ERR("JPG", "Failed to allocate JPEG decoder");
+    LOG_ERR("JPG", "OOM: JPEG decoder");
    return false;
  }

  int rc = jpeg->open("", bmpJpegOpen, bmpJpegClose, bmpJpegRead, bmpJpegSeek, bmpDrawCallback);
  if (rc != 1) {
    LOG_ERR("JPG", "JPEG open failed (err=%d)", jpeg->getLastError());
-    delete jpeg;
    return false;
  }

+  const ScopedCleanup cleanup{[&jpeg]() { jpeg->close(); }};
+
  const int srcWidth = jpeg->getWidth();
  const int srcHeight = jpeg->getHeight();

@@ -411,8 +412,6 @@ bool JpegToBmpConverter::jpegFileToBmpStreamInternal(FsFile& jpegFile, Print& bm
  if (srcWidth <= 0 || srcHeight <= 0 || srcWidth > MAX_IMAGE_WIDTH || srcHeight > MAX_IMAGE_HEIGHT) {
    LOG_DBG("JPG", "Image too large or invalid (%dx%d), max supported: %dx%d", srcWidth, srcHeight, MAX_IMAGE_WIDTH,
            MAX_IMAGE_HEIGHT);
-    jpeg->close();
-    delete jpeg;
    return false;
  }

@@ -472,54 +471,49 @@ bool JpegToBmpConverter::jpegFileToBmpStreamInternal(FsFile& jpegFile, Print& bm
  ctx.scaleY_fp = scaleY_fp;
  ctx.error = false;

-  // RAII guard: frees all heap resources on any return path
-  struct Cleanup {
-    BmpConvertCtx& ctx;
-    JPEGDEC* jpeg;
-    ~Cleanup() {
-      delete[] ctx.rowAccum;
-      delete[] ctx.rowCount;
-      delete ctx.atkinsonDitherer;
-      delete ctx.fsDitherer;
-      delete ctx.atkinson1BitDitherer;
-      free(ctx.mcuBuf);
-      free(ctx.bmpRow);
-      jpeg->close();
-      delete jpeg;
-    }
-  } cleanup{ctx, jpeg};
-
  // MCU row buffer: MAX_MCU_HEIGHT rows × srcWidth columns of grayscale
-  ctx.mcuBuf = static_cast<uint8_t*>(malloc(MAX_MCU_HEIGHT * srcWidth));
+  ctx.mcuBuf = makeUniqueNoThrow<uint8_t[]>(MAX_MCU_HEIGHT * srcWidth);
  if (!ctx.mcuBuf) {
-    LOG_ERR("JPG", "Failed to allocate MCU buffer (%d bytes)", MAX_MCU_HEIGHT * srcWidth);
+    LOG_ERR("JPG", "OOM: MCU buffer (%d bytes)", MAX_MCU_HEIGHT * srcWidth);
    return false;
  }
-  memset(ctx.mcuBuf, 0, MAX_MCU_HEIGHT * srcWidth);
+  memset(ctx.mcuBuf.get(), 0, MAX_MCU_HEIGHT * srcWidth);

-  ctx.bmpRow = static_cast<uint8_t*>(malloc(bytesPerRow));
+  ctx.bmpRow = makeUniqueNoThrow<uint8_t[]>(bytesPerRow);
  if (!ctx.bmpRow) {
-    LOG_ERR("JPG", "Failed to allocate BMP row buffer");
+    LOG_ERR("JPG", "OOM: BMP row buffer");
    return false;
  }

  if (needsScaling) {
-    ctx.rowAccum = new (std::nothrow) uint32_t[outWidth]();
-    ctx.rowCount = new (std::nothrow) uint32_t[outWidth]();
+    ctx.rowAccum = makeUniqueNoThrow<uint32_t[]>(outWidth);
+    ctx.rowCount = makeUniqueNoThrow<uint32_t[]>(outWidth);
    if (!ctx.rowAccum || !ctx.rowCount) {
-      LOG_ERR("JPG", "Failed to allocate scaling buffers");
+      LOG_ERR("JPG", "OOM: scaling buffers");
      return false;
    }
    ctx.nextOutY_srcStart = scaleY_fp;
  }

  if (oneBit) {
-    ctx.atkinson1BitDitherer = new (std::nothrow) Atkinson1BitDitherer(outWidth);
+    ctx.atkinson1BitDitherer = makeUniqueNoThrow<Atkinson1BitDitherer>(outWidth);
+    if (!ctx.atkinson1BitDitherer) {
+      LOG_ERR("JPG", "OOM: Atkinson1BitDitherer");
+      return false;
+    }
  } else if (!USE_8BIT_OUTPUT) {
    if (USE_ATKINSON) {
-      ctx.atkinsonDitherer = new (std::nothrow) AtkinsonDitherer(outWidth);
+      ctx.atkinsonDitherer = makeUniqueNoThrow<AtkinsonDitherer>(outWidth);
+      if (!ctx.atkinsonDitherer) {
+        LOG_ERR("JPG", "OOM: AtkinsonDitherer");
+        return false;
+      }
    } else if (USE_FLOYD_STEINBERG) {
-      ctx.fsDitherer = new (std::nothrow) FloydSteinbergDitherer(outWidth);
+      ctx.fsDitherer = makeUniqueNoThrow<FloydSteinbergDitherer>(outWidth);
+      if (!ctx.fsDitherer) {
+        LOG_ERR("JPG", "OOM: FloydSteinbergDitherer");
+        return false;
+      }
    }
  }

@@ -0,0 +1,54 @@
+#pragma once
+
+#include <cstddef>
+#include <memory>
+#include <new>
+#include <type_traits>
+#include <utility>
+
+// Nothrow versions of std::make_unique. Return nullptr on allocation failure
+// instead of calling abort() (the default when exceptions are disabled on ESP32).
+//
+// Single object:
+//   auto obj = makeUniqueNoThrow<PNG>();
+//   if (!obj) { LOG_ERR("TAG", "OOM"); return false; }
+//
+// Array:
+//   auto buf = makeUniqueNoThrow<uint8_t[]>(size);
+//   if (!buf) { LOG_ERR("TAG", "OOM"); return false; }
+//   buf[0] = 0xFF;
+//   someApi(buf.get(), size);
+//
+
+template <typename T, typename... Args>
+  requires(!std::is_array_v<T>)
+std::unique_ptr<T> makeUniqueNoThrow(Args&&... args) {
+  return std::unique_ptr<T>(new (std::nothrow) T(std::forward<Args>(args)...));
+}
+
+template <typename T>
+  requires std::is_unbounded_array_v<T>
+std::unique_ptr<T> makeUniqueNoThrow(size_t count) {
+  using Elem = std::remove_extent_t<T>;
+  return std::unique_ptr<T>(new (std::nothrow) Elem[count]());
+}
+
+// Helper struct to call a cleanup function on exit from any scope.
+// Use with a lambda to avoid unnecessary allocations from std::function/std::bind:
+// Example:
+//   auto jpeg = makeUniqueNoThrow<JPEGDEC>();
+//   ScopedCleanup cleanup{[&jpeg]{ jpeg->close(); }};
+//
+template <typename F>
+struct [[nodiscard]] ScopedCleanup final {
+  const F fn;
+  explicit ScopedCleanup(F f) : fn{std::move(f)} {}
+  ScopedCleanup(const ScopedCleanup&) = delete;
+  ScopedCleanup& operator=(const ScopedCleanup&) = delete;
+  ScopedCleanup(ScopedCleanup&&) = delete;
+  ScopedCleanup& operator=(ScopedCleanup&&) = delete;
+  ~ScopedCleanup() { fn(); }
+};
+
+template <typename F>
+ScopedCleanup(F) -> ScopedCleanup<F>;