Fix atomvm:read_priv and detect truncated packs

[bettio]

The packbeam (.avm) scanners were unbounded and trusted section sizes,
so a lookup miss (e.g. atomvm:read_priv/2 for an unpacked file) ran off
the pack into erased flash and crashed the VM. A truncated pack failed
the same way.

Bound and validate every scan, detect truncation at load, and add a
build-time guard for the one pack known at build time. No on-disk
format change.

Also user visible:

• scans stop only at the end marker, so sections after a data file
  are found
• atomvm:read_priv/2 raises invalid_avm on a corrupt entry instead
  of reporting a missing file
• rejecting an invalid .avm no longer leaks the file mapping

These changes are made under both the "Apache 2.0" and the "GNU Lesser
General Public License 2.1 or later" license terms (dual license).

SPDX-License-Identifier: Apache-2.0 OR LGPL-2.1-or-later

[petermm]

PR Review: AVM pack bounds and truncation hardening

Range reviewed: f7594af1cee35aafb2d23b644937b7f242d45d6e..db1b7c1be81e579e65cc67bc12643829fbdae07e (last four commits)
Scope: 23 files, 617 insertions, 110 deletions
Verdict: Request changes — 2 high-confidence merge blockers, 1 additional high-severity JIT bug, and several smaller correctness/coverage issues.

The bounded section walker is a strong improvement over the previous unbounded scans, and the new read_priv/2 payload check correctly distinguishes a corrupt entry from a missing resource. The load-time validation added around it is not yet strong enough to support the changelog claim, however, and the public binary-loading path introduces an alignment fault on strict-alignment targets.

Findings

1. [Blocker] Validation dereferences potentially unaligned binary data

Files: src/libAtomVM/avmpack.c:74-84, src/libAtomVM/nifs.c:5491-5505, src/libAtomVM/nifs.c:5773-5783

read_section() casts avmpack_binary + offset to const uint32_t * and dereferences it. atomvm:add_avm_pack_binary/2 now invokes this parser before deciding whether to copy the binary, but AtomVM sub-binaries can start at any byte offset (term_binary_data() returns the parent pointer plus the sub-binary offset). A valid AVM pack wrapped in a one-byte-offset sub-binary therefore performs an unaligned uint32_t load. This is undefined behavior in C and can fault on RP2, STM32, and other strict-alignment targets.

The read_priv/2 prefix read has the same assumption: a section payload is aligned relative to the pack base, not necessarily to an unaligned sub-binary base.

Small fix: use alignment-safe loads for both section words and the priv length prefix. memcpy avoids both alignment and strict-aliasing issues.

diff --git a/src/libAtomVM/avmpack.c b/src/libAtomVM/avmpack.c
@@
-    const uint32_t *header = (const uint32_t *) ((const uint8_t *) avmpack_binary + offset);
-    uint32_t section_size = ENDIAN_SWAP_32(header[0]);
-    uint32_t flags = ENDIAN_SWAP_32(header[1]);
-    const char *name = (const char *) (header + 3);
+    const uint8_t *header = (const uint8_t *) avmpack_binary + offset;
+    uint32_t section_size_be;
+    uint32_t flags_be;
+    memcpy(&section_size_be, header, sizeof(section_size_be));
+    memcpy(&flags_be, header + sizeof(section_size_be), sizeof(flags_be));
+    uint32_t section_size = ENDIAN_SWAP_32(section_size_be);
+    uint32_t flags = ENDIAN_SWAP_32(flags_be);
+    const char *name = (const char *) header + AVMPACK_SECTION_HEADER_SIZE;
diff --git a/src/libAtomVM/nifs.c b/src/libAtomVM/nifs.c
@@
-            uint32_t file_size = READ_32_ALIGNED((uint32_t *) bin_data);
+            uint32_t file_size_be;
+            memcpy(&file_size_be, bin_data, sizeof(file_size_be));
+            uint32_t file_size = ENDIAN_SWAP_32(file_size_be);

Add a regression using a valid pack at storage + 1, including the add_avm_pack_binary/2 and read_priv/2 paths. A naturally aligned C fixture does not exercise this failure.

---

2. [Blocker] avmpack_check_complete() accepts an unreachable terminator

Files: src/libAtomVM/avmpack.c:181-193, src/libAtomVM/nifs.c:5498-5505, src/platforms/generic_unix/lib/sys.c:434-440, src/platforms/emscripten/src/lib/sys.c:692-706, src/platforms/esp32/components/avm_sys/sys.c:483-493

For exact-size inputs, avmpack_check_complete() validates only the magic bytes and the final 16 bytes. It does not verify that walking the section chain reaches that final marker.

A file can therefore contain:

1. a valid 24-byte AVM header;
2. a first section whose declared size extends past the file; and
3. a syntactically valid end marker in the final 16 bytes.

That file is accepted by atomvm:add_avm_pack_binary/2 and the exact-size platform loaders even though every normal traversal stops at the corrupt first section. This contradicts the new changelog statement that truncated or oversized packs are rejected at load.

Small fix: use the bounded walk and require it to compute exactly the supplied size.

diff --git a/src/libAtomVM/avmpack.c b/src/libAtomVM/avmpack.c
@@
 bool avmpack_check_complete(const void *avmpack_binary, uint32_t exact_size)
 {
-    // Require a 4-byte aligned size so the tail read below stays aligned.
-    if (!avmpack_is_valid(avmpack_binary, exact_size)
-        || exact_size < AVMPACK_SIZE + AVMPACK_END_MARKER_SIZE || (exact_size & 3) != 0) {
-        return false;
-    }
-
-    struct AVMPackSection section;
-
-    return read_section(avmpack_binary, exact_size, exact_size - AVMPACK_END_MARKER_SIZE, &section)
-        == AVMPackSectionEnd;
+    uint32_t real_size;
+    return avmpack_compute_size(avmpack_binary, exact_size, &real_size)
+        && real_size == exact_size;
 }

Add a test with an oversized first section and a valid marker at EOF. The current oversized-section test calls only avmpack_compute_size() and does not expose this acceptance path.

---

3. [High] JIT stream creation proceeds when one of several packs has no reachable terminator

Files: src/libAtomVM/jit_stream_flash.c:136-173, src/libAtomVM/jit_stream_flash.c:227-234, src/libAtomVM/jit_stream_flash.c:409-428

globalcontext_find_first_jit_entry() sets valid_cache = false when a pack lookup fails, but continues scanning and returns an append address if any other pack has a terminator. globalcontext_find_last_jit_entry() rejects that invalid state, after which nif_jit_stream_flash_new() calls globalcontext_find_first_jit_entry() again and checks only whether the returned pointer is null; it ignores is_valid.

With one malformed pack and one valid pack, jit_stream_flash:new/1 can consequently write a new cache entry. Finalization cannot mark the cache valid because globalcontext_set_cache_valid() correctly refuses to continue when a pack has no terminator. This leaves orphaned flash writes and causes repeated recompilation/flash wear.

The existing test covers only the single-pack/no-terminator case, where max_end_offset remains null.

The is_valid flag cannot simply be rejected in nif_jit_stream_flash_new(): lowercase end deliberately means “valid append location, cache not finalized yet.” Missing terminators need a separate condition.

Small fix: return no append position unless every pack has a reachable terminator, while preserving the lowercase/uppercase cache-state distinction.

diff --git a/src/libAtomVM/jit_stream_flash.c b/src/libAtomVM/jit_stream_flash.c
@@
     bool valid_cache = true;
+    bool all_have_end = true;
@@
         if (!avmpack_find_section_by_flag(avmpack_data->data, avmpack_data->size, END_OF_FILE_MASK,
                 END_OF_FILE, &end_offset, &end_size, &end_name)) {
             valid_cache = false;
+            all_have_end = false;
             continue;
         }
@@
-    if (IS_NULL_PTR(max_end_offset)) {
+    if (!all_have_end || IS_NULL_PTR(max_end_offset)) {
         *is_valid = false;
         return NULL;
     }

Add a regression registering one valid and one terminatorless pack; jit_stream_flash:new/1 must raise without writing flash.

---

4. [Medium] Invalid Unix packs leak their mapping, and oversized files are narrowed before validation

Files: src/platforms/generic_unix/lib/sys.c:434-447

The newly added invalid-pack branch returns without mapped_file_close(mapped), leaking the mapping, descriptor, and MappedFile allocation. Repeated attempts to open malformed AVMs can exhaust process resources.

The same path casts mapped->size to uint32_t before validation or storage. A file larger than UINT32_MAX is validated against only its low 32 bits and then records that truncated size. atomvm:add_avm_pack_binary/2 already rejects this case explicitly; file loaders should preserve the same contract. Similar unchecked narrowing exists in the Emscripten loader, and ESP32 narrows off_t/size_t through int.

Small fix for generic Unix:

diff --git a/src/platforms/generic_unix/lib/sys.c b/src/platforms/generic_unix/lib/sys.c
@@
-    if (UNLIKELY(!avmpack_check_complete(mapped->mapped, (uint32_t) mapped->size))) {
+    if (UNLIKELY(mapped->size > UINT32_MAX
+            || !avmpack_check_complete(mapped->mapped, (uint32_t) mapped->size))) {
+        mapped_file_close(mapped);
         return AVM_OPEN_INVALID;
     }

Apply the same “range-check, then narrow once” rule in Emscripten and retain off_t/size_t until after validation in the ESP32 file branch.

---

5. [Medium] The new parser regression executable is never run in CI

Files: tests/CMakeLists.txt:24-38, .github/workflows/build-and-test.yaml:795-876, .github/workflows/build-linux-artifacts.yaml:233-248

test-avmpack is built, but unlike the other native test executables it is not executed by the main workflow, under Valgrind, or by the Linux artifact workflow. A compiled-but-unrun test would not catch assertion failures in CI.

Small fix: mirror the existing native-test workflow entries.

diff --git a/.github/workflows/build-and-test.yaml b/.github/workflows/build-and-test.yaml
@@
+    - name: "Test: test-avmpack with valgrind"
+      if: matrix.library-arch == ''
+      working-directory: build
+      run: valgrind --error-exitcode=1 ./tests/test-avmpack
+
+    - name: "Test: test-avmpack"
+      working-directory: build
+      run: ./tests/test-avmpack

Also add make test-avmpack and ./tests/test-avmpack to the artifact workflow beside the other native tests.

---

6. [Low] read_priv/2 documentation and type spec disagree

File: libs/eavmlib/src/atomvm.erl:161-173

The updated documentation correctly says a genuine miss returns undefined, but the spec still permits only binary().

diff --git a/libs/eavmlib/src/atomvm.erl b/libs/eavmlib/src/atomvm.erl
@@
--spec read_priv(App :: atom(), Path :: list()) -> binary().
+-spec read_priv(App :: atom(), Path :: list()) -> binary() | undefined.

---

7. [Low] STM32 reports free flash using the startup payload size instead of the pack size

File: src/platforms/stm32/src/main.c:336-344

The changed lookup contract returns the section payload size. The diagnostic subtracts startup_beam_size from the entire application region and therefore counts the AVM header, other sections, names, padding, and terminator as free. The newly computed avm_size is the correct value.

diff --git a/src/platforms/stm32/src/main.c b/src/platforms/stm32/src/main.c
@@
-    AVM_LOGD(TAG, "Free application flash space: %lu KiB", ((size - startup_beam_size) / 1024));
+    AVM_LOGD(TAG, "Free application flash space: %lu KiB", ((size - avm_size) / 1024));

Adjacent risk discovered (not introduced by these four commits)

The Oracle also identified a serious pre-existing JIT design issue in globalcontext_find_first_jit_entry(): it derives a flash-cache address by taking the maximum terminator pointer across all registered AVM packs. atomvm:add_avm_pack_binary/2 can register heap-backed packs, and on RP2 heap/SRAM addresses sort above XIP flash addresses. The JIT can therefore round a heap pointer to a sector and pass it to flash operations; comparing pointers from unrelated allocations is itself not a portable ordering operation.

This should be tracked separately if it is intentionally outside this PR. The safe fix is not a cast: AVMPackData needs an explicit “flash-backed/JIT-anchor eligible” capability or the platform must provide the cache anchor directly. Persisted JIT-entry walking is also unbounded against corrupt JITEntry.size values and should be hardened when that subsystem is addressed.

Positive notes

• The central read_section() routine gives lookup and fold operations one bounded validation path instead of duplicating unsafe pointer arithmetic.
• Subtraction-based bounds checks (section_size > avmpack_size - offset) avoid overflow in the primary section-size comparison.
• Name discovery uses bounded memchr() and checks padded-name fit before exposing payload pointers.
• Storing the real pack size in AVMPackData is the correct ownership boundary for later lookups.
• Partition-backed boot paths use avmpack_compute_size() to distinguish partition capacity from the AVM image's real size.
• read_priv/2 now validates both the four-byte prefix and the claimed payload length, and restores in_use on the corruption path.
• The ESP32 configure-time partition-size check prevents silently truncated boot.avm images before flashing.
• Changelog and pack-format documentation were updated in the same series.

Verification performed

• Inspected the complete four-commit diff and all changed AVM parser, NIF, JIT, platform, documentation, and test call sites.
• Audited every current call to avmpack_data_init, avmpack_find_section_by_flag, avmpack_find_section_by_name, avmpack_fold, avmpack_check_complete, and avmpack_compute_size.
• Ran git diff --check HEAD~4..HEAD — passed.
• Built test-avmpack and test-jit_stream_flash from the existing build tree — passed.
• Ran ./build/tests/test-avmpack — passed.
• Ran ./build/tests/test-jit_stream_flash — passed.
• Consulted the Oracle for an independent memory-safety and cross-platform review; its high-confidence findings were reconciled against current source and commit blame before inclusion above.

Platform firmware builds (ESP32, RP2, STM32, and Emscripten) were not run locally.

[petermm]

fresh AMP: your call on these

PR review: AVM pack bounds and truncation handling

Range reviewed: HEAD~6..HEAD (23f62eedc through 2a6c79bf6)
Recommendation: Request changes
Oracle: Consulted as an independent second pass; its supported findings are incorporated below.

Summary

The new bounded section parser is a substantial safety improvement: it uses subtraction-based bounds checks, handles unaligned pack bases, validates section names before string operations, and propagates exact pack sizes through AVMPackData. The new focused tests pass and cover several important malformed-pack cases.

One issue should block merge: the JIT still locates the end marker with a flags-only lookup, but both ordinary data sections and the end marker use flags 0. A valid data section can therefore be mistaken for the pack end, causing the JIT cache to be placed too early or the data section name to be rewritten. Three additional correctness/resource issues should also be addressed.

Findings

[High] JIT end-marker lookup matches ordinary zero-flag data sections

Files: src/libAtomVM/avmpack.c:125-144, src/libAtomVM/jit_stream_flash.c:136-173, src/libAtomVM/jit_stream_flash.c:176-224

avmpack_find_section_by_flag returns the first section for which

(section.flags & flags_mask) == flags_val

before checking whether the parsed section is the terminator. The JIT calls it with END_OF_FILE_MASK == 255 and END_OF_FILE == 0, so the first regular section with flags 0 is indistinguishable from the actual end marker. Zero-flag data sections are valid and are explicitly constructed by the new test at tests/test-avmpack.c:174-175.

If such a section precedes the terminator:

1. globalcontext_find_first_jit_entry uses that section's payload pointer as the pack end and can choose a JIT append sector before later AVM sections.
2. globalcontext_set_cache_valid treats the section name as the validity marker and attempts to replace its first three bytes with "END".

This can reject caching, corrupt a resource name, or erase/write flash occupied by later pack sections.

The flags ambiguity predates this range, but the PR modifies this exact lookup path and claims that scans now stop only at the end marker. The new bounded implementation should not preserve the ambiguity in the JIT safety path.

Suggested fix: expose a terminator-specific lookup instead of encoding “end” as a flags query. For example:

diff --git a/src/libAtomVM/avmpack.h b/src/libAtomVM/avmpack.h
@@
 int avmpack_find_section_by_flag(const void *avmpack_binary, uint32_t avmpack_size,
     uint32_t flags_mask, uint32_t flags_value, const void **ptr, uint32_t *size, const char **name);
+bool avmpack_find_end(
+    const void *avmpack_binary, uint32_t avmpack_size, const void **ptr, const char **name);

diff --git a/src/libAtomVM/avmpack.c b/src/libAtomVM/avmpack.c
@@
+bool avmpack_find_end(
+    const void *avmpack_binary, uint32_t avmpack_size, const void **ptr, const char **name)
+{
+    uint32_t offset = AVMPACK_SIZE;
+    struct AVMPackSection section;
+    enum AVMPackSectionKind kind;
+
+    while ((kind = read_section(avmpack_binary, avmpack_size, offset, &section))
+        != AVMPackSectionInvalid) {
+        if (kind == AVMPackSectionEnd) {
+            *ptr = section.data;
+            *name = section.name;
+            return true;
+        }
+        offset += section.size;
+    }
+    return false;
+}

diff --git a/src/libAtomVM/jit_stream_flash.c b/src/libAtomVM/jit_stream_flash.c
@@
-        if (!avmpack_find_section_by_flag(avmpack_data->data, avmpack_data->size,
-                END_OF_FILE_MASK, END_OF_FILE, &end_offset, &end_size, &end_name)) {
+        if (!avmpack_find_end(
+                avmpack_data->data, avmpack_data->size, &end_offset, &end_name)) {

Apply the JIT replacement at both call sites. Add a regression test with a zero-flag regular section followed by another section and the real terminator; assert that the returned end pointer is immediately after the real terminator and that JIT allocation starts after the complete pack.

[Medium] Invalid ESP32 partition packs leak flash mappings

File: src/platforms/esp32/components/avm_sys/sys.c:348-406,449-465

esp32_sys_mmap_partition creates a DATA mapping and, on Xtensa JIT builds, may also create an IBUS mapping and append it to s_xtensa_part_mappings. The caller then returns directly when avmpack_compute_size rejects the pack or when ESP32PartAVMPack allocation fails. The destructor that unmaps the DATA handle is only reachable after successful metadata allocation.

Truncated packs newly take the validation-failure path, so repeatedly opening one can exhaust mapping resources. Calling only spi_flash_munmap(part_handle) is not a complete fix on Xtensa because the helper may already have registered a second IBUS mapping.

The invalid-header failure path already leaked before this range; truncation validation broadens the set of inputs that take it.

Suggested fix: separate mapping from registration/ownership. Map DATA first, validate the pack, allocate ESP32PartAVMPack, and only then create/register the optional IBUS mapping. Alternatively, add a single cleanup helper that removes the matching s_xtensa_part_mappings entry and unmaps both handles, and invoke it on every failure after esp32_sys_mmap_partition succeeds.

Add failure-path tests for both invalid pack data and metadata allocation failure, asserting that DATA and IBUS mappings and the registry entry are released.

[Medium] Unix and Emscripten loaders narrow file sizes before validation

Files: src/platforms/generic_unix/lib/sys.c:434-449, src/platforms/emscripten/src/lib/sys.c:692-718

The generic Unix mapped-file size is unsigned long and the Emscripten loader reports size_t, but both are cast to uint32_t before validation and storage. On a 64-bit target, a file larger than UINT32_MAX is validated using only its low 32 bits. For example, a sparse file of UINT32_MAX + 41 bytes can become an apparent 40-byte pack after truncation of the size argument.

The binary NIF already handles this correctly by rejecting bin_size > UINT32_MAX before casting.

Suggested fix: follow the same pattern in both loaders:

diff --git a/src/platforms/generic_unix/lib/sys.c b/src/platforms/generic_unix/lib/sys.c
@@
     if (IS_NULL_PTR(mapped)) {
         return AVM_OPEN_CANNOT_OPEN;
     }
-    if (UNLIKELY(!avmpack_is_complete(mapped->mapped, (uint32_t) mapped->size))) {
+    if (UNLIKELY(mapped->size > UINT32_MAX
+        || !avmpack_is_complete(mapped->mapped, (uint32_t) mapped->size))) {
         mapped_file_close(mapped);
         return AVM_OPEN_INVALID;
     }

diff --git a/src/platforms/emscripten/src/lib/sys.c b/src/platforms/emscripten/src/lib/sys.c
@@
-    if (UNLIKELY(!avmpack_is_complete(data, (uint32_t) data_size))) {
+    if (UNLIKELY(data_size > UINT32_MAX
+        || !avmpack_is_complete(data, (uint32_t) data_size))) {

A sparse-file test on generic Unix would cover this without allocating 4 GiB of physical storage.

[Low] JIT compares unrelated pointers with relational operators

File: src/libAtomVM/jit_stream_flash.c:136-168

end_offset > max_end_offset compares pointers that can belong to different mapped pack objects, and initially compares a pack pointer with NULL. Relational pointer comparison is only defined for pointers into the same array object. Embedded compilers are likely to produce the intended address comparison, but the C optimizer is not required to preserve it.

This comparison predates the reviewed commits but remains part of the modified JIT end-boundary path.

Suggested fix: compare integer addresses explicitly:

diff --git a/src/libAtomVM/jit_stream_flash.c b/src/libAtomVM/jit_stream_flash.c
@@
-    const void *max_end_offset = NULL;
+    uintptr_t max_end_addr = 0;
@@
-        if (end_offset > max_end_offset) {
-            max_end_offset = end_offset;
+        uintptr_t end_addr = (uintptr_t) end_offset;
+        if (end_addr > max_end_addr) {
+            max_end_addr = end_addr;
         }
@@
-    if (IS_NULL_PTR(max_end_offset)) {
+    if (max_end_addr == 0) {
@@
-    uintptr_t max_end_offset_page = ((((uintptr_t) max_end_offset) - 1) & ~(FLASH_SECTOR_SIZE - 1));
+    uintptr_t max_end_offset_page = ((max_end_addr - 1) & ~(FLASH_SECTOR_SIZE - 1));

Additional test gaps

• atomvm:read_priv/2: cover a genuine miss (undefined), a payload shorter than the four-byte length prefix (invalid_avm), and a valid resource returning exact contents. The current test covers only an oversized declared length.
• Parser boundary: cover a regular section ending exactly at the supplied bound and malformed section sizes near UINT32_MAX.
• ESP32 mapping ownership: cover cleanup after validation and allocation failures as described above.

Verification performed

• git diff --check HEAD~6..HEAD — passed.
• cmake --build build --target test-avmpack test-jit_stream_flash -j4 — passed.
• build/tests/test-avmpack — passed.
• build/tests/test-jit_stream_flash — passed.

These tests do not currently exercise the blocking zero-flag data-section/JIT interaction.

Pre-existing issue noticed (not introduced by this range)

src/platforms/esp32/components/avm_sys/sys.c:499-503 checks avmpack_data after allocating in_memory_avm. Since avmpack_data is still NULL in that branch, file-backed ESP32 AVM loading always returns AVM_OPEN_FAILED_ALLOC; if the check were ever reached with a non-NULL stale value, a failed allocation would instead be dereferenced. This line predates the reviewed commits, so it is not part of the merge recommendation, but the adjacent loader changes cannot be effective on that path until it is corrected.

diff --git a/src/platforms/esp32/components/avm_sys/sys.c b/src/platforms/esp32/components/avm_sys/sys.c
@@
     struct InMemoryAVMPack *in_memory_avm = malloc(sizeof(struct InMemoryAVMPack));
-    if (IS_NULL_PTR(avmpack_data)) {
+    if (IS_NULL_PTR(in_memory_avm)) {
         free(file_data);
         return AVM_OPEN_FAILED_ALLOC;
     }