Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion .github/workflows/coverity.yml
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,9 @@ jobs:
architecture: x64

- name: Install mkl_fft 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
Expand Down
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
### Changed
* Multi-iterator constructors now return a status corresponding to allocation success or failure, and raise `MemoryError` instead of `ValueError` [gh-373](https://github.com/IntelPython/mkl_fft/pull/373)
* `_direct_fftnd` now also checks the status returned by the backend instead of discarding it [gh-373](https://github.com/IntelPython/mkl_fft/pull/373)
* Pinned Cython in the Coverity Scan workflow so generated code stays stable between scans, and added `coverity/README.md` documenting the known false-positive families and the scan review checklist [gh-374](https://github.com/IntelPython/mkl_fft/pull/374)

### Fixed
* Declared `f_ndim` as a C `int` in `_allocate_result` so the buffer size is computed in C rather than through a Python object, resolving a Coverity out-of-bounds (OVERRUN) false positive [gh-364](https://github.com/IntelPython/mkl_fft/pull/364)
Expand Down
99 changes: 99 additions & 0 deletions coverity/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
# Triaging Coverity Scan findings

Static analysis runs on [Coverity Scan](https://scan.coverity.com) via
[`.github/workflows/coverity.yml`](../.github/workflows/coverity.yml) (weekly + on
demand). Analysis runs on Black Duck's servers; triage is done in the Scan web UI.

Every outstanding finding is a false positive — either in Cython-generated
boilerplate in `_pydfti.c`, or in a handful of branches in our own `.pyx` that are
dead-by-construction (Cython already guarantees the guard, or the branch is an
intentional placeholder). This guide records the verified findings and how to keep
triage from resetting.

## Where findings come from

`cov-build` captures two C translation units (the `.h` files under
`mkl_fft/src/` are `#include`d into them, not compiled on their own):

- **Template-generated** `mklfft.c` (from `mkl_fft/src/mklfft.c.src` via
`_vendored/process_src_template.py`, wired up in `meson.build`). This is *not*
Cython — it is our oneMKL DFTI descriptor/compute logic, just type-specialized
for float32/float64/complex64/complex128. It pulls in the hand-written helpers
in `mkl_fft/src/multi_iter.h` and `mkl_fft/src/mklfft.h`. A real bug can surface
here, so treat it like hand-written code, **not** boilerplate — review every
finding.
- **Cython-generated** `_pydfti.c` (from `mkl_fft/_pydfti.pyx`): `__Pyx_*` /
`__pyx_pw_*` / `__pyx_tp_*` helpers and wrappers are boilerplate — findings here
are ~always false positives. `__pyx_pf_*` functions are the C translation of our
`.pyx` bodies; a real `.pyx` bug could surface there, so keep them in scope
(Coverity can't see Python-level invariants — see the `_allocate_result`
OVERRUN fixed in [gh-364](https://github.com/IntelPython/mkl_fft/pull/364)).

## Keeping triage durable: the Cython pin

A Cython *version* bump regenerates `_pydfti.c` wholesale, which churns the
Coverity CIDs and silently drops their triage — the same boilerplate then returns
under new CIDs. So **Cython is pinned in `coverity.yml`** (not `pyproject.toml`,
so shipped wheels are unaffected). The pin works only because the build runs with
`--no-build-isolation`; bumping it means re-triaging the boilerplate.

## Reducing the noise: a Project Component

**Project Settings → Components** buckets defects by a path regex. Define one to
group (not hide) the Cython unit so it can be filtered out of view — path-based,
so it survives regeneration:

- **Name:** `Generated-code` **Path regex:** `.*_pydfti\.c`

Do **not** group `mklfft.c` (or the `mkl_fft/src/*.h` helpers) — that is our DFTI
logic, not boilerplate. Group only — do **not** mark it *ignored*, as that also
drops the `__pyx_pf_*` bodies (see [declined](#evaluated-and-declined)).

## Review checklist

Don't blanket-ignore the generated file — prioritise instead:

1. **Findings in `mklfft.c` and `mkl_fft/src/*.h`** — review every one; this is
our type-specialized oneMKL DFTI code and its inlined iterator/cache helpers,
not boilerplate.
2. **High/Medium findings in `__pyx_pf_*`** — verify against `_pydfti.pyx`; if it's
a Python-level invariant Coverity can't see, mark `False Positive` with a
reason. If it's a genuine defect, fix the `.pyx` (as with the `_allocate_result`
OVERRUN).
3. **Known false-positive families below** — carry the recorded disposition;
match on **checker + mechanism**, not CID (CIDs reset on a Cython bump or an
engine upgrade).

## Known false-positive families

Match on **checker + mechanism**, not CID — CIDs reset on a Cython bump or engine
upgrade. Helper names are from Cython 3.3.0 and vary between versions. All Minor
severity, no runtime or security impact. Every family below is triaged
**Intentional / Ignore**.

### Cython-generated boilerplate (in `_pydfti.c` `__Pyx_*` / `__pyx_tp_*` helpers)

| Family | Checker | Why it's a false positive |
| --- | --- | --- |
| `tp_traverse` slots — `__Pyx_Coroutine_traverse`, `__Pyx_CyFunction_traverse`, and the genexpr/closure scope traversals for `_genexpr` and `_get_element_strides` (one mechanism) | DEADCODE | Cython emits a uniform base-type traversal preamble `e = __Pyx_call_type_traverse(o, 1, v, a); if (e) return e;`. The traversed object derives from a base whose traverse contributes nothing, so the helper returns 0 and the early-return is dead. The preamble *is* needed for objects deriving from a GC type. |
| `__Pyx_VectorcallBuilder_AddArg`, `__Pyx_PyCode_New`, `__Pyx_ParseKeywordDict` | DEADCODE | Dead branches in version/ABI-guarded runtime helpers — arms selected out by the CPython version the wheel is built against, or by an argument shape the call site never produces. |
| `_c2r_fft1d_impl` reference-cleanup epilogue | UNUSED_VALUE | Cython's generated `finally`/decref epilogue assigns a temp (e.g. resets a borrowed slot to `NULL`) that is never read again before the function returns. |
| `__Pyx_Generator_Replace_StopIteration` | CHECKED_RETURN | Boilerplate deliberately drops the return value of a call whose result is not needed on that path. |

### Our `.pyx`, dead-by-construction (in `__pyx_pf_*` bodies)

| Family | Checker | Why it's a false positive |
| --- | --- | --- |
| Redundant `is NULL` guard after an array-returning call — `_process_arguments` and `_direct_fftnd` | DEADCODE | The `.pyx` has an explicit `if <array> is NULL:` guard, but because the variable is typed `cnp.ndarray`, Cython already emits its own NULL/error check right after the returning call (e.g. `PyArray_CheckFromAny`). By the time our guard runs the value is provably non-NULL, so our guard body is dead. Harmless defensive code — kept for readability. |
| `_c2r_fft1d_impl` in-place stub | DEADCODE | `in_place = 0` is assigned immediately before `if in_place:`, so the `# TODO: Provide in-place functionality` branch is provably unreachable. Intentional placeholder for future in-place `irfft` support. |

## Evaluated and declined

- **Modeling files** correct the behavior of *called* functions; our FPs are
intraprocedural (dead branches, compile-time-constant guards, `#if`), which
models can't reach.
- **Dropping the generated unit** (hard exclude), e.g. after `cov-build`:
```bash
cov-manage-emit --dir cov-int --tu-pattern "file('.*_pydfti\\.c')" delete
```
Also drops the `__pyx_pf_*` bodies, so it's disabled in favour of the checklist.
Loading