diff --git a/docs/tickets/lean4-0001-lake-guardcycle-callstack-list-quadratic.md b/docs/tickets/lean4-0001-lake-guardcycle-callstack-list-quadratic.md new file mode 100644 index 000000000..ecd49882c --- /dev/null +++ b/docs/tickets/lean4-0001-lake-guardcycle-callstack-list-quadratic.md @@ -0,0 +1,76 @@ +# lean4-0001: guardCycle() O(N²) — List.contains on call stack in topological build traversal + +**Target:** leanprover/lean4 +**Severity:** HIGH +**CWE:** CWE-407 (Inefficient Algorithmic Complexity) +**MOAD:** MOAD-0001 (A Sedimentary Defect) +**File:** `src/lake/Lake/Util/Cycle.lean:83`, `src/lake/Lake/Build/Topological.lean:98` +**Language:** Lean 4 +**Status:** open + +## Description + +`guardCycle` uses `List.contains` for cycle detection inside Lake's topological +dependency resolver. Every module build traversal calls `guardCycle` once per +node, each time scanning the entire ancestor call stack linearly. For a project +with N modules in a linear dependency chain, total cost reaches O(N²). + +## Root Cause + +```lean +-- Lake/Util/Cycle.lean:14 +public abbrev CallStack κ := List κ -- plain List, no hash index + +-- Lake/Util/Cycle.lean:79–86 +@[inline] public def guardCycle + [BEq κ] [Monad m] [MonadCycle κ m] (key : κ) (act : m α) +: m α := do + let parents ← getCallStack + if parents.contains key then -- O(N) scan every call + throwCycle <| key :: (parents.partition (· != key)).1 ++ [key] + else + withCallStack (key :: parents) act -- prepend: stack grows to depth D +``` + +Called from `recFetchAcyclic` at `Lake/Build/Topological.lean:98` for every +node in the module dependency graph during a build. + +With a linear chain A → B → C → … → Z (N modules): +- node 1: scan depth 0 +- node 2: scan depth 1 +- node N: scan depth N-1 +- **Total: O(N²/2) comparisons** + +## Complexity + +| N modules (linear chain) | guardCycle calls | comparisons | +|--------------------------|-----------------|-------------| +| 100 | 100 | ~5,000 | +| 500 | 500 | ~125,000 | +| 1000 | 1000 | ~500,000 | + +Real projects (Mathlib4: ~2000 files) hit this on dependency-heavy subgraphs. + +## Fix + +Carry a parallel `HashSet` (or `NameHashSet` / `Std.HashSet`) for O(1) +membership test. Keep the `List` only for cycle path reporting. + +```lean +-- Proposed: pair (HashSet for O(1) contains, List for cycle reporting) +public abbrev CallStack κ := Std.HashSet κ × List κ + +@[inline] public def guardCycle + [BEq κ] [Hashable κ] [Monad m] [MonadCycle κ m] (key : κ) (act : m α) +: m α := do + let (set, list) ← getCallStack + if set.contains key then -- O(1) + throwCycle <| key :: (list.partition (· != key)).1 ++ [key] + else + withCallStack (set.insert key, key :: list) act +``` + +## Speedup Estimate + +Linear chain N=1000: O(N²) → O(N). ~1000× faster at that scale. +Typical projects (N=50–200 in hot subgraphs): 50–200× faster. diff --git a/docs/tickets/lean4-0002-kernel-inductive-std-find-nested-loop.md b/docs/tickets/lean4-0002-kernel-inductive-std-find-nested-loop.md new file mode 100644 index 000000000..58c3eb222 --- /dev/null +++ b/docs/tickets/lean4-0002-kernel-inductive-std-find-nested-loop.md @@ -0,0 +1,49 @@ +# lean4-0002: kernel/inductive.cpp O(K×N) — std::find in nested loop for inductive type validation + +**Target:** leanprover/lean4 +**Severity:** MEDIUM +**CWE:** CWE-407 (Inefficient Algorithmic Complexity) +**MOAD:** MOAD-0001 (A Sedimentary Defect) +**File:** `src/kernel/inductive.cpp:529–532` +**Language:** C++ +**Status:** open + +## Description + +Inside the kernel's inductive type validator, `std::find` on a plain `buffer` +runs inside a `for` loop over another buffer. For a constructor with K type-former +arguments and N result arguments, cost reaches O(K×N). + +## Root Cause + +```cpp +// inductive.cpp:526–532 +buffer result_args; +get_app_args(type, result_args); +/* Check condition 2: every argument in to_check must occur in result_args */ +for (expr const & arg : to_check) { // O(K) outer + if (std::find(result_args.begin(), result_args.end(), arg) == result_args.end()) // O(N) inner + return true; /* Condition 2 failed */ +} +``` + +`to_check` holds type-former arguments; `result_args` holds application +arguments from the return type. Both grow with constructor arity. + +## Fix + +Use `std::unordered_set` (or Lean's `expr_set`) for `result_args`: + +```cpp +expr_set result_set(result_args.begin(), result_args.end()); // O(N) build +for (expr const & arg : to_check) { + if (result_set.find(arg) == result_set.end()) // O(1) lookup + return true; +} +``` + +## Related + +`inductive.cpp:543` — same `std::find` pattern for fresh universe param name +generation (while loop scanning `m_lparams`). Low severity in practice since +universe parameter counts stay small, but structurally identical defect. diff --git a/docs/tickets/lean4-0003-library-util-fresh-name-std-find-while.md b/docs/tickets/lean4-0003-library-util-fresh-name-std-find-while.md new file mode 100644 index 000000000..7b1b5fd79 --- /dev/null +++ b/docs/tickets/lean4-0003-library-util-fresh-name-std-find-while.md @@ -0,0 +1,52 @@ +# lean4-0003: library/util.cpp + kernel/inductive.cpp — O(N²) fresh name generation via std::find in while loop + +**Target:** leanprover/lean4 +**Severity:** LOW-MEDIUM +**CWE:** CWE-407 (Inefficient Algorithmic Complexity) +**MOAD:** MOAD-0001 (A Sedimentary Defect) +**Files:** `src/library/util.cpp:69–72`, `src/kernel/inductive.cpp:543–546` +**Language:** C++ +**Status:** open + +## Description + +Two identical patterns: a `while` loop generates fresh names by appending +an incrementing index, checking for collisions via `std::find` on a plain +vector each iteration. Cost is O(N²) if N existing names must be checked +before a fresh one is found. + +## Root Cause + +```cpp +// library/util.cpp:69–72 +while (std::find(lp_names.begin(), lp_names.end(), l) != lp_names.end()) { + l = name("l").append_after(i); + i++; +} + +// kernel/inductive.cpp:543–546 +while (std::find(m_lparams.begin(), m_lparams.end(), u) != m_lparams.end()) { + u = name("u").append_after(i); + i++; +} +``` + +Both scan the full list per attempt. With N existing names: O(N²) to find +a free slot at position N. + +## Fix + +Convert to `std::unordered_set` (or `name_set`) before the loop: + +```cpp +name_set existing(lp_names.begin(), lp_names.end()); +while (existing.count(l)) { + l = name("l").append_after(i++); +} +``` + +## Severity Note + +Universe parameter counts are typically < 10 in practice. Performance +impact is negligible today but the pattern is structurally a sedimentary +defect and will bite as Lean handles larger polymorphic libraries. diff --git a/docs/tickets/lean4-0004-irinterpreter-double-checked-locking-race.md b/docs/tickets/lean4-0004-irinterpreter-double-checked-locking-race.md new file mode 100644 index 000000000..d8262e4d8 --- /dev/null +++ b/docs/tickets/lean4-0004-irinterpreter-double-checked-locking-race.md @@ -0,0 +1,62 @@ +# lean4-0004: ir_interpreter.cpp — double-checked locking without memory barrier in symbol lookup + +**Target:** leanprover/lean4 +**Severity:** HIGH +**CWE:** CWE-362 (Concurrent Execution Using Shared Resource with Improper Synchronization) +**MOAD:** MOAD-0005 (A Thundering Herd) +**File:** `src/library/ir_interpreter.cpp:828–869` +**Language:** C++ +**Status:** open + +## Description + +`lookup_symbol` in the IR interpreter uses a double-checked locking pattern +without memory barriers. A first check on `m_symbol_cache` runs without any +lock. A second check on `g_native_symbol_cache` runs under shared lock. +Between the two, the lock is released and re-acquired as exclusive. On +weakly-ordered architectures, two concurrent threads can both pass the +unguarded first check, both acquire exclusive lock in sequence, and insert +duplicate entries — or worse, observe partially-constructed cache entries. + +## Root Cause + +```cpp +// ir_interpreter.cpp:828–869 (simplified) +auto e = m_symbol_cache.find(fn); // (1) unguarded check +if (e != m_symbol_cache.end()) { + return e->second; +} +std::shared_lock lock(*g_native_symbol_cache_mutex); +auto ne = g_native_symbol_cache->find(fn); // (2) shared-locked check +if (ne != g_native_symbol_cache->end()) { + m_symbol_cache.insert({ fn, e_new }); // inserts without exclusive lock + return e_new; +} +lock.unlock(); +std::unique_lock unique_lock(*g_native_symbol_cache_mutex); +ne = g_native_symbol_cache->find(fn); // (3) double-check under exclusive lock +if (ne == g_native_symbol_cache->end()) { + // ... compute ... + g_native_symbol_cache->insert({ fn, e_new.m_native }); +} +``` + +Step (1) reads `m_symbol_cache` without synchronization. On x86 this is +generally safe due to TSO, but on ARM/RISC-V reordering can expose +partially-initialized entries from another thread's prior insert. + +## Fix + +Use `std::atomic` or ensure `m_symbol_cache` access is guarded by +the same mutex. Alternatively, use a single-phase double-check under the +exclusive lock: + +```cpp +{ + std::shared_lock lock(*g_native_symbol_cache_mutex); + auto e = m_symbol_cache.find(fn); + if (e != m_symbol_cache.end()) return e->second; +} +std::unique_lock lock(*g_native_symbol_cache_mutex); +// recheck under exclusive lock, then compute+insert +``` diff --git a/docs/tickets/lean4-0005-lake-job-registration-concurrent-race.md b/docs/tickets/lean4-0005-lake-job-registration-concurrent-race.md new file mode 100644 index 000000000..1d5c22ef1 --- /dev/null +++ b/docs/tickets/lean4-0005-lake-job-registration-concurrent-race.md @@ -0,0 +1,60 @@ +# lean4-0005: Lake/Build/Run.lean — concurrent job registration races on JobQueue IO.Ref + +**Target:** leanprover/lean4 +**Severity:** HIGH +**CWE:** CWE-362 (Concurrent Execution Using Shared Resource with Improper Synchronization) +**MOAD:** MOAD-0005 (A Thundering Herd) +**Files:** `src/lake/Lake/Build/Job/Register.lean:43`, `src/lake/Lake/Build/Run.lean:157` +**Language:** Lean 4 +**Status:** open + +## Description + +Multiple async build tasks call `registerJob` concurrently. Each calls +`registeredJobs.modify (·.push job)` on a shared `IO.Ref (Array OpaqueJob)` +without a mutex. Lean's `IO.Ref.modify` is not atomic with respect to +concurrent tasks — two tasks can both read the same array, both push their +job, and one registration gets lost. + +## Root Cause + +```lean +-- Lake/Build/Context.lean:42 +public def JobQueue := IO.Ref (Array OpaqueJob) + +-- Lake/Build/Job/Register.lean:43 +(← getBuildContext).registeredJobs.modify (·.push job) +-- ↑ read-modify-write on shared IO.Ref, no mutex +``` + +Concurrently spawned tasks (via `Job.async`) all share the same +`BuildContext.registeredJobs`. Each calls `.modify` independently: +- Task A reads array [j1, j2] +- Task B reads array [j1, j2] +- Task A writes [j1, j2, jA] +- Task B writes [j1, j2, jB] ← overwrites Task A's registration +- jA is lost from the monitor + +```lean +-- Lake/Build/Run.lean:157 — monitor reads the same ref +let newJobs ← (← read).jobs.modifyGet ((·, #[])) +``` + +The monitor drain races with concurrent registrations. + +## Fix + +Wrap `JobQueue` with a `Mutex` or use `IO.Mutex`: + +```lean +public def JobQueue := IO.Mutex (Array OpaqueJob) + +-- register: +(← getBuildContext).registeredJobs.atomically (·.push job) + +-- poll: +let newJobs ← (← read).jobs.atomically (fun arr => (arr, #[])) +``` + +Or use a `Channel`/`Queue` abstraction that is safe for concurrent push +from producers and drain from the monitor consumer. diff --git a/docs/tickets/lean4-0006-kernel-trace-thread-local-options-leak.md b/docs/tickets/lean4-0006-kernel-trace-thread-local-options-leak.md new file mode 100644 index 000000000..fabcd483d --- /dev/null +++ b/docs/tickets/lean4-0006-kernel-trace-thread-local-options-leak.md @@ -0,0 +1,42 @@ +# lean4-0006: kernel/trace.cpp — LEAN_THREAD_PTR(g_opts) leaks trace options across tasks on thread pool reuse + +**Target:** leanprover/lean4 +**Severity:** MEDIUM +**CWE:** CWE-668 (Exposure of Resource to Wrong Sphere) +**MOAD:** MOAD-0003 (A Leaked Context) +**File:** `src/kernel/trace.cpp:16` +**Language:** C++ +**Status:** open + +## Description + +`g_opts` — the trace options pointer — is stored as a thread-local variable +via `LEAN_THREAD_PTR`. Lean's async task system uses a thread pool. When a +thread finishes one elaboration task and picks up a new one, the thread-local +`g_opts` from the previous task remains set. The new task inherits the old +trace configuration, causing incorrect trace output (silence when tracing +expected, or noise when silence expected). + +## Root Cause + +```cpp +// kernel/trace.cpp:16 +LEAN_THREAD_PTR(const options, g_opts); + +// kernel/trace.cpp:25 +bool is_trace_class_enabled(name const & n) { + if (!g_opts) return false; + return is_trace_class_enabled_core(*g_opts, n); +} +``` + +`g_opts` is set per-elaboration invocation but never explicitly cleared on +task completion. Thread pool threads reuse their `g_opts` across tasks. + +## Fix + +Reset `g_opts` to `nullptr` at task boundaries (on task start/end in the +thread pool scheduler), or pass options explicitly via context rather than +thread-local. The pattern of setting thread-locals before a task and +clearing after is already used in other parts of the runtime +(`reset_thread_local()`). Apply the same discipline here. diff --git a/docs/tickets/lean4-0007-runtime-process-windows-env-var-quadratic.md b/docs/tickets/lean4-0007-runtime-process-windows-env-var-quadratic.md new file mode 100644 index 000000000..321973839 --- /dev/null +++ b/docs/tickets/lean4-0007-runtime-process-windows-env-var-quadratic.md @@ -0,0 +1,66 @@ +# lean4-0007: runtime/process.cpp — O(N²) Windows environment variable inheritance + +**Target:** leanprover/lean4 +**Severity:** LOW-MEDIUM +**CWE:** CWE-407 (Inefficient Algorithmic Complexity) +**MOAD:** MOAD-0001 (A Sedimentary Defect) +**File:** `src/runtime/process.cpp:247–254` +**Language:** C++ +**Platform:** Windows only +**Status:** open + +## Description + +When spawning child processes on Windows with `inherit_env=true`, the runtime +iterates all inherited environment variable keys and checks each against +`new_env_vars` (an `unordered_map`) using `.count({key_begin, key_end})`. +This constructs a temporary `std::string` from a char range per iteration. +With N environment variables and M override vars, cost is O(N × M) due to +repeated string construction and hash lookup inside the loop. + +## Root Cause + +```cpp +// runtime/process.cpp:237–254 +std::unordered_map> new_env_vars; +// ... populate new_env_vars from explicit overrides ... + +while (*key_begin) { + char *key_end = strchr(key_begin, '='); + char *entry_end = key_end + strlen(key_end); + if (!new_env_vars.count({key_begin, key_end})) { // constructs std::string each iteration + new_envp = std::copy(key_begin, entry_end + 1, new_envp); + } + key_begin = entry_end + 1; +} +``` + +`.count({key_begin, key_end})` constructs a `std::string` on each of N +iterations. The map itself is O(1) per lookup, but string construction +is non-trivial. On systems with 200–500 env vars, this is measurable. + +## Fix + +Use `std::string_view` for the key lookup (C++17), or pre-build a +`std::unordered_set` from `new_env_vars` keys before the loop +and use `.count()` on it with a string_view. The `std::string` allocation +per iteration is the waste to eliminate. + +```cpp +// Build key set once, outside loop +std::unordered_set override_keys; +for (auto const & kv : new_env_vars) override_keys.insert(kv.first); + +while (*key_begin) { + char *key_end = strchr(key_begin, '='); + std::string_view key(key_begin, key_end - key_begin); + if (!override_keys.count(std::string(key))) { // still O(1), one alloc per lookup + ... + } +} +``` + +## Severity Note + +Windows-only code path. Environment variable counts rarely exceed 500. +Impact is real but not critical. Low priority compared to lean4-0001 and lean4-0004.