From c36bd99e1e1feadf6ae8949fd3ee0a15c498b54d Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 5 Sep 2026 14:07:11 +0000 Subject: [PATCH] Fix EXECUTE/?/DUMP/TYPE/DECIMAL-HEX-OCTAL/ALIGN defects from proof sweep MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit proof/FINDINGS.md's Isabelle/HOL word-source sweep (§4) flagged five real defects; this fixes all five and records resolution in that doc: - EXECUTE (system_words.c): cast a popped cell straight to a DictEntry* and called through it with only a null check. Now validates via a new shared vm_dict_entry_ok(), promoted out of starforth_words.c's ENTROPY@/ENTROPY! guard (dictionary_management.c) so EXECUTE gets the same live-entry check. - ? and DUMP (format_words.c): dereferenced the popped cell as a raw host pointer, bypassing vm_addr_ok entirely (out-of-VM-bounds read). Both now go through VM_ADDR/vm_addr_ok/vm_load_cell/vm_ptr like every other memory word (@, `,`, editor_words.c). - TYPE (io_words.c): bounds check computed addr+count in signed 64-bit arithmetic, which can overflow and bypass the check on large operands. Replaced with vm_addr_ok(), which is written to avoid that overflow. - DECIMAL/HEX/OCTAL (format_words.c): wrote only the BASE memory cell, never vm->base, the host-mirror field number-output words actually read via current_base() -- so these words silently affected number parsing but never printing. Now call the existing vm_set_base() (previously only used at boot init), which updates both. vm_get_base/vm_set_base promoted to public declarations in include/vm.h. - ALIGN vs ALLOT/,/C,/2, (dictionary_words.c): disagreed on dictionary growth ceiling (2MB vs 5MB). Investigated which was correct rather than blindly widening: vm_get_block_addr() maps block N to vm->memory + N*BLOCK_SIZE across the full 5MB arena, and USER_BLOCKS_START (block 2048) lines up exactly with DICTIONARY_MEMORY_SIZE -- so ALLOT/,/C,/2, letting `here` grow past 2MB could silently corrupt live block/user data sharing that memory. Tightened ALLOT/,/C,/2, to DICTIONARY_MEMORY_SIZE to match ALIGN. Verified: hosted (amd64) and kernel (amd64, __STARKERNEL__) both build clean with -Wall -Werror; hosted POST suite 1012/1012 passing (0 regressions); manually exercised EXECUTE, ?/DUMP, TYPE, HEX/DECIMAL/OCTAL, and large-ALLOT rejection in the REPL. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_014Qf6YcnHgaEtEygq3knx19 --- include/vm.h | 13 +++++++++++++ proof/FINDINGS.md | 21 +++++++++++++++++++++ src/dictionary_management.c | 17 +++++++++++++++++ src/memory_management.c | 2 +- src/word_source/dictionary_words.c | 12 ++++++++---- src/word_source/format_words.c | 25 ++++++++++++------------- src/word_source/io_words.c | 16 ++++++++++------ src/word_source/starforth_words.c | 25 ++----------------------- src/word_source/system_words.c | 2 +- 9 files changed, 85 insertions(+), 48 deletions(-) diff --git a/include/vm.h b/include/vm.h index e9f0451..f9239c6 100644 --- a/include/vm.h +++ b/include/vm.h @@ -138,6 +138,13 @@ void vm_store_u8(struct VM* vm, vaddr_t addr, uint8_t v); cell_t vm_load_cell(struct VM* vm, vaddr_t addr); /* requires alignment */ void vm_store_cell(struct VM* vm, vaddr_t addr, cell_t v); +/* Numeric BASE accessors: keep the FORTH-visible BASE cell (vm->base_addr) + * and the host-mirror field (vm->base, read by number-output words via + * current_base()) in sync. Always use these instead of writing base_addr + * directly. */ +unsigned vm_get_base(const struct VM* vm); +void vm_set_base(struct VM* vm, unsigned b); + /* Explicit stack<->offset conversions (keep intent obvious) */ static inline vaddr_t VM_ADDR(cell_t c) { return (vaddr_t)(uint64_t)c; } static inline cell_t CELL(vaddr_t a) { return (cell_t)(int64_t)a; } @@ -638,6 +645,12 @@ DictEntry* vm_dictionary_find_latest_by_func(VM* vm, word_func_t func); DictEntry* vm_dictionary_lookup_by_word_id(VM* vm, uint32_t word_id); +/* Validate that `candidate` is a live, currently-registered dictionary entry + * for this VM (walks vm->latest under dict_lock; never dereferences an + * unverified pointer). Use before executing/inspecting through any xt/addr + * popped off the data stack (EXECUTE, ENTROPY@/!, etc). */ +int vm_dict_entry_ok(VM* vm, DictEntry* candidate); + void vm_dictionary_track_entry(VM* vm, DictEntry* entry); void vm_dictionary_untrack_entry(VM* vm, DictEntry* entry); diff --git a/proof/FINDINGS.md b/proof/FINDINGS.md index e5f38c3..12c958b 100644 --- a/proof/FINDINGS.md +++ b/proof/FINDINGS.md @@ -147,22 +147,43 @@ kernel word-source) — see this document's closing status line for result. since `EXECUTE` is a core, ubiquitous primitive rather than a diagnostic word. **Flagged as the highest-severity single-word finding in the sweep.** + **RESOLVED 2026-09-05:** now validates via the (newly shared) + `vm_dict_entry_ok()` — the same dictionary-walk check `ENTROPY@`/`ENTROPY!` + already used, promoted out of `starforth_words.c` into + `dictionary_management.c` so `EXECUTE` can call it too. - **`?` and `DUMP`** (`format_words.c`) cast the popped cell straight to a host pointer and dereference it, bypassing `vm_addr_ok` — an out-of-VM-bounds read. + **RESOLVED 2026-09-05:** both now go through `VM_ADDR`/`vm_addr_ok`/ + `vm_load_cell`/`vm_ptr`, matching `@`/`,`/editor_words.c`'s pattern. - **`TYPE`** (`io_words.c`) has a signed-overflow bypass in its bounds check (machine-checked witness in the proof). + **RESOLVED 2026-09-05:** replaced the manual `addr + count > VM_MEMORY_SIZE` + sum with `vm_addr_ok(vm, addr, count)`, which is written to avoid exactly + this overflow. - **`DECIMAL`/`HEX`/`OCTAL`** (`format_words.c`) write only the memory cell at `base_addr`, never `vm->base` (the separate host-mirror field number- *output* words actually read via `current_base()`) — proved as `decimal_does_not_change_vm_base` et al. Net effect: these words silently affect number *parsing* but never number *printing*. + **RESOLVED 2026-09-05:** all three now call the existing `vm_set_base()` + (previously only used at boot init), which updates both `base_addr` and + `vm->base`; `vm_get_base()`/`vm_set_base()` promoted to public + declarations in `include/vm.h` so word-source files can reach them. - **`LATEST`** (`dictionary_words.c`) has a body identical to `HERE` (both push `vm->here`) — does not consult `vm->latest` despite its doc comment claiming otherwise. - **`ALIGN`** bounds-checks `here` against `DICTIONARY_MEMORY_SIZE` (2MB) while `ALLOT`/`,`/`C,`/`2,` check against `VM_MEMORY_SIZE` (5MB) instead — two different ceilings for the same pointer. + **RESOLVED 2026-09-05:** the two ceilings disagreeing was real, but + `ALIGN`'s 2MB was the correct one, not `ALLOT`'s 5MB — `vm_get_block_addr()` + maps block N directly to `vm->memory + N*BLOCK_SIZE` for the *entire* 5MB + arena, and `USER_BLOCKS_START` (block 2048) lines up exactly with + `DICTIONARY_MEMORY_SIZE`, so letting dictionary growth run past 2MB (as + `ALLOT`/`,`/`C,`/`2,` previously allowed) would silently corrupt live + block/user data sharing that same memory. Tightened `ALLOT`/`,`/`C,`/`2,` + to `DICTIONARY_MEMORY_SIZE` to match `ALIGN`, not the other way around. - **`INFER-*` (`array_ptr`, `inference_words.c`)** sets `vm->error` *and* still pushes a placeholder value anyway — violates the "error or push, never both" shape essentially every other word in the sweep follows. diff --git a/src/dictionary_management.c b/src/dictionary_management.c index 1e0a3d5..0889377 100644 --- a/src/dictionary_management.c +++ b/src/dictionary_management.c @@ -157,6 +157,23 @@ DictEntry *vm_dictionary_lookup_by_word_id(VM *vm, uint32_t word_id) { return vm->word_id_map[word_id]; } +int vm_dict_entry_ok(VM *vm, DictEntry *candidate) { + if (!vm || !candidate) { + return 0; + } + + int found = 0; + sf_mutex_lock(&vm->dict_lock); + for (DictEntry *e = vm->latest; e != NULL; e = e->link) { + if (e == candidate) { + found = 1; + break; + } + } + sf_mutex_unlock(&vm->dict_lock); + return found; +} + /* --- helpers ------------------------------------------------------------- */ static inline void *sf_xrealloc(void *p, size_t nbytes) { diff --git a/src/memory_management.c b/src/memory_management.c index 47917ac..0fb28fd 100644 --- a/src/memory_management.c +++ b/src/memory_management.c @@ -56,7 +56,7 @@ void *vm_allot(VM *vm, size_t bytes) { return NULL; } - /* Ensure we don't allocate beyond dictionary space (first 1024 blocks = 1MB) */ + /* Ensure we don't allocate beyond dictionary space (first DICTIONARY_BLOCKS blocks = DICTIONARY_MEMORY_SIZE bytes) */ log_message(LOG_DEBUG, "vm_allot: Requesting %zu bytes, current here=%zu, limit=%d", bytes, vm->here, DICTIONARY_MEMORY_SIZE); diff --git a/src/word_source/dictionary_words.c b/src/word_source/dictionary_words.c index 5642327..a97c6d7 100644 --- a/src/word_source/dictionary_words.c +++ b/src/word_source/dictionary_words.c @@ -71,7 +71,11 @@ void dictionary_word_allot(VM *vm) { } cell_t n = vm_pop(vm); cell_t new_here = vm->here + n; - if (new_here < 0 || new_here > (cell_t) VM_MEMORY_SIZE) { + /* Dictionary growth is bounded to DICTIONARY_MEMORY_SIZE, not the full + * VM_MEMORY_SIZE: blocks >= USER_BLOCKS_START live in the same flat + * vm->memory array (see vm_get_block_addr), so letting `here` grow past + * the dictionary region would silently corrupt live block/user data. */ + if (new_here < 0 || new_here > (cell_t) DICTIONARY_MEMORY_SIZE) { vm->error = 1; return; } @@ -90,7 +94,7 @@ void dictionary_word_comma(VM *vm) { } cell_t n = vm_pop(vm); vm_align(vm); - if (vm->here + (cell_t) sizeof(cell_t) > VM_MEMORY_SIZE) { + if (vm->here + (cell_t) sizeof(cell_t) > DICTIONARY_MEMORY_SIZE) { vm->error = 1; return; } @@ -114,7 +118,7 @@ void dictionary_word_c_comma(VM *vm) { return; } cell_t c = vm_pop(vm); - if (vm->here + 1 > VM_MEMORY_SIZE) { + if (vm->here + 1 > DICTIONARY_MEMORY_SIZE) { vm->error = 1; return; } @@ -140,7 +144,7 @@ void dictionary_word_2comma(VM *vm) { cell_t high = vm_pop(vm); cell_t low = vm_pop(vm); vm_align(vm); - if (vm->here + (cell_t)(2 * sizeof(cell_t)) > VM_MEMORY_SIZE) { + if (vm->here + (cell_t)(2 * sizeof(cell_t)) > DICTIONARY_MEMORY_SIZE) { vm->error = 1; return; } diff --git a/src/word_source/format_words.c b/src/word_source/format_words.c index 72c19e5..a0271b8 100644 --- a/src/word_source/format_words.c +++ b/src/word_source/format_words.c @@ -157,19 +157,19 @@ void format_word_base(VM *vm) { /* DECIMAL ( -- ) — set BASE=10 */ void format_word_decimal(VM *vm) { if (!vm) { return; } - vm_store_cell(vm, vm->base_addr, (cell_t) 10); + vm_set_base(vm, 10); } /* HEX ( -- ) — set BASE=16 */ void format_word_hex(VM *vm) { if (!vm) { return; } - vm_store_cell(vm, vm->base_addr, (cell_t) 16); + vm_set_base(vm, 16); } /* OCTAL ( -- ) — set BASE=8 */ void format_word_octal(VM *vm) { if (!vm) { return; } - vm_store_cell(vm, vm->base_addr, (cell_t) 8); + vm_set_base(vm, 8); } /* <# ( -- ) */ @@ -399,13 +399,12 @@ void format_word_question(VM *vm) { vm->error = 1; return; } - cell_t addr = vm_pop(vm); - cell_t *ptr = (cell_t *) (uintptr_t) addr; - if (!ptr) { + vaddr_t addr = VM_ADDR(vm_pop(vm)); + if (!vm_addr_ok(vm, addr, sizeof(cell_t))) { vm->error = 1; return; } - print_number_formatted(vm, *ptr, 0, 0); + print_number_formatted(vm, vm_load_cell(vm, addr), 0, 0); putchar(' '); } @@ -416,21 +415,21 @@ void format_word_dump(VM *vm) { return; } cell_t u = vm_pop(vm); - cell_t addr = vm_pop(vm); + vaddr_t addr = VM_ADDR(vm_pop(vm)); if (u < 0) { vm->error = 1; return; } - uint8_t *ptr = (uint8_t *) (uintptr_t) addr; - if (!ptr) { + size_t n = (size_t) u; + if (!vm_addr_ok(vm, addr, n)) { vm->error = 1; return; } - size_t n = (size_t) u; - int addr_width = (int) (sizeof(uintptr_t) * 2); + const uint8_t *ptr = vm_ptr(vm, addr); + int addr_width = (int) (sizeof(vaddr_t) * 2); for (size_t i = 0; i < n; i += 16) { - unsigned long long a = (unsigned long long) ((uintptr_t) addr + (uintptr_t) i); + unsigned long long a = (unsigned long long) (addr + i); printf("%0*llX: ", addr_width, a); size_t j = 0; for (; j < 16 && i + j < n; j++) printf("%02X ", ptr[i + j]); diff --git a/src/word_source/io_words.c b/src/word_source/io_words.c index e8ad212..1fc1bb4 100644 --- a/src/word_source/io_words.c +++ b/src/word_source/io_words.c @@ -122,22 +122,26 @@ static void io_word_type(VM *vm) { } cell_t count = vm_pop(vm); - cell_t addr = vm_pop(vm); + vaddr_t addr = VM_ADDR(vm_pop(vm)); - // Add bounds checking - if (addr < 0 || count < 0 || (addr + count) > VM_MEMORY_SIZE) { - log_message(LOG_ERROR, "TYPE: Invalid range [%ld, %ld)", (long) addr, (long) (addr + count)); + // Bounds check via vm_addr_ok (avoids the addr+count overflow a manual + // sum would risk for large signed operands) + if (count < 0 || !vm_addr_ok(vm, addr, (size_t) count)) { + log_message(LOG_ERROR, "TYPE: Invalid range [addr=%llu, count=%ld)", + (unsigned long long) addr, (long) count); vm->error = 1; return; } // Output characters from VM memory + const uint8_t *src = vm_ptr(vm, addr); for (cell_t i = 0; i < count; i++) { - putchar(vm->memory[addr + i]); + putchar(src[i]); } fflush(stdout); - log_message(LOG_DEBUG, "TYPE: Output %ld characters from address %ld", (long) count, (long) addr); + log_message(LOG_DEBUG, "TYPE: Output %ld characters from address %llu", + (long) count, (unsigned long long) addr); } /** diff --git a/src/word_source/starforth_words.c b/src/word_source/starforth_words.c index b3576af..2486045 100644 --- a/src/word_source/starforth_words.c +++ b/src/word_source/starforth_words.c @@ -78,27 +78,6 @@ static uint64_t g_prng_state = 1; * Note: Exposed as ENTROPY@ for FORTH compatibility, but measures execution heat. * @param vm Pointer to the VM instance */ -/** - * @brief Validate that an address is a valid DictEntry pointer - * @param vm The VM instance - * @param candidate The address to validate - * @return 1 if valid, 0 if not - */ -static int is_valid_dict_entry(VM* vm, DictEntry* candidate) -{ - if (!candidate) return 0; - - /* Walk dictionary to verify this is a real entry - must hold dict_lock */ - int found = 0; - sf_mutex_lock(&vm->dict_lock); - for (DictEntry* e = vm->latest; e != NULL; e = e->link) - { - if (e == candidate) { found = 1; break; } - } - sf_mutex_unlock(&vm->dict_lock); - return found; -} - void starforth_word_execution_heat_fetch(VM* vm) { if (vm->dsp < 0) @@ -119,7 +98,7 @@ void starforth_word_execution_heat_fetch(VM* vm) } /* Guardrail: Validate the pointer is actually a DictEntry */ - if (!is_valid_dict_entry(vm, entry)) + if (!vm_dict_entry_ok(vm, entry)) { vm->error = 1; log_message(LOG_ERROR, "ENTROPY@: invalid dictionary entry address %p (not in dictionary)", (void*)entry); @@ -158,7 +137,7 @@ void starforth_word_execution_heat_store(VM* vm) } /* Guardrail: Validate the pointer is actually a DictEntry */ - if (!is_valid_dict_entry(vm, entry)) + if (!vm_dict_entry_ok(vm, entry)) { vm->error = 1; log_message(LOG_ERROR, "ENTROPY!: invalid dictionary entry address %p (not in dictionary)", (void*)entry); diff --git a/src/word_source/system_words.c b/src/word_source/system_words.c index 7ddbd08..7b2aae7 100644 --- a/src/word_source/system_words.c +++ b/src/word_source/system_words.c @@ -569,7 +569,7 @@ static void system_word_execute(VM *vm) { if (vm->dsp < 0) { vm->error = 1; return; } cell_t xt = vm_pop(vm); DictEntry *entry = (DictEntry *)(uintptr_t)xt; - if (!entry || !entry->func) { vm->error = 1; return; } + if (!vm_dict_entry_ok(vm, entry) || !entry->func) { vm->error = 1; return; } vm->current_executing_entry = entry; entry->func(vm); }