From e4a51883170327b46174030c08fa68877d012148 Mon Sep 17 00:00:00 2001 From: Anton Volkov Date: Mon, 24 Aug 2026 13:43:19 +0200 Subject: [PATCH] Stabilize Coverity Scan triage and document known false positives Pin Cython in the Coverity Scan workflow (only) so the generated mklrand.cpp is byte-stable between scans. Coverity derives CIDs from a hash of the analyzed code, so an unpinned Cython bump regenerates the file, resets every CID, and silently discards prior triage on the Cython-boilerplate false positives. Production builds keep Cython unpinned in pyproject.toml, so this does not affect shipped wheels or Python support. Add coverity/README.md recording the known false-positive families in generated code (DEADCODE / UNUSED_VALUE / CHECKED_RETURN / OUT_OF_BOUNDS), the verification of the High-severity _seed_impl OOB finding, the real INTEGER_OVERFLOW fix (gh-156), and a review checklist that keeps first-party src/*.cpp and __pyx_pf_* bodies in scope rather than blanket-excluding the generated unit. --- .github/workflows/coverity.yml | 4 +- CHANGELOG.md | 1 + coverity/README.md | 177 +++++++++++++++++++++++++++++++++ 3 files changed, 181 insertions(+), 1 deletion(-) create mode 100644 coverity/README.md diff --git a/.github/workflows/coverity.yml b/.github/workflows/coverity.yml index 86c4e661..22e4d37e 100644 --- a/.github/workflows/coverity.yml +++ b/.github/workflows/coverity.yml @@ -50,7 +50,9 @@ jobs: architecture: x64 - name: Install build dependencies - run: pip install meson-python ninja cmake cython "numpy>=2" mkl-devel + # Cython is pinned here only (not in pyproject.toml) to keep the generated + # code stable between scans, so Coverity CIDs and their triage survive + run: pip install meson-python ninja cmake "cython==3.3.0" "numpy>=2" mkl-devel - name: Download Coverity Build Tool timeout-minutes: 15 diff --git a/CHANGELOG.md b/CHANGELOG.md index eff67b71..d16d3421 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added ### Changed +* Pinned Cython in the Coverity Scan workflow so generated code stays stable between scans, and added `coverity/README.md` documenting the known Cython-boilerplate false positives and the scan review checklist [gh-164](https://github.com/IntelPython/mkl_random/pull/164) ### Fixed diff --git a/coverity/README.md b/coverity/README.md new file mode 100644 index 00000000..7c21d608 --- /dev/null +++ b/coverity/README.md @@ -0,0 +1,177 @@ +# Triaging Coverity Scan findings + +This is the guide for reviewing static-analysis results for `mkl_random`. It has +three parts: + +1. **[Policy](#policy)** — how to approach *any* scan: where findings come from, + why triage keeps resetting, and the checklist to work through each new report. +2. **[Known findings](#known-findings)** — a catalog of findings already + reviewed, so they are not re-investigated from scratch every scan. +3. **[Evaluated and declined](#evaluated-and-declined)** — approaches considered + for cutting the noise, and why they were not adopted. + +Static analysis runs on the free [Coverity Scan](https://scan.coverity.com) +service via [`.github/workflows/coverity.yml`](../.github/workflows/coverity.yml) +(weekly, plus `workflow_dispatch`). Analysis happens on Black Duck's servers; +triage (Classification / Action / Comment) is done in the Coverity Scan web UI +and is keyed by **CID**. + +--- + +## Policy + +### Where findings come from + +Coverity analyzes two kinds of source: + +- **Generated code** — `mklrand.cpp`, produced by Cython from `mklrand.pyx`. +- **Hand-written code** — the C++ under `mkl_random/src/` and its headers. + +Within the generated `mklrand.cpp`, two kinds of functions appear: + +- **`__Pyx_*`, `__pyx_pw_*`, `__pyx_tp_*`, `__pyx_mdef_*`** — Cython runtime + boilerplate and Python-level wrappers. Findings here are ~always false + positives. **Not editable** — regenerated on every build. +- **`__pyx_pf_*`** — the C translation of the *bodies* of our `.pyx` functions. + A genuine logic bug in `mklrand.pyx` could in principle surface here, so these + are **not** blanket-dismissed — but in practice every `__pyx_pf_*` finding to + date has also been a false positive, because Coverity cannot see Python-level + invariants (dtype sizes ≥ 0, fixed-length return tuples, etc.). + +The vast majority of findings are boilerplate artifacts in generated code, driven +by Cython's single-source-for-many-build-configs templating (macros that expand +to constants, `#if`-selected version branches). They are not defects in code we +maintain. + +### Why triage resets, and the Cython pin + +A Coverity CID is meant to survive small code edits, but a Cython *version* bump +regenerates `mklrand.cpp` wholesale — renaming helper functions and reshuffling +line structure — which churns the CIDs across the generated unit and silently +drops the triage attached to them. The same boilerplate then reappears under new +CID numbers. This is exactly what happened once already. + +To keep triage durable, **Cython is pinned in `coverity.yml`**. +The pin only takes effect because the build step runs with +`--no-build-isolation`, so meson-python uses the pinned Cython from the +environment rather than build-isolating and pulling the latest; if that flag is +ever removed, the pin becomes a no-op. Production builds leave Cython unpinned in +`pyproject.toml`, so this does not constrain shipped wheels or Python support. +Bumping the pin is a deliberate act; expect to re-triage the boilerplate +afterwards using the [known-findings catalog](#known-findings) below. + +(The generated bytes also depend on the numpy/cpython `.pxd` files, so the pin is +not an absolute guarantee — but those change far less often than Cython itself.) + +(Per-function suppression that would let Cython float freely is a Coverity +*Connect* feature, not available on the free Scan service — analysis runs on +Black Duck's servers, so the only repo-side lever is which translation units are +uploaded. See the [hard-exclude option](#optional-dropping-the-generated-unit) +for the one path-based alternative.) + +### Group generated code with a Project Component + +Coverity Scan's **Project Settings → Components** lets you define a named +component from a **regex matched against each defect's file path**. Defects are +then bucketed under their component, so you can filter the generated-code noise +out of view in one click while the hand-written code stays front-and-centre. This +is path-based, so it survives Cython version bumps (unlike CID-keyed triage). + +Recommended component to group (not hide) the generated unit: + +- **Name:** `Cython-generated` +- **Path regex:** `.*/mklrand\.cpython.*` + +That pattern matches only the generated `mklrand.cpp` (the analyzed path looks +like `/build/cp312/mklrand.cpython-312-...`); the hand-written sources live under +`mkl_random/src/` — with an underscore, no `mklrand` substring — so they are not +caught. + +Two caveats: + +- A component is **path-granular**, so it cannot separate the `__Pyx_*` + boilerplate from the `__pyx_pf_*` bodies (both live in `mklrand.cpp`). That is + fine for *grouping*; it is why the same page's option to mark a component + *ignored* (dropping its defects from analysis entirely) is **not** recommended + here — it would also drop the `__pyx_pf_*` bodies. Same trade-off as the + [hard-exclude option](#optional-dropping-the-generated-unit). +- Grouping complements the Cython pin; it does not replace it. The pin keeps + triage from resetting; the component keeps the noise visually contained. + +### Review checklist for each new scan + +Do **not** blanket-ignore the generated unit — that could hide a genuine +`.pyx`-logic bug. Instead, prioritise: + +1. **Any finding in the hand-written code** under `mkl_random/src/`. This is the + most likely place for a genuine defect — review every one. +2. **Any High/Medium finding in a `__pyx_pf_*` function** (our translated logic). + Verify against the `.pyx` source; if it reduces to a Python-level invariant + Coverity can't see (dtype size, fixed tuple length, guarded index), mark it + `False Positive` with a one-line reason. +3. **Everything matching the boilerplate families below** — triage + `False Positive` / `Ignore` in bulk, referencing this file. + +--- + +## Known findings + +Match a new finding on its **checker + mechanism**, not its CID number — CIDs get +reassigned when the Cython pin is bumped or when Black Duck upgrades the analysis +engine. The example function names below are from the pinned Cython 3.3.0 output; +Cython renames these helpers between versions (e.g. the vectorcall builder was +`__Pyx_VectorcallBuilder_AddArg` before 3.3.0), so treat them as illustrative. + +### False-positive families in generated `mklrand.cpp` — triage `False Positive` / `Ignore` + +| Family | Checker | Why it's a false positive | +| --- | --- | --- | +| `__pyx_tp_traverse_*`, `__Pyx_CyFunction_traverse` | DEADCODE | `__Pyx_call_type_traverse` expands to the constant-`0` macro on standard-CPython builds, so `if (e) return e;` is dead. Guard is live only in the Limited-API build variant. | +| `__Pyx_PyCode_New`, `__Pyx_CallSlotAsVectorcallUnpackDict` | DEADCODE | `__Pyx_PyTuple_SET_ITEM` expands to the void `PyTuple_SET_ITEM` yielding constant `0`, so `if (... != 0)` is dead. Guard is live only in the Limited-API variant. | +| `__Pyx_AddTraceback` | DEADCODE | `c_line` is fixed at `0` unless the optional `CYTHON_CLINE_IN_TRACEBACK` feature is enabled, making the `-c_line` branch dead by default. | +| `__Pyx_ParseKeywordDict` | DEADCODE | In the CPython < 3.13 branch `found` is only ever 0/1, so `if (found < 0)` is dead; the guard serves the ≥ 3.13 `PyDict_GetItemRef` path. | +| `__Pyx_PyLong_As_*` (npy_int16/uint8/int32/uint32/npy_bool/int/uint16/unsigned_int/npy_int8/irk_brng_t, …) | DEADCODE | Per-C-type integer-conversion helper templates; dead branches are compile-time-selected version/overflow guards. | +| `__pyx_pf_*` bodies with temp cleanup (e.g. `choice`, `multivariate_normal`) | UNUSED_VALUE | `__pyx_t_N = 0;` nulls a temporary after its reference is transferred (`__Pyx_DECREF_SET` / assignment), guarding the error path against a double-DECREF. Dead only on the straight-line path. | +| `__pyx_pw_*` keyword wrappers (`rand`, `randn`, …) | CHECKED_RETURN | `PyDict_Size` (via `__Pyx_NumKwargs_VARARGS`) is captured into `__pyx_kwds_len` and checked on the *next* line (`if (... < 0) __PYX_ERR(...)`). The statistical heuristic misfires because the check is one line from the call, not inline. | + +There is also a **`sanity_check_for_cython`** DEADCODE finding, from meson's own +compiler-probe translation unit rather than `mklrand.cpp` — same disposition +(`False Positive` / `Ignore`), it just lives outside the generated file. + +### Individually verified false positive + +- **OUT_OF_BOUNDS in `_seed_impl`** (flagged High): the tuple unpack + `brng_token, stream_id = _parse_brng_argument(brng)` (`mklrand.pyx:1571`) + generates `PyTuple_GET_ITEM(sequence, 1)`, guarded by the generated + `if (unlikely(size != 2)) { … __PYX_ERR(...) }` — so the index-1 read is only + reached when `size == 2`. Independently, `_parse_brng_argument` always returns a + fixed 2-tuple (`mklrand.pyx:1533`). Coverity does not tie the `ob_item[1]` + access back to the guard across the macro. No out-of-bounds access is possible. + +--- + +## Evaluated and declined + +### Modeling files + +Coverity's modeling-file feature corrects the *behavior of called functions* it +can't infer (custom allocators, panics, sanitizers). Almost all of our false +positives are intraprocedural artifacts of generated code, `#if` branches, and +macros — none of which a callee model can reach. The one partial exception is the +`PyDict_Size` CHECKED_RETURN family, which a function model could influence — but +that is a couple of findings against a CPython/Cython-internal callee, not worth +the modeling maintenance. So modeling files are not adopted here. + +### Optional: dropping the generated unit + +If the boilerplate ever outweighs its value, the generated translation unit can be +removed from analysis *before upload* (repo-side, durable, path-based) by +inserting this after the `cov-build` step in `coverity.yml`: + +```bash +cov-manage-emit --dir cov-int --tu-pattern "file('.*mklrand.*\\.cpp')" delete +``` + +This is a "hard exclude" — it also drops the `__pyx_pf_*` bodies, trading away the +(so-far theoretical) chance of catching a `.pyx`-logic bug for zero boilerplate +noise. Left disabled by default in favour of the review checklist above.