|
| 1 | +# Triaging Coverity Scan findings |
| 2 | + |
| 3 | +Static analysis runs on [Coverity Scan](https://scan.coverity.com) via |
| 4 | +[`.github/workflows/coverity.yml`](../.github/workflows/coverity.yml) (weekly + on |
| 5 | +demand). Analysis runs on Black Duck's servers; triage is done in the Scan web UI. |
| 6 | + |
| 7 | +Every finding to date is a false positive in **generated** code — the |
| 8 | +Cython-generated `_patch_numpy.c` and the `generate_umath.py`-generated |
| 9 | +`__umath_generated.c` — not in code we maintain. This guide records the verified |
| 10 | +findings and how to keep triage from resetting. |
| 11 | + |
| 12 | +## Where findings come from |
| 13 | + |
| 14 | +`cov-build` captures four C translation units: |
| 15 | + |
| 16 | +- **Hand-written** `mkl_umath/src/ufuncsmodule.c` (ufunc registration + module |
| 17 | + init). Most likely place for a real bug — review every finding. |
| 18 | +- **Template-generated** `mkl_umath_loops.c` (from `mkl_umath_loops.c.src` via |
| 19 | + `_vendored/process_src_template.py`). Not Cython — this is our MKL VM loop logic, |
| 20 | + just type-specialized for float32/float64/complex64/complex128. A real loop bug |
| 21 | + can surface here, so treat it like hand-written code, not boilerplate. |
| 22 | +- **Generated** `__umath_generated.c` (from `mkl_umath/generate_umath.py`): the |
| 23 | + ufunc registration tables built by the `make_ufuncs` template. Findings here are |
| 24 | + ~always false positives in shared generator boilerplate. |
| 25 | +- **Cython-generated** `_patch_numpy.c` (from `_patch_numpy.pyx`): `__Pyx_*` / |
| 26 | + `__pyx_pw_*` / `__pyx_tp_*` helpers and wrappers are boilerplate — findings here |
| 27 | + are ~always false positives. `__pyx_pf_*` functions are the C translation of our |
| 28 | + `.pyx` bodies; a real `.pyx` bug could surface here, though so far all have been |
| 29 | + false positives too (Coverity can't see Python-level invariants). |
| 30 | + |
| 31 | +## Keeping triage durable: the Cython pin |
| 32 | + |
| 33 | +A Cython *version* bump regenerates `_patch_numpy.c` wholesale, which churns the |
| 34 | +Coverity CIDs and silently drops their triage — the same boilerplate then returns |
| 35 | +under new CIDs. So **Cython is pinned in `coverity.yml`** (not `pyproject.toml`, |
| 36 | +so shipped wheels are unaffected). The pin works only because the build runs with |
| 37 | +`--no-build-isolation`; bumping it means re-triaging the boilerplate. The |
| 38 | +`__umath_generated.c` findings churn likewise if `generate_umath.py` or the NumPy |
| 39 | +codegen it mirrors changes. |
| 40 | + |
| 41 | +## Reducing the noise: a Project Component |
| 42 | + |
| 43 | +**Project Settings → Components** buckets defects by a path regex. Define one to |
| 44 | +group (not hide) the generated units so they can be filtered out of view — |
| 45 | +path-based, so it survives regeneration: |
| 46 | + |
| 47 | +- **Name:** `Generated-code` **Path regex:** `.*(_patch_numpy|__umath_generated).*\.c` |
| 48 | + |
| 49 | +This matches only the two generated units. Do **not** group `mkl_umath_loops.c` or |
| 50 | +`ufuncsmodule.c` — those carry our loop and module logic. Group only — do **not** |
| 51 | +mark it *ignored*, as that also drops the `__pyx_pf_*` bodies (see |
| 52 | +[declined](#evaluated-and-declined)). |
| 53 | + |
| 54 | +## Review checklist |
| 55 | + |
| 56 | +Don't blanket-ignore the generated files — prioritise instead: |
| 57 | + |
| 58 | +1. **Findings in `ufuncsmodule.c` and `mkl_umath_loops.c`** — review every one; |
| 59 | + the loop file is our type-specialized MKL VM code, not boilerplate. |
| 60 | +2. **High/Medium findings in `__pyx_pf_*`** — verify against `_patch_numpy.pyx`; if |
| 61 | + it's a Python-level invariant Coverity can't see, mark `False Positive` with a |
| 62 | + reason. |
| 63 | +3. **Known false-positive families below** — carry the recorded disposition; |
| 64 | + match on **checker + mechanism**, not CID (CIDs reset on a Cython bump, a |
| 65 | + `generate_umath.py` change, or an engine upgrade). |
| 66 | + |
| 67 | +## Known false-positive families |
| 68 | + |
| 69 | +Match on **checker + mechanism**, not CID (CIDs reset on a Cython bump or engine |
| 70 | +upgrade). Helper names below are from Cython 3.3.0 and vary between versions. |
| 71 | +All Minor severity, no runtime or security impact. |
| 72 | + |
| 73 | +| Family | Checker | Why it's a false positive | |
| 74 | +| --- | --- | --- | |
| 75 | +| `InitOperators`, `__umath_generated.c` (`make_ufuncs` template) | DEADCODE | The template emits `identity = {expr}; if (has_identity && identity == NULL) return -1;` for every ufunc. For `fmax`/`fmin`/`floor` the identity is `ReorderableNone` = `(Py_INCREF(Py_None), Py_None)`, a non-NULL singleton, so `identity == NULL` is provably false and `return -1` is dead. The check *is* needed for other identities (`PyLong_FromLong`, …). | |
| 76 | +| `__pyx_tp_traverse_...__patch_impl` (Cython `tp_traverse` slot) | DEADCODE | Cython emits a uniform base-type traversal preamble `e = __Pyx_call_type_traverse(...); if (e) return e;`. `_patch_impl` derives from `object`, whose traverse contributes nothing, so the helper returns 0 and the early-return is dead. | |
| 77 | +| `__pyx_pf_..._is_patched` (Cython codegen for `with self._lock:`) | DEADCODE | Cython expands `with` into try/except/finally with both an exception path and a normal-exit path calling `__exit__`. On the normal-exit path the pending-exception temp is NULL, so the `if (__pyx_t_N) {...}` exception-dispatch body is dead. | |
| 78 | +| `__Pyx__Import` (Cython generic import runtime helper) | DEADCODE | With `level == -1` the helper does `package_sep = strchr(__Pyx_MODULE_NAME, '.')` to decide on a relative import. The module is compiled with the plain name `_patch_numpy` (`meson.build`), which has no `.`, so `strchr` always returns NULL and the relative-import branch is dead. The variable is "constant" because the module name is a compile-time constant, not a missing assignment. | |
| 79 | +| `_patch_impl.__cinit__`, `_patch_numpy.pyx:117` | FORWARD_NULL | `self.functions` is left NULL only when `expected_count == 0` (the sum of `ntypes` over all ufuncs). The `self.functions[...]` deref runs only inside `for pi in range(...ntypes)`, i.e. only when `ntypes >= 1`, which forces `expected_count >= 1` and a successful malloc (failure raises `MemoryError`). Coverity can't correlate the two loops through the opaque `ntypes`/`getattr`. | |
| 80 | + |
| 81 | +The four DEADCODE families mark **Intentional**, the FORWARD_NULL marks **False |
| 82 | +Positive** — all with disposition **Ignore**. Optional hardening for the |
| 83 | +FORWARD_NULL: add an explicit `if self.functions is NULL: raise RuntimeError(...)` |
| 84 | +guard before the deref. Not required for correctness. |
| 85 | + |
| 86 | +The three Cython DEADCODE families (`tp_traverse`, the `with`-statement codegen, |
| 87 | +the import helper) plus the FORWARD_NULL are purely Cython `__pyx_*` / `__Pyx_*` |
| 88 | +code, not editable at source level; the `Generated-code` component above retires |
| 89 | +them in bulk. New DEADCODE in generated boilerplate follows the same disposition. |
| 90 | + |
| 91 | +## Evaluated and declined |
| 92 | + |
| 93 | +- **Modeling files** correct the behavior of *called* functions; our FPs are |
| 94 | + intraprocedural (dead branches, compile-time-constant guards, `#if`), which |
| 95 | + models can't reach. |
| 96 | +- **Dropping the generated units** (hard exclude), e.g. after `cov-build`: |
| 97 | + ```bash |
| 98 | + cov-manage-emit --dir cov-int --tu-pattern "file('.*(_patch_numpy|__umath_generated).*\\.c')" delete |
| 99 | + ``` |
| 100 | + Also drops the `__pyx_pf_*` bodies, so it's disabled in favour of the checklist. |
0 commit comments