Bugfix #560: false positive crash on out-of-bounds memory writes

[m3y54m]

Description

Fixes #560 - Prevents false positive crashes when out-of-bounds memory writes wrap into the low register address range.

Problem

The avr_core_watch_write() function was checking for low register access (< 32) after wrapping out-of-bounds addresses via modulo arithmetic. This caused legitimate out-of-bounds writes to be misreported as "low register" violations after address wrapping.

Solution

Reordered the validation checks to test for low register access before any address modification, ensuring only genuine low register writes trigger the error.

Changes

• Moved low register check before address wrapping in simavr/sim/sim_core.c
• Prevents false positive crashes while maintaining proper validation

Impact

Doesn't crash and continues execution correctly

[m3y54m]

Updated my proposed fix:

1. Removed addr = addr % (avr->ramend + 1) which incorrectly wrapped out-of-bounds addresses
2. Removed if (addr < 32) check that could only trigger after wrapping
3. Out-of-bounds writes/reads now return immediately (write ignored, read returns 0) matching real AVR hardware behavior

This change makes simavr match real hardware behavior where out-of-bounds memory access is silently ignored.

[m3y54m]

Update

Root Cause

• Issue 1: Incorrect wrapping formula (addr % (ramend+1)) wrapped to registers instead of RAM
• Issue 2: Impossible safety check (addr < 32) that could never trigger normally, only after bad wrapping
• Result: False positive crashes

Proposed Fix

• Correct wrapping formula: ramstart + ((addr - ramstart) % ramsize)
• Wraps within RAM space (0x0100-0x08FF), not into registers

[m3y54m]

Verification

I verified my fix with the following program, which confirms that simavr now behaves identically to the real hardware:

#include <avr/io.h>
#include <stdio.h>

#define F_CPU 16000000
#define BAUD 9600
#include <util/setbaud.h>
#include <util/delay.h>

static void init_uart(void) {
  UBRR0 = UBRR_VALUE;
  UCSR0A = (USE_2X << U2X0);
  UCSR0B = _BV(TXEN0);
}

static int uart_putchar(char c, FILE *stream) {
  if (c == '\n')
    uart_putchar('\r', stream);
  loop_until_bit_is_set(UCSR0A, UDRE0);
  UDR0 = c;
  return 0;
}

static FILE mystdout = FDEV_SETUP_STREAM(uart_putchar, NULL, _FDEV_SETUP_WRITE);

int main(void) {
  init_uart();
  stdout = &mystdout;
  
  printf("=== Test Start ===\n");
  printf("RAMEND = 0x%04x\n", RAMEND);
  
  // Get initial state
  uint16_t sp_before = SP;
  
  printf("BEFORE:\n");
  printf("  SP = 0x%04x\n", sp_before);
  
  // ATmega328p: ramend = 0x08ff, ramstart = 0x0100
  // Clear RAMSTART to ensure clean test
  volatile uint8_t *ramstart_ptr = (volatile uint8_t *)0x0100;
  *ramstart_ptr = 0x00;
  uint8_t ramstart_before = *ramstart_ptr;
  printf("  RAMSTART (0x0100) = 0x%02x\n", ramstart_before);
  
  // Writing to 0x0900 (RAMEND+1)
  printf("\nWriting 0x42 to 0x0900 (RAMEND+1)...\n");
  
  // Do the write and r0 check in a single asm block to avoid clobbering
  uint8_t r0_after;
  asm volatile(
    "clr r0\n"                     // Clear r0
    "sts 0x0900, %1\n"            // Write 0x42 to 0x0900 (RAMEND+1)
    "mov %0, r0\n"                // Read r0 back
    : "=r" (r0_after)
    : "r" ((uint8_t)0x42)
    : "memory"
  );
  
  // Get state after write
  uint16_t sp_after = SP;
  uint8_t ramstart_after = *ramstart_ptr;
  
  printf("\nAFTER:\n");
  printf("  SP = 0x%04x\n", sp_after);
  printf("  r0 = 0x%02x\n", r0_after);
  printf("  RAMSTART (0x0100) = 0x%02x\n", ramstart_after);
  
  printf("\nANALYSIS:\n");
  printf("  SP changed: %s\n", (sp_before != sp_after) ? "YES" : "NO");
  printf("  RAMSTART changed: %s\n", (ramstart_before != ramstart_after) ? "YES" : "NO");
  
  if (r0_after == 0x42) {
    printf("  Result: WRAPPED to r0 (address 0x0000)\n");
  } else if (ramstart_after == 0x42) {
    printf("  Result: WRAPPED to RAMSTART (address 0x0100)\n");
  } else if (r0_after == 0x00 && ramstart_after == 0x00) {
    printf("  Result: NO WRAP (write ignored)\n");
  } else {
    printf("  Result: UNEXPECTED (r0=0x%02x, RAMSTART=0x%02x)\n", r0_after, ramstart_after);
  }
  
  // Print something to prove mcu has not crashed
  printf("=== Test Complete ===\n");

  while(1);
}

Test result

Real hardware (atmega328p 16MHz)

=== Test Start ===
RAMEND = 0x08ff
BEFORE:
  SP = 0x08f9
  RAMSTART (0x0100) = 0x00

Writing 0x42 to 0x0900 (RAMEND+1)...

AFTER:
  SP = 0x08f1
  r0 = 0x00
  RAMSTART (0x0100) = 0x42

ANALYSIS:
  SP changed: YES
  RAMSTART changed: YES
  Result: WRAPPED to RAMSTART (address 0x0100)
=== Test Complete ===

simavr (latest master)

Loaded 2504 bytes of Flash data at 0
=== Test Start ===..
RAMEND = 0x08ff..
BEFORE:..
  SP = 0x08f9..
  RAMSTART (0x0100) = 0x00..
..
Writing 0x42 to 0x0900 (RAMEND+1).....
CORE: *** Invalid write address PC=0148 SP=08f1 O=93c0 Address 0000=42 low registers
avr_sadly_crashed
avr_gdb_init listening on port 1234

simavr (latest master + my fix)

Loaded 2504 bytes of Flash data at 0
=== Test Start ===..
RAMEND = 0x08ff..
BEFORE:..
  SP = 0x08f9..
  RAMSTART (0x0100) = 0x00..
..
Writing 0x42 to 0x0900 (RAMEND+1).....
..
AFTER:..
  SP = 0x08f1..
  r0 = 0x00..
  RAMSTART (0x0100) = 0x42..
..
ANALYSIS:..
  SP changed: YES..
  RAMSTART changed: YES..
  Result: WRAPPED to RAMSTART (address 0x0100)..
=== Test Complete ===..

[edgar-bonet]

LGTM!

[gatk555]

Can it be squashed to a single commit? This site offers that as an option when merging, but I do not know what it does to comments.

[m3y54m]

@gatk555 Done