chore(review): adversarial code review of cpp23 wave (PRs #41-#58)

changelog.d/changed/cpp23-wave-adversarial-review.md

@@ -0,0 +1,9 @@
+## chore(review): adversarial code review — C++23 wave (PRs #41–#58)
+
+Read-only adversarial review of the cpp23 conversion wave. Found 4 CRITICAL, 2 HIGH,
+10 MEDIUM, 3 LOW issues across all 9 PRs. Key findings:
+- `strtof` precision bug in `dict.cpp` causing score corruption on high-precision options (#48)
+- `strlen - 5U` unsigned underflow heap-overflow in `model.cpp` (#54)
+- `operator new` / `free()` allocator mismatch in `ref.cpp` (#58)
+- Non-NUL-terminated `string_view::data()` passed to `strtol` in `opt.cpp` (#43)
+See `docs/research/cpp23-wave-adversarial-review-20260528.md` for the full findings table.

core/AGENTS.md

@@ -457,3 +457,40 @@ the corrected methodology.
   — `enable_avx512=true` with `enable_asm=false` issues a warning (no-op, not an error);
   — `enable_hipcc=true` with `enable_hip=false` issues a warning (no-op, not an error).
   The checks run at configuration time (before `subdir()` calls) to catch misconfigurations early. The principle: every option that depends on another must `error()` on the bad combo, never silently no-op. See [`src/meson.build` lines 100–111, 74–76, 142–144](src/meson.build).
+
+- **C→C++23 conversion safety invariants** (adversarial review 2026-05-28,
+  `docs/research/cpp23-wave-adversarial-review-20260528.md`):
+  When converting a `.c` TU to `.cpp` with `std::string_view` / `std::optional` /
+  `std::unique_ptr` idioms, verify all of the following before merging:
+
+  1. **`string_view::data()` + C-string functions**: `strtol`, `strtod`, `strtof`,
+     `strcmp`, `strlen`, `printf("%s", sv.data())` all require NUL-termination.
+     If the `string_view` is constructed from a C-string literal or a full C-string
+     argument it is safe; if it could ever be a substring slice, copy to `std::string`
+     first or add `assert(sv.data()[sv.size()] == '\0')`.
+
+  2. **`strtof` vs `strtod` precision**: returning `float` from `strtof` and assigning
+     to `double` silently loses precision. If the downstream use is `snprintf("%g", dv)`
+     the output will be at `float` precision (~7 sig figs), not `double` (~15). Use
+     `strtod` when the result variable is `double`.
+
+  3. **`make_unique` / `operator new` vs C-caller `free()`**: if a struct is allocated
+     by `std::make_unique` (uses `operator new`) but C callers may also call `free()`
+     on the same pointer (e.g. pre-existing teardown paths), this is UB / heap
+     corruption. Document in the header that `operator delete` (via `vmaf_ref_close`
+     or equivalent) is the ONLY valid deallocator; search all C callers for direct
+     `free(ptr)` on that type.
+
+  4. **`strlen(x) - N` unsigned underflow**: subtracting an integer from `size_t`
+     (returned by `strlen`) when `strlen(x) < N` wraps to a huge value. Always
+     check `strlen(x) >= N` first, or use `(len >= N ? len - N : 0)`.
+
+  5. **Recursion in converted code**: the Power of 10 rule 1 (no recursion) applies
+     equally to `.cpp` files. `mkdirp` is the known violator; future conversions must
+     replace recursive path-splitting with an iterative approach.
+
+  6. **`[[nodiscard]]` on declarations vs definitions**: placing `[[nodiscard]]` only
+     on the `.cpp` definition without mirroring it in the `extern "C"` declaration in
+     the header means C++ callers that only see the header will not get the diagnostic.
+     Always add `[[nodiscard]]` to the header declaration (inside the `extern "C"` block
+     — C compilers silently ignore the attribute).

docs/rebase-notes.md

@@ -40021,3 +40021,16 @@ No rebase impact: research-only documents, no source code changes. The profiling
 When a follow-up PR implements the `integer_ssim_score.cu` `extern "C"` fix (Research-0736
 recommendation 1), that PR must also update `ssim_cuda.c` host glue and verify bit-exact
 parity against the CPU integer_ssim extractor on the Netflix golden fixture.
+## C++23 wave adversarial review (2026-05-28)
+
+Read-only review of PRs #41, #43, #44, #45, #48, #51, #54, #56, #58.
+No files were modified by this review. The review digest is in
+`docs/research/cpp23-wave-adversarial-review-20260528.md`.
+
+Critical issues that must be fixed before merge:
+- PR #43 `opt.cpp`: `strtol`/`strtod` on potentially non-NUL-terminated `string_view::data()`
+- PR #48 `dict.cpp`: `strtof` (float) assigned to `double` — precision loss on option values
+- PR #54 `model.cpp`: `strlen(model->name) - 5U` unsigned underflow → heap overflow
+- PR #58 `ref.cpp`: `make_unique` / C-caller `free()` allocator mismatch
+
+No rebase impact from the review itself; all findings are fixes required in those PRs.