Skip to content

Commit 4689d78

Browse files
committed
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.
1 parent 36aaadf commit 4689d78

3 files changed

Lines changed: 138 additions & 1 deletion

File tree

‎.github/workflows/coverity.yml‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,14 @@ jobs:
5050
architecture: x64
5151

5252
- name: Install build dependencies
53-
run: pip install meson-python ninja cmake cython "numpy>=2" mkl-devel
53+
# Cython is pinned here (and ONLY here) so the generated mklrand.cpp is
54+
# byte-stable between scans. Coverity assigns CIDs from a hash of the
55+
# analyzed code, so an unpinned Cython bump regenerates the file and
56+
# resets every CID -- silently discarding all prior triage on the
57+
# Cython-boilerplate false positives (see coverity/README.md). Production
58+
# builds intentionally leave Cython unpinned in pyproject.toml. Bump this
59+
# deliberately, and expect to re-triage the boilerplate afterwards.
60+
run: pip install meson-python ninja cmake "cython==3.2.9" "numpy>=2" mkl-devel
5461

5562
- name: Download Coverity Build Tool
5663
timeout-minutes: 15

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
99
### Added
1010

1111
### Changed
12+
* Pinned Cython in the Coverity Scan workflow so generated code stays byte-stable between scans (keeping CIDs and their triage from resetting on Cython releases), and added `coverity/README.md` documenting the known Cython-boilerplate false positives and the scan review checklist
1213

1314
### Fixed
1415

‎coverity/README.md‎

Lines changed: 129 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,129 @@
1+
# Coverity triage policy
2+
3+
Static analysis for `mkl_random` runs on the free [Coverity Scan](https://scan.coverity.com)
4+
service via [`.github/workflows/coverity.yml`](../.github/workflows/coverity.yml)
5+
(weekly, plus `workflow_dispatch`). Analysis happens on Black Duck's servers;
6+
triage (Classification / Action / Comment) is done in the Coverity Scan web UI
7+
and is keyed by **CID**.
8+
9+
This document exists because the extension is built with **Cython**, and the vast
10+
majority of Coverity findings are in the **generated** `mklrand.cpp` translation
11+
unit, not in code we maintain. Those findings are boilerplate artifacts that
12+
recur on every scan. This file records which families are known false positives,
13+
how to triage them once, and — importantly — where the *real* signal lives so it
14+
never gets lost in the noise.
15+
16+
## Where findings come from
17+
18+
The build has three C/C++ translation units captured by `cov-build`:
19+
20+
| Source | Nature | Who owns it |
21+
| --- | --- | --- |
22+
| `mkl_random/mklrand.pyx` → generated `mklrand.cpp` | machine-generated by Cython | Cython (regenerated every build) |
23+
| `mkl_random/src/mkl_distributions.cpp` | hand-written C++ | us |
24+
| `mkl_random/src/*.cpp` (other src files) | hand-written C++ | us |
25+
26+
Within the generated `mklrand.cpp`, two kinds of functions appear:
27+
28+
- **`__Pyx_*`, `__pyx_pw_*`, `__pyx_tp_*`, `__pyx_mdef_*`** — Cython runtime
29+
boilerplate and Python-level wrappers. Findings here are ~always false
30+
positives (see families below). **Not editable** — regenerated on every build.
31+
- **`__pyx_pf_*`** — the C translation of the *bodies* of our `.pyx` functions.
32+
A genuine logic bug in `mklrand.pyx` could in principle surface here, so these
33+
are **not** blanket-dismissed — but in practice every `__pyx_pf_*` finding to
34+
date has also been a false positive, because Coverity cannot see Python-level
35+
invariants (dtype sizes ≥ 0, fixed-length return tuples, etc.).
36+
37+
## CID stability and the Cython pin
38+
39+
Coverity derives a CID from a hash of the analyzed code. When Cython regenerates
40+
`mklrand.cpp` differently — which a new Cython release does — **every CID in the
41+
generated unit changes**, and all prior triage is silently lost. This is exactly
42+
what happened when the scan picked up a new Cython release: a fresh batch of
43+
identical boilerplate reappeared under new CID numbers, plus one new
44+
High-severity finding that needed a look (it was benign — see below).
45+
46+
To keep triage durable, **Cython is pinned in `coverity.yml` only**
47+
(`cython==3.2.9`). Production builds leave Cython unpinned in `pyproject.toml`,
48+
so this does not constrain shipped wheels or future Python support. Bumping the
49+
pin is a deliberate act; expect to re-triage the boilerplate afterwards using the
50+
table below.
51+
52+
## Known false-positive families (triage as `False Positive` / `Ignore`)
53+
54+
All of these are in generated `mklrand.cpp`. Root cause and example CIDs from the
55+
scans reviewed on 2026-08-24:
56+
57+
| Family | Checker | Why it's a false positive |
58+
| --- | --- | --- |
59+
| `__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. (e.g. CID 652704, 652707) |
60+
| `__Pyx_VectorcallBuilder_AddArg`, `__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. (e.g. CID 652697, 652705) |
61+
| `__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. (e.g. CID 652698) |
62+
| `__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. (e.g. CID 652703) |
63+
| `__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. |
64+
| `sanity_check_for_cython` | DEADCODE | Cython/meson build sanity-check stub. |
65+
| `__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. (e.g. CID 652699, 652700) |
66+
| `__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 (__pyx_kwds_len < 0) return NULL;`). The statistical heuristic misfires because the check is one line from the call, not inline. (e.g. CID 652972, 652973) |
67+
68+
### Verified: CID 653949 (OUT_OF_BOUNDS in `_seed_impl`) — false positive
69+
70+
Flagged High. The tuple unpack `brng_token, stream_id = _parse_brng_argument(brng)`
71+
(`mklrand.pyx:1571`) generates `PyTuple_GET_ITEM(sequence, 1)`. It is guarded by
72+
the generated `if (unlikely(size != 2)) { … __PYX_ERR(...) }` check, so the
73+
index-1 read is only reached when `size == 2`. Independently,
74+
`_parse_brng_argument` always returns a fixed 2-tuple (`mklrand.pyx:1533`).
75+
Coverity does not tie the `ob_item[1]` access back to the `size != 2` guard
76+
across the macro. No out-of-bounds access is possible.
77+
78+
## Real findings fixed
79+
80+
- **CID 652701** (INTEGER_OVERFLOW, `irk_rand_uint32_vec`, hand-written
81+
`mkl_distributions.cpp`): `npy_int32 shift = (npy_uint32)INT_MAX + 1` = 2**31
82+
overflowed the signed type. Fixed by declaring `shift` as `npy_uint32`
83+
(behavior unchanged; all uses are modulo-2**32). See gh-156.
84+
85+
This is the only real defect the scans surfaced so far, and it was in
86+
**hand-written** code — consistent with the guidance below.
87+
88+
## Review checklist for each new scan
89+
90+
Do **not** blanket-ignore the generated unit — that could hide a genuine
91+
`.pyx`-logic bug. Instead, prioritise:
92+
93+
1. **Any finding in `mkl_random/src/*.cpp`** (hand-written C++). This is where the
94+
only real defect so far lived. Review every one.
95+
2. **Any High/Medium finding in a `__pyx_pf_*` function** (our translated logic).
96+
Verify against the `.pyx` source; if it reduces to a Python-level invariant
97+
Coverity can't see (dtype size, fixed tuple length, guarded index), mark it
98+
`False Positive` with a one-line reason.
99+
3. **Everything matching the boilerplate families above** — triage
100+
`False Positive` / `Ignore` in bulk, referencing this file.
101+
102+
## Optional: component grouping in the Scan UI
103+
104+
If the Scan project's settings expose a component map, add a component that
105+
isolates the generated unit so the noise can be filtered in one click. Suggested
106+
file-path regex (matches the meson build output seen in the reports,
107+
`/build/cpXXX/mklrand.cpython-XXX-…`):
108+
109+
```
110+
.*/mklrand\.cpython-[0-9]+.*\.(c|cpp)$
111+
```
112+
113+
Hand-written first-party code stays outside this component and remains the
114+
default review target.
115+
116+
## Optional: dropping the generated unit entirely
117+
118+
If the boilerplate ever outweighs its value, the generated translation unit can
119+
be removed from analysis *before upload* (repo-side, durable, path-based) by
120+
inserting this after the `cov-build` step in `coverity.yml`:
121+
122+
```bash
123+
cov-manage-emit --dir cov-int --tu-pattern "file('.*mklrand.*\\.cpp')" delete
124+
```
125+
126+
This is the "hard exclude" — it also drops the `__pyx_pf_*` bodies, so it trades
127+
away the (so-far theoretical) chance of catching a `.pyx`-logic bug for zero
128+
boilerplate noise. Left disabled by default in favour of the review checklist
129+
above.

0 commit comments

Comments
 (0)