ubsan and watchdogs for task_stack and kheap

Author: no382001

Makefile

@@ -8,9 +8,10 @@ CC = gcc
 GDB = gdb
 LD = ld -m elf_i386
 CFLAGSNO = -fno-pie -nostdlib -fno-builtin -nodefaultlibs -nostartfiles -Wno-error=comment
+USAN = -fsanitize=undefined -fno-sanitize=shift
 CFLAGS = -g -O0 -m32 -fno-pie -ffreestanding -nostdlib -fno-builtin -nodefaultlibs -nostartfiles -Werror -Wpedantic -Wall -Wextra -I$(shell pwd)
-
 NETWORKING = -netdev tap,id=my_tap0,ifname=tap0 -device rtl8139,netdev=my_tap0
+RUN = qemu-system-i386 -m 4 -serial stdio -kernel $(BUILD_DIR)/kernel.elf -drive file=disk/fat16.img,format=raw
 
 $(shell mkdir -p $(BUILD_DIR)/boot $(BUILD_DIR)/kernel $(BUILD_DIR)/drivers $(BUILD_DIR)/cpu $(BUILD_DIR)/libc $(BUILD_DIR)/apps $(BUILD_DIR)/net)
 
@@ -28,8 +29,11 @@ $(BUILD_DIR)/%.o: %.asm
 $(BUILD_DIR)/%.bin: %.asm
 	nasm $< -f bin -o $@
 
+net: all
+	$(RUN) $(NETWORKING)
 run: all
-	qemu-system-i386 -m 4 -serial stdio -kernel $(BUILD_DIR)/kernel.elf -drive file=disk/fat16.img,format=raw $(NETWORKING)
+	$(RUN)
+
 #################
 disk/fat16.img:
 	cd disk && ./make_disk.sh
@@ -46,7 +50,7 @@ bits:
 	find . -type f -name "*.h" 2>/dev/null | sed 's|^./||' | awk '{print "#include \"" $$0 "\""}' >> $(OUTPUT_FILE)
 
 debug: all
-	qemu-system-i386 -m 4 -serial stdio -s -S -kernel $(BUILD_DIR)/kernel.elf -drive file=disk/fat16.img,format=raw -netdev user,id=mynet0 -device rtl8139,netdev=mynet0
+	$(RUN) $(NETWORKING)
 
 attach:
 	${GDB} -ex "target remote localhost:1234" -ex "symbol-file $(BUILD_DIR)/kernel.elf"

TODO

@@ -34,17 +34,12 @@ build disk:
   on generation save the img, remove it from clean and keep backups
   when trying to generate new one, make a file comparator that shows the differences in file
 
-notes:
-- make sure that the function really takes va_list and not just ... bc its UB
-
 adjustments:
  - adjust the linker file, put heap on _end
  - inconsistent type usage
 
 multitaskin:
  - im not confident that semaphore actually does what it should, and the interruption logic could be improved with a better model like having priorities, placing currently io-blocked tasks to top-prio, right now its just a hack, wasting some cycles for sure
- - detach the timer from the scheduler put it on the separate 0x30 interrupt vecotr
- - wrapper so functions can return properly, completed state and de-schedule
  - signalling, recovery strategies
 
 forking should work now?
@@ -63,19 +58,15 @@ c9:
 - port the rtl8139, arp, dhcp, udp drivers
 https://github.com/szhou42/osdev/blob/master/src/kernel/network/udp.c#L12
 
-- change everythiung to debug serial
-- actually try it
 - the shit that i changed to use the stack, somewhere could crash, if phsy addresses get passed around like that contantly
-
-- maybe add canaries
-- maybe write some more tests
-
-https://os.phil-opp.com/allocator-designs/
-
+  - what?
 
 
 TODO:
   [x] fix the scheduler, provide a wrapper so fns can finish
-  watchdog for thread stack and kheap magic
-  thread the ehternet packets
-  add some log groups for debugging
+  [x] usan
+  [ ] asan
+  [x] watchdog for thread stack and kheap magic
+  [ ] more comprehensive thread reg/unreg
+  [ ] thread the ehternet packets
+  [ ] add some log groups for debugging

cpu/task.c

@@ -1,17 +1,20 @@
 
 #include "task.h"
+#include "apps/hexdump.h"
 #include "cpu/semaphore.h"
 #include "drivers/keyboard.h"
 #include "drivers/serial.h"
 #include "libc/mem.h"
 #include "libc/utils.h"
 
+/**/
 #undef serial_debug
 #define serial_debug(...)
+/**/
 
 volatile int current_task_idx = 0;
 volatile int active_tasks = 0;
-static task_t tasks[MAX_TASK] = {0};
+task_t tasks[MAX_TASK] = {0};
 
 void switch_stack_and_jump(uint32_t stack, uint32_t task);
 void switch_stack(uint32_t esp, uint32_t ebp);
@@ -54,26 +57,30 @@ void schedule(registers_t *regs) {
   if (current_task_idx >= active_tasks) {
     current_task_idx = 0;
   }
-  serial_debug("schedule pass from %s to %s", tasks[old_task_idx].name,
-               tasks[current_task_idx].name);
 
   task_t *task_to_run = &tasks[current_task_idx];
 
   if (task_to_run->status == TASK_WAITING_FOR_START) {
     task_to_run->status = TASK_RUNNING;
     switch_stack_and_jump(task_to_run->esp, (uint32_t)task_wrapper);
     return;
+  } else if (task_to_run->status != TASK_RUNNING) {
+
   } else if (task_to_run->status != TASK_RUNNING) {
     return; // skip this task
   }
+  serial_debug("schedule pass from %d:%s to %d:%s", old_task_idx,
+               tasks[old_task_idx].name, current_task_idx,
+               tasks[current_task_idx].name);
 
   memcpy(regs, &task_to_run->ctx, sizeof(registers_t));
 
   // no need for switch_stack - the interrupt return will handle it
 }
 
 bool check_stack_overflow(int task_id) {
-  void *stack_bottom = (void *)(tasks[task_id].ebp - tasks[task_id].stack_size);
+  void *stack_bottom =
+      (void *)(tasks[task_id].ebp - tasks[task_id].stack_size + 4);
   return (*(uint32_t *)stack_bottom != STACK_CANARY);
 }
 
@@ -86,11 +93,8 @@ void create_task(int task_id, char *name, task_function_t f, void *stack,
   memcpy(task.name, name, strlen(name));
   task.status = TASK_WAITING_FOR_START;
 
-  uint32_t stack_top = (uint32_t)stack - stack_size;
-  uint32_t stack_bottom = (uint32_t)stack; // leave space for canary
-
-  serial_debug("stack_top: %x, stack_bottom: %x, size: %d", stack_top,
-               stack_bottom, stack_size);
+  uint32_t stack_bottom = (uint32_t)stack;
+  uint32_t stack_top = (uint32_t)stack + stack_size - 4; // canary
 
   *(uint32_t *)stack_bottom = STACK_CANARY;
 
@@ -115,9 +119,32 @@ void create_task(int task_id, char *name, task_function_t f, void *stack,
   memcpy((void *)&tasks[task_id], (void *)&task, sizeof(task_t));
   active_tasks++;
 
+  serial_debug("task %d created: name='%s', status=%d, function=%x, "
+               "stack=%x-%x, esp=%x, ebp=%x, stack_size=%d, canary=%x",
+               task_id, task.name, task.status, (uint32_t)task.task,
+               stack_bottom, stack_top, task.esp, task.ebp, task.stack_size);
+  // hexdump(stack, 16, 8);
   asm volatile("sti");
 }
 
+void task_stack_check() {
+  while (1) {
+    // dont do anything with main
+    for (int i = 2; i < active_tasks; i++) {
+      if (check_stack_overflow(i)) {
+        serial_printff("stack overflow in task %d:%s ebp was at %x", i,
+                       tasks[i].name, tasks[i].ebp);
+        asm volatile("cli");
+        void *stack_bottom = (void *)(tasks[i].ebp - tasks[i].stack_size);
+        hexdump(stack_bottom, 64, 8);
+        while (1) {
+        }
+      }
+    }
+  }
+}
+
+void kheap_watchdog();
 void init_tasking() {
   memset(tasks, 0, sizeof(tasks));
 
@@ -127,4 +154,10 @@ void init_tasking() {
 
   current_task_idx = 0;
   active_tasks = 1;
+
+  void *ss = kmalloc(1024);
+  create_task(1, "task_stack_check", task_stack_check, ss, 1024);
+
+  void *s2 = kmalloc(5000);
+  create_task(2, "kheap_watchdog", kheap_watchdog, s2, 5000);
 }

kernel/kernel.c

@@ -33,7 +33,7 @@ void selftest() {
 
   uint32_t ss = 1024;
   void *fnstack = kmalloc_a(ss);
-  create_task(1, "sched_test", &sched_test_fn, fnstack, ss);
+  create_task(3, "sched_test", &sched_test_fn, fnstack, ss);
   int i = 1000000;
   while (1 != sem) {
     i--;
@@ -42,6 +42,7 @@ void selftest() {
     }
   }
   kfree(fnstack);
+  serial_debug("selftest finished!");
 }
 
 void kernel_main(void) {

libc/heap.c

@@ -5,6 +5,9 @@
 // https://git.9front.org/plan9front/plan9front/0861d0d0f283a9917721214fa3dc1c51a778213d/sys/src/9/port/xalloc.c/f.html
 // i have no idea what license it is
 
+#undef serial_printff
+#define serial_printff(...)
+
 #define nil 0
 #define nelem(x) (sizeof(x) / sizeof((x)[0]))
 
@@ -314,3 +317,123 @@ void *kmalloc_ap(uint32_t size, uint32_t *phys) {
 }
 
 void *kmalloc(uint32_t size) { return kmalloc_int(size, 0, 0); }
+
+void kheap_watchdog() {
+  serial_debug("heap watchdog started");
+
+  while (1) {
+
+    hole_t *h;
+    int free_chunks = 0;
+    uintptr_t free_memory = 0;
+    bool free_list_corrupted = false;
+
+    for (h = xlists.table; h != NULL; h = h->link) {
+      free_chunks++;
+      free_memory += h->size;
+
+      if (h->addr + h->size != h->top) {
+        serial_printff("FREE LIST CORRUPTION: Hole at %x has inconsistent "
+                       "bounds (addr=%x, size=%d, top=%x)",
+                       (uintptr_t)h, h->addr, h->size, h->top);
+        free_list_corrupted = true;
+      }
+
+      if (h->link != NULL &&
+          ((uintptr_t)h->link < KHEAP_START ||
+           (uintptr_t)h->link >= KHEAP_START + KHEAP_INITIAL_SIZE)) {
+        serial_printff("FREE LIST CORRUPTION: Hole at %x has invalid link %x",
+                       (uintptr_t)h, (uintptr_t)h->link);
+        free_list_corrupted = true;
+      }
+    }
+
+// create a map of free regions to avoid checking them for allocated blocks
+#define MAX_FREE_REGIONS 32
+    struct {
+      uintptr_t start;
+      uintptr_t end;
+    } free_regions[MAX_FREE_REGIONS];
+    int free_region_count = 0;
+
+    for (h = xlists.table; h != NULL && free_region_count < MAX_FREE_REGIONS;
+         h = h->link) {
+      free_regions[free_region_count].start = KHEAP_START + h->addr;
+      free_regions[free_region_count].end = KHEAP_START + h->top;
+      free_region_count++;
+    }
+
+    uintptr_t current_addr = KHEAP_START;
+    int corrupted_blocks = 0;
+    int valid_blocks = 0;
+    uintptr_t allocated_memory = 0;
+
+    while (current_addr < KHEAP_START + KHEAP_INITIAL_SIZE) {
+      // check if current address is in a free region
+      bool in_free_region = false;
+      for (int i = 0; i < free_region_count; i++) {
+        if (current_addr >= free_regions[i].start &&
+            current_addr < free_regions[i].end) {
+          // skip to the end of this free region
+          current_addr = free_regions[i].end;
+          in_free_region = true;
+          break;
+        }
+      }
+
+      if (in_free_region) {
+        continue;
+      }
+
+      block_header_t *block = (block_header_t *)current_addr;
+
+      // Basic validation for what might be a block header
+      if (block->size >= sizeof(block_header_t) &&
+          block->size < 65535 && // Max reasonable block size for uint16_t
+          (current_addr + block->size) <= (KHEAP_START + KHEAP_INITIAL_SIZE)) {
+
+        if (block->magix == (uint16_t)magichole) {
+          // this looks like a valid block
+          valid_blocks++;
+          allocated_memory += block->size;
+
+          current_addr += block->size;
+        } else {
+          if ((block->size & 0x3) == 0 && // size should be aligned to 4 bytes
+              block->size > 0) {
+
+            serial_printff("POSSIBLE CORRUPTED BLOCK at %x: size=%d, magix=%x "
+                           "(expected %x)",
+                           current_addr, block->size, block->magix,
+                           (uint16_t)magichole);
+
+            hexdump((const char *)current_addr - 16, 64, 16);
+            corrupted_blocks++;
+
+            current_addr += block->size;
+          } else {
+            current_addr += 4;
+          }
+        }
+      } else {
+        current_addr += 4;
+      }
+    }
+
+    serial_printff("heap watchdog stats: %d valid blocks (%d bytes), %d "
+                   "corrupted blocks, %d free holes (%d bytes)",
+                   valid_blocks, allocated_memory, corrupted_blocks,
+                   free_chunks, free_memory);
+
+    if (free_list_corrupted) {
+      serial_printff("WARNING: Free list integrity check failed!");
+    }
+
+    serial_printff("heap summary from xlists:");
+    xsummary();
+
+    for (volatile int i = 0; i < 5000000; i++) {
+      asm volatile("nop");
+    }
+  }
+}

libc/mem.c

@@ -7,18 +7,23 @@ void *memcpy(void *dest, const void *src, size_t num_bytes) {
   uint8_t *d = (uint8_t *)dest;
   const uint8_t *s = (const uint8_t *)src;
 
+  // align destination to 4 bytes if possible
   while (((uintptr_t)d % 4) && num_bytes) {
     *d++ = *s++;
     num_bytes--;
   }
 
-  while (num_bytes >= 4) {
-    *((uint32_t *)d) = *((uint32_t *)s);
-    d += 4;
-    s += 4;
-    num_bytes -= 4;
+  // use 4-byte copies if both pointers are aligned
+  if (((uintptr_t)s % 4) == 0) {
+    while (num_bytes >= 4) {
+      *((uint32_t *)d) = *((uint32_t *)s);
+      d += 4;
+      s += 4;
+      num_bytes -= 4;
+    }
   }
 
+  // byte-by-byte for remaining or unaligned portions
   while (num_bytes--) {
     *d++ = *s++;
   }

libc/page.c

@@ -47,7 +47,7 @@ static uint32_t first_frame() { // free frame
     {
       // at least one bit is free
       for (j = 0; j < 32; j++) {
-        uint32_t toTest = 0x1 << j;
+        uint32_t toTest = 0x1 << j; // TODO shift out of bounds
         if (!(frames[i] & toTest)) {
           return (i * 4 * 8 + j);
         }