fix(core): cpp23 adversarial review — 2 HIGH + 10 MEDIUM findings cleanup#83
Merged
Merged
Conversation
67e6410 to
6c0b9ae
Compare
…anup Apply constinit to atomic globals in cpu.cpp, split dict_internal.h, tighten signed/unsigned casts in fex_ctx_vector, fix narrow-cast warnings in log/opt/output/picture_copy/mkdirp/luminance_tools/feature_name. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
6c0b9ae to
4439000
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Rollup of fixes for the 2 HIGH and 10 MEDIUM findings from the adversarial code
review of the C→C++23 conversion wave (VMAFx/vmafx PR #78). 3 LOW findings left
as nits per scope directive.
Rebases trivially on top of the cpp23 PRs (#41–#58) when they land.
Each commit targets exactly the file(s) from the corresponding cpp23 PR.
Findings addressed
HIGH (2)
core/src/log.cpp: Addassert(sv.data()[sv.size()] == '\0')before passingstring_view::data()tofprintf %s. Arrays are constexpr string literals so asserts always pass; they document and enforce the NUL invariant (CERT STR32-C; Power of 10 chore(deps): Update fedora:45 Docker digest to 0c1f63e #5).core/src/feature/mkdirp.cpp: Replace recursivemkdirp()with iterative prefix-walk bounded bypathname.size(). Eliminates stack-overflow risk on deeply-nested adversarial paths (Power of 10 chore(meta): post-cutover URL sweep — lusoris/vmaf → VMAFx/vmafx #1, chore(docs): update mkdocs site_url to vmafx.github.io/vmafx #2).MEDIUM (10)
core/src/opt.h: Add[[nodiscard]]tovmaf_option_setdeclaration insideextern "C"block so C++ TUs see the attribute (CERT ERR33-C).core/src/fex_ctx_vector.cpp: Remove dead try/catch + abandoned vector fromfeature_extractor_vector_init— the vector was discarded before its RAII could help (Power of 10 chore(deps): Pin dependencies #3).core/src/fex_ctx_vector.cpp: Addassert(capacity <= SIZE_MAX/2/sizeof(*fex_ctx))before capacity-doubling realloc (CERT INT30-C).core/src/dict.cpp+dict_internal.h+test/test_dict.cpp: Extractisnumerictodict_internal.hasinline bool. Replace ODR-risky#include "dict.cpp"in test with#include "dict_internal.h"(Power of 10 chore(deps): Update Helm release prometheus-pushgateway to >=2.17.0 #8).core/src/feature/luminance_tools.cpp: Remove redundantstaticfrom functions in anonymous namespace — anonymous namespace already provides internal linkage;statictriggers-Wredundant-decls(Power of 10 chore(deps): Update rocm/rocm-terminal Docker tag to v6.4 #10).core/src/feature/picture_copy.cpp: Add comment documenting intentional negative-stride semantics for bottom-up image layout (CERT INT31-C clarification).core/src/feature/feature_name.cpp: Checkvmaf_dictionary_copyreturn value before wrapping inDictPtr— null dereference on OOM otherwise (CERT MEM31-C).core/src/output.cpp: LogVMAF_LOG_LEVEL_WARNINGwhenLocaleGuardlocale-push returns null — silent failure degrades decimal-separator correctness.core/src/output.cpp: Addstatic_assert(VMAF_POOL_METHOD_NB == 5)afterpool_method_name[]— catches out-of-bounds loop if enum grows (JPL Rule 23 spirit).core/src/cpu.cpp: Addconstinittog_flags/g_flags_maskatomics — prevents static-initialisation-order fiasco if a static initializer in another TU callsvmaf_get_cpu_flags()before cpu.cpp is constructed (C++20/23).Deferred (3 LOWs, per scope directive)
[[nodiscard]]C-visible limitation inmem.h— acceptable per stated design intent.aligned_malloc— pre-existing, addressed separately.vmaf_log_level/isttythread-safety — pre-existing C issue;std::atomicupgrade deferred.Deep-dive deliverables (ADR-0108)
meson setup build-cleanup core -Denable_cuda=false -Denable_sycl=false && ninja -C build-cleanup && meson test -C build-cleanup --suite=fast(after cpp23 PRs land).changelog.d/fixed/cpp23-review-high-medium-cleanup.md.docs/rebase-notes.mdentry.Per-surface docs
Sentinel: no user-discoverable surface changed. All fixes are internal code-quality / safety improvements with no change to computed scores, CLI flags, or public C API.
🤖 Generated with Claude Code