Fortify image_helpers

[bettio]

It was possible to crash AtomVM by using load_image with invalid data.
Fortify it, so it returns an error instead of crashing.

[petermm]

PR Review — "Fortify image_helpers" (08bb49a)

Summary

The commit hardens handle_load_image in image_helpers.c so that
invalid PNG input returns {Ref, {error, Reason}} instead of crashing AtomVM. It now
checks tuple arity, binary type, spng_ctx_new, spng_set_png_buffer,
spng_decoded_image_size, malloc, and spng_decode_image, and refactors the reply
into send_decoded_image / send_load_image_error helpers.

The direction is correct and most of the new error handling is sound. However it does
not fully achieve its stated goal ("invalid/hostile input should never crash"):
one unchecked AtomVM allocation path remains, so a large valid PNG header can still
crash the VM.

Verdict: changes requested — do not merge as-is.

---

Findings

🔴 Blocker — unchecked AtomVM binary allocation can still crash on large images

send_decoded_image() builds the result with term_from_literal_binary():

// image_helpers.c (send_decoded_image)
term_put_tuple_element(return_tuple, 1,
    term_from_literal_binary(data, size, &heap, ctx->global));

In AtomVM, for binaries larger than the heap-binary threshold,
term_from_literal_binary() → term_create_uninitialized_binary() →
term_alloc_refc_binary(), which returns term_invalid_term() when the off-heap
refc allocation fails (src/libAtomVM/term.c, term_alloc_refc_binary).
term_from_literal_binary() then immediately does:

memcpy((void *) term_binary_data(binary), data, size);  // term.h:1996-1998

on that invalid term → dereference of garbage → crash. So a decoded RGBA8 image large
enough to fail the refc allocation (megabytes/GB; out_size = width*height*4 with
attacker-controlled dimensions) still crashes AtomVM, which is exactly the failure mode
this commit set out to remove.

Smallest fix — allocate explicitly and check before copying:

BEGIN_WITH_STACK_HEAP(TUPLE_SIZE(2) + REF_SIZE + term_binary_heap_size(size), heap);

term bin = term_create_uninitialized_binary(size, &heap, ctx->global);
if (UNLIKELY(term_is_invalid_term(bin))) {
    END_WITH_STACK_HEAP(heap, ctx->global);
    send_load_image_error(ref, pid, OUT_OF_MEMORY_ATOM, ctx);
    return;
}
memcpy((void *) term_binary_data(bin), data, size);

term return_tuple = term_alloc_tuple(2, &heap);
term_put_tuple_element(return_tuple, 0, ref);
term_put_tuple_element(return_tuple, 1, bin);
...

Better still: allocate the AtomVM binary first and decode spng_decode_image() directly
into term_binary_data(bin), dropping the separate malloc(out_size) entirely. That
removes one full-image allocation (halves peak memory for large images) and the manual
free(out).

Preferred fix — decode directly into a checked AtomVM binary:

diff --git a/image_helpers.c b/image_helpers.c
index 0000000..0000000 100644
--- a/image_helpers.c
+++ b/image_helpers.c
@@
-static void send_decoded_image(term ref, term pid, const void *data, size_t size, Context *ctx)
-{
-    // TODO: change return format to {ok, binary}
-    BEGIN_WITH_STACK_HEAP(TUPLE_SIZE(2) + REF_SIZE + term_binary_heap_size(size), heap);
-
-    term return_tuple = term_alloc_tuple(2, &heap);
-    term_put_tuple_element(return_tuple, 0, ref);
-    term_put_tuple_element(return_tuple, 1, term_from_literal_binary(data, size, &heap, ctx->global));
-
-    int local_process_id = term_to_local_process_id(pid);
-    globalcontext_send_message(ctx->global, local_process_id, return_tuple);
-
-    END_WITH_STACK_HEAP(heap, ctx->global)
-}
-
 static void send_load_image_error(term ref, term pid, term reason, Context *ctx)
 {
@@
-    void *out = malloc(out_size);
-    if (UNLIKELY(!out)) {
-        fprintf(stderr, "handle_load_image: cannot allocate %zu bytes for decoded image\n", out_size);
-        spng_ctx_free(png_ctx);
-        send_load_image_error(ref, pid, OUT_OF_MEMORY_ATOM, ctx);
-        return;
-    }
+    BEGIN_WITH_STACK_HEAP(TUPLE_SIZE(2) + REF_SIZE + term_binary_heap_size(out_size), heap);
 
-    ret = spng_decode_image(png_ctx, out, out_size, SPNG_FMT_RGBA8, 0);
+    term out_bin = term_create_uninitialized_binary(out_size, &heap, ctx->global);
+    if (UNLIKELY(term_is_invalid_term(out_bin))) {
+        END_WITH_STACK_HEAP(heap, ctx->global);
+        fprintf(stderr, "handle_load_image: cannot allocate %zu bytes for decoded image\n", out_size);
+        spng_ctx_free(png_ctx);
+        send_load_image_error(ref, pid, OUT_OF_MEMORY_ATOM, ctx);
+        return;
+    }
+
+    ret = spng_decode_image(png_ctx, (void *) term_binary_data(out_bin), out_size, SPNG_FMT_RGBA8, 0);
     spng_ctx_free(png_ctx);
     if (UNLIKELY(ret != SPNG_OK)) {
         fprintf(stderr, "handle_load_image: spng_decode_image failed: %s\n", spng_strerror(ret));
-        free(out);
+        END_WITH_STACK_HEAP(heap, ctx->global);
         send_load_image_error(ref, pid, invalid_image_reason(ctx), ctx);
         return;
     }
 
-    send_decoded_image(ref, pid, out, out_size, ctx);
+    term return_tuple = term_alloc_tuple(2, &heap);
+    term_put_tuple_element(return_tuple, 0, ref);
+    term_put_tuple_element(return_tuple, 1, out_bin);
 
-    free(out);
+    int local_process_id = term_to_local_process_id(pid);
+    globalcontext_send_message(ctx->global, local_process_id, return_tuple);
+
+    END_WITH_STACK_HEAP(heap, ctx->global)
 }

---

🟡 Should-fix — no decoded-size / dimension cap before allocating attacker-controlled memory

out_size from spng_decoded_image_size() is passed straight to malloc():

ret = spng_decoded_image_size(png_ctx, SPNG_FMT_RGBA8, &out_size);
...
void *out = malloc(out_size);

spng rejects zero dimensions and arithmetic overflow, but its defaults allow very large
dimensions (max_width/max_height = spng_u32max) and chunk cache up to SIZE_MAX. A
valid PNG header can therefore force a multi-MB/GB allocation attempt before any error is
returned — a cheap decompression/allocation DoS on an embedded target.

Smallest fix — after spng_ctx_new(), constrain spng, and/or cap out_size:

spng_set_image_limits(png_ctx, MAX_IMG_W, MAX_IMG_H);   // appropriate to the display
spng_set_chunk_limits(png_ctx, MAX_CHUNK, MAX_TOTAL);   // if untrusted ancillary chunks possible
// and/or:
if (UNLIKELY(out_size > MAX_DECODED_IMAGE_BYTES)) {
    spng_ctx_free(png_ctx);
    send_load_image_error(ref, pid, OUT_OF_MEMORY_ATOM, ctx);
    return;
}

---

Verified non-issues (checked against AtomVM source)

• ref on a stack heap is safe. globalcontext_send_message() → mailbox_send()
  copies the term tree into owned mailbox storage synchronously before returning, and the
  incoming message is disposed only after process_message() returns. Same pattern the
  pre-commit code and other handlers in dcs_lcd_display_driver.c
  use.
• term_binary_heap_size(size) is the correct stack-heap sizing. For large binaries
  only a small boxed refc descriptor lives on the heap (bytes are off-heap), so the stack
  heap does not overflow for MB-scale images. (The remaining problem is purely the
  unchecked off-heap allocation in the Blocker above.)
• invalid_image_reason(ctx) is safe before BEGIN_WITH_STACK_HEAP.
  globalcontext_make_atom() returns an immediate atom term and does not allocate into or
  GC the later stack heap; it falls back to ERROR_ATOM on failure.
• term_get_tuple_arity(req) is safe. The caller already verifies
  term_is_tuple(req) && term_get_tuple_arity(req) >= 1 before calling
  handle_load_image (dcs_lcd_display_driver.c and the other drivers).
• No leak / double-free in the new branches. png_ctx is freed on every path after a
  successful spng_ctx_new(); out is freed on decode failure and after a successful send.
• ERROR_ATOM, BADARG_ATOM, OUT_OF_MEMORY_ATOM are valid predefined atoms
  (src/libAtomVM/defaultatoms.def) and are used appropriately.
• out_size == 0 cannot occur for a valid decoded RGBA8 image (spng rejects zero
  width/height).

---

Recommendation

Fix the Blocker (unchecked term_from_literal_binary allocation) before merge — it is the
one remaining path that defeats the commit's purpose. Add the decoded-size/dimension cap
(Should-fix) as well; it is cheap and important for embedded targets.