fix(core): clear *pool on init failure to prevent UAF in vmaf_gpu_picture_pool_init

changelog.d/fixed/gpu-picture-pool-uaf-init-failure.md

@@ -0,0 +1,13 @@
+- `vmaf_gpu_picture_pool_init()` use-after-free on failure paths.
+  The combined assignment `*const p = *pool = malloc(...)` published
+  the pool pointer to the caller's `*pool` argument *before* any later
+  failure path ran. On `goto free_p` (pic-array malloc or
+  pthread_mutex_init failure) the function freed `p` but left `*pool`
+  dangling — the natural `vmaf_close()` teardown then called
+  `vmaf_gpu_picture_pool_close()` on freed memory (UAF + potential
+  double-free, since the caller stores the handle in the long-lived
+  `VmafContext.cuda.ring_buffer`). Fix: clear `*pool = NULL` at every
+  failure label so a non-zero return reliably signals "pool not
+  constructed". Adds a CPU-only `test_gpu_picture_pool_uaf` regression
+  in `suite=fast` that exercises the `goto free_p` arm via an
+  oversized `pic_cnt` malloc-fail.

core/src/AGENTS.md

@@ -123,3 +123,24 @@ When porting an upstream Netflix/vmaf commit that modifies the original
 `libvmaf/src/metadata_handler.c`, apply the diff content to
 `core/src/metadata_handler.cpp` (C code is valid C++; the `extern "C"` block
 in the header stays). Run `make test-netflix-golden` post-port.
+
+### 10. Out-parameter init functions must clear the caller's handle on every failure path
+
+Functions with the shape `int X_init(X **out, ...)` that publish the
+allocation via the caller's `*out` must guarantee `*out == NULL` on any
+non-success return — including failure paths that take an internal `goto`
+and free the object before returning. The trap is the combined-assignment
+idiom `X *const p = *out = malloc(...);` which publishes the pointer to
+the caller *before* later `goto free_*` paths free it.
+
+If the caller stores the handle in a long-lived context (e.g.
+`VmafContext.cuda.ring_buffer`), the natural teardown (`vmaf_close()` →
+`X_close(*out)`) will then UAF on the freed object. The fix is mechanical:
+set `*out = NULL` after every `free()` in the failure-cleanup chain (and
+explicitly on the early-malloc-failure path even though the assignment
+already stored NULL there). The contract this pins is: "caller may inspect
+`*out` only on success; a non-zero return guarantees `*out == NULL`."
+
+Pattern: see `vmaf_gpu_picture_pool_init` in
+[`gpu_picture_pool.c`](gpu_picture_pool.c). Regression test:
+`core/test/test_gpu_picture_pool_uaf.c`.

core/src/gpu_picture_pool.c

@@ -53,9 +53,20 @@ int vmaf_gpu_picture_pool_init(VmafGpuPicturePool **pool, VmafGpuPicturePoolConf
 
     int err = 0;
 
+    /* The combined assignment `*const p = *pool = malloc(...)` publishes the
+     * pointer to the caller's *pool argument *before* any later failure
+     * path. If a subsequent step (pthread_mutex_init / pic-array malloc /
+     * alloc_picture_callback) fails we free(p), but unless we also clear
+     * *pool the caller is left with a dangling pointer that the natural
+     * vmaf_close() teardown then double-frees via
+     * vmaf_gpu_picture_pool_close(). Set *pool = NULL on every failure
+     * label so the caller can safely treat a non-zero return as
+     * "pool not constructed". */
     VmafGpuPicturePool *const p = *pool = malloc(sizeof(*p));
-    if (!p)
+    if (!p) {
+        *pool = NULL;
         goto fail;
+    }
     memset(p, 0, sizeof(*p));
     p->cfg = cfg;
 
@@ -78,6 +89,7 @@ int vmaf_gpu_picture_pool_init(VmafGpuPicturePool **pool, VmafGpuPicturePoolConf
     free(p->pic);
 free_p:
     free(p);
+    *pool = NULL;
 fail:
     return err;
 }

core/test/meson.build

@@ -687,6 +687,18 @@ test_locale_handling = executable('test_locale_handling',
     link_with : get_option('default_library') == 'both' ? libvmaf.get_static_lib() : libvmaf,
 )
 
+# CPU-only regression test for the vmaf_gpu_picture_pool_init() UAF
+# (caller's *pool was left dangling on failure paths). Built on every
+# matrix entry — not gated by enable_cuda — so a future regression on
+# the pure-C init shape is caught even when no GPU backend is in scope.
+test_gpu_picture_pool_uaf = executable('test_gpu_picture_pool_uaf',
+    ['test.c', 'test_gpu_picture_pool_uaf.c', '../src/gpu_picture_pool.c'],
+    include_directories : [libvmaf_inc, test_inc, include_directories('../src/')],
+    link_with : get_option('default_library') == 'both' ? libvmaf.get_static_lib() : libvmaf,
+    dependencies: [pthread_dependency],
+)
+test('test_gpu_picture_pool_uaf', test_gpu_picture_pool_uaf, suite : ['fast'])
+
 if get_option('enable_cuda')
 test_gpu_picture_pool = executable('test_gpu_picture_pool',
     ['test.c', 'test_gpu_picture_pool.c', '../src/gpu_picture_pool.c', '../src/cuda/picture_cuda.c'],

core/test/test_gpu_picture_pool_uaf.c

@@ -0,0 +1,141 @@
+/**
+ *
+ *  Copyright 2026 Lusoris and Claude (Anthropic)
+ *
+ *     Licensed under the BSD+Patent License (the "License");
+ *     you may not use this file except in compliance with the License.
+ *     You may obtain a copy of the License at
+ *
+ *         https://opensource.org/licenses/BSDplusPatent
+ *
+ *     Unless required by applicable law or agreed to in writing, software
+ *     distributed under the License is distributed on an "AS IS" BASIS,
+ *     WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ *     See the License for the specific language governing permissions and
+ *     limitations under the License.
+ *
+ */
+
+/* Regression test for the use-after-free that lurked in
+ * vmaf_gpu_picture_pool_init() before this PR.
+ *
+ * The bug: the combined assignment
+ *     VmafGpuPicturePool *const p = *pool = malloc(sizeof(*p));
+ * published `p` to the caller's `*pool` argument *before* any of the
+ * later failure paths ran. If the pic-array malloc or pthread_mutex_init
+ * failed, the function freed `p` via the `goto free_p` chain but left
+ * `*pool` holding the dangling pointer. The caller (libvmaf.c) then
+ * stored that dangling pointer in the long-lived VmafContext, and the
+ * natural vmaf_close() teardown double-freed it via
+ * vmaf_gpu_picture_pool_close().
+ *
+ * The fix: clear *pool to NULL at each failure label. This test
+ * exercises the `goto free_p` failure path by passing a `pic_cnt` so
+ * large that `malloc(sizeof(VmafPicture) * pic_cnt)` is virtually
+ * guaranteed to return NULL on any sane host, then asserts that
+ * `*pool` is NULL on return. Pure-CPU build — no GPU backend needed.
+ *
+ * Run cost: microseconds. Lives in suite=fast so the CPU-only
+ * meson_setup that every CI matrix entry runs catches a future
+ * regression. */
+
+#include <errno.h>
+#include <stdlib.h>
+
+#include "test.h"
+
+#include "gpu_picture_pool.h"
+
+/* Stub callbacks. We never expect alloc to be reached on the
+ * free_p path under test, but the init pre-checks require both
+ * pointers to be non-NULL. */
+static int stub_alloc_unreached(VmafPicture *pic, void *cookie)
+{
+    (void)pic;
+    (void)cookie;
+    return -ENOMEM;
+}
+
+static int stub_free_noop(VmafPicture *pic, void *cookie)
+{
+    (void)pic;
+    (void)cookie;
+    return 0;
+}
+
+/* Sentinel non-NULL pointer used to detect that
+ * vmaf_gpu_picture_pool_init cleared the handle on failure. We take
+ * the address of a static byte rather than casting an integer literal
+ * to a pointer: same effect (distinct from NULL, easy to spot in a
+ * debugger) without the clang-tidy `performance-no-int-to-ptr` violation
+ * the int-to-ptr cast would trip. */
+static char sentinel_storage;
+#define SENTINEL ((VmafGpuPicturePool *)&sentinel_storage)
+
+/* Triggers the `goto free_p` arm: malloc(sizeof(VmafPicture) * pic_cnt)
+ * is asked for ~2^31 elements, which on every realistic host fails. If
+ * the host happens to fulfil the request, the test reports skip rather
+ * than spuriously fail — the goal is to assert the *failure-cleanup
+ * shape*, not to brute-force OOM. */
+static char *test_pool_handle_cleared_on_pic_array_alloc_failure(void)
+{
+    VmafGpuPicturePoolConfig cfg = {
+        /* UINT_MAX/2 entries * sizeof(VmafPicture) overflows or
+         * exhausts virtual memory on any commodity x86_64/arm64
+         * host. Pure C — no GPU dependency. */
+        .pic_cnt = 0x7FFFFFFFu,
+        .cookie = NULL,
+        .alloc_picture_callback = stub_alloc_unreached,
+        .free_picture_callback = stub_free_noop,
+        .synchronize_picture_callback = NULL,
+    };
+
+    /* Seed with a non-NULL sentinel so a regression that fails to
+     * clear *pool at the free_p label is detectable. The pre-fix
+     * code left this pointing at the now-freed pool struct. */
+    VmafGpuPicturePool *pool = SENTINEL;
+    int err = vmaf_gpu_picture_pool_init(&pool, cfg);
+
+    if (err == 0) {
+        /* Host gave us 32 GiB. Free the live pool and skip. */
+        (void)vmaf_gpu_picture_pool_close(pool);
+        (void)fprintf(stderr, "[skip: host fulfilled implausibly large alloc] ");
+        return NULL;
+    }
+
+    /* The actual UAF gate: the caller's handle must be NULL on the
+     * failure path. Otherwise a downstream vmaf_close() would call
+     * vmaf_gpu_picture_pool_close() on freed memory. */
+    mu_assert("pool handle must be NULL after init failure (UAF regression)", pool == NULL);
+
+    return NULL;
+}
+
+/* The -EINVAL early-return path runs before malloc(), so *pool is
+ * untouched. The contract is "caller only reads *pool on success",
+ * which we pin here. */
+static char *test_pool_handle_not_clobbered_on_einval_early_return(void)
+{
+    VmafGpuPicturePoolConfig cfg = {
+        .pic_cnt = 0, /* triggers the early EINVAL */
+        .cookie = NULL,
+        .alloc_picture_callback = stub_alloc_unreached,
+        .free_picture_callback = stub_free_noop,
+        .synchronize_picture_callback = NULL,
+    };
+
+    VmafGpuPicturePool *pool = SENTINEL;
+    int err = vmaf_gpu_picture_pool_init(&pool, cfg);
+
+    mu_assert("init must reject pic_cnt=0 with -EINVAL", err == -EINVAL);
+    mu_assert("early-return EINVAL must not touch *pool", pool == SENTINEL);
+
+    return NULL;
+}
+
+char *run_tests(void)
+{
+    mu_run_test(test_pool_handle_cleared_on_pic_array_alloc_failure);
+    mu_run_test(test_pool_handle_not_clobbered_on_einval_early_return);
+    return NULL;
+}

docs/rebase-notes.md

@@ -6,6 +6,24 @@ syncs from `upstream/master` (Netflix/vmaf). Required by
 
 ---
 
+## gpu-picture-pool-uaf-on-init-failure (2026-05-30)
+
+**Files touched:** `core/src/gpu_picture_pool.c`, `core/test/test_gpu_picture_pool_uaf.c`, `core/test/meson.build`
+
+**Rebase impact:** Minor. The fix touches `gpu_picture_pool.c`, which is
+fork-added by ADR-0239 (promotion of upstream's `cuda/ring_buffer.c` into a
+backend-agnostic helper). Upstream Netflix/vmaf still ships the original
+`cuda/ring_buffer.c`; if a future upstream sync ports the same UAF guard
+to their file, the change there will be in a different TU and won't
+conflict. The fork-local test (`test_gpu_picture_pool_uaf.c`) is wholly
+new and CPU-only — no upstream collision possible.
+
+The shape of the fix (`*pool = NULL` on every goto-free label) is
+mechanically replayable; if upstream later refactors `ring_buffer_init`
+the same way, the diffs will be parallel rather than colliding.
+
+---
+
 ## cuda-ms-ssim-vert-lcs-horiz-ldg (2026-05-29, ADR-0757)
 
 **Files touched:**

docs/state.md

@@ -221,6 +221,7 @@ landed fix yet._
 
 ## Recently closed
 
+| **T-GPU-PICTURE-POOL-UAF-INIT-FAILURE-2026-05-30** | `vmaf_gpu_picture_pool_init()` in `core/src/gpu_picture_pool.c` published the pool pointer to the caller's `*pool` argument via the combined assignment `*const p = *pool = malloc(...)` *before* any subsequent failure path ran. On `goto free_p` (pic-array malloc or pthread_mutex_init failure) the function freed `p` but `*pool` still held the dangling pointer, and the natural `vmaf_close()` teardown called `vmaf_gpu_picture_pool_close()` on the freed memory — UAF + potential double-free, since `libvmaf.c:326` stores the handle in the long-lived `VmafContext.cuda.ring_buffer`. Fix: clear `*pool = NULL` at every failure label (after `!p` malloc check and after `free(p)` at the `free_p` label) so a non-zero return reliably signals "pool not constructed". CPU-only regression test (`test_gpu_picture_pool_uaf`) added in `suite=fast`. | no ADR: bug fix per CLAUDE §12 r8 | fix/gpu-picture-pool-uaf-on-init-failure | `meson setup build-cpu core -Denable_cuda=false -Denable_sycl=false -Denable_hip=false && ninja -C build-cpu && meson test -C build-cpu test_gpu_picture_pool_uaf` — pass. | (2026-05-30) |
 | **T-CI-CONFLICT-MARKERS-PR50-2026-05-29** | Commit `24bb5daf89` (post-merge-train sweep #50) introduced committed git conflict markers in 38 files across `.github/`, `ai/`, `core/`, and `docs/`. Most files resolved by PR #108 (CUDA `integer_vif_cuda.c`), PR #174 (CI YAML), PR #243 (70 files). This closeout PR resolved the last 2 residual markers in `.semgrepignore` (4 blocks, post-ADR-0700 paths kept) and `docs/backends/sycl/overview.md` (1 block, took the HEAD `core/include/libvmaf/libvmaf.h` path). | no ADR: maintenance fix | chore/state-md-stale-entries-sweep-20260530 | `git grep -nE '^(<{7}\|={7}\|>{7})( \|$)' -- ':(exclude)docs/research/' ':(exclude)docs/adr/_index_fragments/' ':(exclude)*.lock' ':(exclude)CHANGELOG.md'` returns empty. | (2026-05-30) |
 | **T-METAL-MT1-DISPATCH-FLOAT-MS-SSIM-2026-05-29** | `g_metal_features[]` in `core/src/metal/dispatch_strategy.c` lacked `"float_ms_ssim_metal"`. `vmaf_metal_dispatch_supports()` returned 0 for the float MS-SSIM Metal extractor even after ADR-0490 / T-VULKAN-METAL-DEAD-SCAFFOLDS-2026-05-18 wired the TU into meson. Callers routing through the dispatch table (ADR-0420 gate, ADR-0421 consumer) silently fell back to CPU on every Apple Silicon run. Fixed by adding the entry in the table, adjacent to `"float_ms_ssim"`. | no ADR: only-one-way fix | fix/metal-pr117-actionable-findings-20260529 | Smoke: `vmaf_metal_dispatch_supports(ctx, "float_ms_ssim_metal") == 1` on Apple Silicon. | (2026-05-29) |
 | **T-METAL-MT2-ARC-RETAIN-BALANCE-2026-05-29** | `vmaf_metal_state_init_external` in `core/src/metal/picture_import.mm` called `CFRetain((__bridge CFTypeRef)device)` (or `queue`) followed immediately by `(__bridge_retained void *)device` — accumulating +2 retain counts. `vmaf_metal_state_free` releases via a single `__bridge_transfer` (-1 retain), leaving one reference permanently live per init/close cycle for both `device` and `queue`. On an external-device path called by the FFmpeg `libvmaf_metal` filter, every filter graph teardown leaked the `id<MTLDevice>` and `id<MTLCommandQueue>` references. Fixed by removing both `CFRetain` calls; `__bridge_retained` alone is the correct single ownership transfer. | no ADR: only-one-way fix | fix/metal-pr117-actionable-findings-20260529 | Smoke: `leaks --atExit -- vmaf --backend metal ...` returns 0 Metal object leaks per init/teardown cycle. macOS CI smoke required; Linux host: static-analysis only. | (2026-05-29) |