fix(gpu): cu_state leak on import failure + gpu_dispatch_env TOCTOU (ADR-0840)

changelog.d/fixed/gpu-dispatch-toctou.md

@@ -0,0 +1,9 @@
+- Fix `cu_state` resource leak in `core/tools/vmaf.c`: when
+  `vmaf_cuda_import_state` fails after a successful
+  `vmaf_cuda_state_init`, the allocated `cu_state` is now freed before
+  returning -1 (CWE-401). ADR-0840.
+- Fix lock-free TOCTOU in `core/src/gpu_dispatch_env.c`: paired
+  `atomic_thread_fence(memory_order_release)` / `memory_order_acquire`
+  fences ensure `row->value` is visible to readers on weakly-ordered
+  architectures (ARM64, POWER) before `row->var_name` is published.
+  ADR-0840.

core/src/gpu_dispatch_env.c

@@ -14,6 +14,7 @@
  */
 #include "gpu_dispatch_env.h"
 
+#include <stdatomic.h>
 #include <stddef.h>
 #include <stdlib.h>
 #include <string.h>
@@ -75,17 +76,25 @@ const char *vmaf_gpu_dispatch_env_get(const char *var_name)
 
     /* Fast path: look for an already-populated row without locking.
      * The pointer comparison on var_name is safe because callers pass
-     * string literals that outlive the process. */
+     * string literals that outlive the process.
+     *
+     * Acquire fence after matching var_name pairs with the release fence
+     * on the publish side (ADR-0840), ensuring value is read after
+     * var_name has been fully written by the populating thread. */
     for (unsigned i = 0U; i < GPU_DISPATCH_ENV_TABLE_CAP; i++) {
-        if (g_rows[i].var_name == var_name)
+        if (g_rows[i].var_name == var_name) {
+            atomic_thread_fence(memory_order_acquire);
             return g_rows[i].value;
+        }
     }
 
     /* Also check by string equality in case two TUs use different
      * pointer addresses for the same variable name. */
     for (unsigned i = 0U; i < GPU_DISPATCH_ENV_TABLE_CAP; i++) {
-        if (g_rows[i].var_name && strcmp(g_rows[i].var_name, var_name) == 0)
+        if (g_rows[i].var_name && strcmp(g_rows[i].var_name, var_name) == 0) {
+            atomic_thread_fence(memory_order_acquire);
             return g_rows[i].value;
+        }
     }
 
     /* Slow path: snapshot the variable under the lock. */
@@ -124,9 +133,14 @@ const char *vmaf_gpu_dispatch_env_get(const char *var_name)
      * callers, not hypothetical concurrent setenv from user code. */
     /* NOLINT(concurrency-mt-unsafe): see above. */
     const char *val = getenv(var_name); /* NOLINT(concurrency-mt-unsafe) */
-    row->var_name = var_name;
     if (val)
         row->value = strdup(val);
+    /* Publish value before making the row visible via var_name.
+     * The paired acquire fence on the fast-path reader ensures that
+     * the value load cannot be speculated before the var_name match.
+     * ADR-0840. */
+    atomic_thread_fence(memory_order_release);
+    row->var_name = var_name;
     lock_release();
     return row->value;
 }

core/tools/vmaf.c

@@ -661,6 +661,7 @@ static int init_gpu_backends(VmafContext *vmaf, const CLISettings *c
             err |= vmaf_cuda_import_state(vmaf, cu_state);
             if (err) {
                 (void)fprintf(stderr, "problem during vmaf_cuda_import_state\n");
+                vmaf_cuda_state_free(cu_state);
                 return -1;
             }
             *cuda_active_out = true;

docs/adr/0840-gpu-dispatch-toctou-fence.md

@@ -0,0 +1,75 @@
+# ADR-0840: Fix cu_state leak on import failure and gpu_dispatch_env TOCTOU
+
+- **Status**: Accepted
+- **Date**: 2026-05-29
+- **Deciders**: lusoris
+- **Tags**: `cuda`, `security`, `framework`, `ci`
+
+## Context
+
+Two independent correctness bugs were identified in the GPU dispatch path during an
+audit (source: audit a4003b2235845d570):
+
+**Bug 1 — cu_state leak (CWE-401, memory leak on error path):**
+In `core/tools/vmaf.c`, `vmaf_cuda_state_init` allocates `cu_state`. If the
+subsequent `vmaf_cuda_import_state` call fails, the function returned -1 without
+calling `vmaf_cuda_state_free(cu_state)`, leaking the allocation for the lifetime of
+the process. The leak only occurs on the error path (GPU init succeeds but
+import fails), but it is a CWE-401 resource leak.
+
+**Bug 2 — gpu_dispatch_env lock-free fast-path TOCTOU (LOW severity):**
+`vmaf_gpu_dispatch_env_get` in `core/src/gpu_dispatch_env.c` uses a lock-free fast
+path that reads `g_rows[i].var_name` and `g_rows[i].value` without memory fences.
+On weakly-ordered architectures (ARM64, POWER), a CPU or compiler reorder can cause
+the reader to observe a non-NULL `var_name` while `value` is still uninitialized.
+The publisher inside the mutex wrote `value` first, then `var_name`, but without a
+release fence the ordering is not guaranteed to be visible to observers outside the
+lock. A paired release/acquire fence pair makes the ordering formally correct per
+ISO C11 §7.17 and eliminates the TOCTOU hazard.
+
+## Decision
+
+Apply both fixes in the same PR:
+
+1. Add `vmaf_cuda_state_free(cu_state);` before the `return -1` in the
+   `vmaf_cuda_import_state` failure arm in `core/tools/vmaf.c`.
+
+2. In `core/src/gpu_dispatch_env.c`:
+   - Add `#include <stdatomic.h>`.
+   - Insert `atomic_thread_fence(memory_order_release)` after writing `row->value`
+     and before writing `row->var_name` on the publish path (inside the lock).
+   - Insert `atomic_thread_fence(memory_order_acquire)` after matching
+     `g_rows[i].var_name` and before reading `g_rows[i].value` on both fast-path
+     loops.
+
+Both are pure bug fixes with no interface change; no ADR would normally be required
+(per CLAUDE.md §12 r8), but the weak-memory fence pattern is a non-obvious
+correctness idiom that a future maintainer could accidentally revert — warranting
+a decision record to explain the pairing invariant.
+
+## Alternatives considered
+
+| Option | Pros | Cons | Why not chosen |
+|---|---|---|---|
+| Full mutex on every fast-path read | Trivially correct | Defeats the purpose of the fast-path cache; adds lock contention on every GPU dispatch decision | Performance regression on the hot path |
+| Switch to `_Atomic const char *` fields | Eliminates manual fences; idiomatic C11 | Requires changing the struct layout and all write sites; higher diff surface | Disproportionate churn for a two-line fix |
+| Keep existing code, add TOCTOU comment | No code change | Leaves a real data race on ARM64 | Does not fix the bug |
+
+## Consequences
+
+- **Positive**: CWE-401 resource leak closed; lock-free read path is now formally
+  correct on all architectures including ARM64 and POWER.
+- **Negative**: Minimal — two `atomic_thread_fence` calls on an already-cold path
+  (first call per var_name only); immeasurable overhead.
+- **Neutral / follow-ups**: The fence pairing must be preserved if the publish
+  path is ever refactored. Comment blocks in the code reference this ADR as the
+  rationale.
+
+## References
+
+- Source audit reference: a4003b2235845d570
+- CWE-401: Missing Release of Memory after Effective Lifetime
+- ISO C11 §7.17 (atomics / memory model)
+- ADR-0461: `gpu_dispatch_env` once-snapshotted pattern
+- ADR-0157: `vmaf_cuda_state_free` API introduction (CUDA preallocation leak fix)
+- Related PR: fix/gpu-dispatch-toctou-fence-20260529

docs/rebase-notes.md

@@ -6,13 +6,20 @@ syncs from `upstream/master` (Netflix/vmaf). Required by
 
 ---
 
-## cpp23-shadow-const-fixes (2026-05-29, ADR-0839)
+## gpu-dispatch-toctou-fence (2026-05-29, ADR-0840)
 
-no rebase impact: REASON — all changed files (`core/src/feature/feature_collector.cpp`,
-`core/src/fex_ctx_vector.cpp`, `core/src/sycl/common.cpp`) are fork-local C++23
-conversions with no upstream counterpart in Netflix/vmaf master. The upstream originals
-(`feature_collector.c`, `fex_ctx_vector.c`) are pure C; the `.cpp` files were created by
-the fork's C++23 wave (ADR-0708 ff.) and are not subject to upstream rebase churn.
+**Files touched:**
+`core/tools/vmaf.c`, `core/src/gpu_dispatch_env.c`
+
+**Rebase impact:** None. Both files are fork-local additions. `core/tools/vmaf.c`
+has upstream touches only in the pre-existing CUDA block (which is itself
+fork-local); `gpu_dispatch_env.c` has no upstream counterpart. No rebase conflict
+is possible on a clean upstream sync.
+
+The `atomic_thread_fence` pairing (publish: release after value, before var_name;
+read: acquire after var_name match, before value read) must be preserved if the
+fast-path loop or publish path is ever refactored — see ADR-0840 for the
+formal C11 memory-model rationale.
 
 ---