Repository navigation
Fix read_priv crash on a lookup miss and reject truncated AVM packs - #2340
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
22d6864 to
2a6c79b
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
atomvm:read_priv and detect truncated packs|
Going to push the updated PR, that address the previous review comments.
|
|
lgtm, AMP found one issue, I made it do test and fix: Review: AtomVM PR #2340
Findings, line numbers, and proposed diffs refer to the reviewed head above. P1 — ESP32/LX6 rejection releases DBUS while retaining a stale executable translationLocation: The new invalid-pack and allocation-failure paths call
Impact: On classic ESP32/LX6, DBUS and IBUS use separate allocation regions, so retaining A's IBUS mapping does not keep its DBUS window occupied. If that DBUS window is reused, native execution can jump into the wrong partition and crash or execute incorrect code. Independently of address reuse, rejection retains the IBUS mapping and translation record. Do not generalize this allocation explanation to every Xtensa chip: ESP32-S3 has a combined executable/readable region, and SDK configuration matters. Evidence boundary: The mapping registration, incomplete cleanup, and oldest-first lookup are established by source inspection. Actual MMU-window reuse and wrong-code execution were not reproduced on ESP32 hardware. The failure scenario is conditional on that reuse. The successful-close destructor has a related pre-existing lifecycle issue; the new rejection cleanup introduces another path that releases DBUS while retaining the translation. Suggested fixAssociate each ordinary translation with the exact partition mmap wrapper handle that registered it. Forget that wrapper's translation before releasing DBUS, under Preserve IBUS mappings for VM lifetime. In the inspected ESP-IDF implementations, enclosed duplicate mappings can return a borrowed virtual address without acquiring an independent MMU reference. Releasing the original IBUS mapping could invalidate another wrapper's borrowed address, including the JIT cache's permanent globals. The proposed fix therefore removes stale translations without adding IBUS unmaps; it is not a complete mapping-lifetime repair. Relevant ESP-IDF v5.5.5 source:
The two patches below together contain the complete local fix and regression test Implementation patchThis includes the declaration, handle association, helper implementation, --- a/src/platforms/esp32/components/avm_sys/include/esp32_sys.h
+++ b/src/platforms/esp32/components/avm_sys/include/esp32_sys.h
@@ -101,4 +101,6 @@
const void *esp32_sys_mmap_partition(
const char *partition_name, spi_flash_mmap_handle_t *handle, int *size);
+void esp32_sys_munmap_partition(spi_flash_mmap_handle_t handle);
+
#endif
--- a/src/platforms/esp32/components/avm_sys/sys.c
+++ b/src/platforms/esp32/components/avm_sys/sys.c
@@ -342,6 +342,9 @@
uintptr_t ibus_base;
size_t size;
spi_flash_mmap_handle_t ibus_handle;
+ // Associates this translation with a wrapper, not an independent MMU reference.
+ bool has_partition_handle;
+ spi_flash_mmap_handle_t partition_handle;
};
static struct xtensa_part_mapping *s_xtensa_part_mappings = NULL;
static int s_xtensa_part_count = 0;
@@ -398,6 +401,8 @@
s_xtensa_part_mappings[s_xtensa_part_count].ibus_base = (uintptr_t) ibus_ptr;
s_xtensa_part_mappings[s_xtensa_part_count].size = partition->size;
s_xtensa_part_mappings[s_xtensa_part_count].ibus_handle = ibus_handle;
+ s_xtensa_part_mappings[s_xtensa_part_count].has_partition_handle = true;
+ s_xtensa_part_mappings[s_xtensa_part_count].partition_handle = *handle;
s_xtensa_part_count++;
pthread_mutex_unlock(&s_xtensa_part_mutex);
}
@@ -411,6 +416,29 @@
return mapped_memory;
}
+void esp32_sys_munmap_partition(spi_flash_mmap_handle_t handle)
+{
+#if !defined(CONFIG_IDF_TARGET_ARCH_RISCV) && !defined(AVM_NO_JIT)
+ pthread_mutex_lock(&s_xtensa_part_mutex);
+ for (int i = 0; i < s_xtensa_part_count; i++) {
+ struct xtensa_part_mapping *mapping = &s_xtensa_part_mappings[i];
+ if (mapping->has_partition_handle && mapping->partition_handle == handle) {
+ // Forget this wrapper's translation before its DBUS address can be reused.
+ // Keep IBUS handles alive as before: another mmap wrapper (including the
+ // JIT cache) may borrow the same MMU block without retaining a reference.
+ memmove(mapping, mapping + 1,
+ (s_xtensa_part_count - i - 1) * sizeof(*mapping));
+ s_xtensa_part_count--;
+ break;
+ }
+ }
+ spi_flash_munmap(handle);
+ pthread_mutex_unlock(&s_xtensa_part_mutex);
+#else
+ spi_flash_munmap(handle);
+#endif
+}
+
struct ESP32PartAVMPack
{
struct AVMPackData base;
@@ -428,7 +456,7 @@
UNUSED(global);
struct ESP32PartAVMPack *part_avm = CONTAINER_OF(obj, struct ESP32PartAVMPack, base);
- spi_flash_munmap(part_avm->part_handle);
+ esp32_sys_munmap_partition(part_avm->part_handle);
free(obj);
}
@@ -457,13 +485,13 @@
}
uint32_t avm_size;
if (UNLIKELY(!avmpack_compute_size(part_data, size, &avm_size))) {
- spi_flash_munmap(part_handle);
+ esp32_sys_munmap_partition(part_handle);
return AVM_OPEN_INVALID;
}
struct ESP32PartAVMPack *part_avm = malloc(sizeof(struct ESP32PartAVMPack));
if (IS_NULL_PTR(part_avm)) {
- spi_flash_munmap(part_handle);
+ esp32_sys_munmap_partition(part_handle);
return AVM_OPEN_FAILED_ALLOC;
}
avmpack_data_init(&part_avm->base, &esp32_part_avm_pack_info, avm_size);
@@ -994,6 +1022,8 @@
s_xtensa_part_mappings[s_xtensa_part_count].ibus_base = (uintptr_t) ibus_ptr;
s_xtensa_part_mappings[s_xtensa_part_count].size = map_size;
s_xtensa_part_mappings[s_xtensa_part_count].ibus_handle = ibus_handle;
+ s_xtensa_part_mappings[s_xtensa_part_count].has_partition_handle = false;
+ s_xtensa_part_mappings[s_xtensa_part_count].partition_handle = 0;
s_xtensa_part_count++;
pthread_mutex_unlock(&s_xtensa_part_mutex);
addr = (uintptr_t) ibus_ptr + offset_in_page;Regression-test patchThe existing Unity runner discovers this test automatically. It uses the scratch diff --git a/src/platforms/esp32/test/main/test_main.c b/src/platforms/esp32/test/main/test_main.c
--- a/src/platforms/esp32/test/main/test_main.c
+++ b/src/platforms/esp32/test/main/test_main.c
@@ -20,6 +20,7 @@
#include "unity.h"
#include <errno.h>
+#include <inttypes.h>
#include <stdio.h>
#include <string.h>
@@ -200,6 +201,62 @@
}
#ifndef AVM_NO_JIT
+#if CONFIG_IDF_TARGET_ESP32
+TEST_CASE("test_partition_native_mapping_reuse", "[test_run]")
+{
+ // These are scratch partitions in partitions-test.csv, not the JIT cache.
+ const esp_partition_t *rejected = esp_partition_find_first(
+ ESP_PARTITION_TYPE_DATA, ESP_PARTITION_SUBTYPE_ANY, "data");
+ const esp_partition_t *valid = esp_partition_find_first(
+ ESP_PARTITION_TYPE_DATA, ESP_PARTITION_SUBTYPE_ANY, "lib.avm");
+ TEST_ASSERT_NOT_NULL(rejected);
+ TEST_ASSERT_NOT_NULL(valid);
+ TEST_ESP_OK(esp_partition_erase_range(rejected, 0, SPI_FLASH_SEC_SIZE));
+ TEST_ESP_OK(esp_partition_erase_range(valid, 0, SPI_FLASH_SEC_SIZE));
+ static const uint8_t empty_pack[] = {
+ '#', '!', '/', 'u', 's', 'r', '/', 'b', 'i', 'n', '/', 'e', 'n', 'v', ' ', 'A',
+ 't', 'o', 'm', 'V', 'M', '\n', 0, 0,
+ 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 'e', 'n', 'd', 0
+ };
+ TEST_ESP_OK(esp_partition_write(valid, 0, empty_pack, sizeof(empty_pack)));
+
+ // Observe A's DBUS address without registering an AtomVM translation.
+ const void *rejected_dbus;
+ spi_flash_mmap_handle_t probe_handle;
+ TEST_ESP_OK(esp_partition_mmap(rejected, 0, rejected->size,
+ SPI_FLASH_MMAP_DATA, &rejected_dbus, &probe_handle));
+ spi_flash_munmap(probe_handle);
+
+ struct AVMPackData *pack = NULL;
+ TEST_ASSERT_EQUAL(AVM_OPEN_INVALID,
+ sys_open_avm_from_file(NULL, "/dev/partition/by-name/data", &pack));
+ TEST_ASSERT_EQUAL(AVM_OPEN_OK,
+ sys_open_avm_from_file(NULL, "/dev/partition/by-name/lib.avm", &pack));
+ TEST_ASSERT_NOT_NULL(pack);
+ bool reused = pack->data == rejected_dbus;
+ ModuleNativeEntryPoint entry = sys_map_native_code(pack->data, 4);
+ size_t physical = spi_flash_cache2phys((const void *) entry);
+ printf("Rejected partition: DBUS=%p; replacement: DBUS=%p native=%p physical=0x%x expected=0x%" PRIx32 "\n",
+ rejected_dbus, pack->data, (void *) entry, (unsigned) physical, valid->address);
+ avmpack_data_destroy(pack, NULL);
+ TEST_ASSERT_TRUE_MESSAGE(reused, "Test must exercise actual DBUS window reuse");
+ TEST_ASSERT_EQUAL_HEX32(valid->address, physical);
+
+ // Closing a valid pack must also forget its translation before DBUS reuse.
+ TEST_ESP_OK(esp_partition_write(rejected, 0, empty_pack, sizeof(empty_pack)));
+ TEST_ASSERT_EQUAL(AVM_OPEN_OK,
+ sys_open_avm_from_file(NULL, "/dev/partition/by-name/data", &pack));
+ reused = pack->data == rejected_dbus;
+ entry = sys_map_native_code(pack->data, 4);
+ physical = spi_flash_cache2phys((const void *) entry);
+ printf("After close: DBUS=%p native=%p physical=0x%x expected=0x%" PRIx32 "\n",
+ pack->data, (void *) entry, (unsigned) physical, rejected->address);
+ avmpack_data_destroy(pack, NULL);
+ TEST_ASSERT_TRUE_MESSAGE(reused, "Test must exercise DBUS reuse after close");
+ TEST_ASSERT_EQUAL_HEX32(rejected->address, physical);
+}
+#endif
+
TEST_CASE("test_jit_compile", "[test_run]")
{
prepare_event_queue();Test configuration: Build Scope and limitations:
Regression needed: Reject A, force B to reuse A's DBUS window, and verify that B's native entry point resolves to B's physical flash. Cover both validation failure and wrapper allocation failure; two registrations sharing a DBUS base but having different handles; ownerless fallback records; record-order preservation; and JIT-disabled/RISC-V controls. No new IBUS unmaps should occur. A host stub can check bookkeeping but cannot establish actual MMU reuse, SDK borrowed-address lifetimes, or native execution correctness. Non-blocking — Describe tail-only validation accurately
This preserves lazy paging and is not another memory-safety blocker: bounded scanners stop at corrupt headers, while region loaders and JIT terminator discovery walk the chain. However, the PR description's claim that every pack is validated by walking the chain at load does not match exact-size loaders. The header's unconditional promise to reject trailing bytes is also stronger than the implementation. A local probe using the public parser API produced: The second case is a valid 40-byte empty pack followed by another 16-byte terminator. The tail predicate accepts all 56 bytes, although the chain ends at byte 40. The third case has an oversized first section and an unreachable valid tail marker. Preserve the chosen lazy-loading behavior, but narrow the documentation and PR description. For example: --- a/src/libAtomVM/avmpack.h
+++ b/src/libAtomVM/avmpack.h
@@
- * known (a file size, an in-memory binary). Trailing bytes after the terminator and a size above
- * \c UINT32_MAX, which 32-bit section sizes cannot describe, are rejected.
+ * known (a file size, an in-memory binary). A missing final marker or a size above
+ * \c UINT32_MAX is rejected. This tail-only check does not prove that the final marker is
+ * reachable, or that no earlier terminator precedes trailing data.Suggested PR-description wording:
Other safety checks in this push
Verification performedRun against the reviewed head in an isolated worktree: cmake -S . -B /tmp/pr2340-build
cmake --build /tmp/pr2340-build --target test-avmpack test-jit_stream_flash -j4
/tmp/pr2340-build/tests/test-avmpack
/tmp/pr2340-build/tests/test-jit_stream_flash
git diff --check d4d4db553d41f5a83ae549b57e2cf26611eb80f6..HEAD
Additional useful coverage, not additional blockers: DeliveryAt the time of the review above, the diffs were suggestions only. No GitHub review comments have been posted. Local Wokwi follow-up — 2026-10-25The proposed ESP32 translation cleanup is now applied locally, with a regression Built the full ESP32 test firmware using ESP-IDF v5.5.5 with
Evidence and the local rerun script are in Evidence boundary: Actual DBUS reuse and incorrect/correct physical native |
|
I realized that while addressing the issues I was finding the PR scope was expanding. I pushed again, less commits and smaller scope. There will be a follow up PR after this. |
petermm
left a comment
There was a problem hiding this comment.
Makes sense, AMP only had this extreme nitpick - do ship!
Small non-blocking documentation nit: the tail-only check cannot rule out an earlier terminator followed by trailing data ending in another valid marker. No behavior change needed:
--- a/src/libAtomVM/avmpack.h
+++ b/src/libAtomVM/avmpack.h
@@
* @details Reads the header and the last 16 bytes only, so a memory-mapped pack stays lazily
- * paged. Trailing bytes after the terminator and sizes above \c UINT32_MAX are rejected.
+ * paged. A missing final marker or a size above \c UINT32_MAX is rejected. This does not
+ * validate the section chain or rule out an earlier terminator followed by trailing data.The packbeam scanners were unbounded and trusted every section size, stopping only on a header with flags 0. When the end marker was not in the mapped region (an image truncated or overflowed by its partition, or a corrupt header) a lookup miss, e.g. read_priv for a file that is not packed, walked into erased flash, took 0xFFFFFFFF for a section size and crashed. Keep the pack size in AVMPackData, bound every scan with it and validate each header, so a miss on a damaged pack ends the walk. The end marker is matched by its zero size and "end" name in any case, as the JIT flash cache rewrites it to "END" in place. On rp2 the bound is the 16 MiB XIP window rather than XIP_SRAM_BASE, which spans past the flash. A pack whose marker is not found no longer provides the JIT cache with an append position. The scanners also reported the whole section size as the payload size, so code:get_object_code/1 returned the module followed by the next header. They now return the payload size. Signed-off-by: Davide Bettio <davide@uninstall.it>
Only the 24-byte magic header was checked at load, so a pack larger than the region holding it, e.g. an image flashed to a too-small partition, loaded and failed late: every lookup past the truncation point missed, so modules failed to load and read_priv returned undefined, and the JIT flash cache, finding no end marker, refused to append native code. Reject such packs when they are loaded. When the exact size is known (a file, a binary, the escript pack) the pack must end with the end marker; only the tail is read, so a memory-mapped pack stays lazily paged. When only an upper bound is known (partitions, rp2 and stm32 flash) the section chain is walked to the marker, which also gives the scanners the real size. Trailing data and sizes above 32 bits are rejected. Signed-off-by: Davide Bettio <davide@uninstall.it>
A priv entry is <<Size:32, Data>> and read_priv trusted Size, so a corrupt prefix produced a binary extending past the entry. Check it against the section payload and raise invalid_avm when it does not fit; a resource that is not in any pack still returns undefined. The prefix is read unaligned: a pack kept in place from a const binary, such as a module literal, may sit at any byte offset. Signed-off-by: Davide Bettio <davide@uninstall.it>
The packbeam (.avm) scanners were unbounded and trusted every section
size. They stopped only on a header with flags 0, so when the end marker
was not inside the mapped region (an image truncated or overflowed by
its partition, or a corrupt header) a lookup miss, e.g.
atomvm:read_priv/2 for a file that is not packed, walked into erased
flash, took 0xFFFFFFFF for a section size and crashed the VM
(LoadProhibited on ESP32). The scanners also reported the whole section
size as the payload size, so code:get_object_code/1 returned the module
followed by the next section's header and name, and read_priv trusted
the length prefix of a priv entry, so a corrupt entry produced a binary
extending past the pack.
Bound every scan with the pack size, validate every section header,
return the payload size, and reject truncated or incomplete packs when
they are loaded. No on-disk format change: packs produced by
atomvm_packbeam load unchanged.
Highlights:
header in find_by_name/_by_flag/fold; a miss on a damaged pack ends
the walk instead of leaving it
exactly the module bytes
in-memory binaries, the embedded escript pack) must end with the
terminator, checked from its tail so memory-mapped packs stay lazily
paged; a pack in a region (partitions, rp2/stm32 flash) is walked to
its terminator to find its real size; sizes above 32 bits and data
after the terminator are rejected
on a corrupt entry; a missing resource still returns undefined
test-jit_stream_flash
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