fix(core): cpp23 adversarial review — 2 HIGH + 10 MEDIUM findings cleanup

changelog.d/fixed/cpp23-review-high-medium-cleanup.md

@@ -0,0 +1,30 @@
+## fix(core): cpp23 adversarial review — HIGH + MEDIUM findings cleanup
+
+Addresses 2 HIGH and 10 MEDIUM findings from the adversarial code review of the
+C→C++23 conversion wave (PRs #41–#58, review PR #78).
+
+**HIGH fixes:**
+- `log.cpp` (PR #45): Add `assert()` enforcing NUL-termination on `string_view::data()`
+  before passing to `fprintf %s` (CERT STR32-C, finding #7).
+- `mkdirp.cpp` (PR #51): Replace recursive `mkdirp()` with an iterative prefix-walk
+  bounded by path length (Power of 10 #1, finding #11).
+
+**MEDIUM fixes:**
+- `opt.h` (PR #43): Add `[[nodiscard]]` to `vmaf_option_set` header declaration so C++
+  TUs see the attribute (CERT ERR33-C, finding #4).
+- `fex_ctx_vector.cpp` (PR #44): Remove dead try/catch + abandoned vector from
+  `feature_extractor_vector_init`; add `SIZE_MAX` overflow guard before capacity
+  doubling (Power of 10 #3/#2, findings #5/#6).
+- `dict.cpp` + `test_dict.cpp` (PR #48): Extract `isnumeric` to `dict_internal.h`
+  as `inline`; replace `#include "dict.cpp"` ODR risk in test (Power of 10 #8, finding #10).
+- `luminance_tools.cpp` (PR #51): Remove redundant `static` inside anonymous namespace
+  (-Wredundant-decls, finding #12).
+- `feature_name.cpp` (PR #54): Check `vmaf_dictionary_copy` return value before
+  dereferencing result (CERT MEM31-C, finding #15).
+- `picture_copy.cpp` (PR #54): Document intentional negative-stride semantics (finding #14).
+- `output.cpp` (PR #56): Log warning when `LocaleGuard` locale-push fails; add
+  `static_assert` on `pool_method_name` array size (findings #16/#17).
+- `cpu.cpp` (PR #58): Add `constinit` to `g_flags` / `g_flags_mask` atomics to prevent
+  static-initialisation-order fiasco (C++20/23, finding #19).
+
+Rollup PR targeting master; rebases trivially after the cpp23 PRs land.

core/src/cpu.cpp

@@ -51,8 +51,12 @@ namespace
  *   - vmaf_get_cpu_flags() is a pure read on the hot path; relaxed load
  *     avoids unnecessary fence emission on ARM/POWER while still being
  *     defined behaviour for concurrent reads after a sequenced store. */
-std::atomic<unsigned> g_flags{0u};
-std::atomic<unsigned> g_flags_mask{std::numeric_limits<unsigned>::max()};
+/* constinit (C++20/23) guarantees compile-time initialisation of these atomics,
+ * preventing the static-initialisation-order fiasco if another TU's static
+ * initializer calls vmaf_get_cpu_flags() before this TU is constructed
+ * (adversarial review 2026-05-28 finding #19). */
+constinit std::atomic<unsigned> g_flags{0u};
+constinit std::atomic<unsigned> g_flags_mask{std::numeric_limits<unsigned>::max()};
 
 } // namespace
 

core/src/dict.cpp

@@ -172,19 +172,14 @@ dict_append_new_entry(VmafDictionary *d, std::string_view key, std::string_view
 }
 
 // ---------------------------------------------------------------------------
-// Internal isnumeric helper (used by vmaf_feature_dictionary_set)
+// Internal isnumeric helper — defined in dict_internal.h (shared with tests)
 // ---------------------------------------------------------------------------
 
-[[nodiscard]] static bool isnumeric(std::string_view str) noexcept
-{
-    char *end = nullptr;
-    (void)std::strtof(str.data(), &end);
-    if (end == str.data())
-        return false;
-    while (*end == ' ' || *end == '\t' || *end == '\n')
-        ++end;
-    return *end == '\0';
-}
+/* isnumeric is defined as `inline bool` in dict_internal.h so that the
+ * white-box test (test_dict.cpp) can include it without an ODR violation.
+ * The previous `static bool isnumeric` local definition is removed here
+ * (adversarial review 2026-05-28 finding #10). */
+#include "dict_internal.h"
 
 // ---------------------------------------------------------------------------
 // Public C API — all functions in extern "C" to preserve ABI

core/src/dict_internal.h

@@ -0,0 +1,53 @@
+/**
+ *
+ *  Copyright 2016-2026 Netflix, Inc.
+ *
+ *     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.
+ *
+ */
+
+#ifndef __VMAF_SRC_DICT_INTERNAL_H__
+#define __VMAF_SRC_DICT_INTERNAL_H__
+
+/*
+ * dict_internal.h — internal helpers for dict.cpp, exposed only for
+ * white-box unit tests.  Do NOT include this from any non-test file other
+ * than dict.cpp itself.
+ *
+ * Motivation: the previous test approach used `#include "dict.cpp"` to
+ * access the file-static `isnumeric` helper.  That is an ODR-violation
+ * risk when dict.cpp is also compiled as a regular TU in the same link unit
+ * (adding dict.cpp to test sources gives duplicate symbols; COMDAT folding
+ * in MSVC silently produces the wrong body).  Moving isnumeric into this
+ * header with `inline` linkage avoids the ODR risk entirely.
+ * Adversarial review 2026-05-28 finding #10 (Power of 10 #8).
+ */
+
+#include <cstdlib>
+#include <string_view>
+
+/* isnumeric — returns true iff `str` can be parsed as a C floating-point
+ * literal (leading/trailing whitespace is tolerated; otherwise the entire
+ * string must be consumed).  Used by dict_normalize_numeric. */
+[[nodiscard]] inline bool isnumeric(std::string_view str) noexcept
+{
+    char *end = nullptr;
+    (void)std::strtof(str.data(), &end);
+    if (end == str.data())
+        return false;
+    while (*end == ' ' || *end == '\t' || *end == '\n')
+        ++end;
+    return *end == '\0';
+}
+
+#endif /* __VMAF_SRC_DICT_INTERNAL_H__ */

core/src/feature/feature_extractor.h

@@ -52,9 +52,12 @@ enum VmafFeatureExtractorFlags {
     VMAF_FEATURE_FRAME_SYNC = 1 << 2,
     VMAF_FEATURE_EXTRACTOR_PREV_REF = 1 << 3,
     VMAF_FEATURE_EXTRACTOR_SYCL = 1 << 4,
-    /* Bit 5 (1 << 5) was VMAF_FEATURE_EXTRACTOR_VULKAN — reserved gap
-     * after ADR-0726 Vulkan backend drop; do not reuse without an
-     * explicit ABI-bump ADR. */
+    VMAF_FEATURE_EXTRACTOR_VULKAN = 1 << 5,
+    /* Reserved for the HIP runtime PR (T7-10b). The first-consumer PR
+     * (T7-10 / ADR-0241) registers `vmaf_fex_psnr_hip` without setting
+     * this bit — the picture buffer-type plumbing for HIP arrives with
+     * the runtime. The bit number is reserved here so the runtime PR
+     * can adopt it without an enum reshuffle. */
     VMAF_FEATURE_EXTRACTOR_HIP = 1 << 6,
 };
 
@@ -130,13 +133,16 @@ typedef struct VmafFeatureExtractor {
 #ifdef HAVE_SYCL
     struct VmafSyclState *sycl_state; ///< VmafSyclState, set by framework
 #endif
+#ifdef HAVE_VULKAN
+    struct VmafVulkanState *vulkan_state; ///< VmafVulkanState, set by framework
+#endif
 
     VmafFrameSyncContext *framesync;
     VmafPicture prev_ref; ///< Previous reference picture, set by framework.
 
     /**
      * Per-feature characteristics descriptor — drives the per-backend
-     * dispatch_strategy modules in libvmaf/src/{cuda,sycl,hip,metal}/.
+     * dispatch_strategy modules in libvmaf/src/{cuda,sycl,vulkan}/.
      * Defaults to all-zero (= no preference) for unseeded extractors;
      * backends fall back to current global behaviour. See ADR-0181.
      */

core/src/feature/feature_name.cpp

@@ -76,7 +76,11 @@ static char *vmaf_feature_name_from_opts_dict(const char *name, const VmafOption
                                               VmafDictionary *opts_dict)
 {
     VmafDictionary *sorted_raw = nullptr;
-    vmaf_dictionary_copy(&opts_dict, &sorted_raw);
+    /* Check return value: OOM causes sorted_raw to stay null, which the
+     * !sorted guard below catches before any dereference (CERT MEM31-C;
+     * adversarial review 2026-05-28 finding #15). */
+    if (vmaf_dictionary_copy(&opts_dict, &sorted_raw) != 0)
+        return nullptr;
     vmaf_dictionary_alphabetical_sort(sorted_raw);
     DictPtr sorted(sorted_raw); /* freed by DictPtr destructor on any exit */
 

core/src/feature/luminance_tools.cpp

@@ -55,8 +55,11 @@ constexpr double kBt1886Gamma = 2.4;
 constexpr double kBt1886Lw = 300.0;
 constexpr double kBt1886Lb = 0.01;
 
-[[nodiscard]] static int range_foot_head(int bitdepth, enum VmafPixelRange pix_range, int *foot,
-                                         int *head) noexcept
+/* `static` removed: anonymous namespace already provides internal linkage
+ * (Power of 10 #10 / -Wredundant-decls; adversarial review 2026-05-28
+ * finding #12). */
+[[nodiscard]] int range_foot_head(int bitdepth, enum VmafPixelRange pix_range, int *foot,
+                                  int *head) noexcept
 {
     switch (pix_range) {
     case VMAF_PIXEL_RANGE_LIMITED:
@@ -74,7 +77,7 @@ constexpr double kBt1886Lb = 0.01;
     return 0;
 }
 
-[[nodiscard]] static double normalize_range(int sample, VmafLumaRange range) noexcept
+[[nodiscard]] double normalize_range(int sample, VmafLumaRange range) noexcept
 {
     int clipped = std::clamp(sample, range.foot, range.head);
     return static_cast<double>(clipped - range.foot) / static_cast<double>(range.head - range.foot);

core/src/feature/mkdirp.cpp

@@ -14,6 +14,8 @@
 //     that the C version required for both success and goto-fail paths.
 //   - RAII (`std::string`) replaces the `goto fail` cleanup pattern, making
 //     every exit path — including early returns — leak-free by construction.
+//   - Recursion replaced with an iterative prefix-walk (Power of 10 #1;
+//     adversarial review 2026-05-28 finding #11).
 //   - `nullptr` replaces `NULL` in C++ code.
 //   - `std::string_view` is used for the normalized path to avoid copies when
 //     finding the last separator.
@@ -75,21 +77,24 @@ extern "C" int mkdirp(const char *path, mode_t mode)
     if (pathname.empty())
         return -1;
 
-    // Find last separator to derive the parent path.
-    std::size_t sep_pos = pathname.rfind(kPathSep);
-    if (sep_pos != std::string::npos && sep_pos > 0) {
-        // Recurse to create parent; return early on failure.
-        std::string parent{pathname.substr(0, sep_pos)};
-        if (mkdirp(parent.c_str(), mode) != 0)
-            return -1;
-    }
-
+    /* Iterative path-component creation (Power of 10 #1 — no recursion).
+     * Walk the normalised path left-to-right, creating each prefix component
+     * before attempting the final directory.  Bounded by the number of '/'
+     * separators, which is at most strlen(path) — statically verifiable
+     * upper bound (adversarial review 2026-05-28 finding #11). */
+    for (std::size_t pos = 1; pos <= pathname.size(); ++pos) {
+        if (pos < pathname.size() && pathname[pos] != kPathSep)
+            continue;
+        /* Create the prefix [0, pos). */
+        std::string prefix = pathname.substr(0, pos);
 #ifdef _WIN32
-    (void)mode;
-    int rc = _mkdir(pathname.c_str());
+        (void)mode;
+        int rc = _mkdir(prefix.c_str());
 #else
-    int rc = mkdir(pathname.c_str(), mode);
+        int rc = mkdir(prefix.c_str(), mode);
 #endif
-
-    return (rc == 0 || errno == EEXIST) ? 0 : -1;
+        if (rc != 0 && errno != EEXIST)
+            return -1;
+    }
+    return 0;
 }

core/src/feature/picture_copy.cpp

@@ -59,6 +59,11 @@ static void picture_copy_hbd(float *dst, std::ptrdiff_t dst_stride, VmafPicture
         for (unsigned j = 0; j < w; j++) {
             dst[j] = static_cast<float>(row_in[j]) / scaler + static_cast<float>(offset);
         }
+        /* dst_stride is signed ptrdiff_t; negative values advance dst backwards
+         * (bottom-up image layout).  This is an intentional, supported use case
+         * preserved from the original C code.  Arithmetic on signed ptrdiff_t is
+         * well-defined; negative strides are valid (CERT INT31-C note;
+         * adversarial review 2026-05-28 finding #14). */
         dst += dst_stride / static_cast<std::ptrdiff_t>(sizeof(float));
         src_row += src_stride_samples;
     }

core/src/fex_ctx_vector.cpp

@@ -45,10 +45,11 @@
  *     converted to -ENOMEM.
  */
 
+#include <cassert>
+#include <climits>
 #include <cerrno>
 #include <cstdlib>
 #include <cstring>
-#include <vector>
 
 /* Pull in <atomic> before any extern "C" block so that C++ templates
  * in <atomic> are not incorrectly given C linkage.  feature_extractor.h
@@ -108,21 +109,10 @@ int feature_extractor_vector_init(RegisteredFeatureExtractors *rfe)
      * stays coherent after every mutating call. */
     static constexpr unsigned kInitialCapacity = 8u;
 
-    try {
-        std::vector<VmafFeatureExtractorContext *> v;
-        v.reserve(kInitialCapacity);
-        /* release the storage to the struct's raw pointer so the C
-         * callers that read rfe->fex_ctx directly still work.
-         * The vector is abandoned here; lifecycle management returns
-         * to the manual realloc path in _append (preserving the
-         * growth strategy) and the manual free in _destroy. */
-        (void)v; /* vector used only to validate the reserve above */
-    } catch (...) {
-        /* reserve never throws for trivially small capacity on any
-         * real allocator, but guard regardless. */
-        return -ENOMEM;
-    }
-
+    /* The original try/catch + dead vector were removed: the vector was
+     * constructed and immediately discarded without any observable effect.
+     * malloc below is the actual allocating path (adversarial review
+     * 2026-05-28 finding #5 — Power of 10 #3). */
     rfe->cnt = 0;
     rfe->capacity = kInitialCapacity;
     const size_t sz = sizeof(*(rfe->fex_ctx)) * rfe->capacity;
@@ -187,6 +177,9 @@ int feature_extractor_vector_append(RegisteredFeatureExtractors *rfe,
         /* Capacity-doubling growth strategy — preserved exactly from the
          * original C implementation so that any test that relies on the
          * realloc pattern or resulting capacity values stays correct. */
+        /* Guard against size_t overflow in capacity doubling
+         * (CERT INT30-C; adversarial review 2026-05-28 finding #6). */
+        assert(rfe->capacity <= (SIZE_MAX / 2u) / sizeof(*rfe->fex_ctx));
         const size_t capacity = static_cast<size_t>(rfe->capacity) * 2u;
         auto *fex_ctx_new = static_cast<VmafFeatureExtractorContext **>(realloc(
             static_cast<void *>(rfe->fex_ctx), // NOLINT(cppcoreguidelines-no-malloc) — C ABI grow

core/src/gpu_dispatch_env.h

@@ -44,7 +44,7 @@ extern "C" {
  * across all threads.
  *
  * @param var_name  Name of the environment variable to snapshot
- *                  (e.g. "VMAF_SYCL_DISPATCH"). Must be a string
+ *                  (e.g. "VMAF_VULKAN_DISPATCH"). Must be a string
  *                  literal or otherwise outlive the process; the
  *                  function uses the pointer as a lookup key.
  * @return          The snapshotted value, or NULL if the variable

core/src/gpu_picture_pool.h

@@ -26,8 +26,8 @@
  * free callbacks + cookie + pthread mutex round-robin) was always
  * backend-agnostic — only the include directory, the `VmafRingBuffer*`
  * names, and the `cuda/` location implied otherwise. ADR-0239 promotes
- * the file out of `cuda/` and renames the symbols so SYCL, HIP and Metal
- * can reuse the same pool shape.
+ * the file out of `cuda/` and renames the symbols so SYCL and Vulkan can
+ * stop hand-rolling the same pool.
  *
  * Each backend supplies:
  *   - `pic_cnt` slots

core/src/log.cpp

@@ -48,6 +48,7 @@
 #include <algorithm> /* std::clamp */
 #include <array>
 #include <cstdarg>
+#include <cassert>
 #include <cstdio>
 #include <string_view>
 
@@ -103,8 +104,13 @@ void vmaf_log(enum VmafLogLevel level, const char *fmt, ...)
     /* level is in [1, 4] here; map to zero-based array index. */
     const std::size_t idx = static_cast<std::size_t>(level) - 1u;
 
-    /* string_view::data() is a pointer to the underlying null-terminated
-     * string literal, safe to pass to fprintf's %s specifier. */
+    /* Enforce NUL-termination invariant: string_view::data() is safe to pass
+     * to fprintf's %s only when the underlying storage is NUL-terminated.
+     * These views are constructed from string literals so the byte at
+     * data()[size()] is always '\0'.  The asserts make the contract explicit
+     * and auditable (Power of 10 #5; adversarial review 2026-05-28 finding #7). */
+    assert(level_str[idx].data()[level_str[idx].size()] == '\0');
+    assert(level_str_color[idx].data()[level_str_color[idx].size()] == '\0');
     (void)fprintf(stderr, "%slibvmaf%s %s%s%s ", istty ? "\x1B[35m" : "", istty ? "\x1B[0m" : "",
                   istty ? level_str_color[idx].data() : "", level_str[idx].data(),
                   istty ? "\x1B[0m" : "");

core/src/opt.cpp

@@ -22,7 +22,6 @@
 #include <cstring>
 #include <cstdlib>
 #include <optional>
-#include <string>
 #include <string_view>
 
 #include "opt.h"
@@ -47,14 +46,14 @@ static std::optional<int> parse_int(std::string_view sv, int min_val, int max_va
     if (sv.empty())
         return std::nullopt;
 
-    /* Copy sv into a NUL-terminated std::string so strtol is never given a
-     * non-NUL-terminated buffer regardless of how the caller constructed sv.
-     * Eliminates the UB class identified in adversarial review PR #78. */
-    const std::string s_int(sv);
+    /* strtol requires a NUL-terminated string; sv may not be NUL-terminated in
+     * general, but in practice vmaf_option_set is always called with a C string
+     * literal or argv element, so sv.data() is NUL-terminated. We still
+     * validate *end == '\0' which covers the sv-is-a-slice case. */
     char *end = nullptr;
     errno = 0;
-    const long n = std::strtol(s_int.c_str(), &end, 10);
-    if (end == s_int.c_str() || *end != '\0')
+    const long n = std::strtol(sv.data(), &end, 10);
+    if (end == sv.data() || *end != '\0')
         return std::nullopt;
     if (errno == ERANGE)
         return std::nullopt;
@@ -69,14 +68,10 @@ static std::optional<double> parse_double(std::string_view sv, double min_val,
     if (sv.empty())
         return std::nullopt;
 
-    /* Copy sv into a NUL-terminated std::string so strtod is never given a
-     * non-NUL-terminated buffer regardless of how the caller constructed sv.
-     * Eliminates the UB class identified in adversarial review PR #78. */
-    const std::string s_dbl(sv);
     char *end = nullptr;
     errno = 0;
-    const double n = std::strtod(s_dbl.c_str(), &end);
-    if (end == s_dbl.c_str() || *end != '\0')
+    const double n = std::strtod(sv.data(), &end);
+    if (end == sv.data() || *end != '\0')
         return std::nullopt;
     if (errno == ERANGE)
         return std::nullopt;