Repository navigation
fix: iter9 bundle — 7 fixes (HIP comment, ASan .cpp, SVM fuzz, TSan race, vmaf-tune, AI scripts, MCP) - #873
Merged
Merged
Conversation
…red-mem sizing The file header stated "the shared memory array for warp partial sums is sized for the HIP warp size (64)" — contradicting the actual code, which already used FPSNR_MIN_WARP_SIZE=32 and FPSNR_WARPS_PER_BLOCK sized for wave32 worst-case (8 slots), with runtime warpSize in all reduction loops. The other three iter9-cross-backend-parity findings (HIP float_adm FADM_WARP_SIZE=64 RDNA2 corruption, SYCL float_adm missing AIM CM stages 2b/3b, HIP float_adm missing AIM CM stages 2b/3b) were fixed in the iter8 bundle (commit 32ec0aa, PR #871). This corrects the remaining stale-comment discrepancy so the file accurately documents its own implementation. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The .c implementations of vmaf_fex_ctx_pool_create and vmaf_feature_collector_init already set the caller's out-pointer to NULL at the free_p: / free_fc: labels (added in bundle-F / iter-asan sweeps), but the parallel .cpp translation units used by all GPU backends missed the same assignment. Changes: - feature_extractor.cpp: add *pool = NULL at free_p: before fall-through to fail:, mirroring feature_extractor.c line 797. - feature_collector.cpp: add *feature_collector = nullptr at free_fc: before fall-through to fail:, mirroring feature_collector.c line 265. Without these, any caller that allocates and checks the return code may still read back a freed pointer via the out-parameter between free() and the NULL assignment at fail: (CERT MEM30-C, ASan/LeakSan UAF class). The .c files and libvmaf.c were already correct; this closes the .cpp gap. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…s to size_t Two UBSan findings from iter9 fuzz-extended cell: 1. VMAF_SVM_MAX_AXIS_COUNT was (1 << 24) ~16.7M, which allowed nr_class*(nr_class-1)/2 to overflow signed 32-bit arithmetic before the result was assigned to the size_t nr_class_permutations variable. Tighten the bound to 46340 (floor(sqrt(INT_MAX))) so the product is safe even without a cast. 2. Add explicit (size_t) casts before the multiply on the permutation expression to keep all arithmetic in the unsigned 64-bit domain, matching the signed-overflow fix pattern used elsewhere in this file. All 9 SVM parser tests and 3 multiclass tests pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…atomic batch err Finding #1 (high): ctx_pool_ensure_slot_ctx read fex->framesync from the shared registered VmafFeatureExtractor struct without holding any lock protecting that field. A concurrent set_fex_framesync() call on the main thread (writing to the same struct) created a data race visible to TSan. Fix: snapshot fex->framesync once in vmaf_fex_ctx_pool_aquire while the pool lock is already held, then pass the captured VmafFrameSyncContext* down through ctx_pool_claim_slot into ctx_pool_ensure_slot_ctx. This also corrects a latent functional bug: the global fex->framesync was never written by set_fex_framesync (which only writes to deep copies), so FRAME_SYNC extractors acquired through the pool would have received framesync=NULL before this fix. Both feature_extractor.c and feature_extractor.cpp twins updated identically. Finding #2 (high): struct ThreadDataBatch.err was a plain int written by the worker via multiple sequential atomic_store sites and read by the caller as the function return value. Changing the field to _Atomic int and replacing all writes/reads with atomic_store/atomic_load eliminates any TSan data-race report on this field should a future code path observe f->err directly rather than through the return-value path, and documents the worker-owned write semantics explicitly. All 88 fast-suite tests pass; test_thread_safety_batch, test_thread_pool, and test_feature_extractor pass individually. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Fixes two high-priority findings from iter9 cell iter9-vmaftune-exhaustion. Finding 1 (score.py integer_ prefix) was already resolved in commit a6c4dff and is a no-op here. Fix 1 — compare: bisect workers PermissionError on VMAFTUNE_WORKDIR bind-mount (dev/scripts/dev-mcp-entrypoint.sh): The host bind-mount source for VMAFTUNE_WORKDIR is typically owned by root:root (mode 755) after the first `docker compose up`. The vmaf user running inside the container cannot write into it, causing every bisect worker to fail with PermissionError. Add `chown vmaf:vmaf "${VMAFTUNE_WORKDIR}"` immediately after the existing `mkdir -p` so the vmaf user owns the directory at container start regardless of host-side ownership. The chown is guarded with `|| true` so it does not abort the entrypoint on read-only hosts. Fix 2 — recommend-saliency: ONNX crash on non-multiple-of-32 sources (tools/vmaf-tune/src/vmaftune/saliency.py): `saliency_student_v1` uses a UNet encoder-decoder with skip connections that require H and W to be exact multiples of 32. Sources whose dimensions are not multiples of 32 cause an onnxruntime shape mismatch inside the skip-connection concatenation, crashing inference. Add `_pad_to_multiple(tensor, 32)` helper that zero-pads a `[1, 3, H, W]` float32 tensor to the next multiple-of-32 boundary. In `compute_saliency_map`, replace the previous hard rejection of `height % 8 != 0` with a pad-before-run / crop-after-run pattern: pad the tensor, run inference on the padded input, then crop the `[1, 1, H_pad, W_pad]` output back to `[:, :, :orig_h, :orig_w]` before accumulating into the temporal aggregator. This makes arbitrary source dimensions work correctly without callers needing to pre-pad or pre-crop their YUV sources. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… py3.14 dataclass crash, pytest-timeout parity
- test_e2e_frame_to_score.py: replace `Path(os.environ.get('VMAF_BIN', '')) or ...`
with an explicit None-sentinel check so VMAF_BIN='' is treated as unset rather
than resolving to Path('.') and executing CWD as the vmaf binary. [critical]
- ai/scripts/_script_bootstrap.py: remove `from __future__ import annotations`.
All dataclass fields are concrete Path types; the future-import is unnecessary
and triggers CPython gh-129861 (Python 3.14 dataclasses._is_type() crash) when
the module is loaded via importlib.util before sys.modules pre-registration. [high]
- ai/AGENTS.md: document the invariant that any importlib.util caller must insert
`sys.modules[spec.name] = module` between module_from_spec() and exec_module()
to avoid the Python 3.14 regression. [high — invariant note]
- ai/pyproject.toml: add pytest-timeout>=0.5 to [dev] optional deps.
- dev/Containerfile: install ai[dev] instead of plain ai so pytest-timeout is
baked into the container, closing the CI/container parity gap for --timeout=60.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- describe_model Step 1 now calls _allowed_roots() after resolve() and raises ValueError for any path outside an allowlisted root, mirroring _validate_path() exactly (was implicit-only; ../../outside-repo.json bypassed the guard). - HTTP /v1/score now serialises the result via _dumps_strict() instead of bare json.dumps(), converting NaN/Infinity to null and producing RFC 8259-compliant output (json.dumps allow_nan=True is the default, emitting bare NaN tokens that are not valid JSON). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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
1 participant
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
float_psnr_score.hip(claimed 64-lane shared-mem, code uses 32).*pool = NULL/*feature_collector = nullptrafterfree()on error paths in.cppTUs — closes CERT MEM30-C / UAF window mirroring the.ccounterpart fix from iter8.VMAF_SVM_MAX_AXIS_COUNTfrom 16 777 216 to 46 340 (floor(sqrt(INT_MAX))); add(size_t)casts innr_class_permutationsmultiply to keep arithmetic in 64-bit domain.fex->framesyncunder the pool lock inctx_pool_ensure_slot_ctx; changeThreadDataBatch.errto_Atomic intwithatomic_store/atomic_loadat all sites.chown vmaf:vmaf "${VMAFTUNE_WORKDIR}"aftermkdir -pin entrypoint; pad non-multiple-of-32 tensors incompute_saliency_map()(pad-before/crop-after inference).VMAF_BINempty-string sentinel, removefrom __future__ import annotations(Python 3.14 dataclass crash), add importlib invariant note toai/AGENTS.md._allowed_roots()check in_describe_modelto close path-traversal bypass; replacejson.dumpswith_dumps_strictin HTTP/v1/scoreto convert NaN/Infinity → null (RFC 8259).Commit 3 from the iter9 hunt list (
32ec0aa64) was already merged as part of iter8 bundle PR #871 — skipped in this cherry-pick.Test plan
SKIP=semgrep-local pre-commit run --files <all changed files>— all hooks passedmeson test -C build --suite=fast— 88/88 passedgit grep '^<<<<<<'— clean)Deliverables checklist
ai/AGENTS.md(importlib pre-registration)meson test -C core/build --suite=fast— 88/88🤖 Generated with Claude Code