fix(argv): fix double page offset bug in write_byte_to_stack

Fixed a critical bug where argc/argv was being written to the wrong
physical address when setting up the stack for new processes.

The bug was in write_byte_to_stack(): translate_page() returns the
FULL physical address (frame base + page offset), but we were then
adding page_offset again, causing the write to go to frame + 2*offset
instead of frame + offset.

When the process ran and read from the correct virtual address, the
MMU translated to frame + offset, which was all zeros because we
never wrote there.

Changes:
- kernel/src/process/manager.rs: Remove duplicate page_offset addition
- userspace/tests/cat.rs: Use naked _start to properly capture RSP
- xtask/src/main.rs: Add cat /hello.txt test to interactive tests
- kernel/src/memory/stack.rs: Add stack bounds tests with proper >= check
- Add exec_stack_argv_test for regression testing

Fixes `cat /hello.txt` showing "missing file operand" when argc was
correctly set to 2 by the kernel.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Author: ryanbreen

kernel/src/main.rs

@@ -727,6 +727,10 @@ fn kernel_main_continue() -> ! {
         log::info!("=== EXEC TEST: exec with argv ===");
         test_exec::test_exec_argv();
 
+        // Test exec with stack-allocated argv (regression test for black_box fix)
+        log::info!("=== EXEC TEST: exec with stack-allocated argv ===");
+        test_exec::test_exec_stack_argv();
+
         // Test getdents64 syscall for directory listing
         log::info!("=== FS TEST: getdents64 directory listing ===");
         test_exec::test_getdents();

kernel/src/memory/stack.rs

@@ -115,6 +115,14 @@ impl GuardedStack {
     }
 
     /// Find free virtual address space for stack allocation
+    ///
+    /// This function uses a simple incrementing allocator. The bounds checking is
+    /// performed BEFORE modifying state, ensuring that on error, the allocation
+    /// pointer is not corrupted.
+    ///
+    /// The bounds check uses `>=` (not `>`) because the boundary addresses are
+    /// non-canonical: USER_STACK_REGION_END (0x8000_0000_0000) is the first
+    /// non-canonical address in the lower half of the address space.
     fn find_free_virtual_space(
         size: usize,
         privilege: ThreadPrivilege,
@@ -127,26 +135,21 @@ impl GuardedStack {
         unsafe {
             match privilege {
                 ThreadPrivilege::User => {
-                    // Check bounds BEFORE allocating to prevent overflow
-                    // USER_STACK_REGION_END is the canonical boundary (0x8000_0000_0000)
-                    // We need to ensure the entire stack fits STRICTLY BELOW the boundary
-                    // because 0x8000_0000_0000 is non-canonical
-                    let proposed_end = NEXT_USER_STACK_ADDR.saturating_add(size as u64);
-                    if proposed_end >= USER_STACK_REGION_END || proposed_end < NEXT_USER_STACK_ADDR {
-                        return Err("Out of virtual address space for user stacks");
-                    }
+                    // Use the extracted bounds check function to verify allocation is valid
+                    // This check happens BEFORE modifying NEXT_USER_STACK_ADDR
+                    let proposed_end = check_user_stack_bounds(NEXT_USER_STACK_ADDR, size as u64)?;
 
+                    // Bounds check passed - now safe to update state
                     let addr = VirtAddr::new(NEXT_USER_STACK_ADDR);
                     NEXT_USER_STACK_ADDR = proposed_end;
                     Ok(addr)
                 }
                 ThreadPrivilege::Kernel => {
-                    // Check bounds BEFORE allocating
-                    let proposed_end = NEXT_KERNEL_STACK_ADDR.saturating_add(size as u64);
-                    if proposed_end >= 0xFFFF_CA00_0000_0000 || proposed_end < NEXT_KERNEL_STACK_ADDR {
-                        return Err("Out of virtual address space for kernel stacks");
-                    }
+                    // Use the extracted bounds check function to verify allocation is valid
+                    // This check happens BEFORE modifying NEXT_KERNEL_STACK_ADDR
+                    let proposed_end = check_kernel_stack_bounds(NEXT_KERNEL_STACK_ADDR, size as u64)?;
 
+                    // Bounds check passed - now safe to update state
                     let addr = VirtAddr::new(NEXT_KERNEL_STACK_ADDR);
                     NEXT_KERNEL_STACK_ADDR = proposed_end;
                     Ok(addr)
@@ -259,3 +262,289 @@ pub fn is_guard_page_fault(fault_addr: VirtAddr) -> Option<&'static GuardedStack
     }
     None
 }
+
+// ============================================================================
+// Bounds Check Logic - Extracted for Testing
+// ============================================================================
+//
+// The bounds checking logic is critical for preventing:
+// 1. Stack allocations that would extend into non-canonical address space
+// 2. Integer overflow when computing proposed_end
+// 3. Off-by-one errors at the boundary (>= not > to prevent allocation AT boundary)
+//
+// These helper functions extract the pure logic for unit testing.
+
+/// Check if a proposed user stack allocation is valid.
+///
+/// This function encapsulates the bounds checking logic used by `find_free_virtual_space`
+/// for user stacks. It verifies:
+/// 1. The proposed_end doesn't reach or exceed USER_STACK_REGION_END
+/// 2. Integer overflow didn't occur (saturating_add wraparound detection)
+///
+/// # Arguments
+/// * `current_addr` - The current allocation pointer (where the next stack would start)
+/// * `size` - The size of the stack allocation (including guard page)
+///
+/// # Returns
+/// * `Ok(proposed_end)` - The end address if allocation would be valid
+/// * `Err(&'static str)` - Error message if allocation would be invalid
+#[inline]
+pub fn check_user_stack_bounds(current_addr: u64, size: u64) -> Result<u64, &'static str> {
+    let proposed_end = current_addr.saturating_add(size);
+
+    // Check 1: Would the allocation extend to or past the boundary?
+    // We use >= because USER_STACK_REGION_END (0x8000_0000_0000) is non-canonical
+    // and we cannot allocate AT that address
+    if proposed_end >= USER_STACK_REGION_END {
+        return Err("Out of virtual address space for user stacks");
+    }
+
+    // Check 2: Did saturating_add wrap around (overflow)?
+    // If proposed_end < current_addr, we overflowed
+    if proposed_end < current_addr {
+        return Err("Out of virtual address space for user stacks");
+    }
+
+    Ok(proposed_end)
+}
+
+/// Check if a proposed kernel stack allocation is valid.
+///
+/// Similar to `check_user_stack_bounds` but for kernel stack region.
+/// The kernel stack region ends at 0xFFFF_CA00_0000_0000.
+///
+/// # Arguments
+/// * `current_addr` - The current allocation pointer
+/// * `size` - The size of the stack allocation
+///
+/// # Returns
+/// * `Ok(proposed_end)` - The end address if allocation would be valid
+/// * `Err(&'static str)` - Error message if allocation would be invalid
+#[inline]
+pub fn check_kernel_stack_bounds(current_addr: u64, size: u64) -> Result<u64, &'static str> {
+    const KERNEL_STACK_REGION_END: u64 = 0xFFFF_CA00_0000_0000;
+
+    let proposed_end = current_addr.saturating_add(size);
+
+    if proposed_end >= KERNEL_STACK_REGION_END {
+        return Err("Out of virtual address space for kernel stacks");
+    }
+
+    if proposed_end < current_addr {
+        return Err("Out of virtual address space for kernel stacks");
+    }
+
+    Ok(proposed_end)
+}
+
+// ============================================================================
+// Unit Tests for Stack Allocation Bounds Checking
+// ============================================================================
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    // Test constants - these mirror the actual values from layout.rs
+    const TEST_USER_STACK_REGION_START: u64 = 0x7FFF_FF00_0000;
+    const TEST_USER_STACK_REGION_END: u64 = 0x8000_0000_0000;
+    const TEST_KERNEL_STACK_REGION_START: u64 = 0xFFFF_C900_0000_0000;
+    const TEST_KERNEL_STACK_REGION_END: u64 = 0xFFFF_CA00_0000_0000;
+
+    // Standard stack sizes for testing
+    const PAGE_SIZE: u64 = 4096;
+    const STACK_64K: u64 = 64 * 1024;  // 64 KiB stack
+    const STACK_WITH_GUARD: u64 = STACK_64K + PAGE_SIZE;  // Stack + guard page
+
+    // ========================================================================
+    // User Stack Bounds Check Tests
+    // ========================================================================
+
+    #[test]
+    fn test_user_stack_bounds_normal_allocation() {
+        // Normal allocation well within bounds should succeed
+        let current = TEST_USER_STACK_REGION_START;
+        let size = STACK_WITH_GUARD;
+
+        let result = check_user_stack_bounds(current, size);
+        assert!(result.is_ok(), "Normal allocation should succeed");
+
+        let proposed_end = result.unwrap();
+        assert_eq!(proposed_end, current + size);
+        assert!(proposed_end < TEST_USER_STACK_REGION_END);
+    }
+
+    #[test]
+    fn test_user_stack_bounds_exactly_at_boundary_fails() {
+        // Allocation that would end EXACTLY at the boundary should FAIL
+        // because >= check means we can't allocate AT 0x8000_0000_0000
+        let remaining_space = TEST_USER_STACK_REGION_END - TEST_USER_STACK_REGION_START;
+        let current = TEST_USER_STACK_REGION_START;
+        let size = remaining_space;  // Would put proposed_end exactly at boundary
+
+        let result = check_user_stack_bounds(current, size);
+        assert!(result.is_err(), "Allocation exactly at boundary should fail");
+        assert_eq!(result.unwrap_err(), "Out of virtual address space for user stacks");
+    }
+
+    #[test]
+    fn test_user_stack_bounds_one_byte_before_boundary_succeeds() {
+        // Allocation that ends one byte BEFORE the boundary should succeed
+        let remaining_space = TEST_USER_STACK_REGION_END - TEST_USER_STACK_REGION_START;
+        let current = TEST_USER_STACK_REGION_START;
+        let size = remaining_space - 1;  // One byte less than boundary
+
+        let result = check_user_stack_bounds(current, size);
+        assert!(result.is_ok(), "Allocation one byte before boundary should succeed");
+
+        let proposed_end = result.unwrap();
+        assert_eq!(proposed_end, TEST_USER_STACK_REGION_END - 1);
+    }
+
+    #[test]
+    fn test_user_stack_bounds_overflow_protection() {
+        // Test that integer overflow is caught
+        // Start near the end and try to allocate a huge size
+        let current = TEST_USER_STACK_REGION_END - PAGE_SIZE;  // Near the end
+        let size = u64::MAX;  // Huge size that would overflow
+
+        let result = check_user_stack_bounds(current, size);
+        assert!(result.is_err(), "Overflow should be caught");
+    }
+
+    #[test]
+    fn test_user_stack_bounds_past_boundary_fails() {
+        // Allocation that would extend past the boundary should fail
+        let remaining_space = TEST_USER_STACK_REGION_END - TEST_USER_STACK_REGION_START;
+        let current = TEST_USER_STACK_REGION_START;
+        let size = remaining_space + PAGE_SIZE;  // Past the boundary
+
+        let result = check_user_stack_bounds(current, size);
+        assert!(result.is_err(), "Allocation past boundary should fail");
+    }
+
+    #[test]
+    fn test_user_stack_bounds_state_not_modified_on_error() {
+        // This test verifies the SEMANTIC requirement that state is not modified on error.
+        // Since check_user_stack_bounds is a pure function (no side effects),
+        // this test validates that design: the function returns Result without
+        // modifying any external state.
+        //
+        // The actual find_free_virtual_space function only updates NEXT_USER_STACK_ADDR
+        // AFTER the bounds check passes, ensuring state consistency on error.
+
+        let current = TEST_USER_STACK_REGION_START;
+        let size = u64::MAX;  // Would overflow
+
+        // Call the function multiple times with invalid input
+        let result1 = check_user_stack_bounds(current, size);
+        let result2 = check_user_stack_bounds(current, size);
+
+        // Both should return the same error
+        assert!(result1.is_err());
+        assert!(result2.is_err());
+        assert_eq!(result1.unwrap_err(), result2.unwrap_err());
+    }
+
+    #[test]
+    fn test_user_stack_bounds_consecutive_allocations() {
+        // Simulate consecutive allocations until exhaustion
+        let mut current = TEST_USER_STACK_REGION_START;
+        let size = STACK_WITH_GUARD;
+        let mut allocation_count = 0;
+
+        loop {
+            match check_user_stack_bounds(current, size) {
+                Ok(proposed_end) => {
+                    allocation_count += 1;
+                    current = proposed_end;
+                    // Safety: don't run forever
+                    if allocation_count > 10000 {
+                        break;
+                    }
+                }
+                Err(_) => break,
+            }
+        }
+
+        // Verify we made some allocations before exhaustion
+        assert!(allocation_count > 0, "Should have made at least one allocation");
+
+        // Verify we stopped before or at the boundary
+        assert!(current <= TEST_USER_STACK_REGION_END,
+                "Current pointer should not exceed boundary");
+    }
+
+    // ========================================================================
+    // Kernel Stack Bounds Check Tests
+    // ========================================================================
+
+    #[test]
+    fn test_kernel_stack_bounds_normal_allocation() {
+        let current = TEST_KERNEL_STACK_REGION_START;
+        let size = STACK_WITH_GUARD;
+
+        let result = check_kernel_stack_bounds(current, size);
+        assert!(result.is_ok(), "Normal kernel allocation should succeed");
+    }
+
+    #[test]
+    fn test_kernel_stack_bounds_at_boundary_fails() {
+        let remaining_space = TEST_KERNEL_STACK_REGION_END - TEST_KERNEL_STACK_REGION_START;
+        let current = TEST_KERNEL_STACK_REGION_START;
+        let size = remaining_space;
+
+        let result = check_kernel_stack_bounds(current, size);
+        assert!(result.is_err(), "Kernel allocation at boundary should fail");
+    }
+
+    // ========================================================================
+    // Edge Case Tests
+    // ========================================================================
+
+    #[test]
+    fn test_zero_size_allocation() {
+        // Zero-size allocation should technically succeed (proposed_end == current)
+        let current = TEST_USER_STACK_REGION_START;
+        let size = 0;
+
+        let result = check_user_stack_bounds(current, size);
+        assert!(result.is_ok(), "Zero-size allocation should succeed");
+        assert_eq!(result.unwrap(), current);
+    }
+
+    #[test]
+    fn test_single_page_allocation() {
+        let current = TEST_USER_STACK_REGION_START;
+        let size = PAGE_SIZE;
+
+        let result = check_user_stack_bounds(current, size);
+        assert!(result.is_ok());
+        assert_eq!(result.unwrap(), current + PAGE_SIZE);
+    }
+
+    #[test]
+    fn test_max_valid_allocation() {
+        // Maximum valid allocation: fills entire region minus 1 byte
+        let current = TEST_USER_STACK_REGION_START;
+        let max_size = TEST_USER_STACK_REGION_END - TEST_USER_STACK_REGION_START - 1;
+
+        let result = check_user_stack_bounds(current, max_size);
+        assert!(result.is_ok(), "Max valid allocation should succeed");
+        assert_eq!(result.unwrap(), TEST_USER_STACK_REGION_END - 1);
+    }
+
+    #[test]
+    fn test_boundary_address_is_non_canonical() {
+        // Verify that USER_STACK_REGION_END is indeed the canonical boundary
+        // 0x8000_0000_0000 is the first non-canonical address in the lower half
+        assert_eq!(TEST_USER_STACK_REGION_END, 0x8000_0000_0000);
+
+        // Any allocation that would reach this address must fail
+        let current = TEST_USER_STACK_REGION_END - PAGE_SIZE;
+        let size = PAGE_SIZE;  // Would end exactly at boundary
+
+        let result = check_user_stack_bounds(current, size);
+        assert!(result.is_err(), "Allocation reaching boundary must fail");
+    }
+}

kernel/src/process/manager.rs

@@ -1966,17 +1966,15 @@ impl ProcessManager {
         value: u8,
     ) -> Result<(), &'static str> {
         // Translate virtual address to physical
+        // NOTE: translate_page uses translate_addr which returns the FULL physical
+        // address including the page offset - do NOT add page_offset again!
         let phys_addr = page_table.translate_page(VirtAddr::new(virt_addr))
             .ok_or("Failed to translate stack address")?;
 
-        // Calculate offset within page
-        let page_offset = virt_addr & 0xFFF;
-        let phys_with_offset = phys_addr + page_offset;
-
         // Write via direct physical memory mapping
         // The kernel has a direct mapping of all physical memory
         let phys_offset = crate::memory::physical_memory_offset();
-        let kernel_virt = phys_offset + phys_with_offset.as_u64();
+        let kernel_virt = phys_offset + phys_addr.as_u64();
 
         unsafe {
             core::ptr::write_volatile(kernel_virt.as_mut_ptr::<u8>(), value);