Clause Code Review #1

[hoglet67]

PiTubeDirect (indigo-dev) — Code Review

Scope: the full src/ tree of the indigo-dev branch.
Method: verification by direct read of the highest-severity items, plus four parallel deep audits covering the RISC-V co-processor and new infrastructure, the framebuffer, the bare-metal core infrastructure, and the ARM-native / SWI / debugger code. Every finding cites a real file:line.

Severity legend: Critical = exploitable memory corruption or guaranteed crash; High = correctness bug under reachable input; Medium = bug under unusual conditions or hard-to-trigger UB; Low = robustness / portability; Nit = style.

---

Executive summary

The most serious issues are memory-safety defects reachable from host- or guest-controlled input: unbounded host-controlled receiveString writes, the OPC7 and RISC-V memory-region masks that don't match their backing buffers, the _fast_scroll runaway-copy mask, the doCmdFill infinite loop, and the info.c off-by-one. The RISC-V co-processor — the newest code in the tree — repeats the codebase's recurring "address mask wider than the buffer" defect.

There is also a cluster of missing-volatile / missing-barrier issues in the state shared between the FIQ handler and thread context. -flto=auto is enabled in this branch, which widens the optimizer's cross-translation-unit view and makes those issues materially more likely to bite — they should be treated as a coordinated fix.

The build configuration is otherwise in good shape: a strong warning set (-Wformat=2, -Wcast-qual, -pedantic, -Wsign-conversion, -Wmaybe-uninitialized, -Wshadow, -Wnull-dereference).

---

Critical

C1. Unbounded host-controlled write via receiveString (no length parameter)

Files: tube-lib.c:98 (receiveString signature), tube-isr.c:513, tube-isr.c:162-174 (state-machine variant), tube-swi.c:1140

receiveString(unsigned char reg, unsigned char terminator, char *buf) has no length parameter — it loops do { buf[i++] = ...; } while (c != terminator);. Two dangerous call sites:

• tube-isr.c:513 — receiveString(R2_ID, 0x00, eblk->errorMsg) writes into isrErrorBlock.errorMsg, a 252-byte buffer (tube-env.h:9). A host sending an error string longer than 251 bytes with no 0x00 terminator overflows the static ErrorBlock_type and corrupts adjacent BSS. The state-machine ISR variant at tube-isr.c:162-174 (ERROR_R2_STRING, *emsg++ = c) has the same flaw.
• tube-swi.c:1140 — reg[1] = receiveString(R2_ID, '\r', (char *)reg[0]) in tube_ReadLine. The host is told a max line length (buf_len, capped at 0xFF) but receiveString ignores it; a host that streams more than buf_len bytes overruns the client program's OS_ReadLine buffer — directly host-controlled memory corruption.

Fix: add a size_t buflen parameter; stop at buflen-1, force a terminator. Pass buf_len from tube_ReadLine and sizeof(errorMsg) from the ISR.

C2. OPC7 memory mask is wider than the allocated buffer (~4 MB OOB read/write)

File: copro-opc7.c:24,34 vs :72

copro_opc7_read_mem/write_mem mask addr &= 0xFFFFF (1 M words), but copro_opc7_poweron_reset allocates copro_mem_reset(0x20000) — 0x20000 bytes = 0x8000 uint32_t entries. Any guest access with addr >= 0x8000 reads/writes up to ~4 MB past the buffer. Reachable by any OPC7 client ROM.

Fix: mask addr &= 0x7FFF, or allocate 0x400000 bytes to match the mask.

C3. _fast_scroll size-rounding mask is wrong (#0x4F should be #0x3F)

File: armc-start.S:376

_fast_scroll:
        push    {r4, r5, r6, r8, r9, r10, r11, lr}
        bic     r2, r2, #0x4F          ; clears bits 0-3 AND bit 6
_fast_scroll_loop:
        ...
        subs    r2, r2, #64
        bne     _fast_scroll_loop

The loop decrements R2 by 64 per pass and exits on bne, so R2 must be an exact multiple of 64 — bits 0-5 cleared, i.e. #0x3F. #0x4F clears bits {0,1,2,3,6} instead. A request of exactly 64 (0x40) becomes 0x00; the first subs yields 0xFFFFFFC0, bne is taken, and the copy runs away through memory until it wraps — a hang plus widespread corruption. The sibling _fast_clear correctly uses #0x3F.

Fix: bic r2, r2, #0x3F.

C4. RISC-V copro: tube-MMIO detection is a bitmask-equality test, not a range check → OOB RAM access

File: copro-riscv.c:70-73 (and the read/write paths at :87,:111,:127,:143)

ADDR_MASK = MINI_RV32_RAM_SIZE - 1 = 0x00FFFFFF; the buffer from copro_mem_reset(MINI_RV32_RAM_SIZE) is exactly 0x01000000 bytes. The tube-region test is (addr & TUBE_MASK) == TUBE_MASK with TUBE_MASK = 0x00FFFFE0 — that only matches addresses where every bit of 0xFFFFE0 is set, which is not a range check. So:

• addr = 0x00FFFFFD does not match → falls into the RAM branch → *(uint32_t *)(memory + 0xFFFFFD) writes bytes 0xFFFFFD..0x1000000, one past the 16 MB buffer.
• addr = 0x00FFFFE1 (bit 0 set, bit 1 clear) also misses the tube test and reads/writes RAM in the region that is supposed to be MMIO.

Reachable by any RISC-V client ROM that touches the top of its address space. This is the same defect class as C2.

Fix: use a real range check — if (addr >= MINI_RV32_RAM_SIZE - 32) { /* MMIO */ } — and reject unaligned accesses that straddle the RAM/MMIO boundary (or over-allocate the buffer by a few bytes).

---

High

H1. time_t 32-bit overflow corrupts every OSWORD &0E sub-3 timestamp

File: tube-swi.c:984-987

time_t centiseconds = 365 * 70 + 17;   // 25567
centiseconds *= 24 * 60 * 60;          // 2,208,988,800 — already > INT32_MAX
centiseconds += mktime(&t);
centiseconds *= 100;

On this bare-metal target time_t is signed 32-bit. 25567 * 86400 = 2,208,988,800 overflows INT32_MAX at line 985, before mktime is even added; the *= 100 overflows catastrophically. The comment claims the calculation is "built up progressively to avoid 32-bit integer overflow" — that mitigation does not work. The result is garbage centisecond values in SAVE load/exec addresses.

Fix: use an explicit uint64_t accumulator independent of time_t.

H2. doCmdFill infinite loop when end == UINT_MAX (duplicated in three more places)

Files: tube-commands.c:277 (doCmdFill), tube-commands.c:370 (doCmdCrc), debugger.c:892,908

for (unsigned int i = start; i <= end; i++)

*FILL 0 FFFFFFFF AA makes i++ wrap from 0xFFFFFFFF to 0 and the loop never terminates. doCmdFill keeps writing *(unsigned char *)i = data across all of memory; doCmdCrc and the two debugger sites just hang.

Fix: for (i = start; ; i++) { ...; if (i == end) break; } in all four places.

H3. cmdline[buf->byte_length] = 0 — OOB write from a firmware-controlled length

File: info.c:103-104

memcpy(cmdline, buf->data.buffer_8, buf->byte_length);
cmdline[buf->byte_length] = 0;

cmdline is PROP_SIZE bytes. If the firmware returns byte_length == PROP_SIZE, the NUL write is one byte past the end (and the memcpy itself overflows if byte_length > PROP_SIZE). The .noinit placement makes the overrun clobber whatever static follows.

Fix:

size_t n = buf->byte_length;
if (n >= PROP_SIZE) n = PROP_SIZE - 1;
memcpy(cmdline, buf->data.buffer_8, n);
cmdline[n] = 0;

H4. command[256] may be left unterminated; downstream parser walks off the end

File: tube-swi.c:757-763

char command[256];
do { c = (command[index++] = *ptr++); }
while (c != 0x00 && c != 0x0a && c != 0x0d && index < sizeof(command));

If the source command reaches 256 bytes with no terminator, the loop exits with index == 256 and no NUL written. The subsequent skip-spaces-and-stars code, the lptr[0..3] reads, and the commandBuffer copy all walk off the end (and may overflow the 256-byte commandBuffer). tube_CLI is reachable from the host via OS_CLI.

Fix: loop while index < sizeof(command) - 1, then unconditionally command[index] = 0;.

H5. VDU 23,0,10/11 cursor-shape values not clamped → OOB framebuffer write

File: framebuffer.c:794-799, :392-402, amplified by screen_modes.c:1212-1240

cursor_start/cursor_end are taken as buf[2] & 0x1f (0..31) with no clamp to the font height. invert_cursor then loops for (i = start; i <= end; i++) calling screen->get_pixel/set_pixel directly — and default_get/set_pixel_*bpp have no bounds checks. With a small font (8 px) and cursor_end = 31, this reads/writes well outside the character cell and can run off the framebuffer entirely.

Fix: clamp cursor_start/cursor_end to [0, font_height-1]; add bounds checks in default_*_pixel_*bpp.

H6. graphics_cursor_tab — uint8_t overflow and double origin addition

File: framebuffer.c:752-767

x and y are uint8_t. x *= (uint8_t)(font_width << screen->xeigfactor) overflows mod 256 for any column ≥ 16; x += (uint8_t)g_window.left truncates again; then g_x_pos = g_window.left + x adds the window origin a second time. The cursor lands at a wholly wrong position, which then flows into plotting (the code comments "Deliberately don't range check here").

Fix: compute in int, scale, add the origin exactly once.

H7. fb_custom_mode — infinite loop / signed-shift UB / divide-by-zero on zero or negative pixel count

File: framebuffer.c:1873-1900, screen_modes.c:968

do { new_screen->xeigfactor++; x_pixels <<= 1; } while (x_pixels < 1280);

With x_pixels == 0, 0 << 1 == 0 and the loop never terminates (and xeigfactor overflows). With a negative x_pixels, << is UB. If y_pixels == 0 reaches default_init_screen, screen_modes.c:968 does num_buffers = MAX_VIRTUAL_HEIGHT / screen->height — divide by zero. Reachable from VDU 23,22 and OS_ScreenMode_impl with arbitrary values. get_screen_mode(...) is also not NULL-checked before new_screen is written.

Fix: reject/clamp x_pixels/y_pixels to a sane minimum at the top of fb_custom_mode; NULL-check get_screen_mode.

H8. 16bpp sprite capture uses the X coordinate for Y

File: primitives.c:1890

*data++ = (uint16_t)get_pixel(screen, x1 + xp, y1 + xp);   // should be y1 + yp

The 32bpp (:1897) and 8bpp (:1904) paths correctly use y1 + yp. Every captured sprite in 16-bpp mode is a diagonal smear; for non-square sprites it also reads out of the intended region.

Fix: y1 + yp.

H9. doCmdPiLIFE — integer overflow in sx*sy and UB shifts from unvalidated eigfactors

File: tube-commands.c:595-598,646-649,676-679

calloc(sx * sy, 1) — sx/sy come straight from a user sscanf("%u %u") with no bounds check. sx * sy can overflow to a small value; calloc then succeeds with a tiny allocation and the nested loops write far out of bounds. sx/sy of 0 make sx - 1/sy - 1 underflow to UINT_MAX. xeigfactor/yeigfactor from OS_ReadModeVariable are int; x << xeigfactor and x >> (8 - xeigfactor) are UB for out-of-range or negative values. (The calloc return is now NULL-checked.)

Fix: validate sx, sy against a sane max and reject 0/1; ensure sx*sy doesn't overflow; clamp the eigfactors to 0..8.

H10. No memory barriers around the FIQ-driven tube register I/O

File: tube-ula.c — no _data_memory_barrier() in tube_io_handler or its callees

tube_host_read/tube_host_write/tube_io_handler run in FIQ context and write tube_regs[] (strongly-ordered device memory) interleaved with normal cached accesses to ph1[], pstat[], etc. With no barrier, the FIQ can return — and the GPU/host observe stale register values — before the write actually lands. On Pi 3/4 this is a real ordering hazard.

Fix: add _data_memory_barrier() at the end of tube_io_handler before return tube_irq;, and bracket the peripheral accesses.

H11. OPC5LS / OPC6 / OPC7 / F100 dispatchers hang on NMI

Files: copro-opc5ls.c, copro-opc6.c:104, copro-opc7.c, copro-f100.c

Each builds tube_irq_copy = tube_irq & (RESET_BIT + NMI_BIT + IRQ_BIT) but only handles RESET_BIT and IRQ_BIT. NMI is never serviced and never acked; tubeContinueRunning() returns false while NMI_BIT is set, so the exec loop exits, the outer loop re-enters, and exits again — an infinite tight loop if NMI is ever raised. (PDP-11, Z80, MC6809, lib6502, ARM2, 80186 all correctly ack NMI.)

Fix: drop NMI_BIT from the mask (these CPUs have no tube NMI line), or add if (tube_irq_copy & NMI_BIT) tube_ack_nmi();.

H12. 6809 disassembler get_memw shifts the address instead of the byte

File: mc6809nc/mc6809_dis.c:26-28

static inline unsigned int get_memw(unsigned int addr) {
   return read8((uint16_t)(addr << 8)) | read8((uint16_t)(addr + 1));
}

The high byte should be read8(addr) << 8. As written it reads from the wrong (aliased) address addr<<8 and never shifts the result into bits 8-15. Every 16-bit operand in 6809 disassembly is wrong.

Fix: return ((uint32_t)read8((uint16_t)addr) << 8) | read8((uint16_t)(addr + 1));

H13. RISC-V copro: sub-word tube accesses are mis-routed

File: copro-riscv.c:70-71

Once an address passes the (broken, see C4) tube test, the register index is (addr >> 2) & 7 with no alignment enforcement. A byte/halfword store to e.g. 0x00FFFFE5 is treated as a full word write to a tube register.

Fix: range-check first, then require (addr & 3) == 0 before computing the register index, or explicitly define byte-access semantics.

H14. RISC-V copro: warm reset leaves most CPU state stale

File: copro-riscv.c:202-229 (copro_riscv_reset)

copro_riscv_reset() runs on every Tube RESET edge but, unlike copro_riscv_poweron_reset(), does not memset the state. It resets pc, mcause, timers, and parts of mstatus, but leaves regs[0..31], mscratch, mtvec, mip, mepc, mtval, cyclel/h untouched. A pending mip bit or stale mtvec surviving a reset means the first instruction after reset can immediately trap to a stale vector; regs[] retaining old contents is a non-deterministic-boot hazard.

Fix: clear mip (at least the dispatcher-managed bits) and zero regs[].

H15. jit_debug.c: return strlen(buf) after buf was advanced — OOB read

File: jit_debug.c:196

In the i_P register-print branch, buf is advanced by *buf++ through the loop, then return strlen(buf) measures from the advanced pointer into uninitialised bytes — an OOB read returning a nonsense length, which the caller then uses to advance its own pointer. Line 195 wrote a trailing space but no NUL.

Fix: capture char *start = buf; before the loop and return (size_t)(buf - start);.

H16. jit_debug.c: "buffer too small" check writes the warning then continues anyway

File: jit_debug.c:176-178

if (bufsize < 40) { strncpy(buf, "buffer too small!!!", bufsize); } — no return. Execution falls through into the loop that writes ~40 bytes via *buf++, overrunning the buffer just declared too small.

Fix: return immediately after the warning.

H17. armc-start.S: stray + character breaks the multicore build

File: armc-start.S:285 — +.section ".text._init_core"

A literal + at the start of the line. Inside #ifdef USE_MULTICORE, so dead today, but the assembler will choke the moment multicore is enabled.

Fix: remove the +.

H18. OS_ReadLine_impl writes one byte past the caller's buffer

File: swi_impl.c:301-311

The received character is stored (*(((char *)reg[0]) + ptr) = c;, line 301) before the full-buffer check (if (ptr >= buf_size), line 311). When ptr == buf_size, a byte — and possibly the terminating CR at line 304 — is written one past the end of the caller's buffer.

Fix: test ptr >= buf_size before the store, or stage into a bounded local buffer.

H19. Shared static char strbuf[1000] re-entered between main loop and UART-IRQ command dispatch

File: debugger.c:250

strbuf is used by disassemble_addr (called from the CPU-emulation/breakpoint path) and by doCmdDis/doCmdRegs (called from UART-IRQ command dispatch). A breakpoint hit firing disassemble_addr while the main loop is mid-doCmdRegs corrupts the buffer for both.

Fix: use separate buffers, or gate command processing on stopped.

H20. The disassembler / debugger buffer-handling family

Files: lib6502_debug.c, mame/arm_debug.c, NS32016/NSDis.c, mc6809nc/mc6809_dis.c, cpu80186/cpu80186_debug.c, opc5ls/opc5ls_debug.c, opc6/opc6_debug.c, opc7/opc7_debug.c, f100/f100_debug.c, pdp11/pdp11_debug.c, yaze/simz80.c, jit_debug.c, riscv/riscv_debug.c

One cut-and-paste template, three compounding bugs, replicated across ~13 files:

1. snprintf-return underflow — buf += snprintf(...); bufsize -= snprintf-return. snprintf returns the would-be length; on truncation len > bufsize, so buf is advanced past the end and bufsize (unsigned) underflows to a huge value, after which every append writes unbounded OOB. (StringAppend in NSDis.c:60-68, stringAppend in mc6809_dis.c:646-654, cpu80186_debug.c:91-106, etc.)
2. return strlen(buf) after the pointer was advanced — guaranteed OOB read in every flag-register printer (lib6502_debug.c:156, mame/arm_debug.c:197, mc6809_debug.c:166, opc5ls/opc5ls_debug.c:251, opc6/opc6_debug.c:259, opc7/opc7_debug.c:255, f100/f100_debug.c:383, pdp11/pdp11_debug.c:149, yaze/simz80.c:394, jit_debug.c:196).
3. "buffer too small" check that doesn't actually stop writing — prints the warning then overruns anyway (lib6502_debug.c:136, mame/arm_debug.c:163, mc6809_debug.c:146, the opc* / f100 / pdp11 debug files, yaze/simz80.c:374, jit_debug.c:176).

Fix: one shared template — clamp the snprintf return to the remaining size before subtracting; track a start pointer and return the delta; return after the "too small" warning. Fix it once, propagate.

H21. lib6502.c jsr macro sets PC to 0 when a JSR callback returns 0

File: lib6502.c:712-724

The jsr macro was rewritten in this branch and a regression slipped in:

ea = (word)(MEM(PC));
PC++;
... push return address ...
ea |= (word)(MEM(PC) << 8);              // ea = JSR target
if (mpu->callbacks->call[ea]) {
   externalise();
   if ((ea = (word)mpu->callbacks->call[ea](mpu, ea, 0))) {   // ea CLOBBERED
      internalise();
   }
}
PC = (word)ea;

lib6502's call callback convention is: return non-zero to redirect, return 0 to mean "execute the instruction normally". The previous code stored the callback result in a separate addr variable, so a 0 return left ea (the original target) intact for PC = ea. The rewrite assigns the result straight back into ea, so a 0 return overwrites the target with 0 and PC is set to 0 instead of the JSR target. Any JSR to an address that has a registered call callback returning 0 jumps the emulated 6502 to address 0.

Fix: keep the callback result in a separate variable, as the original did:

if (mpu->callbacks->call[ea]) {
   externalise();
   word addr = (word)mpu->callbacks->call[ea](mpu, ea, 0);
   if (addr) { internalise(); ea = addr; }
}
PC = (word)ea;

---

Medium

M1. tube_irq read-modify-written from thread context without masking FIQ

File: tube-ula.c:161-166 (tube_ack_nmi), :948 (disable_tube)

tube_irq is RMW'd in FIQ context (tube_io_handler → tube_host_read/write) and in thread context. The thread-context paths use _disable_interrupts_cspr(), which masks IRQ but not FIQ. The FIQ handler can preempt tube_ack_nmi between its read and write of tube_irq and lose a concurrently-set bit. disable_tube does tube_irq &= ~TUBE_ENABLE_BIT with no masking at all. -flto=auto gives the compiler more freedom to reorder around these.

Fix: use a FIQ-masking critical section for every thread-context tube_irq RMW.

M2. FIQ-shared tube FIFO state is not volatile

File: tube-ula.c:73-77

ph1[24], ph3_1, hp1, hp2, hp3[2], hp4, ph3pos, hp3pos, ph1rdpos/wrpos/len are plain (non-volatile) statics written by the FIQ handler and read/written by thread-context tube_parasite_read/write. Only pstat[4] is volatile. The compiler may cache these across points where a FIQ can modify them — e.g. tube_parasite_read case 5 tests hp3pos, then does hp3[0]=hp3[1]; hp3pos--; which can apply to a stale value.

Fix: make all FIQ-shared state volatile, combined with FIQ-masking critical sections.

M3. UART ring-buffer head/tail have no memory barrier

File: rpi-aux.c:24-25,49-50,220-221

tx_head/tx_tail are volatile int (no compiler caching) but volatile provides no memory barrier. The producer does tx_buffer[tmp_head] = c; tx_head = tmp_head; — on Pi 3/4 the ISR can observe the updated tx_head before the tx_buffer[] store is globally visible and transmit a stale byte.

Fix: _data_memory_barrier() between the buffer write and the index update, both producer and consumer.

M4. FIQ/SWI vector words rewritten without I-cache invalidate

File: tube-client.c:90,119-120

init_emulator writes the FIQ/SWI vector words and follows with _data_memory_barrier(). A DMB orders the store but does not invalidate the I-cache or flush prefetch state for the modified vector location (the vectors live in normal cacheable memory). On Pi 3/4 a stale fetched vector can be used when switching copro.

Fix: clean the D-cache line, invalidate I-cache, and ISB after rewriting a vector word.

M5. jit_debug.c: snprintf-return underflow and negative-size_t strncpy

File: jit_debug.c:71-116

bufsize -= len; where len is the snprintf return value — on truncation bufsize (a uint32_t) underflows to a huge value and subsequent strncpy/snprintf are handed bogus enormous sizes. strncpy(buf, "...", padding - offset) at :92 passes a wrapped (huge) length when offset > padding, and buf += padding - 1 - offset can move buf backwards so buf[-1] = 0 at :116 writes before the intended region. strncpy(buf, instr, bufsize) at :82 may also not NUL-terminate.

Fix: clamp every length to the remaining bufsize before subtracting; bail when bufsize is exhausted; guard offset < padding.

M6. RISC-V copro: 1 ms timer ticks are lost under load

File: copro-riscv.c:277-282

When elapsed_cycles >= arm_cycles_per_ms, the code sets elapsed_us = 1000 and advances the anchor by exactly one millisecond. If the main loop stalls for >2 ms (long debug disassembly, contention), multiple elapsed milliseconds collapse into a single tick and the RISC-V timerl runs slow.

Fix: use a while loop, or compute elapsed_us = (elapsed_cycles / arm_cycles_per_ms) * 1000 and advance the anchor accordingly.

M7. RISC-V copro: instruction fetch / load-store bounds checks ignore the tube MMIO window

File: riscv/mini-rv32ima.h:156-162,226,272,468

The fetch guard is only ofs_pc >= MINI_RV32_RAM_SIZE; a PC in 0x00FFFFE0..0x00FFFFFC passes it and the fetch is dispatched to copro_riscv_read_mem32 → tube_parasite_read, i.e. executing from a tube register. The core's own access-fault machinery only fires for the stock 0x10000000 window, never the tube window at 0x00FFFFE0, so all MMIO correctness rests on the broken mask in C4.

Fix: trap PC in the MMIO window; route tube MMIO through the core's MINIRV32_HANDLE_MEM_*_CONTROL hooks.

M8. RISC-V emulator core: implementation-defined signed right shift

File: riscv/mini-rv32ima.h:346

SRA/SRAI use ((int32_t)rs1) >> (rs2 & 0x1F). Correct on GCC/ARM (arithmetic shift) but implementation-defined — and -pedantic is enabled.

Fix: implement SRA via unsigned shift + explicit sign replication, or document the toolchain dependency.

M9. riscv_disas.c / riscv_debug.c: snprintf-return underflow pattern

File: riscv_disas.c:361-363,449-456; riscv_debug.c:146-153,210-212

The same buf += snprintf(...); bufsize -= snprintf-return pattern as H20. Currently unreachable with normal buffer sizes (the first writes are short fixed-width fields) but it is the same bug class, and riscv_debug.c:210 returns the would-be length rather than bytes written.

Fix: clamp after each snprintf; dbg_reg_print should return min(ret, bufsize-1).

M10. default_set_pixel_*bpp / get_pixel have no bounds checks

File: screen_modes.c:1212-1240

The raw setters compute fb + (h - y - 1)*pitch + x*bpp and write with no clamp. Most callers clip first, but several bypass the wrapper: invert_cursor (see H5), default_clear_screen/scroll_screen, tt_draw_character, the font writers. Any defect upstream becomes an arbitrary OOB write into the peripheral address space.

Fix: early-return inside the bpp setters if x or y is out of range.

M11. func_ptr typedef is an unprototyped () — signature mismatch UB

File: copro-defs.h

typedef void (*func_ptr)(); is called via copro_def->emulator(copro_def->type), but most emulator definitions take no parameter while 65tube/65tubejit/lib6502 take int type. Calling a function through a differently-typed pointer is UB in C11; it works on the ARM ABI by luck.

Fix: make the typedef typedef void (*func_ptr)(int type); and update every emulator signature.

M12. last_r12 single global clobbered by nested SWIs

File: tube-swi.c:43, written :560, read :504

C_SWI_Handler re-enables IRQs and SWIs can nest (OS_ReadLine blocks, an event handler runs and issues a SWI). The inner SWI overwrites the single global last_r12; if the outer SWI then generates an error, the error handler is entered with the wrong R12. ARM BASIC depends on R12 preservation.

Fix: save/restore last_r12 per invocation (push on entry, pop on exit).

M13. _user_exec ARMv6 path: no DSB between D-cache clean and I-cache invalidate

File: copro-armnativeasm.S:179-185

The ARMv6 path does D-cache clean → I-cache invalidate → prefetch flush with no DSB in between. The clean must be guaranteed complete (reach the point of unification) before the I-cache invalidate, otherwise freshly written code may not be visible to the instruction fetch.

Fix: insert mcr p15,0,r4,c7,c10,4 (DSB) after the D-cache clean, before the I-cache invalidate. (Also add a DSB before the ICIALLU on the ARMv7 path.)

M14. parse1params return value tested with < 0 — errors silently ignored

File: debugger.c:1165 (doCmdIn), :1317 (doCmdClear)

parseNparams returns 0 (ok) or 1 (error), never negative. if (parse1params(...) < 0) is never true, so a bad/missing parameter is ignored and the code proceeds with an uninitialized (doCmdIn) or zero (doCmdClear) address. doCmdIn then does an I/O read at a garbage address.

Fix: if (parse1params(...)) — nonzero means error.

M15. Abbreviated debugger commands silently pick the first prefix match

File: debugger.c:717-742

strncmp(cmdString, cmd, minLen) == 0 returns the first table entry matching the typed prefix. The table has many shared prefixes: b matches breakx but never breakr/breakw/breaki/breako; c matches continue before crc/clear. Typing c silently runs continue. No ambiguity detection.

Fix: continue scanning after a match; reject on a second prefix-match.

M16. genericBreakpoint / genericClear duplicate-check compares unmasked address

File: debugger.c:693-699, :1292

setBreakpoint stores addr & mask, but the duplicate-detection loop compares the raw addr. Re-adding a masked breakpoint at the same logical address misses the dedup and grows the table; clearing by the typed address won't match a masked breakpoint.

Fix: compare (list[i].addr == (addr & mask) && list[i].mask == mask).

M17. tube_SynchroniseCodeAreas ignores R0/R1/R2

File: tube-swi.c:1318-1321

Always does an unconditional full CleanDataCache() + _invalidate_icache() regardless of R0 (whole space vs address range) and R1/R2. Functionally safe but wasteful, and not spec-compliant. ARM BASIC startup hits this hard.

Fix: when R0 bit 0 is clear, do range-based maintenance on [R1, R2).

M18. lookup_swi_name returns a pointer to a static buffer

File: tube-swi.c:451-488

static char name[SWI_NAME_LEN] is returned to the caller; a nested SWI during name lookup corrupts it.

Fix: take an output-buffer pointer.

M19. PDP-11 unaligned 16-bit access via synthesized uint16_t *

File: copro-pdp11.c:60,69

*(uint16_t *)(memory + addr) at runtime-arbitrary offsets is a hard alignment trap on ARMv5 and undefined under -fstrict-aliasing. Also addr == 0xFFFF writes 2 bytes starting at offset 0xFFFF of a 0x10000-byte buffer — 1-byte OOB.

Fix: memcpy(&memory[addr], &data, 2) or byte-wise assembly; mask addr &= 0xFFFE for word ops.

M20. OPC6 op_adc/op_sub/op_sbc truncate carry-out to 16 bits

File: opc6/opc6.c:127,148,153,158,167

res = (uint16_t)(s.reg[dst] + ea_ed + cin); casts to uint16_t before assignment to the uint32_t res, so the carry-out bit is lost and cout = (res >> 16) & 1 always reads 0. op_add and op_cmp correctly use 32-bit width — inconsistent. Breaks multi-precision arithmetic in OPC6 guest code.

Fix: remove the (uint16_t) cast in the adc/sub/sbc/inc/dec paths.

M21. Non-USE_MEMORY_POINTER memory paths dereference guest addresses as host pointers

Files: NS32016/mem32016.c, cpu80186/mem80186.c, copro-arm2.c, copro-z80.c, copro-mc6809nc.c, lib6502.c

The #else branches dereference the emulated address directly as a host pointer — a wild access / Data Abort on bare metal. The build only survives because USE_MEMORY_POINTER is expected to be defined globally, but it is not set in any header under src/. If the flag is ever dropped, every copro builds clean and then faults.

Fix: #ifndef USE_MEMORY_POINTER #error ... #endif, or delete the dead raw-pointer branches.

M22. copy_font_character off-by-one bounds check

File: fonts.c:131

if (c > font->num_chars) return; should be >=. With num_chars == 256, c == 256 computes a dst 28 entries past the end of font->buffer. Current callers don't hit it, but the guard is wrong.

Fix: if (c >= font->num_chars) return;.

M23. initialize_font NULL-deref on calloc failure

File: fonts.c:308

font->buffer = calloc(size, 1); is not NULL-checked. Immediately after, font->set_rounding(font, 0) → copy_font_character writes through font->buffer → NULL dereference if calloc failed.

Fix: check the return and bail (or retain the previous buffer).

M24. Off-by-one in horizontal-line area fills

File: primitives.c:1156,1159,1172,1183,1186,1199

The fill loops use x_right + 1 < g_x_max / x_left - 1 > g_x_min, stopping one pixel short of the clip boundary. The rightmost/leftmost column of the graphics window is never filled.

Fix: use <= g_x_max / >= g_x_min.

M25. prim_fill_chord — float divide-by-zero / nan→int UB

File: primitives.c:1505-1506

end_dx = (int)roundf(((float) end_dx) * radius / radius2); — if x2,y2 == xc,yc then radius2 == 0.0f → division yields inf/nan; casting nan/inf to int is UB. Reachable via PLOT 168 with the second point at the centre.

Fix: guard if (radius2 < 1.0f) return; before the projection.

M26. get_copro_name trusts maxlen without bounding it

File: copro-defs.c

name is a fixed 256-byte buffer; if a caller passes maxlen >= 256, name[maxlen] = '\0' writes OOB.

Fix: clamp maxlen against sizeof(name) - 1; use snprintf.

M27. doCmdGo / doCmdArmBasic jump to an unvalidated user-supplied address

File: tube-commands.c:242-255,385-392

doCmdGo does sscanf(params,"%x",&address); ((FunctionPtr_Type)address)(); with zero validation. doCmdArmBasic calls a function pointer declared with no parameters but passes params. Partly inherent to a "GO" command, but there is no sanity check.

Fix: at minimum reject obviously-bad addresses (0, peripheral MMIO range).

M28. doCmdSearch — end - len unsigned underflow

File: debugger.c:1128

for (unsigned int addr = start; addr <= end - len; addr++) — if len > end, end - len underflows to a huge value and the loop scans almost the entire address space calling cpu->memread.

Fix: check end >= len && end - len >= start first.

M29. rpi-aux.c RPI_AuxMiniUartString emits a spurious NUL for empty / terminator-adjacent strings

File: rpi-aux.c:115-159

In NUL-terminated mode (len == 0) the character is buffered before the terminator check, so an empty string still transmits one 0x00 byte. Both the IRQ and non-IRQ paths have this.

Fix: check *c before buffering.

M30. Reset path uses longjmp from interrupt context

File: tube-isr.c, copro-armnative.c

copro_armnative_reset longjmps back to a setjmp saved in reboot. In the non-reentrant variant the longjmp leaves the CPU in FIQ mode (banked regs leak, FIQ-disabled bit retained). setjmp/longjmp across interrupt boundaries is UB in C.

Fix: explicit mode transition to SVC/USR before longjmp.

---

Low / Nit

L1. extern unsigned int copro in the debugger drops the volatile qualifier

File: debugger.c:24 — the definition is volatile. Mismatched volatile on the same object across translation units is UB; the debugger may cache copro and miss an IRQ-driven copro switch, so getCpu() returns a stale pointer. Match the declaration.

L2. RESET_BIT + NMI_BIT + IRQ_BIT using + instead of |

File: tube-ula.c:205 and most copro-*.c dispatchers. Works today (the bits are disjoint), but + invites a real bug the day a non-power-of-two flag is added. Use |.

L3. vdu_operation_table entries 32..255 are NULL until fb_initialize

File: framebuffer.c:137-172. Any fb_writec on a byte ≥32 before fb_initialize runs dispatches through a NULL handler. Statically initialise to {0, vdu_default}.

L4. fb_writes(NULL) crashes; fb_get_address() declared but never defined

File: framebuffer.c:2001, framebuffer.h:98. Add a if (!string) return; guard to fb_writes; remove the dead fb_get_address declaration.

L5. linenoise.c corrupted escape sequence \x1u instead of \x1b

File: linenoise.c:568 — \x1u parses as \x1 + literal u. Only reached in multi-line mode (disabled by default). Fix to "\r\x1b[0K\x1b[1A".

L6. linenoiseEditHistoryNext doesn't check the strdup result

File: linenoise.c:704 — on OOM history[...] becomes NULL and the next history navigation dereferences NULL. Keep the old pointer if strdup fails.

L7. prim_set_dot_pattern_len accepts negative len

File: primitives.c:834-845 — else if (len <= 64) matches negative values; the dotted-line index then never wraps and OOB-reads g_dot_pattern[64..]. Use len > 0 && len <= 64.

L8. _invalidate_dcache is destructive (invalidate without clean)

File: armc-start.S — discards dirty lines; correct only at early boot, but it is exported globally and any mid-run caller loses dirty data. Verify all callers; _clean_invalidate_dcache is the safe variant.

L9. _prefetch_abort_handler_ cleanup uses the data-abort return offset

File: armc-start.S:231-251 — the shared cleanup does sub lr,lr,#8 (data-abort offset) but the prefetch-abort return offset is #4. Latent because dump_info never returns, but a non-watchpoint prefetch abort that did return would resume at the wrong address.

L10. RISC-V copro: no clock-pointer NULL check

File: copro-riscv.c:259,265 — get_clock_rates(ARM_CLK_ID) is dereferenced unconditionally; if it returns NULL the copro faults immediately.

L11. RISC-V copro: NMI is silently dropped (functional gap)

File: copro-riscv.c:300-318 — copro-riscv masks only RESET_BIT + IRQ_BIT, so it avoids the NMI-hang of H11 — but the BBC Tube protocol uses NMI for fast R3 block transfers; a client ROM that relies on NMI will never see those events. Document the limitation, or map NMI onto a CSR bit.

L12. rpi-asm-helpers: _get_core() declared unconditionally but defined only for ARMv7+

File: rpi-asm-helpers.h:15 vs rpi-asm-helpers.c:155 (#if __ARM_ARCH >= 7) — an ARMv6 build gets a link error from any caller. Provide a fallback or guard the prototype.

L13. riscv_disas.c: non-static generically-named globals

File: riscv_disas.c:92,99,235,252,287 — format, name, op0, op1, op2 are external symbols with very generic names — link-collision risk. Make them static.

L14. riscv_disas.c: signed-shift of a 20-bit bitfield

File: riscv_disas.c:274 — e->u.i31_12 << 12 promotes to int; for values with bit 19 set, << 12 shifts into the sign bit (UB). Use ((uint32_t)e->u.i31_12) << 12.

L15. jit_debug.c: dbg_memread/dbg_memwrite cast a uint32_t address straight to a pointer with no masking

File: jit_debug.c:53-61 — a debugger memory peek at a large address dereferences arbitrary ARM memory. Mask to the 6502 address space.

L16. info.c: print_tag_value can over-read a mis-sized firmware property

File: info.c:12 — loop bound (buf->byte_length + 3) >> 2; a firmware-reported byte_length larger than the actual buffer reads past buf->data. Clamp against PROP_SIZE/4.

L17. VDU handler function-pointer type mismatch

File: framebuffer.c:1262-1284 — handlers like text_cursor_left are void(void) but assigned to a void(*)(const uint8_t *) slot. Calling through an incompatible function-pointer type is UB; -pedantic may surface this. Give all VDU handlers the uniform signature.

L18. default_scroll_screen replicates bg_col but the widened value is then truncated

File: screen_modes.c:1065-1069 — the replication is dead/pointless (the blank loop's set_pixel truncates it back to one byte). Either drop the replication or call a real fill.

L19. Stray double semicolons / dead code

• screen_modes.c:882 — r->y2 = screen->height - 1;;
• framebuffer.c:1301 — c_fg_gcol = col; ;
• armc-start.S:386-387 — empty .section ".text._fast_clear" with no code
• armc-start.S:203 — stale // R0 = dst comment (_fast_clear actually uses r12)

L20. riscv/test_pi.c: host-side helper crashes with no argument

File: riscv/test_pi.c:8-12 — ignores argc, unconditionally atoi(argv[1]), never checks the calloc return. Host-side test helper, so impact is nil, but it crashes confusingly.

L21. rpi-asm-helpers.h: K&R-style empty () prototypes

File: rpi-asm-helpers.h:9-10 — _get_cpsr() / _get_stack_pointer() declared with empty () rather than (void); inconsistent with the rest of the file and defeats argument checking.

L22. lib6502.c _zpr disassembler computes the branch target from the wrong operand byte

File: lib6502.c:1065 — the rewritten M6502_disassemble _zpr macro (zero-page-relative, used by the R65C02 BBRn/BBSn instructions) computes the branch-target bytes from b[1] (the zero-page address operand) instead of b[2] (the relative-offset operand): htos(s+8,(byte)((ip + 2 + b[1])>>8u)) / htos(s+10,(byte)(ip + 2 + (int8_t)b[1])&0xff). Disassembly-display only, and ZPR opcodes are rarely used, but the displayed target is wrong. Use b[2] for the offset.

L23. pdp11/pdp11_debug.c disasm computes instruction length from a stale table entry on no-match

File: pdp11/pdp11_debug.c:368-389 — D l; is no longer initialised to the table sentinel; on an unrecognised instruction the lookup loop leaves l holding the last real table entry, so len (lines 384-389) is computed from that entry's flag instead of staying at 2. Cosmetic — the disamtable[i].inst == 0 check at line 408 still correctly prints ??? — but the instruction-word display length can be wrong. (The loop always runs at least once for the real table, so l is not actually read uninitialised.) Re-initialise l to the sentinel, or compute len after the inst == 0 check.

L24. mame/arm_debug.c stopped initialising dis.addr_mask

File: mame/arm_debug.c:117 (removed line) — dis.addr_mask = ADDRESS_MASK; was removed, consistent with darm/darm.c no longer masking PC-relative targets with addr_mask. Verify no remaining code path in darm/ reads darm_t::addr_mask — dis is a stack variable, so any surviving reader would now see an uninitialised value.

---

Build system (CMakeLists.txt)

The build configuration is in good shape — CMake minimum is a real version (3.5), and the warning set is strong (-Wmaybe-uninitialized, -Wformat=2, -Wcast-qual, -pedantic, -Wsign-conversion, -Wshadow, -Wnull-dereference). Remaining concerns:

1. -flto=auto + the volatile/barrier gaps — LTO is enabled (CMakeLists.txt:70). It widens the optimizer's cross-translation-unit view and makes M1–M4, H10 and L1 materially more likely to bite. Fix those before relying on LTO in production builds.
2. -Ofast is still set (CMakeLists.txt:43) — it enables -ffast-math and aggressive UB exploitation, which pulls against the newly-enabled -pedantic. Recommend -O2 for production, -Ofast only for benchmarking.
3. Dead RISC-V glob entries — CMakeLists.txt:268-277 globs riscv/riscv.h and riscv/riscv.c, which do not exist (the emulator is the header-only riscv/mini-rv32ima.h, included from copro-riscv.c). file(GLOB) silently yields nothing for those, so the build still works, but the list is misleading. Remove the dead entries.
4. rm *.su post-build — CMakeLists.txt:483-485 runs rm *.su in the build dir with no -f and no path; if no .su files exist the step errors. Use rm -f and a path, or a CMake-native cleanup.
5. -ffixed-r7 commented out — CMakeLists.txt:42; this previously reserved r7 for the Fast-6502 FIQ handler. Verify the FIQ handlers and copro-65tubeasm.S no longer depend on it.
6. The file(GLOB ...) calls still list explicit filenames (no actual glob pattern) — set(...) would be clearer and dodges CMake's "GLOB not recommended for sources" guidance.

---

The new RISC-V co-processor

copro-riscv.c, riscv/mini-rv32ima.h, riscv/riscv_disas.c, riscv/riscv_debug.c, plus ROM data — functional, but it ships with the codebase's recurring "address mask wider than the buffer" defect (C4), mis-routed sub-word MMIO (H13), a warm-reset that leaves most CPU state stale (H14), a timer that drifts under load (M6), a fetch/MMIO gap (M7), the RISC-V signed-shift caveat (M8), and the disassembler buffer-handling family (M9, plus the jit_debug.c issues H15/H16/M5). It does correctly avoid the OPC-family NMI hang (the dispatcher masks only RESET_BIT + IRQ_BIT), though that means NMI-driven R3 transfers are silently unsupported (L11).

jit_debug.c (the JIT disassembly debug helper) carries the disassembler buffer bugs (H15, H16, M5, L15).

The new header splits (tube-irqbits.h, tube-pins.h, tube-debug.h, rpi-auxreg.h, rpi-base-asm.h, rpi-mailboxregs.h, rpi-asm-helpers.c/.h) are mostly a clean reorganisation; the only issues are the ARMv6 _get_core prototype mismatch (L12) and a couple of K&R-style empty () prototypes (L21).

---

Top-priority punch list

1. C4 — RISC-V copro OOB RAM access; reachable by any client ROM.
2. C1 — receiveString unbounded host-controlled writes; the most directly exploitable issue in the tree.
3. C2 — OPC7 ~4 MB OOB read/write; one-line fix (0xFFFFF → 0x7FFF).
4. C3 / H17 — armc-start.S _fast_scroll #0x4F→#0x3F and the stray + at line 285; two one-character fixes, the first causes runaway memory corruption.
5. H2 — doCmdFill infinite loop, duplicated into doCmdCrc and the debugger; fix the loop idiom in all four places.
6. H1 — time_t overflow corrupting every OSWORD &0E timestamp; switch to uint64_t.
7. H20 — the disassembler / debugger buffer-handling family; one shared template bug across ~13 files; fix once and propagate.
8. The FIQ-shared-state cluster (H10, M1, M2, M3, M4, L1) — newly more dangerous under -flto=auto; address as a coordinated change: volatile on all FIQ-shared state + consistent FIQ-masking critical sections + barriers around peripheral I/O and vector rewrites.
9. H18 — one-byte buffer overflow in OS_ReadLine_impl; reorder the bounds check.
10. H11 — OPC5/6/7/F100 NMI hang; drop NMI_BIT from the wake mask or ack it.

---

Coverage

Reviewed: the full indigo-dev src/ tree. Files personally read or grepped for verification: armc-start.S, tube-ula.c, info.c, copro-opc7.c, tube-commands.c, tube-swi.c, tube-lib.c/.h, tube-isr.c, framebuffer/primitives.c, opc6/opc6.c, copro-opc6.c, mc6809nc/mc6809_dis.c, lib6502.c, pdp11/pdp11_debug.c, CMakeLists.txt, plus the riscv/ directory listing. Four parallel sub-audits covered the new RISC-V copro + new infrastructure, the framebuffer, the bare-metal core infrastructure, and the ARM-native / SWI / debugger code.

The vendored CPU emulator cores and their disassembler / debug glue (mame/arm.c, NS32016/, cpu80186/, yaze/, lib6502.c, mc6809nc/, opc5ls/, opc6/, opc7/, pdp11/, f100/, 65816/, darm/) were audited at the integration-glue and memory-interface level, and every file with a non-trivial change in this branch was diffed line-by-line for newly-introduced defects. That pass found one real regression — H21 (lib6502.c jsr macro) — plus two minor display bugs (L22, L23) and one item to verify (L24). The remaining vendored-core changes in this branch are genuine fixes or cosmetic: the cpu80186 illegal-opcode default: case and the PUSH SP 8086-bug emulation, the NS32016 float→int-via-int32_t UB fix, the pdp11 DIV overflow-check rewrite, yaze/simz80.c macro-parenthesisation hygiene, and pervasive const-correctness. The new iop80186.c DMA emulation (x86_dma and the 0xFFC0-0xFFCB port handlers) is bounds-safe (addresses are confined to the 1 MB RAM buffer), though x86_dma transfers one byte per call with no internal transfer-count. The cores byte-identical to the prior branch (opc5ls/opc5ls.c, opc6/opc6.c, opc7/opc7.c, mc6809nc/mc6809.c, parts of NS32016/) carry their findings unchanged. Per-opcode bodies of the large cores were spot-checked, not exhaustively re-read. Embedded ROM blobs (tuberom_*.c, programs.c data, the pandora/ headers, Client86_v1_01.h) were not reviewed as code.

Not independently verified, flagged for maintainers: the -ffixed-r7 removal impact on the FIQ fast path (build note 5); whether BSS is still zeroed before kernel_main after armc-cstartup.c's removal.