docs(reviewer): add 4 gap audits — numerical, input-validation, thread-safety, dependency/license
Closes the dimensions the prior audits did not cover: - numerical-stability: tolerance hierarchy (FD-step vs Newton tol), discontinuous clamps, cancellation in cotangent/area, magic constants. - input-validation: malformed JSON/XML deserialization, NaN/Inf vertex coords on load, schema-drift handling. - thread-safety: no mutable static state (good); undocumented same-mesh concurrency contract; Eigen threading. - dependency-license: existing THIRD-PARTY-LICENSES matrix is sound but predicated on the MIT premise that G0 undermines; test meshes have no provenance/license. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
Tarik Moussa
139
doc/reviewer/thread-safety-audit-2026-05-31.md
Normal file
139
doc/reviewer/thread-safety-audit-2026-05-31.md
Normal file
@@ -0,0 +1,139 @@
|
||||
# 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
|
||||
|
||||
> **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.
|
||||
Reference in New Issue
Block a user