74d41e99a8
Session 1 (2026-05-31) resolved 26 findings across Sonnet/Haiku/Opus; 298/298 CGAL tests green. Bring the audit docs in line with reality: - README.md: G0 banner now records that the original authors were contacted by email about porting/relicensing rights (awaiting reply); headline-status cells updated per finding; new "Session 1 — implemented" record; link to the plan. - Per-audit resolution banners (api-performance, numerical-stability, input-validation, test-coverage, math-citation, thread-safety) + G0-blocked banners (cgal-submission, dependency-license). - NEW finding-orchestration.md: the meta-plan mapping every finding to a session, an implementing model (Haiku/Sonnet/Opus by the "how much must be understood" rule), and an Opus review gate after each implementation session. Records S1 (done) and lays out S2–S6 so the next session can be picked up cold. Docs only — no code change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
145 lines
6.4 KiB
Markdown
145 lines
6.4 KiB
Markdown
# Thread-Safety & Reentrancy Audit — ConformalLabpp
|
|
|
|
**Date:** 2026-05-31
|
|
**Auditor:** External reviewer (Claude Opus 4.8)
|
|
**Scope:** Concurrency contract of the header-only library — reentrancy, hidden
|
|
mutable global state, and safety of parallel solves.
|
|
**Focus:** Can two threads use this library at the same time, and is that contract
|
|
documented? Currently undocumented everywhere.
|
|
|
|
Status legend: 🔴 Critical · 🟡 Important · 🔵 Polish
|
|
|
|
> **Status (2026-05-31):** no code change needed (no mutable static state — good).
|
|
> The one open item is **documenting the same-mesh concurrency contract** —
|
|
> scheduled **S4** (Haiku→Opus review) in
|
|
> [`finding-orchestration.md`](finding-orchestration.md).
|
|
|
|
> **Good news up front:** a scan for mutable function-local `static` variables (the
|
|
> classic hidden-shared-state hazard) found **none**. All `static` in the headers are
|
|
> stateless factory/accessor functions (`MobiusMap::identity`,
|
|
> `Default_conformal_map_traits::theta_map`, etc.). So the library is *close* to
|
|
> trivially reentrant — the findings below are about the mesh-mutation contract and
|
|
> the missing documentation, not about data races in hidden globals.
|
|
|
|
---
|
|
|
|
## Summary table
|
|
|
|
| ID | Sev | Title | Location |
|
|
|----|-----|-------|----------|
|
|
| T1 | 🟡 | No documented threading contract anywhere — users cannot know what is safe | all public headers |
|
|
| T2 | 🟡 | The solve pipeline mutates the mesh via property maps → concurrent solves on the *same* mesh are unsafe and silently so | `*_functional.hpp`, `newton_solver.hpp` |
|
|
| T3 | 🔵 | Property-map *creation* (`add_property_map`) on a shared mesh from two threads is a data race on CGAL's internal map registry | `setup_*_maps`, traits accessors |
|
|
| T4 | 🔵 | Eigen threading not pinned — relies on Eigen's defaults; undocumented | `newton_solver.hpp` |
|
|
|
|
---
|
|
|
|
## T1 — 🟡 No documented threading contract
|
|
|
|
### Evidence
|
|
No header, no `doc/` file, and no README section states whether the API is
|
|
thread-safe, reentrant, or single-threaded-only. `briefing.md` positions the library
|
|
as potential "infrastructure for your work" — which invites multi-threaded use it
|
|
never characterizes.
|
|
|
|
### Problem
|
|
Without a stated contract, a user must read the implementation to discover (T2) that
|
|
distinct meshes are safe but a shared mesh is not. The safe pattern is real and
|
|
simple — it just needs to be written down.
|
|
|
|
### Fix
|
|
Add a short "Thread safety" section to the main API doc (and a sentence in the key
|
|
headers) stating the contract, e.g.:
|
|
> *The library performs no synchronization and uses no shared mutable global state.
|
|
> Operations on **distinct** `ConformalMesh` objects are independent and may run
|
|
> concurrently. Operations on a **single** mesh (including any `setup_*_maps`,
|
|
> `assign_*`, gradient, Hessian, or `newton_*` call) mutate that mesh's property maps
|
|
> and must not run concurrently on the same mesh.*
|
|
|
|
### Acceptance criteria
|
|
- A "Thread safety" subsection exists in the public API documentation and matches the
|
|
actual behavior described in T2/T3.
|
|
|
|
---
|
|
|
|
## T2 — 🟡 Solve pipeline mutates the mesh; same-mesh concurrency is unsafe
|
|
|
|
### Evidence
|
|
- `setup_*_maps(mesh)` adds property maps to `mesh`.
|
|
- `assign_*_dof_indices(mesh, m)` writes `m.v_idx` / `m.e_idx` into the mesh.
|
|
- `compute_*_lambda0_from_mesh(mesh, m)` writes `m.lambda0`.
|
|
- `newton_*` reads those maps and (via the gradient/Hessian) reads the mesh
|
|
repeatedly; the functions take `ConformalMesh& mesh` (non-const).
|
|
|
|
### Problem
|
|
Two threads calling `newton_euclidean(mesh, …)` on the **same** `mesh` race on the
|
|
shared property maps — a silent data race, not a clean error. Because the signature
|
|
is `ConformalMesh&` (mutable) this is technically "expected", but nothing warns the
|
|
user, and the natural-looking "solve the same input mesh under two geometries in
|
|
parallel" is exactly the unsafe case.
|
|
|
|
### Fix
|
|
- Document the contract (T1).
|
|
- Optionally, offer a value-semantics convenience overload that takes the mesh by
|
|
`const&` and operates on an internal copy, for users who want fire-and-forget
|
|
parallel solves. (Larger change; document first.)
|
|
|
|
### Acceptance criteria
|
|
- The same-mesh hazard is documented; if a const/copying overload is added, a test
|
|
runs two concurrent solves on copies without a race (clean under ThreadSanitizer).
|
|
|
|
---
|
|
|
|
## T3 — 🔵 Concurrent property-map creation races CGAL's registry
|
|
|
|
### Evidence
|
|
`setup_*_maps` and the traits accessors call `mesh.add_property_map<…>(name, default)`.
|
|
|
|
### Problem
|
|
CGAL's `Surface_mesh::add_property_map` mutates the mesh's internal property
|
|
container. Two threads creating maps on the same mesh concurrently is undefined
|
|
behavior at the CGAL level — a sub-case of T2 but worth calling out because the
|
|
traits accessors (`Conformal_map_traits.h:158-179`) create-on-demand, so even a
|
|
"read-only-looking" first access mutates.
|
|
|
|
### Fix
|
|
Covered by the T1 contract ("any `setup_*` … on the same mesh must not run
|
|
concurrently"). No code change beyond documentation unless the const overload (T2) is
|
|
pursued.
|
|
|
|
---
|
|
|
|
## T4 — 🔵 Eigen threading is unpinned and undocumented
|
|
|
|
### Evidence
|
|
`newton_solver.hpp` uses `SimplicialLDLT` / `SparseQR` without calling
|
|
`Eigen::setNbThreads(…)` or documenting OpenMP behavior.
|
|
|
|
### Problem
|
|
Eigen may use OpenMP threads internally depending on build flags. For a library that
|
|
might itself be called from parallel user code, the nesting behavior (and whether
|
|
`EIGEN_DONT_PARALLELIZE` is wanted) is unspecified. Distinct Eigen objects are
|
|
thread-safe, so this is low risk — but it should be a documented choice.
|
|
|
|
### Fix
|
|
State the intended Eigen threading mode in the build/threading docs; consider pinning
|
|
it explicitly (`EIGEN_DONT_PARALLELIZE` for predictability, or document that callers
|
|
control it).
|
|
|
|
---
|
|
|
|
## What is already good
|
|
|
|
- **No mutable static locals** anywhere in the headers — the single most important
|
|
reentrancy property holds.
|
|
- All `static` members are pure functions (factories/accessors) — no shared state.
|
|
- The header-only, value-oriented design (results returned by value as
|
|
`NewtonResult` / `Conformal_map_result`) means outputs are not shared.
|
|
- Distinct-mesh concurrency genuinely *is* safe today — the contract just needs
|
|
documenting (T1), not implementing.
|
|
|
|
## Suggested order
|
|
1. **T1** (write the contract) — trivial, unblocks safe parallel use immediately.
|
|
2. **T2/T3** (covered by the contract; optional const overload later).
|
|
3. **T4** (pin/document Eigen threading) — polish.
|