fix(thread-safety): Address TOCTOU races in global state initializati…

.claude/settings.local.json

@@ -13,7 +13,8 @@
       "Bash(grep -r \"#include\" /c/projects/winkernellite/include/*.h)",
       "Bash(grep -r \"^#ifndef\\\\|^#define\" /c/projects/winkernellite/include/*.h)",
       "Bash(grep:*)",
-      "Bash(./build/bin/runTests.exe)"
+      "Bash(./build/bin/runTests.exe)",
+      "Bash(git branch:*)"
     ]
   }
 }

include/Resource.h

@@ -82,12 +82,12 @@ extern "C" {
 /* Global critical region counter (protected by its own critical section) */
 extern LONG g_WinKernelLite_KernelApcDisableCount;
 extern CRITICAL_SECTION g_WinKernelLite_KernelApcDisableLock;
-extern BOOLEAN g_WinKernelLite_KernelApcDisableLockInitialized;
+extern INIT_ONCE g_WinKernelLite_KernelApcDisableInitOnce;
 
 /* Global system resources list */
 extern LIST_ENTRY g_WinKernelLite_SystemResourcesList;
 extern CRITICAL_SECTION g_WinKernelLite_SystemResourcesLock;
-extern BOOLEAN g_WinKernelLite_SystemResourcesInitialized;
+extern INIT_ONCE g_WinKernelLite_SystemResourcesInitOnce;
 
 #define IsOwnedExclusive(R) ((R)->Flag & ResourceOwnedExclusive)
 

openspec/changes/fix-global-state-thread-safety/.openspec.yaml

@@ -0,0 +1,2 @@
+schema: spec-driven
+created: 2026-03-22

openspec/changes/fix-global-state-thread-safety/design.md

@@ -0,0 +1,60 @@
+## Context
+
+Three separate initialization paths in WinKernelLite use check-then-act patterns without synchronization. All are in `.c` files after the recent refactor, making them straightforward to fix.
+
+**Pattern 1 - Pointer initialization** (`GetGlobalState`):
+```c
+if (g_WinKernelLite_GlobalState == NULL) {
+    temp = HeapAlloc(...);
+    // ... initialize temp ...
+    g_WinKernelLite_GlobalState = temp;  // RACE: two threads both write
+}
+```
+
+**Pattern 2 - Boolean-guarded one-time init** (`EnsureSystemResourcesListInitialized`, `KeEnterCriticalRegion`, etc.):
+```c
+if (!g_Initialized) {
+    InitializeCriticalSection(&g_Lock);  // RACE: double init
+    g_Initialized = TRUE;
+}
+```
+
+## Goals / Non-Goals
+
+**Goals:**
+- Eliminate all TOCTOU races in global state initialization
+- Lock-free on the fast path (after initialization completes)
+- Use Windows-native primitives only
+
+**Non-Goals:**
+- Making heap tracking operations (TrackAllocation, UntrackAllocation, etc.) thread-safe - that is a separate change
+- Adding thread safety to debug state initialization (Debug.h/Debug.c) - separate change
+
+## Decisions
+
+### 1. Use InterlockedCompareExchangePointer for GetGlobalState
+
+**Decision:** Allocate and fully initialize a `GLOBAL_STATE` into a local temp pointer, then atomically CAS it into `g_WinKernelLite_GlobalState`. If CAS fails (another thread won the race), free the temp.
+
+**Rationale:** This is the standard Windows pattern for lazy pointer initialization. It's lock-free, requires no additional global state, and the fast path is a single pointer read. The kernel itself uses this pattern (`ExInterlockedCompareExchangePointer`).
+
+**Alternative considered:** `InitOnceExecuteOnce` - works but adds an `INIT_ONCE` variable and a callback function for what is fundamentally a pointer-swap operation. CAS is simpler here.
+
+### 2. Use InitOnceExecuteOnce for CRITICAL_SECTION initialization
+
+**Decision:** Replace the boolean flags (`g_WinKernelLite_KernelApcDisableLockInitialized`, `g_WinKernelLite_SystemResourcesInitialized`) with `INIT_ONCE` variables. Use `InitOnceExecuteOnce` to guarantee exactly-once initialization.
+
+**Rationale:** `InitializeCriticalSection` cannot be safely called twice on the same `CRITICAL_SECTION`. `INIT_ONCE` is the Windows-blessed mechanism for this exact scenario. It handles all synchronization internally and the fast path (after init) is a single interlocked read.
+
+**Alternative considered:** Manual CAS on the boolean flag - fragile because `InitializeCriticalSection` can throw an exception, leaving the flag in an inconsistent state. `InitOnceExecuteOnce` handles errors correctly.
+
+### 3. Remove boolean flag globals, add INIT_ONCE globals
+
+**Decision:** Replace the 2 boolean globals (`g_WinKernelLite_KernelApcDisableLockInitialized`, `g_WinKernelLite_SystemResourcesInitialized`) with 2 `INIT_ONCE` globals. Initialize them with `INIT_ONCE_STATIC_INIT` (zero-init, no runtime constructor needed).
+
+**Rationale:** Cleaner API surface, removes the race-prone boolean pattern entirely.
+
+## Risks / Trade-offs
+
+- **[Risk] CAS loop on high contention** -> Mitigation: In practice, `GetGlobalState` is called once early in process lifetime. The losing thread just frees its allocation. No spin loop.
+- **[Risk] InitOnceExecuteOnce callback complexity** -> Mitigation: Callbacks are trivial 3-line functions. The alternative (manual CAS + exception handling) is more complex and error-prone.

openspec/changes/fix-global-state-thread-safety/proposal.md

@@ -0,0 +1,29 @@
+## Why
+
+Code review identified TOCTOU (Time-of-Check-Time-of-Use) race conditions in all global state initialization paths. Multiple threads calling `GetGlobalState()`, `EnsureSystemResourcesListInitialized()`, or `KeEnterCriticalRegion()` simultaneously can double-initialize, leaking memory or corrupting state. Now that these implementations are in `.c` files (after the refactor-headers-to-source change), we can apply proper Windows synchronization primitives.
+
+## What Changes
+
+- Fix `GetGlobalState()` in `src/KernelHeap.c`: use `InterlockedCompareExchangePointer` to atomically publish the global state pointer. Allocate to a temp variable, initialize fully, then CAS into the global. If CAS fails (another thread won), free the temp.
+- Fix `EnsureSystemResourcesListInitialized()` in `src/Resource.c`: replace the `BOOLEAN` flag check with `InitOnceExecuteOnce` using a static `INIT_ONCE` variable.
+- Fix `KeEnterCriticalRegion()`, `KeLeaveCriticalRegion()`, and `GetKernelApcDisableCount()` in `src/Resource.c`: replace the `BOOLEAN` flag check with `InitOnceExecuteOnce` using a static `INIT_ONCE` variable.
+- Remove the now-unnecessary `g_WinKernelLite_KernelApcDisableLockInitialized` and `g_WinKernelLite_SystemResourcesInitialized` boolean flags from `Resource.h` and `Resource.c`.
+
+## Capabilities
+
+### New Capabilities
+- `thread-safe-init`: Requirements for thread-safe one-time initialization of global state
+
+### Modified Capabilities
+<!-- No existing spec-level behavior changes. -->
+
+## Impact
+
+- **src/KernelHeap.c**: `GetGlobalState()` modified to use CAS pattern
+- **src/Resource.c**: 4 functions modified to use `InitOnceExecuteOnce`; 2 boolean globals removed
+- **include/Resource.h**: Remove `extern` declarations for 2 boolean flags; add `INIT_ONCE` externs
+- **Tests**: No changes - behavior is identical, just thread-safe
+
+## Version Impact
+
+**PATCH** - Bug fix. No API changes. Existing single-threaded behavior is identical; multi-threaded behavior is corrected from broken to correct.

openspec/changes/fix-global-state-thread-safety/specs/thread-safe-init/spec.md

@@ -0,0 +1,30 @@
+## ADDED Requirements
+
+### Requirement: GetGlobalState SHALL be thread-safe
+`GetGlobalState()` SHALL use `InterlockedCompareExchangePointer` to atomically publish the global state pointer. If two threads race, exactly one SHALL succeed and the other SHALL free its local allocation without leaking memory.
+
+#### Scenario: Concurrent first call from two threads
+- **WHEN** two threads call `GetGlobalState()` simultaneously while `g_WinKernelLite_GlobalState` is NULL
+- **THEN** exactly one `GLOBAL_STATE` allocation SHALL survive, the other SHALL be freed, and both threads SHALL receive the same valid pointer
+
+#### Scenario: Fast path after initialization
+- **WHEN** `GetGlobalState()` is called after the global state is already initialized
+- **THEN** the function SHALL return the existing pointer without allocating or performing any synchronization beyond a pointer read
+
+### Requirement: CRITICAL_SECTION initialization SHALL use InitOnceExecuteOnce
+All `CRITICAL_SECTION` globals that are lazily initialized SHALL use `InitOnceExecuteOnce` with a static `INIT_ONCE` variable to guarantee exactly-once initialization.
+
+#### Scenario: Concurrent EnsureSystemResourcesListInitialized
+- **WHEN** two threads call `EnsureSystemResourcesListInitialized()` simultaneously before the system resources list is initialized
+- **THEN** `InitializeCriticalSection` SHALL be called exactly once, and both threads SHALL proceed safely after initialization completes
+
+#### Scenario: Concurrent KeEnterCriticalRegion
+- **WHEN** two threads call `KeEnterCriticalRegion()` simultaneously before the APC disable lock is initialized
+- **THEN** `InitializeCriticalSection` for the APC disable lock SHALL be called exactly once
+
+### Requirement: Boolean initialization flags SHALL be removed
+The `g_WinKernelLite_KernelApcDisableLockInitialized` and `g_WinKernelLite_SystemResourcesInitialized` boolean globals SHALL be replaced by `INIT_ONCE` variables. No race-prone boolean check-then-act patterns SHALL remain.
+
+#### Scenario: No boolean flags in Resource.h or Resource.c
+- **WHEN** the codebase is searched for `KernelApcDisableLockInitialized` or `SystemResourcesInitialized`
+- **THEN** no boolean declarations or checks SHALL be found; only `INIT_ONCE` variables SHALL control initialization state

openspec/changes/fix-global-state-thread-safety/tasks.md

@@ -0,0 +1,10 @@
+## 1. KernelHeap thread-safe init
+
+- [x] 1.1 Fix `GetGlobalState()` in `src/KernelHeap.c`: allocate and initialize `GLOBAL_STATE` into a local `temp`, then use `InterlockedCompareExchangePointer` to atomically set `g_WinKernelLite_GlobalState`. If CAS fails (another thread won), free `temp` and return the winner's pointer.
+- [x] 1.2 Build and run all tests to verify KernelHeap fix
+
+## 2. Resource thread-safe init
+
+- [x] 2.1 In `src/Resource.c` and `include/Resource.h`: replace `g_WinKernelLite_SystemResourcesInitialized` (BOOLEAN) with a static `INIT_ONCE` variable. Rewrite `EnsureSystemResourcesListInitialized()` to use `InitOnceExecuteOnce` with a callback that calls `InitializeCriticalSection` and `InitializeListHead`.
+- [x] 2.2 In `src/Resource.c` and `include/Resource.h`: replace `g_WinKernelLite_KernelApcDisableLockInitialized` (BOOLEAN) with a static `INIT_ONCE` variable. Rewrite `KeEnterCriticalRegion()`, `KeLeaveCriticalRegion()`, and `GetKernelApcDisableCount()` to use `InitOnceExecuteOnce` with a callback that calls `InitializeCriticalSection`. Remove the inline init checks from all three functions.
+- [x] 2.3 Build and run all tests to verify Resource fixes

src/KernelHeap.c

@@ -21,34 +21,44 @@
 GLOBAL_STATE* g_WinKernelLite_GlobalState = NULL;
 
 GLOBAL_STATE* GetGlobalState(void) {
-    if (g_WinKernelLite_GlobalState == NULL) {
-        HEAP_TRACE("GetGlobalState: Creating new global state");
-        GLOBAL_STATE* temp = (GLOBAL_STATE*)HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, sizeof(GLOBAL_STATE));
-        if (temp != NULL) {
-            HEAP_VERBOSE("GetGlobalState: Allocated global state at %p (size: %zu)", temp, sizeof(GLOBAL_STATE));
-            /* CriticalSection initialization removed for performance */
-            temp->HeapHandle = GetProcessHeap();
-            HEAP_VERBOSE("GetGlobalState: Using heap handle %p", temp->HeapHandle);
-            /* Initialize the linked lists immediately when creating global state */
-            InitializeListHead(&temp->MemoryAllocations);
-            InitializeListHead(&temp->FreedMemoryList);
-            /* Initialize other fields to safe defaults */
-            temp->AllocationCount = 0;
-            temp->TotalBytesAllocated = 0;
-            temp->CurrentBytesAllocated = 0;
-            temp->PeakBytesAllocated = 0;
-            temp->DoubleFreeCount = 0;
-            temp->FreedEntryCount = 0;
-            temp->MaxFreedEntries = 1000; /* Default: keep track of last 1000 freed allocations */
-            temp->NextAllocationId = 1; /* Start allocation IDs at 1 */
-            temp->SuppressErrors = FALSE;
-            temp->TrackFreedMemory = TRUE; /* Enable double-free tracking by default */
-            g_WinKernelLite_GlobalState = temp;
-            HEAP_INFO("GetGlobalState: Global state initialized successfully");
-        } else {
-            HEAP_ERROR("GetGlobalState: Failed to allocate global state");
-        }
+    GLOBAL_STATE* state = g_WinKernelLite_GlobalState;
+    if (state != NULL) {
+        return state; /* Fast path - already initialized */
+    }
+
+    HEAP_TRACE("GetGlobalState: Creating new global state");
+    GLOBAL_STATE* temp = (GLOBAL_STATE*)HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, sizeof(GLOBAL_STATE));
+    if (temp == NULL) {
+        HEAP_ERROR("GetGlobalState: Failed to allocate global state");
+        return NULL;
     }
+
+    HEAP_VERBOSE("GetGlobalState: Allocated global state at %p (size: %zu)", temp, sizeof(GLOBAL_STATE));
+    temp->HeapHandle = GetProcessHeap();
+    HEAP_VERBOSE("GetGlobalState: Using heap handle %p", temp->HeapHandle);
+    InitializeListHead(&temp->MemoryAllocations);
+    InitializeListHead(&temp->FreedMemoryList);
+    temp->AllocationCount = 0;
+    temp->TotalBytesAllocated = 0;
+    temp->CurrentBytesAllocated = 0;
+    temp->PeakBytesAllocated = 0;
+    temp->DoubleFreeCount = 0;
+    temp->FreedEntryCount = 0;
+    temp->MaxFreedEntries = 1000;
+    temp->NextAllocationId = 1;
+    temp->SuppressErrors = FALSE;
+    temp->TrackFreedMemory = TRUE;
+
+    /* Atomically publish: if another thread won the race, free our copy */
+    if (InterlockedCompareExchangePointer(
+            (PVOID*)&g_WinKernelLite_GlobalState, temp, NULL) != NULL) {
+        /* Another thread initialized first - free our allocation */
+        HEAP_TRACE("GetGlobalState: Lost race, freeing duplicate allocation at %p", temp);
+        HeapFree(GetProcessHeap(), 0, temp);
+    } else {
+        HEAP_INFO("GetGlobalState: Global state initialized successfully");
+    }
+
     return g_WinKernelLite_GlobalState;
 }
 

src/Resource.c

@@ -21,35 +21,47 @@
 /* Critical region counter (protected by its own critical section) */
 LONG g_WinKernelLite_KernelApcDisableCount = 0;
 CRITICAL_SECTION g_WinKernelLite_KernelApcDisableLock;
-BOOLEAN g_WinKernelLite_KernelApcDisableLockInitialized = FALSE;
+INIT_ONCE g_WinKernelLite_KernelApcDisableInitOnce = INIT_ONCE_STATIC_INIT;
 
 /* Global system resources list */
 LIST_ENTRY g_WinKernelLite_SystemResourcesList;
 CRITICAL_SECTION g_WinKernelLite_SystemResourcesLock;
-BOOLEAN g_WinKernelLite_SystemResourcesInitialized = FALSE;
+INIT_ONCE g_WinKernelLite_SystemResourcesInitOnce = INIT_ONCE_STATIC_INIT;
+
+static BOOL CALLBACK InitSystemResourcesCallback(PINIT_ONCE InitOnce, PVOID Parameter, PVOID* Context)
+{
+    UNREFERENCED_PARAMETER(InitOnce);
+    UNREFERENCED_PARAMETER(Parameter);
+    UNREFERENCED_PARAMETER(Context);
+    InitializeCriticalSection(&g_WinKernelLite_SystemResourcesLock);
+    InitializeListHead(&g_WinKernelLite_SystemResourcesList);
+    return TRUE;
+}
+
+static BOOL CALLBACK InitKernelApcDisableLockCallback(PINIT_ONCE InitOnce, PVOID Parameter, PVOID* Context)
+{
+    UNREFERENCED_PARAMETER(InitOnce);
+    UNREFERENCED_PARAMETER(Parameter);
+    UNREFERENCED_PARAMETER(Context);
+    InitializeCriticalSection(&g_WinKernelLite_KernelApcDisableLock);
+    return TRUE;
+}
 
 void EnsureSystemResourcesListInitialized(void)
 {
-    if (!g_WinKernelLite_SystemResourcesInitialized) {
-        InitializeCriticalSection(&g_WinKernelLite_SystemResourcesLock);
-        InitializeListHead(&g_WinKernelLite_SystemResourcesList);
-        g_WinKernelLite_SystemResourcesInitialized = TRUE;
-    }
+    InitOnceExecuteOnce(&g_WinKernelLite_SystemResourcesInitOnce,
+                        InitSystemResourcesCallback, NULL, NULL);
 }
 
 void CleanupGlobalResources(void)
 {
-    /* Cleanup APC disable lock if initialized */
-    if (g_WinKernelLite_KernelApcDisableLockInitialized) {
-        DeleteCriticalSection(&g_WinKernelLite_KernelApcDisableLock);
-        g_WinKernelLite_KernelApcDisableLockInitialized = FALSE;
-    }
+    /* Cleanup APC disable lock */
+    DeleteCriticalSection(&g_WinKernelLite_KernelApcDisableLock);
+    g_WinKernelLite_KernelApcDisableInitOnce = (INIT_ONCE)INIT_ONCE_STATIC_INIT;
 
-    /* Cleanup system resources lock if initialized */
-    if (g_WinKernelLite_SystemResourcesInitialized) {
-        DeleteCriticalSection(&g_WinKernelLite_SystemResourcesLock);
-        g_WinKernelLite_SystemResourcesInitialized = FALSE;
-    }
+    /* Cleanup system resources lock */
+    DeleteCriticalSection(&g_WinKernelLite_SystemResourcesLock);
+    g_WinKernelLite_SystemResourcesInitOnce = (INIT_ONCE)INIT_ONCE_STATIC_INIT;
 }
 
 NTSTATUS
@@ -249,19 +261,18 @@ ExReleaseResourceLite(
     LeaveCriticalSection(&Resource->CriticalSection);
 }
 
+static void EnsureKernelApcDisableLockInitialized(void)
+{
+    InitOnceExecuteOnce(&g_WinKernelLite_KernelApcDisableInitOnce,
+                        InitKernelApcDisableLockCallback, NULL, NULL);
+}
+
 VOID
 KeEnterCriticalRegion(
     VOID
 )
 {
-    /* Initialize the critical section if needed */
-    if (!g_WinKernelLite_KernelApcDisableLockInitialized) {
-        InitializeCriticalSection(&g_WinKernelLite_KernelApcDisableLock);
-        g_WinKernelLite_KernelApcDisableLockInitialized = TRUE;
-    }
-
-    /* In kernel mode, this disables normal kernel APCs */
-    /* In our user-mode implementation, we'll use a critical section for thread safety */
+    EnsureKernelApcDisableLockInitialized();
     EnterCriticalSection(&g_WinKernelLite_KernelApcDisableLock);
     InterlockedIncrement(&g_WinKernelLite_KernelApcDisableCount);
     LeaveCriticalSection(&g_WinKernelLite_KernelApcDisableLock);
@@ -272,13 +283,7 @@ KeLeaveCriticalRegion(
     VOID
 )
 {
-    /* Initialize the critical section if needed (safety check) */
-    if (!g_WinKernelLite_KernelApcDisableLockInitialized) {
-        InitializeCriticalSection(&g_WinKernelLite_KernelApcDisableLock);
-        g_WinKernelLite_KernelApcDisableLockInitialized = TRUE;
-    }
-    /* Re-enables normal kernel APCs */
-    /* In our user-mode implementation, we'll use a critical section for thread safety */
+    EnsureKernelApcDisableLockInitialized();
     EnterCriticalSection(&g_WinKernelLite_KernelApcDisableLock);
     InterlockedDecrement(&g_WinKernelLite_KernelApcDisableCount);
     LeaveCriticalSection(&g_WinKernelLite_KernelApcDisableLock);
@@ -288,12 +293,7 @@ LONG GetKernelApcDisableCount(void)
 {
     LONG currentValue;
 
-    /* Initialize the critical section if needed */
-    if (!g_WinKernelLite_KernelApcDisableLockInitialized) {
-        InitializeCriticalSection(&g_WinKernelLite_KernelApcDisableLock);
-        g_WinKernelLite_KernelApcDisableLockInitialized = TRUE;
-    }
-
+    EnsureKernelApcDisableLockInitialized();
     EnterCriticalSection(&g_WinKernelLite_KernelApcDisableLock);
     currentValue = g_WinKernelLite_KernelApcDisableCount;
     LeaveCriticalSection(&g_WinKernelLite_KernelApcDisableLock);