fix(core): sanitizer-pass cleanup — CAMBI option type-mismatch + ADM signed-shift UB

changelog.d/fixed/sanitizer-pass-cleanup.md

@@ -0,0 +1,16 @@
+- **core / cambi**: Fix CAMBI option-parser writing a 4-byte `int`
+  through a 2-byte `uint16_t *` for the `window_size` and
+  `max_log_contrast` options. The misalignment was flagged by UBSan on
+  every `--feature cambi` invocation; the silent struct corruption
+  (writing 4 bytes into a 2-byte field clobbering adjacent state) was
+  not previously visible. Adds shadow `int` slots in `CambiState` and
+  copies them into the existing `uint16_t` runtime fields in `init()`.
+  Bit-exact with prior behaviour on every valid option value.
+  ([ADR-0869](docs/adr/0869-sanitizer-pass-cleanup.md))
+- **core / adm (AVX2 + AVX-512)**: Fix C undefined behaviour in the
+  ADM DWT2 filter packing: `(int) << 16` on a negative `filter[k]`
+  was UB even though the result was cast to `uint32_t` (precedence:
+  cast on result, not on operand). Move the cast inside the shift
+  (`((uint32_t)filter[k] << 16)`). Bit-exact with prior wrap-on-overflow
+  behaviour on every two's-complement target.
+  ([ADR-0869](docs/adr/0869-sanitizer-pass-cleanup.md))

core/src/feature/cambi.c

@@ -141,6 +141,19 @@ typedef struct CambiState {
     unsigned enc_bitdepth;
     unsigned src_width;
     unsigned src_height;
+    /* `window_size_opt` and `max_log_contrast_opt` are the
+     * option-parser write targets — they must be `int` because the
+     * options table declares them as `VMAF_OPT_TYPE_INT`, and the
+     * parser dereferences `(int *)data` to store / read defaults.
+     * Writing a 4-byte int through a `uint16_t *` is undefined
+     * behaviour (misaligned store on the 2-byte-aligned
+     * `max_log_contrast` slot, silent overwrite of the adjacent
+     * `src_window_size` slot on `window_size`). The runtime fields
+     * `window_size` / `max_log_contrast` are populated from these in
+     * `init()` and remain `uint16_t` to keep the inner-loop call
+     * signatures unchanged. */
+    int window_size_opt;
+    int max_log_contrast_opt;
     uint16_t window_size;
     uint16_t src_window_size;
     double topk;
@@ -235,7 +248,7 @@ static const VmafOption options[] = {
     {
         .name = "window_size",
         .help = "Window size to compute CAMBI: 65 corresponds to ~1 degree at 4k",
-        .offset = offsetof(CambiState, window_size),
+        .offset = offsetof(CambiState, window_size_opt),
         .type = VMAF_OPT_TYPE_INT,
         .default_val.i = DEFAULT_CAMBI_WINDOW_SIZE,
         .min = 15,
@@ -292,7 +305,7 @@ static const VmafOption options[] = {
         .help = "Maximum contrast in log luma level (2^max_log_contrast) at 10-bits, "
                 "e.g., 2 is equivalent to 4 luma levels at 10-bit and 1 luma level at 8-bit. "
                 "From 0 to 5: default 2 is recommended for banding from compression.",
-        .offset = offsetof(CambiState, max_log_contrast),
+        .offset = offsetof(CambiState, max_log_contrast_opt),
         .type = VMAF_OPT_TYPE_INT,
         .default_val.i = DEFAULT_CAMBI_MAX_LOG_CONTRAST,
         .min = 0,
@@ -559,6 +572,13 @@ static int init(VmafFeatureExtractor *fex, enum VmafPixelFormat pix_fmt, unsigne
 
     CambiState *s = fex->priv;
 
+    /* Copy parsed option values from the int shadow slots into the
+     * uint16_t runtime fields. Option-parser bounds (0..127 for
+     * window_size, 0..5 for max_log_contrast) guarantee the narrowing
+     * is lossless. See ADR-0790. */
+    s->window_size = (uint16_t)s->window_size_opt;
+    s->max_log_contrast = (uint16_t)s->max_log_contrast_opt;
+
     s->feature_name_dict =
         vmaf_feature_name_dict_from_provided_features(fex->provided_features, fex->options, s);
     if (!s->feature_name_dict)

core/src/feature/x86/adm_avx2.c

@@ -3335,13 +3335,21 @@ void adm_dwt2_16_avx2(const uint16_t *src, const adm_dwt_band_t *dst, AdmBuffer
     int16_t *tmphi = tmplo + w;
     int32_t accum;
 
+    /* Cast each filter coefficient to uint32_t BEFORE the shift —
+     * `filter_*[k]` may be negative, and `(int) << 16` on a negative
+     * value is C undefined behaviour (caught by UBSan). The cast on
+     * the result `(uint32_t)(filter[k] << 16)` does not save us
+     * because precedence makes the shift execute on the signed type
+     * first. Reordering to `((uint32_t)filter[k] << 16)` is bit-exact
+     * with the previous wrap-on-overflow behaviour on every platform
+     * we target (two's complement, defined unsigned wrap). */
     __m256i f01_lo =
-        _mm256_set1_epi32(filter_lo[0] + (uint32_t)(filter_lo[1] << 16) /* + (1 << 16) */);
+        _mm256_set1_epi32(filter_lo[0] + ((uint32_t)filter_lo[1] << 16) /* + (1 << 16) */);
     __m256i f23_lo =
-        _mm256_set1_epi32(filter_lo[2] + (uint32_t)(filter_lo[3] << 16) /* + (1 << 16) */);
-    __m256i f01_hi = _mm256_set1_epi32(filter_hi[0] + (uint32_t)(filter_hi[1] << 16) + (1 << 16));
+        _mm256_set1_epi32(filter_lo[2] + ((uint32_t)filter_lo[3] << 16) /* + (1 << 16) */);
+    __m256i f01_hi = _mm256_set1_epi32(filter_hi[0] + ((uint32_t)filter_hi[1] << 16) + (1 << 16));
     __m256i f23_hi =
-        _mm256_set1_epi32(filter_hi[2] + (uint32_t)(filter_hi[3] << 16) /*+ (1 << 16)*/);
+        _mm256_set1_epi32(filter_hi[2] + ((uint32_t)filter_hi[3] << 16) /*+ (1 << 16)*/);
 
     __m256i accum0_lo;
     __m256i accum0_hi;

core/src/feature/x86/adm_avx512.c

@@ -3552,13 +3552,21 @@ void adm_dwt2_16_avx512(const uint16_t *src, const adm_dwt_band_t *dst, AdmBuffe
     int16_t *tmphi = tmplo + w;
     int32_t accum;
 
+    /* Cast each filter coefficient to uint32_t BEFORE the shift —
+     * `filter_*[k]` may be negative, and `(int) << 16` on a negative
+     * value is C undefined behaviour (caught by UBSan). The cast on
+     * the result `(uint32_t)(filter[k] << 16)` does not save us
+     * because precedence makes the shift execute on the signed type
+     * first. Reordering to `((uint32_t)filter[k] << 16)` is bit-exact
+     * with the previous wrap-on-overflow behaviour on every platform
+     * we target (two's complement, defined unsigned wrap). */
     __m512i f01_lo =
-        _mm512_set1_epi32(filter_lo[0] + (uint32_t)(filter_lo[1] << 16) /* + (1 << 16) */);
+        _mm512_set1_epi32(filter_lo[0] + ((uint32_t)filter_lo[1] << 16) /* + (1 << 16) */);
     __m512i f23_lo =
-        _mm512_set1_epi32(filter_lo[2] + (uint32_t)(filter_lo[3] << 16) /* + (1 << 16) */);
-    __m512i f01_hi = _mm512_set1_epi32(filter_hi[0] + (uint32_t)(filter_hi[1] << 16) + (1 << 16));
+        _mm512_set1_epi32(filter_lo[2] + ((uint32_t)filter_lo[3] << 16) /* + (1 << 16) */);
+    __m512i f01_hi = _mm512_set1_epi32(filter_hi[0] + ((uint32_t)filter_hi[1] << 16) + (1 << 16));
     __m512i f23_hi =
-        _mm512_set1_epi32(filter_hi[2] + (uint32_t)(filter_hi[3] << 16) /*+ (1 << 16)*/);
+        _mm512_set1_epi32(filter_hi[2] + ((uint32_t)filter_hi[3] << 16) /*+ (1 << 16)*/);
 
     __m512i accum0;
     __m512i accum0_lo;

docs/adr/0869-sanitizer-pass-cleanup.md

@@ -0,0 +1,106 @@
+# ADR-0869: Sanitizer-Pass Cleanup — CAMBI Option-Type Mismatch and AVX{2,512} ADM Signed-Shift UB
+
+- **Status**: Accepted
+- **Date**: 2026-05-30
+- **Deciders**: lusoris, Claude (sanitizer audit)
+- **Tags**: `c`, `simd`, `sanitizer`, `correctness`, `cambi`, `adm`
+
+## Context
+
+Master CI runs a scheduled "Sanitizers + Fuzz" job, but the build-matrix
+sanitizer combinations are not run on every PR. A local
+`-Db_sanitize=address,undefined` build of `core/` against the full unit-test
+suite plus extractor binaries (vmaf CLI, vmaf-perShot, vmaf_roi) surfaced
+two real undefined-behaviour findings that the unit tests alone miss
+because the relevant code paths are option- and pixel-format-conditional:
+
+1. **CAMBI option-parser writes through misaligned `uint16_t` slots.**
+   The options table declares `window_size` and `max_log_contrast` as
+   `VMAF_OPT_TYPE_INT`, but the underlying `CambiState` fields are
+   `uint16_t`. The parser's `set_option_int` performs `*(int *)data = ...`,
+   which (a) silently corrupts adjacent struct bytes (`src_window_size`
+   on `window_size`, padding before `heatmaps_path` on
+   `max_log_contrast`), and (b) on `max_log_contrast` lands on a
+   2-byte-aligned address — UBSan flags the misaligned 4-byte store. The
+   mirror read site in `vmaf_feature_name_from_options`
+   (`core/src/feature/feature_name.c:104`) shows the same misalignment
+   on every CLI invocation that includes `--feature cambi`.
+
+2. **AVX2 / AVX-512 ADM DWT2 filter packing left-shifts a negative `int`.**
+   `core/src/feature/x86/adm_avx{2,512}.c` packs two adjacent 16-bit
+   filter coefficients into a 32-bit lane via
+   `_mm{256,512}_set1_epi32(filter[k] + (uint32_t)(filter[k+1] << 16))`.
+   The cast on the *result* of the shift does not save the inner shift
+   from being performed on a signed `int`; when `filter[k+1]` is negative
+   (which it routinely is for the 9/7 DWT coefficients used in HBD
+   inputs), the shift is C undefined behaviour. UBSan reports
+   `left shift of negative value -4240` on every HBD ADM frame.
+
+Neither bug surfaces in the existing unit tests because (a) `test_cambi`
+does not parse user options via the CLI path, and (b) the AVX-512 ADM
+fast path is only reached for HBD inputs sized to the SIMD step width,
+which the unit-test fixtures do not exercise. Both ship in every release
+binary and would eventually bite a CI run on a stricter UBSan profile
+or a compiler that exploits the UB (clang/llvm's UB-driven optimiser is
+already aggressive enough to silently miscompile these patterns on
+`-O3`).
+
+## Decision
+
+We will treat both findings as real correctness bugs and fix them in
+this PR, preserving bit-exact numerical behaviour on every input.
+
+For (1), we introduce shadow `int` slots `window_size_opt` and
+`max_log_contrast_opt` in `CambiState`. The options table targets the
+shadow slots (preserving the option schema). `init()` copies them into
+the existing `uint16_t` runtime fields — the option-parser bounds
+(15..127 and 0..5 respectively) guarantee the narrowing is lossless.
+This keeps the inner-loop signatures (`uint16_t window_size`, `uint16_t
+max_log_contrast`) unchanged so no SIMD or scalar kernel needs to be
+touched.
+
+For (2), we move the `uint32_t` cast inside the shift's left operand:
+`((uint32_t)filter[k+1] << 16)`. Shifting an unsigned operand is fully
+defined and bit-exact with the original wrap-on-overflow behaviour on
+every two's-complement target we ship for.
+
+## Alternatives considered
+
+| Option | Pros | Cons | Why not chosen |
+|---|---|---|---|
+| Introduce `VMAF_OPT_TYPE_UINT16` | Most type-safe; fixes a class of bugs in one stroke | New public option type touches every feature extractor, the dictionary serialiser, and the documentation | Larger blast radius than the bug warrants; only two fields in the entire tree had this mismatch |
+| Change `window_size` / `max_log_contrast` to `int` outright | One-line struct change | Cascades into ~30 call sites that pass `&window_size` to `uint16_t *adjust_window_size`; SIMD kernels take `uint16_t window_size` and would need an explicit narrow at every call | More churn, more diff to audit, no behavioural difference |
+| Shadow int + `init()` bridge (chosen) | One struct addition, one option-table line each, two assignments in init(); inner-loop signatures preserved; option schema preserved | Two shadow fields cost 8 bytes per `CambiState` instance | Minimal blast radius, surgical, easy to review |
+| For (2), `// NOLINT` and ignore | Zero diff | Real C UB; the compiler is allowed to assume the shifted value is non-negative and miscompile downstream code | UB is not a style issue, and the fix is one cast |
+| For (2), branch on the sign of `filter[k+1]` | Defensive | Hot path; branch is purely cosmetic given the cast already produces bit-exact output | Worse performance, no correctness gain |
+
+## Consequences
+
+- **Positive**: ASan + UBSan run clean across the entire unit-test suite
+  (49 fast + 12 dnn + 2 slow = 63 tests, all OK), and CLI invocations
+  exercising every CPU feature extractor across 4:2:0 8-bit, 4:2:2
+  10-bit, and 4:2:0 12-bit inputs. The fork's CI now has a clean
+  baseline for per-PR ASan/UBSan gating without grandfathered failures.
+- **Positive**: A class of silent struct corruption (option-parser
+  writing 4 bytes into a 2-byte field) is eliminated in CAMBI without
+  touching the option-schema ABI.
+- **Negative**: `CambiState` grows by 8 bytes per instance (negligible —
+  CAMBI allocates multi-megabyte LUTs alongside the state).
+- **Neutral / follow-up**: There is a wider pattern-class to audit
+  (`BOOL` option written to an `int` field in
+  `core/src/feature/float_adm.c::AdmState::adm_adm3_apply_hm`, and
+  `unsigned`-typed fields targeted by `INT` options in `CambiState`
+  for `enc_width` et al.). Neither triggers UBSan today and neither
+  produces silent corruption on any platform we ship for (bool/int and
+  int/unsigned share representation on every two's-complement target),
+  but a follow-up audit should sweep them under a stricter option-type
+  schema.
+
+## References
+
+- Source: agent dispatch — repo-level sanitizer audit on master tip
+  `bbcaa8d127`.
+- ADR-0278 (NOLINT citation closeout) — this PR adds no NOLINTs.
+- `core/src/feature/cambi.c`, `core/src/feature/x86/adm_avx2.c`,
+  `core/src/feature/x86/adm_avx512.c`.
+- Related research digest: `docs/research/sanitizer-pass-2026-05-30.md`.

docs/adr/README.md

@@ -835,3 +835,4 @@ ADRs may exist there for local session continuity, but the tracked
 | [ADR-0887](0887-vmaf-model-slopes-feature-mismatch-validation.md) | Reject JSON models whose per-feature arrays (slopes / intercepts / feature_opts_dicts) disagree in length with feature_names; bound `vmaf_model_destroy` walk by `min(feature_cap, n_features)`. Fixes heap-buffer-overflow read surfaced by `fuzz_json_model` in PR #371. | Accepted | 2026-05-30 | security, parser, model, fuzz, hardening, fork-local |
 | [ADR-0871](0871-ssim-dispatch-pthread-once.md) | Guard SSIM/iqa SIMD dispatch-pointer install with a process-wide `pthread_once_t` shared across `float_ssim.c` and `float_ms_ssim.c` via `iqa_ssim_install_dispatch_once()`. Eliminates 10 TSan data-race warnings on `g_ssim_precompute` / `g_ssim_variance` / `g_ssim_accumulate` / `g_iqa_convolve`. Bit-exact with prior behaviour. | Accepted | 2026-05-30 | tsan, threading, simd, ssim, fork-local |
 | [ADR-0870](0870-helm-values-schema-and-container-rebuild-audit.md) | Add `deploy/helm/vmafx/values.schema.json` (enforces `workload` / `gpu.vendor` / `storage.mode` enums, blocks sibling-key typos via `additionalProperties: false`); fix `dev/Containerfile` ADR-0700 path drift (`COPY libvmaf/` → `COPY core/` + new `COPY compat/`, two `cd libvmaf` → `cd core`, `.dockerignore` `core/build*/` siblings). | Accepted | 2026-05-30 | helm, k8s, deploy, devx, dev-mcp, container, rebase-hygiene, fork-local |
+| [ADR-0869](0869-sanitizer-pass-cleanup.md) | Sanitizer-pass cleanup: shadow `int` slots for CAMBI `window_size` / `max_log_contrast` (UBSan misaligned-store + silent struct overwrite); move `(uint32_t)` cast inside DWT2 filter-pack shifts in AVX2 + AVX-512 ADM (signed-shift UB). Bit-exact with prior behaviour. | Accepted | 2026-05-30 | sanitizer, ubsan, cambi, adm, avx2, avx512, fork-local |

docs/adr/_index_fragments/_order.txt

@@ -594,6 +594,7 @@
 # Backfilled by docs-drift-sweep-2026-05-12 (manifest hygiene; numeric order)
 0860-ffmpeg-patch-chain-no-op-vulkan-shim
 0871-ssim-dispatch-pthread-once
+0869-sanitizer-pass-cleanup
 0887-vmaf-model-slopes-feature-mismatch-validation
 0927-opentelemetry-traces-metrics-phase1
 0925-go-generic-registry

docs/rebase-notes.md

@@ -863,6 +863,32 @@ Fork-local files:
 `docs/adr/0871-ssim-dispatch-pthread-once.md`,
 `docs/research/tsan-race-audit-2026-05-30.md`,
 `changelog.d/fixed/tsan-race-audit.md`.
+## sanitizer-pass-cleanup (2026-05-30, ADR-0869)
+
+**Files touched:**
+- `core/src/feature/cambi.c` — adds two `int` shadow slots
+  (`window_size_opt`, `max_log_contrast_opt`) to `CambiState`; the
+  options table targets them; `init()` copies into the existing
+  `uint16_t` runtime fields.
+- `core/src/feature/x86/adm_avx2.c` — moves the `uint32_t` cast inside
+  the shift in four DWT2 filter-packing expressions.
+- `core/src/feature/x86/adm_avx512.c` — same as AVX2.
+
+**Rebase impact:**
+- **CAMBI**: upstream Netflix's `CambiState` does not have the
+  `_opt` shadow slots. On upstream sync, expect a context conflict on
+  the struct definition and on the two option-table entries. Resolution
+  is to keep the fork's shadow slots and the init-bridge assignments;
+  upstream's option entries should be re-pointed at the `_opt` shadows.
+- **ADM AVX2/AVX-512**: the four filter-packing expressions are
+  upstream-mirrored code. On upstream sync, a textual conflict is
+  possible at every occurrence; the fork's resolution is the
+  inside-cast (`((uint32_t)filter[k] << 16)`). Bit-exact with upstream
+  output; safe to keep.
+
+Verified clean under ASan+UBSan against the full unit-test suite (63
+tests OK) and the vmaf CLI on 4:2:0 8-bit, 4:2:2 10-bit, 4:2:0 12-bit.
+Cambi tuned-options feature-name derivation (`cambi_mlc_3_ws_63`) works.
 
 ---