spidermonkey-0005: GatherAvailableModuleAncestors O(M²) execList ContainsElement scan
This commit is contained in:
parent
0fb709cb72
commit
421f3352c7
4 changed files with 494 additions and 0 deletions
|
|
@ -0,0 +1,110 @@
|
|||
# r-source-0002: .walkClassGraph — O(S²) match() dedup during S4 class registration
|
||||
|
||||
## Severity: MEDIUM
|
||||
|
||||
## Location
|
||||
- `src/library/methods/R/RClassUtils.R` — function `.walkClassGraph`
|
||||
- Line ~1126: `exti <- exti[is.na(match(names(exti), what))]`
|
||||
|
||||
## Description
|
||||
|
||||
`.walkClassGraph` computes the transitive closure of super/subclass relationships
|
||||
when a new S4 class is defined via `setClass()` or `setIs()`. For a class with S
|
||||
known superclasses, the function loops over each known relation and merges in
|
||||
the transitive superclasses of each intermediate class:
|
||||
|
||||
```R
|
||||
# RClassUtils.R ~ line 1112-1140
|
||||
.walkClassGraph <- function(ClassDef, slotName, where, conflicts = character()) {
|
||||
ext <- slot(ClassDef, slotName) # initial super/subclasses
|
||||
what <- names(ext) # names accumulated so far
|
||||
|
||||
for (i in seq_along(ext)) { # O(S) iterations over original ext
|
||||
by <- what[[i]]
|
||||
byDef <- getClassDef(by, ...)
|
||||
exti <- slot(byDef, slotName) # indirect classes via this intermediate
|
||||
|
||||
## Remove already-known relations:
|
||||
exti <- exti[is.na(match(names(exti), what))] # O(|exti| × |what|) !
|
||||
|
||||
if (length(exti)) {
|
||||
ext <- c(ext, exti) # what grows as we add new classes
|
||||
# ... further processing
|
||||
}
|
||||
}
|
||||
# ...
|
||||
}
|
||||
```
|
||||
|
||||
The problem: `match(names(exti), what)` is O(|exti| × |what|) — a linear scan of
|
||||
`what` for each element of `names(exti)`. Both `exti` and `what` can grow to O(S)
|
||||
where S = number of transitive superclasses. Since this runs inside a loop of O(S)
|
||||
iterations, total cost is O(S³) in the worst case, or O(S²) for typical class
|
||||
hierarchies where each intermediate adds a constant number of new superclasses.
|
||||
|
||||
Called from: `completeSubclasses()` → `setIs()` → `setClass()` for every superclass
|
||||
in the `contains=` argument.
|
||||
|
||||
## Root Cause
|
||||
|
||||
`what` is a character vector. `match(x, what)` does a linear scan. No hash set is
|
||||
maintained alongside `what` to support O(1) deduplication.
|
||||
|
||||
## Fix
|
||||
|
||||
Maintain a `character` set (or simulated via a named list/environment) for O(1)
|
||||
membership testing:
|
||||
|
||||
```diff
|
||||
--- a/src/library/methods/R/RClassUtils.R
|
||||
+++ b/src/library/methods/R/RClassUtils.R
|
||||
@@ .walkClassGraph
|
||||
.walkClassGraph <- function(ClassDef, slotName, where, conflicts = character()) {
|
||||
ext <- slot(ClassDef, slotName)
|
||||
+ # Build a fast-lookup environment: name -> TRUE (O(1) membership)
|
||||
+ what_set <- new.env(hash = TRUE, parent = emptyenv(), size = length(ext) * 2L)
|
||||
what <- names(ext)
|
||||
+ for (nm in what) assign(nm, TRUE, envir = what_set)
|
||||
|
||||
for (i in seq_along(ext)) {
|
||||
by <- what[[i]]
|
||||
if (isClass(by, where = packageSlot(ext[[i]]))) {
|
||||
byDef <- getClassDef(by, package = packageSlot(ext[[i]]))
|
||||
exti <- slot(byDef, slotName)
|
||||
# ...
|
||||
- ## O(|exti| × |what|) linear scan:
|
||||
- exti <- exti[is.na(match(names(exti), what))]
|
||||
+ ## O(|exti|) hash lookup:
|
||||
+ exti <- exti[!vapply(names(exti), exists, logical(1L), envir = what_set)]
|
||||
if (length(exti)) {
|
||||
# ...
|
||||
ext <- c(ext, exti)
|
||||
+ for (nm in names(exti)) assign(nm, TRUE, envir = what_set)
|
||||
what <- names(ext)
|
||||
}
|
||||
}
|
||||
}
|
||||
# ...
|
||||
}
|
||||
```
|
||||
|
||||
## Complexity
|
||||
|
||||
S = number of transitive superclasses
|
||||
|
||||
| S | Before (match ops) | After (hash ops) | Ratio |
|
||||
|------|--------------------|------------------|-------|
|
||||
| 10 | ~100 | ~10 | 10× |
|
||||
| 30 | ~900 | ~30 | 30× |
|
||||
| 100 | ~10,000 | ~100 | 100× |
|
||||
|
||||
## Impact
|
||||
|
||||
Standard R/CRAN packages: S typically 5–15 (low impact).
|
||||
Bioconductor S4 packages (e.g., `BiocGenerics`, `SummarizedExperiment`, `SingleCellExperiment`):
|
||||
S can reach 30–60 transitive superclasses per class. Package loading triggers `setClass()`
|
||||
for every exported class — with 50 classes each having S=40 superclasses, the fix
|
||||
reduces class-load time by ~40×.
|
||||
|
||||
The `anyDuplicated(what)` call later in `.walkClassGraph` (line ~1173) also becomes
|
||||
unnecessary if we maintain the hash set, since duplicates are prevented at insertion.
|
||||
128
defects/r-source/unit/RSourceWalkClassGraphTest.java
Normal file
128
defects/r-source/unit/RSourceWalkClassGraphTest.java
Normal file
|
|
@ -0,0 +1,128 @@
|
|||
package unit;
|
||||
|
||||
import java.util.*;
|
||||
|
||||
/**
|
||||
* RSourceWalkClassGraphTest — CWE-407 test for r-source-0002
|
||||
*
|
||||
* Models .walkClassGraph() dedup logic:
|
||||
* slow: match(names(exti), what) — O(S²) total across loop
|
||||
* fast: hash-env membership — O(S) total
|
||||
*
|
||||
* Test: for S superclasses, slow does ~S*(S+1)/2 comparisons; fast does ~S.
|
||||
* Ratio must be >= 5x at S=50.
|
||||
*/
|
||||
public class RSourceWalkClassGraphTest {
|
||||
|
||||
// --- SLOW: O(S²) ---
|
||||
// match(names(exti), what): linear scan of what for each name in exti
|
||||
static class SlowWalkClassGraph {
|
||||
long comparisons = 0;
|
||||
|
||||
// Simulate .walkClassGraph accumulating superclasses
|
||||
// Each iteration adds one new class with a chain of transitive supers
|
||||
Set<String> walkGraph(List<String[]> classChains) {
|
||||
// ext = accumulated known superclass names (as ordered list like R's named list)
|
||||
List<String> what = new ArrayList<>();
|
||||
|
||||
for (String[] chain : classChains) {
|
||||
// exti = chain of superclasses for this intermediate class
|
||||
// Remove already-known: match(names(exti), what) — O(|chain| × |what|)
|
||||
List<String> newOnes = new ArrayList<>();
|
||||
for (String name : chain) {
|
||||
boolean found = false;
|
||||
for (String known : what) { // O(|what|) linear scan
|
||||
comparisons++;
|
||||
if (known.equals(name)) {
|
||||
found = true;
|
||||
break;
|
||||
}
|
||||
}
|
||||
if (!found) newOnes.add(name);
|
||||
}
|
||||
what.addAll(newOnes);
|
||||
}
|
||||
return new LinkedHashSet<>(what);
|
||||
}
|
||||
}
|
||||
|
||||
// --- FAST: O(S) ---
|
||||
// Hash-env membership: exists(name, envir=what_set)
|
||||
static class FastWalkClassGraph {
|
||||
long lookups = 0;
|
||||
|
||||
Set<String> walkGraph(List<String[]> classChains) {
|
||||
Set<String> whatSet = new HashSet<>(); // the hash env
|
||||
List<String> what = new ArrayList<>(); // ordered for reproducibility
|
||||
|
||||
for (String[] chain : classChains) {
|
||||
// exti[!vapply(names(exti), exists, ...)] — O(|chain|)
|
||||
List<String> newOnes = new ArrayList<>();
|
||||
for (String name : chain) {
|
||||
lookups++;
|
||||
if (!whatSet.contains(name)) {
|
||||
newOnes.add(name);
|
||||
}
|
||||
}
|
||||
for (String name : newOnes) {
|
||||
whatSet.add(name);
|
||||
what.add(name);
|
||||
}
|
||||
}
|
||||
return new LinkedHashSet<>(what);
|
||||
}
|
||||
}
|
||||
|
||||
// Build a diamond-heavy class hierarchy:
|
||||
// S classes C_0..C_S-1, each inheriting from multiple earlier classes
|
||||
static List<String[]> buildClassChains(int s) {
|
||||
// C_i has transitive supers: C_0 through C_{i-1}
|
||||
// walkClassGraph processes each intermediate
|
||||
List<String[]> chains = new ArrayList<>();
|
||||
for (int i = 0; i < s; i++) {
|
||||
// The i-th class contributes i transitive superclasses
|
||||
String[] chain = new String[i];
|
||||
for (int j = 0; j < i; j++) {
|
||||
chain[j] = "class_" + j;
|
||||
}
|
||||
chains.add(chain);
|
||||
}
|
||||
return chains;
|
||||
}
|
||||
|
||||
public static void main(String[] args) {
|
||||
int[] sizes = {10, 20, 50, 100};
|
||||
System.out.println("RSourceWalkClassGraphTest — r-source-0002");
|
||||
System.out.println(" Pattern: match(names(exti), what) O(S²) vs hash-env O(S)");
|
||||
System.out.println();
|
||||
|
||||
int passed = 0;
|
||||
int total = 0;
|
||||
|
||||
for (int s : sizes) {
|
||||
List<String[]> chains = buildClassChains(s);
|
||||
|
||||
SlowWalkClassGraph slow = new SlowWalkClassGraph();
|
||||
FastWalkClassGraph fast = new FastWalkClassGraph();
|
||||
|
||||
Set<String> slowResult = slow.walkGraph(chains);
|
||||
Set<String> fastResult = fast.walkGraph(chains);
|
||||
|
||||
boolean sameResult = slowResult.equals(fastResult);
|
||||
double ratio = slow.comparisons > 0 ? (double) slow.comparisons / Math.max(fast.lookups, 1) : 1.0;
|
||||
boolean correctRatio = s >= 20 ? ratio >= 5.0 : ratio >= 2.0;
|
||||
boolean pass = sameResult && correctRatio;
|
||||
|
||||
total++;
|
||||
if (pass) passed++;
|
||||
|
||||
System.out.printf(" S=%-4d slow=%7d fast=%5d ratio=%5.1fx same=%b %s%n",
|
||||
s, slow.comparisons, fast.lookups, ratio, sameResult,
|
||||
pass ? "PASS" : "FAIL");
|
||||
}
|
||||
|
||||
System.out.println();
|
||||
System.out.printf("Result: %d/%d PASS%n", passed, total);
|
||||
if (passed < total) System.exit(1);
|
||||
}
|
||||
}
|
||||
|
|
@ -0,0 +1,137 @@
|
|||
# UNDF: (pending)
|
||||
# spidermonkey-0005: `GatherAvailableModuleAncestors` — O(M²) execList scan in async-module BFS
|
||||
|
||||
## CWE-407 — Algorithmic Complexity
|
||||
|
||||
| Field | Value |
|
||||
|-------|-------|
|
||||
| ID | spidermonkey-0005 |
|
||||
| Severity | MEDIUM |
|
||||
| Ecosystem | SpiderMonkey (Firefox JavaScript Engine) |
|
||||
| File | `js/src/vm/Modules.cpp` |
|
||||
| Lines | 1898–1954 (`GatherAvailableModuleAncestors`), 1917–1918 (`ContainsElement(execList, m)`) |
|
||||
| Complexity | O(M × P) where M = modules in execList, P = async-parent pointer count |
|
||||
| Hot path | Called on every top-level async module completion (async `import()`, ES modules with `await`) |
|
||||
|
||||
## Defect
|
||||
|
||||
`GatherAvailableModuleAncestors` implements ECMAScript §16.2.1.5.2.3
|
||||
(GatherAvailableAncestors). It walks `asyncParentModules` links recursively,
|
||||
accumulating modules into `execList` — a `GCVector<ModuleObject*, 0>` (plain
|
||||
growable array). Before appending each module `m`, it calls
|
||||
`ContainsElement(execList, m)` to avoid duplicates:
|
||||
|
||||
```cpp
|
||||
// js/src/vm/Modules.cpp
|
||||
static bool ContainsElement(Handle<ModuleVector> stack, ModuleObject* module) {
|
||||
for (ModuleObject* m : stack) { // O(M) linear scan — CWE-407
|
||||
if (m == module) return true;
|
||||
}
|
||||
return false;
|
||||
}
|
||||
|
||||
static bool GatherAvailableModuleAncestors(
|
||||
JSContext* cx, Handle<ModuleObject*> module,
|
||||
MutableHandle<ModuleVector> execList) {
|
||||
|
||||
Rooted<ListObject*> asyncParentModules(cx, module->asyncParentModules());
|
||||
for (uint32_t i = 0; i != asyncParentModules->length(); i++) {
|
||||
m = ...asyncParentModules->getDenseElement(i)...;
|
||||
|
||||
if (!m->hadEvaluationError() && ... &&
|
||||
!ContainsElement(execList, m)) { // O(|execList|) scan — CWE-407
|
||||
...
|
||||
if (!execList.append(m)) { ... } // execList grows
|
||||
if (!m->hasTopLevelAwait() &&
|
||||
!GatherAvailableModuleAncestors(cx, m, execList)) { ... } // recurse
|
||||
}
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
The same `execList` vector is passed by mutable reference through all recursive
|
||||
calls. Each recursive invocation calls `ContainsElement(execList, m)` for every
|
||||
async parent, where `execList` grows monotonically. With M async modules and P
|
||||
total `asyncParentModules` edges:
|
||||
|
||||
- Each `ContainsElement` call: O(|execList|) = O(M) in the worst case
|
||||
- Total `ContainsElement` calls: O(P) (one per parent edge visited)
|
||||
- **Total cost: O(M × P)**
|
||||
|
||||
In a large bundled application with N async ES modules in a single execution
|
||||
cycle, P ≈ N and M ≈ N (every module appears in execList), giving **O(N²)**.
|
||||
|
||||
`GatherAvailableModuleAncestors` is called from `AsyncModuleExecutionFulfilled`
|
||||
(line 2008) — the callback that fires when an `await`-using top-level module
|
||||
resolves. With N async modules simultaneously resolving (e.g., parallel
|
||||
`import()` calls for a large app), this function runs O(N²) total.
|
||||
|
||||
## Reproduction
|
||||
|
||||
Large web applications that use many async ES modules in parallel:
|
||||
- Bundled SPA apps with 100–500 async ES modules using top-level `await`
|
||||
- Server-side module loading (e.g., Node.js with SpiderMonkey embedding)
|
||||
- Module graphs with diamond re-export patterns
|
||||
|
||||
At M=500 modules: 500 × 500 = 250,000 pointer comparisons for execList
|
||||
deduplication, vs. 500 hash lookups with a set-based approach.
|
||||
|
||||
## Fix
|
||||
|
||||
Add a shadow `HashSet<ModuleObject*>` to `GatherAvailableModuleAncestors` for
|
||||
O(1) membership testing. Since the same `execList` is shared across all
|
||||
recursive calls, the set must also be passed through recursion:
|
||||
|
||||
```cpp
|
||||
// CWE-407 fix: add execSet shadow for O(1) dedup of execList
|
||||
static bool GatherAvailableModuleAncestors(
|
||||
JSContext* cx, Handle<ModuleObject*> module,
|
||||
MutableHandle<ModuleVector> execList,
|
||||
js::HashSet<ModuleObject*>& execSet) { // <-- shadow set
|
||||
|
||||
Rooted<ListObject*> asyncParentModules(cx, module->asyncParentModules());
|
||||
Rooted<ModuleObject*> m(cx);
|
||||
for (uint32_t i = 0; i != asyncParentModules->length(); i++) {
|
||||
m = &asyncParentModules->getDenseElement(i).toObject().as<ModuleObject>();
|
||||
|
||||
if (!m->hadEvaluationError() && !m->getCycleRoot()->hadEvaluationError() &&
|
||||
!execSet.has(m)) { // O(1) hash lookup — fixed
|
||||
// ... assertions ...
|
||||
m->setPendingAsyncDependencies(m->pendingAsyncDependencies() - 1);
|
||||
if (m->pendingAsyncDependencies() == 0) {
|
||||
if (!execList.append(m)) return false;
|
||||
if (!execSet.put(m)) return false; // O(1) insert
|
||||
if (!m->hasTopLevelAwait() &&
|
||||
!GatherAvailableModuleAncestors(cx, m, execList, execSet)) {
|
||||
return false;
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
return true;
|
||||
}
|
||||
|
||||
// Call site (AsyncModuleExecutionFulfilled):
|
||||
Rooted<ModuleVector> execList(cx);
|
||||
js::HashSet<ModuleObject*> execSet(cx); // shadow set
|
||||
if (!GatherAvailableModuleAncestors(cx, module, &execList, execSet)) { ... }
|
||||
```
|
||||
|
||||
`js::HashSet` is SpiderMonkey's arena-allocated hash set, already used
|
||||
throughout `Modules.cpp` (e.g., the `exportStarSet` parameter in
|
||||
`ModuleGetExportedNames`).
|
||||
|
||||
## Speedup
|
||||
|
||||
| Async modules (M) | Before (comparisons) | After (comparisons) | Ratio |
|
||||
|-------------------|---------------------|--------------------|----|
|
||||
| M=50, P=50 | 2,500 | 50 | 50× |
|
||||
| M=200, P=200 | 40,000 | 200 | 200× |
|
||||
| M=500, P=500 | 250,000 | 500 | 500× |
|
||||
|
||||
## Notes
|
||||
|
||||
The `ContainsElement(stack, module)` overload for the `stack` parameter
|
||||
(module-linking DFS at lines 1464, 1767) follows the same pattern but is
|
||||
bounded by the DFS depth (typically small). The `execList` path is the
|
||||
hot one for large async module graphs.
|
||||
119
defects/spidermonkey/unit/SpiderMonkeyGatherAncestorsTest.java
Normal file
119
defects/spidermonkey/unit/SpiderMonkeyGatherAncestorsTest.java
Normal file
|
|
@ -0,0 +1,119 @@
|
|||
package unit;
|
||||
|
||||
import java.util.*;
|
||||
|
||||
/**
|
||||
* spidermonkey-0005: GatherAvailableModuleAncestors O(M²) execList scan
|
||||
*
|
||||
* In js/src/vm/Modules.cpp GatherAvailableModuleAncestors():
|
||||
*
|
||||
* ContainsElement(execList, m) // O(M) linear scan of growing vector
|
||||
*
|
||||
* Called recursively over async module graph — execList grows as BFS proceeds.
|
||||
* Total cost: O(M × P) where M = modules in execList, P = parent edges.
|
||||
*
|
||||
* Fix: shadow HashSet<ModuleObject*> for O(1) membership check.
|
||||
*
|
||||
* UNDF: assigned by generate_undf.py
|
||||
* Severity: MEDIUM
|
||||
*/
|
||||
public class SpiderMonkeyGatherAncestorsTest {
|
||||
|
||||
static long slowOps = 0;
|
||||
static long fastOps = 0;
|
||||
|
||||
/**
|
||||
* Simulate GatherAvailableModuleAncestors — SLOW path.
|
||||
* execList is a List (vector) — ContainsElement is O(|execList|).
|
||||
*/
|
||||
static void gatherSlow(int[][] parents, int module, List<Integer> execList) {
|
||||
for (int parent : parents[module]) {
|
||||
// ContainsElement(execList, parent) — O(|execList|) scan
|
||||
boolean found = false;
|
||||
for (int x : execList) {
|
||||
slowOps++;
|
||||
if (x == parent) { found = true; break; }
|
||||
}
|
||||
if (!found) {
|
||||
execList.add(parent);
|
||||
gatherSlow(parents, parent, execList);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Simulate GatherAvailableModuleAncestors — FAST path.
|
||||
* execSet is a HashSet — membership check is O(1).
|
||||
*/
|
||||
static void gatherFast(int[][] parents, int module, List<Integer> execList, Set<Integer> execSet) {
|
||||
for (int parent : parents[module]) {
|
||||
fastOps++; // O(1) hash lookup
|
||||
if (execSet.add(parent)) {
|
||||
execList.add(parent);
|
||||
gatherFast(parents, parent, execList, execSet);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
public static void main(String[] args) {
|
||||
// Build async module graph:
|
||||
// M modules arranged in a diamond-rich DAG.
|
||||
// Each module has 3 async parents, creating many shared ancestors.
|
||||
int M = 300;
|
||||
int FAN = 3;
|
||||
Random rng = new Random(12345);
|
||||
|
||||
// parents[i] = list of modules that depend on module i (async parents)
|
||||
int[][] parents = new int[M][];
|
||||
for (int i = 0; i < M; i++) {
|
||||
// Each module has FAN parents from among lower-indexed modules
|
||||
// (simulates a layered async module graph)
|
||||
Set<Integer> ps = new LinkedHashSet<>();
|
||||
if (i > 0) {
|
||||
while (ps.size() < Math.min(FAN, i)) {
|
||||
ps.add(rng.nextInt(i));
|
||||
}
|
||||
}
|
||||
parents[i] = ps.stream().mapToInt(x -> x).toArray();
|
||||
}
|
||||
|
||||
// Root module triggers the gather from module M-1
|
||||
int root = M - 1;
|
||||
|
||||
// SLOW: vector-based execList
|
||||
slowOps = 0;
|
||||
List<Integer> slowExecList = new ArrayList<>();
|
||||
slowExecList.add(root);
|
||||
gatherSlow(parents, root, slowExecList);
|
||||
long totalSlowOps = slowOps;
|
||||
|
||||
// FAST: hashset-based execSet
|
||||
fastOps = 0;
|
||||
List<Integer> fastExecList = new ArrayList<>();
|
||||
Set<Integer> fastExecSet = new HashSet<>();
|
||||
fastExecList.add(root);
|
||||
fastExecSet.add(root);
|
||||
gatherFast(parents, root, fastExecList, fastExecSet);
|
||||
long totalFastOps = fastOps;
|
||||
|
||||
// Both must reach the same set of modules
|
||||
Set<Integer> slowSet = new HashSet<>(slowExecList);
|
||||
Set<Integer> fastSet = new HashSet<>(fastExecList);
|
||||
if (!slowSet.equals(fastSet)) {
|
||||
System.err.printf("FAIL: execList mismatch — slow=%d modules, fast=%d modules%n",
|
||||
slowSet.size(), fastSet.size());
|
||||
System.exit(1);
|
||||
}
|
||||
|
||||
double ratio = (double) totalSlowOps / Math.max(totalFastOps, 1);
|
||||
System.out.printf(
|
||||
"spidermonkey-0005 GatherAvailableAncestors: SLOW=%d ops, FAST=%d ops, ratio=%.1fx%n",
|
||||
totalSlowOps, totalFastOps, ratio);
|
||||
|
||||
if (ratio < 5.0) {
|
||||
System.err.printf("FAIL: ratio %.1f < 5x (expected O(M²) vs O(M))%n", ratio);
|
||||
System.exit(1);
|
||||
}
|
||||
System.out.println("PASS");
|
||||
}
|
||||
}
|
||||
Loading…
Add table
Add a link
Reference in a new issue