|
| 1 | +# Triaging Coverity Scan findings |
| 2 | + |
| 3 | +This is the guide for reviewing static-analysis results for `mkl_random`. It has |
| 4 | +three parts: |
| 5 | + |
| 6 | +1. **[Policy](#policy)** — how to approach *any* scan: where findings come from, |
| 7 | + why triage keeps resetting, and the checklist to work through each new report. |
| 8 | +2. **[Known findings](#known-findings)** — a catalog of findings already |
| 9 | + reviewed, so they are not re-investigated from scratch every scan. |
| 10 | +3. **[Evaluated and declined](#evaluated-and-declined)** — approaches considered |
| 11 | + for cutting the noise, and why they were not adopted. |
| 12 | + |
| 13 | +Static analysis runs on the free [Coverity Scan](https://scan.coverity.com) |
| 14 | +service via [`.github/workflows/coverity.yml`](../.github/workflows/coverity.yml) |
| 15 | +(weekly, plus `workflow_dispatch`). Analysis happens on Black Duck's servers; |
| 16 | +triage (Classification / Action / Comment) is done in the Coverity Scan web UI |
| 17 | +and is keyed by **CID**. |
| 18 | + |
| 19 | +--- |
| 20 | + |
| 21 | +## Policy |
| 22 | + |
| 23 | +### Where findings come from |
| 24 | + |
| 25 | +Coverity analyzes two kinds of source: |
| 26 | + |
| 27 | +- **Generated code** — `mklrand.cpp`, produced by Cython from `mklrand.pyx`. |
| 28 | +- **Hand-written code** — the C++ under `mkl_random/src/` and its headers. |
| 29 | + |
| 30 | +Within the generated `mklrand.cpp`, two kinds of functions appear: |
| 31 | + |
| 32 | +- **`__Pyx_*`, `__pyx_pw_*`, `__pyx_tp_*`, `__pyx_mdef_*`** — Cython runtime |
| 33 | + boilerplate and Python-level wrappers. Findings here are ~always false |
| 34 | + positives. **Not editable** — regenerated on every build. |
| 35 | +- **`__pyx_pf_*`** — the C translation of the *bodies* of our `.pyx` functions. |
| 36 | + A genuine logic bug in `mklrand.pyx` could in principle surface here, so these |
| 37 | + are **not** blanket-dismissed — but in practice every `__pyx_pf_*` finding to |
| 38 | + date has also been a false positive, because Coverity cannot see Python-level |
| 39 | + invariants (dtype sizes ≥ 0, fixed-length return tuples, etc.). |
| 40 | + |
| 41 | +The vast majority of findings are boilerplate artifacts in generated code, driven |
| 42 | +by Cython's single-source-for-many-build-configs templating (macros that expand |
| 43 | +to constants, `#if`-selected version branches). They are not defects in code we |
| 44 | +maintain. |
| 45 | + |
| 46 | +### Why triage resets, and the Cython pin |
| 47 | + |
| 48 | +A Coverity CID is meant to survive small code edits, but a Cython *version* bump |
| 49 | +regenerates `mklrand.cpp` wholesale — renaming helper functions and reshuffling |
| 50 | +line structure — which churns the CIDs across the generated unit and silently |
| 51 | +drops the triage attached to them. The same boilerplate then reappears under new |
| 52 | +CID numbers. This is exactly what happened once already. |
| 53 | + |
| 54 | +To keep triage durable, **Cython is pinned in `coverity.yml`**. |
| 55 | +The pin only takes effect because the build step runs with |
| 56 | +`--no-build-isolation`, so meson-python uses the pinned Cython from the |
| 57 | +environment rather than build-isolating and pulling the latest; if that flag is |
| 58 | +ever removed, the pin becomes a no-op. Production builds leave Cython unpinned in |
| 59 | +`pyproject.toml`, so this does not constrain shipped wheels or Python support. |
| 60 | +Bumping the pin is a deliberate act; expect to re-triage the boilerplate |
| 61 | +afterwards using the [known-findings catalog](#known-findings) below. |
| 62 | + |
| 63 | +(The generated bytes also depend on the numpy/cpython `.pxd` files, so the pin is |
| 64 | +not an absolute guarantee — but those change far less often than Cython itself.) |
| 65 | + |
| 66 | +(Per-function suppression that would let Cython float freely is a Coverity |
| 67 | +*Connect* feature, not available on the free Scan service — analysis runs on |
| 68 | +Black Duck's servers, so the only repo-side lever is which translation units are |
| 69 | +uploaded. See the [hard-exclude option](#optional-dropping-the-generated-unit) |
| 70 | +for the one path-based alternative.) |
| 71 | + |
| 72 | +### Review checklist for each new scan |
| 73 | + |
| 74 | +Do **not** blanket-ignore the generated unit — that could hide a genuine |
| 75 | +`.pyx`-logic bug. Instead, prioritise: |
| 76 | + |
| 77 | +1. **Any finding in the hand-written code** under `mkl_random/src/`. This is the |
| 78 | + most likely place for a genuine defect — review every one. |
| 79 | +2. **Any High/Medium finding in a `__pyx_pf_*` function** (our translated logic). |
| 80 | + Verify against the `.pyx` source; if it reduces to a Python-level invariant |
| 81 | + Coverity can't see (dtype size, fixed tuple length, guarded index), mark it |
| 82 | + `False Positive` with a one-line reason. |
| 83 | +3. **Everything matching the boilerplate families below** — triage |
| 84 | + `False Positive` / `Ignore` in bulk, referencing this file. |
| 85 | + |
| 86 | +--- |
| 87 | + |
| 88 | +## Known findings |
| 89 | + |
| 90 | +Match a new finding on its **checker + mechanism**, not its CID number — CIDs get |
| 91 | +reassigned when the Cython pin is bumped or when Black Duck upgrades the analysis |
| 92 | +engine. The example function names below are from the pinned Cython 3.3.0 output; |
| 93 | +Cython renames these helpers between versions (e.g. the vectorcall builder was |
| 94 | +`__Pyx_VectorcallBuilder_AddArg` before 3.3.0), so treat them as illustrative. |
| 95 | + |
| 96 | +### False-positive families in generated `mklrand.cpp` — triage `False Positive` / `Ignore` |
| 97 | + |
| 98 | +| Family | Checker | Why it's a false positive | |
| 99 | +| --- | --- | --- | |
| 100 | +| `__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. | |
| 101 | +| `__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. | |
| 102 | +| `__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. | |
| 103 | +| `__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. | |
| 104 | +| `__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. | |
| 105 | +| `__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. | |
| 106 | +| `__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. | |
| 107 | + |
| 108 | +There is also a **`sanity_check_for_cython`** DEADCODE finding, from meson's own |
| 109 | +compiler-probe translation unit rather than `mklrand.cpp` — same disposition |
| 110 | +(`False Positive` / `Ignore`), it just lives outside the generated file. |
| 111 | + |
| 112 | +### Individually verified false positive |
| 113 | + |
| 114 | +- **OUT_OF_BOUNDS in `_seed_impl`** (flagged High): the tuple unpack |
| 115 | + `brng_token, stream_id = _parse_brng_argument(brng)` (`mklrand.pyx:1571`) |
| 116 | + generates `PyTuple_GET_ITEM(sequence, 1)`, guarded by the generated |
| 117 | + `if (unlikely(size != 2)) { … __PYX_ERR(...) }` — so the index-1 read is only |
| 118 | + reached when `size == 2`. Independently, `_parse_brng_argument` always returns a |
| 119 | + fixed 2-tuple (`mklrand.pyx:1533`). Coverity does not tie the `ob_item[1]` |
| 120 | + access back to the guard across the macro. No out-of-bounds access is possible. |
| 121 | + |
| 122 | +--- |
| 123 | + |
| 124 | +## Evaluated and declined |
| 125 | + |
| 126 | +### Modeling files |
| 127 | + |
| 128 | +Coverity's modeling-file feature corrects the *behavior of called functions* it |
| 129 | +can't infer (custom allocators, panics, sanitizers). Almost all of our false |
| 130 | +positives are intraprocedural artifacts of generated code, `#if` branches, and |
| 131 | +macros — none of which a callee model can reach. The one partial exception is the |
| 132 | +`PyDict_Size` CHECKED_RETURN family, which a function model could influence — but |
| 133 | +that is a couple of findings against a CPython/Cython-internal callee, not worth |
| 134 | +the modeling maintenance. So modeling files are not adopted here. |
| 135 | + |
| 136 | +### Optional: dropping the generated unit |
| 137 | + |
| 138 | +If the boilerplate ever outweighs its value, the generated translation unit can be |
| 139 | +removed from analysis *before upload* (repo-side, durable, path-based) by |
| 140 | +inserting this after the `cov-build` step in `coverity.yml`: |
| 141 | + |
| 142 | +```bash |
| 143 | +cov-manage-emit --dir cov-int --tu-pattern "file('.*mklrand.*\\.cpp')" delete |
| 144 | +``` |
| 145 | + |
| 146 | +This is a "hard exclude" — it also drops the `__pyx_pf_*` bodies, trading away the |
| 147 | +(so-far theoretical) chance of catching a `.pyx`-logic bug for zero boilerplate |
| 148 | +noise. Left disabled by default in favour of the review checklist above. |
0 commit comments