Enhance: 247-opcode ISA, execution tracing, bounds checking, error messages#3
Enhance: 247-opcode ISA, execution tracing, bounds checking, error messages#3SuperInstance wants to merge 1 commit intomainfrom
Conversation
…nds checking, improved errors - Extended ISA from ~80 to 247 opcodes - Execution tracing with configurable callback - Memory bounds checking on all bytecode reads - Register bounds checking on all GP/FP accesses - Stack overflow/underflow protection - Human-readable error detail strings - Opcode info table for debugging/disassembly - Debug build target in Makefile - All 39 existing tests pass
![beta-devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/2443040?s=60&v=4){kind=small}
| case FLUX_STORE8: { rd=f8(v); if(!v->running) break; rs1=f8(v); if(!v->running) break; CHECK_GPR(rs1); h=flux_mem_get(&v->mem,"heap"); if(h) flux_mem_write_u8(h,(size_t)GPR[rd],(uint8_t)GPR[rs1]); } break; | ||
| case FLUX_LOAD16: { rd=f8(v); if(!v->running) break; rs1=f8(v); if(!v->running) break; CHECK_GPR(rd); CHECK_GPR(rs1); h=flux_mem_get(&v->mem,"heap"); GPR[rd]=h?(int32_t)(uint16_t)(h->data[GPR[rs1]] | (h->data[GPR[rs1]+1]<<8)):0; } break; | ||
| case FLUX_STORE16: { rd=f8(v); if(!v->running) break; rs1=f8(v); if(!v->running) break; CHECK_GPR(rs1); h=flux_mem_get(&v->mem,"heap"); if(h) { uint16_t val=(uint16_t)GPR[rs1]; h->data[GPR[rd]]=val&0xFF; h->data[GPR[rd]+1]=(val>>8)&0xFF; } } break; | ||
| case FLUX_LOAD32: { rd=f8(v); if(!v->running) break; rs1=f8(v); if(!v->running) break; CHECK_GPR(rd); CHECK_GPR(rs1); GPR[rd]=h?(int32_t)flux_mem_read_i32(h,(size_t)GPR[rs1]):0; break; } |
There was a problem hiding this comment.
🔴 FLUX_LOAD32 uses stale heap pointer h instead of fetching it
The FLUX_LOAD32 handler at line 776 uses the local variable h without first calling h=flux_mem_get(&v->mem,"heap"). The variable h is initialized to NULL at src/vm.c:554, so unless a prior operation in the same flux_vm_execute call happened to set h, this will always read 0 (the NULL fallback). Compare with FLUX_LOAD8 at src/vm.c:772 and FLUX_STORE32 at src/vm.c:777, both of which correctly fetch the heap pointer before use.
| case FLUX_LOAD32: { rd=f8(v); if(!v->running) break; rs1=f8(v); if(!v->running) break; CHECK_GPR(rd); CHECK_GPR(rs1); GPR[rd]=h?(int32_t)flux_mem_read_i32(h,(size_t)GPR[rs1]):0; break; } | |
| case FLUX_LOAD32: { rd=f8(v); if(!v->running) break; rs1=f8(v); if(!v->running) break; CHECK_GPR(rd); CHECK_GPR(rs1); h=flux_mem_get(&v->mem,"heap"); GPR[rd]=h?(int32_t)flux_mem_read_i32(h,(size_t)GPR[rs1]):0; break; } |
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
| case FLUX_ROL: rd=f8(v); if(!v->running) break; rs1=f8(v); if(!v->running) break; CHECK_GPR(rd); CHECK_GPR(rs1); { int s=GPR[rs1]&31; GPR[rd]=(GPR[rd]<<s)|(GPR[rd]>>(32-s)); } break; | ||
| case FLUX_ROR: rd=f8(v); if(!v->running) break; rs1=f8(v); if(!v->running) break; CHECK_GPR(rd); CHECK_GPR(rs1); { int s=GPR[rs1]&31; GPR[rd]=(GPR[rd]>>s)|(GPR[rd]<<(32-s)); } break; |
There was a problem hiding this comment.
🔴 ROL/ROR trigger undefined behavior when shift amount is 0
When GPR[rs1] & 31 == 0, the expression 32 - s evaluates to 32, and shifting a 32-bit int32_t by 32 is undefined behavior in C (C11 §6.5.7: "If the value of the right operand is negative or is greater than or equal to the width of the promoted left operand, the behavior is undefined"). This affects both FLUX_ROL and FLUX_ROR. A rotate by 0 should be a no-op but may produce arbitrary results or crash under optimization.
| case FLUX_ROL: rd=f8(v); if(!v->running) break; rs1=f8(v); if(!v->running) break; CHECK_GPR(rd); CHECK_GPR(rs1); { int s=GPR[rs1]&31; GPR[rd]=(GPR[rd]<<s)|(GPR[rd]>>(32-s)); } break; | |
| case FLUX_ROR: rd=f8(v); if(!v->running) break; rs1=f8(v); if(!v->running) break; CHECK_GPR(rd); CHECK_GPR(rs1); { int s=GPR[rs1]&31; GPR[rd]=(GPR[rd]>>s)|(GPR[rd]<<(32-s)); } break; | |
| case FLUX_ROL: rd=f8(v); if(!v->running) break; rs1=f8(v); if(!v->running) break; CHECK_GPR(rd); CHECK_GPR(rs1); { int s=GPR[rs1]&31; if(s) { uint32_t u=(uint32_t)GPR[rd]; GPR[rd]=(int32_t)((u<<s)|(u>>(32-s))); } } break; | |
| case FLUX_ROR: rd=f8(v); if(!v->running) break; rs1=f8(v); if(!v->running) break; CHECK_GPR(rd); CHECK_GPR(rs1); { int s=GPR[rs1]&31; if(s) { uint32_t u=(uint32_t)GPR[rd]; GPR[rd]=(int32_t)((u>>s)|(u<<(32-s))); } } break; |
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
| case FLUX_LOAD16: { rd=f8(v); if(!v->running) break; rs1=f8(v); if(!v->running) break; CHECK_GPR(rd); CHECK_GPR(rs1); h=flux_mem_get(&v->mem,"heap"); GPR[rd]=h?(int32_t)(uint16_t)(h->data[GPR[rs1]] | (h->data[GPR[rs1]+1]<<8)):0; } break; | ||
| case FLUX_STORE16: { rd=f8(v); if(!v->running) break; rs1=f8(v); if(!v->running) break; CHECK_GPR(rs1); h=flux_mem_get(&v->mem,"heap"); if(h) { uint16_t val=(uint16_t)GPR[rs1]; h->data[GPR[rd]]=val&0xFF; h->data[GPR[rd]+1]=(val>>8)&0xFF; } } break; |
There was a problem hiding this comment.
🔴 LOAD16/STORE16 bypass bounds-checked memory API, risking buffer overflow
LOAD16 and STORE16 directly access h->data[GPR[rs1]] and h->data[GPR[rs1]+1] without any bounds checking, unlike LOAD8/STORE8 which use the bounds-checked flux_mem_read_u8/flux_mem_write_u8 API (src/vm.c:772-773). Since GPR[rs1] is int32_t, a negative value or a value near the region size can cause an out-of-bounds read/write on the heap data buffer. The flux_mem_read/flux_mem_write functions at src/memory.c:26-27 explicitly check o + l <= r->size before accessing data.
Prompt for agents
LOAD16 and STORE16 at src/vm.c:774-775 directly index into h->data[] without bounds checking, unlike LOAD8/STORE8 which use flux_mem_read_u8/flux_mem_write_u8. These should be rewritten to use the bounds-checked memory API. For example, LOAD16 could use flux_mem_read(h, offset, &tmp, 2) and reassemble the 16-bit value, or a new flux_mem_read_u16 helper could be added to src/memory.c following the pattern of flux_mem_read_u8 at memory.c:30. STORE16 similarly should use flux_mem_write with bounds checking.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
1 participant
Summary
Significant enhancement of the FLUX runtime C implementation:
Extended ISA (80 → 247 opcodes)
Execution Tracing
Safety Improvements
Better Error Messages
Build
Tests