diamond-scan-deeper: kotlin/scala3/groovy/ghc O(2^D) hierarchy traversal — CLEAN + groovy-0001/0002 patches
Kotlin: confirmed CLEAN (DFS.VisitedWithSet throughout, existing marker valid) Scala3: confirmed CLEAN (BaseDataBuilder.addAll deduplicates, existing marker valid) GHC: CLEAN — NameSet/UniqSet/ExpansionFuel tracking in all superclass expansion paths (checkClassCycles, mk_strict_superclasses, transSuperClasses, closeWrtFunDeps) Groovy: CLEAN for diamond — getAllInterfaces/collectAllInterfacesReverseOrder/addAllInterfaces all use if(set.add(node)) guards (GROOVY-11036) groovy-0001/0002 patch docs created (O(N²) membership scans, pre-existing UNDF assigned) Clojure: CLEAN (set-based BFS, supers/ancestors use set-based worklist)
This commit is contained in:
parent
2f32199987
commit
61a717bfcc
4 changed files with 293 additions and 0 deletions
|
|
@ -0,0 +1,89 @@
|
|||
# UNDF: UNDF-2026-000000409
|
||||
# groovy-0001: StaticTypeCheckingVisitor.checkNamedParamsAnnotation — O(E×C) ArrayList.contains membership scan
|
||||
|
||||
## CWE-407 — Algorithmic Complexity: O(E×C) linear scan for named param validation
|
||||
|
||||
| Field | Value |
|
||||
|-------|-------|
|
||||
| ID | groovy-0001 |
|
||||
| Severity | MEDIUM |
|
||||
| Ecosystem | groovy |
|
||||
| Package | org.codehaus.groovy.transform.stc |
|
||||
| File | `src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java` |
|
||||
| Lines | 3205–3242 |
|
||||
| Complexity | O(E×C) where E = map entry count, C = collected annotation names |
|
||||
| Hot path | Static type checking of @NamedParams/@NamedParam annotations on method calls |
|
||||
|
||||
## Defect
|
||||
|
||||
`checkNamedParamsAnnotation` builds `collectedNames` as an `ArrayList<String>`,
|
||||
then performs `collectedNames.contains(name)` inside a loop over `entries.entrySet()`.
|
||||
Each `contains()` is O(C), making the overall check O(E×C) instead of O(E).
|
||||
|
||||
```java
|
||||
// DEFECT: ArrayList.contains() is O(C) inside O(E) loop → O(E×C) total
|
||||
List<String> collectedNames = new ArrayList<>();
|
||||
// ... populate collectedNames via processNamedParam() ...
|
||||
if (!collectedNames.isEmpty()) {
|
||||
for (Map.Entry<Object, MapEntryExpression> entry : entries.entrySet()) {
|
||||
Object name = entry.getKey();
|
||||
if (!collectedNames.contains(name)) { // O(C) scan each iteration
|
||||
addStaticTypeError("unexpected named arg: " + name, entry.getValue());
|
||||
}
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
`processNamedParam` also appends to the same `List<String>` accumulator:
|
||||
```java
|
||||
private void processNamedParam(final AnnotationNode namedParam, ..., final List<String> collectedNames) {
|
||||
...
|
||||
collectedNames.add(name); // appends to ArrayList
|
||||
}
|
||||
```
|
||||
|
||||
With E map entries and C collected names, the inner loop runs E×C comparisons.
|
||||
For methods with many @NamedParam annotations and many call-site named args, this
|
||||
is measurably quadratic: C=100 names × E=100 entries = 10,000 string comparisons
|
||||
vs 200 with a HashSet.
|
||||
|
||||
## Fix
|
||||
|
||||
Change `collectedNames` from `ArrayList<String>` to `Set<String>`:
|
||||
|
||||
```java
|
||||
// FIX: Set.contains() is O(1) → O(E) total
|
||||
Set<String> collectedNames = new HashSet<>();
|
||||
// ... populate same way via processNamedParam() ...
|
||||
if (!collectedNames.isEmpty()) {
|
||||
for (Map.Entry<Object, MapEntryExpression> entry : entries.entrySet()) {
|
||||
Object name = entry.getKey();
|
||||
if (!collectedNames.contains(name)) { // O(1) lookup
|
||||
addStaticTypeError("unexpected named arg: " + name, entry.getValue());
|
||||
}
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
Update `processNamedParam` signature to accept `Set<String>` instead of `List<String>`:
|
||||
|
||||
```java
|
||||
private void processNamedParam(final AnnotationNode namedParam,
|
||||
final Map<Object, MapEntryExpression> entries,
|
||||
final MapExpression args,
|
||||
final Set<String> collectedNames) { // was List<String>
|
||||
...
|
||||
collectedNames.add(name);
|
||||
}
|
||||
```
|
||||
|
||||
## Speedup
|
||||
|
||||
| C (names) | E (entries) | Before ops | After ops | Speedup |
|
||||
|-----------|-------------|------------|-----------|---------|
|
||||
| 10 | 10 | 100 | 10 | 10× |
|
||||
| 50 | 50 | 2,500 | 50 | 50× |
|
||||
| 100 | 100 | 10,000 | 100 | 100× |
|
||||
| 500 | 500 | 250,000 | 500 | 500× |
|
||||
|
||||
At C=E=1000 (pathological @NamedParams with many fields): 1,000,000 → 1,000 ops (1000×).
|
||||
|
|
@ -0,0 +1,87 @@
|
|||
# UNDF: UNDF-2026-000000410
|
||||
# groovy-0002: Verifier — O(V×P) Arrays.asList(params).contains() re-allocation per check
|
||||
|
||||
## CWE-407 — Algorithmic Complexity: O(V×P) repeated array-to-list conversion + linear scan
|
||||
|
||||
| Field | Value |
|
||||
|-------|-------|
|
||||
| ID | groovy-0002 |
|
||||
| Severity | MEDIUM |
|
||||
| Ecosystem | groovy |
|
||||
| Package | org.codehaus.groovy.classgen |
|
||||
| File | `src/main/java/org/codehaus/groovy/classgen/Verifier.java` |
|
||||
| Lines | 963, 1043, 1054 |
|
||||
| Complexity | O(V×P) where V = variable expression visits, P = params array length |
|
||||
| Hot path | Verifier pass during compilation — constructor and method parameter resolution |
|
||||
|
||||
## Defect
|
||||
|
||||
Three sites in `Verifier.java` use `Arrays.asList(params).contains(p)` inside
|
||||
visitor methods that iterate over variable expressions:
|
||||
|
||||
**Site 1 (line 963):** Inside `visitVariableExpression` of an anonymous `CodeVisitorSupport`
|
||||
that traverses a closure block, called for every variable expression:
|
||||
```java
|
||||
// DEFECT: Arrays.asList(params) allocates a new List wrapper each call; .contains() is O(P)
|
||||
if (!Arrays.asList(params).contains(p) && Arrays.asList(method.getParameters()).contains(p)) {
|
||||
// GROOVY-10602
|
||||
```
|
||||
|
||||
**Site 2 (line 1043):** Same pattern in a different `visitVariableExpression`:
|
||||
```java
|
||||
// DEFECT: re-allocates List each invocation, O(P) scan
|
||||
if (p.getInitialExpression() instanceof ConstantExpression && !Arrays.asList(params).contains(p)){
|
||||
```
|
||||
|
||||
**Site 3 (line 1054):**
|
||||
```java
|
||||
// DEFECT: same pattern
|
||||
if (p.hasInitialExpression() && !Arrays.asList(params).contains(p)) {
|
||||
```
|
||||
|
||||
`Arrays.asList()` creates a wrapper each time — O(1) allocation but O(P) scan.
|
||||
Called V times (once per variable expression in the method body), total cost is O(V×P).
|
||||
For a method with P=20 parameters and V=100 variable references, that is 2,000 pointer
|
||||
comparisons vs 100 with a pre-built `Set<Parameter>`.
|
||||
|
||||
## Fix
|
||||
|
||||
Hoist the set construction outside the visitor, share it as an effectively-final capture:
|
||||
|
||||
```java
|
||||
// FIX: build once before the visitor loop
|
||||
Set<Parameter> paramSet = new HashSet<>(Arrays.asList(params));
|
||||
Set<Parameter> methodParamSet = new HashSet<>(Arrays.asList(method.getParameters()));
|
||||
|
||||
// Inside visitVariableExpression:
|
||||
if (!paramSet.contains(p) && methodParamSet.contains(p)) { // O(1) each
|
||||
// GROOVY-10602
|
||||
```
|
||||
|
||||
For sites 2 and 3:
|
||||
```java
|
||||
Set<Parameter> paramSet = new HashSet<>(Arrays.asList(params));
|
||||
// ...
|
||||
if (p.getInitialExpression() instanceof ConstantExpression && !paramSet.contains(p)) {
|
||||
// ...
|
||||
if (p.hasInitialExpression() && !paramSet.contains(p)) {
|
||||
```
|
||||
|
||||
## Speedup
|
||||
|
||||
| P (params) | V (var refs) | Before ops | After ops | Speedup |
|
||||
|------------|--------------|------------|-----------|---------|
|
||||
| 5 | 20 | 100 | 20 | 5× |
|
||||
| 20 | 100 | 2,000 | 100 | 20× |
|
||||
| 50 | 500 | 25,000 | 500 | 50× |
|
||||
| 100 | 1,000 | 100,000 | 1,000 | 100× |
|
||||
|
||||
Typical: P=20, V=50 method body references → 1,000 → 50 ops (20×).
|
||||
Pathological generated code: P=100, V=1000 → 100,000 → 1,000 ops (100×).
|
||||
|
||||
## Note on diamond recursion
|
||||
|
||||
Groovy's interface hierarchy traversal (ClassNode.getAllInterfaces, Traits.collectAllInterfacesReverseOrder,
|
||||
GeneralUtils.addAllInterfaces) all use `if (set.add(node))` guards introduced in GROOVY-11036.
|
||||
These sites are CLEAN for O(2^D) diamond recursion. groovy-0001 and groovy-0002 are
|
||||
O(N²) membership-scan defects, distinct from diamond recursion.
|
||||
50
defects/groovy/patch/groovy-diamond-recursion-CLEAN.md
Normal file
50
defects/groovy/patch/groovy-diamond-recursion-CLEAN.md
Normal file
|
|
@ -0,0 +1,50 @@
|
|||
# Groovy Compiler — Diamond Recursion CWE-407 Scan: CLEAN
|
||||
|
||||
**Pattern:** Recursive O(2^D) traversal of interface/trait hierarchy without visited set
|
||||
**Scan date:** 2026-03-29
|
||||
**Scope:** `src/main/java/org/codehaus/groovy/`
|
||||
|
||||
## Method
|
||||
|
||||
Searched for recursive ClassNode hierarchy traversal without proper `if (set.add())` guards.
|
||||
Traced all callers of `getInterfaces()`, `getSuperClass()`, `getAllInterfaces()`,
|
||||
`getInterfacesAndSuperInterfaces()`, and `collectAllInterfacesReverseOrder()`.
|
||||
|
||||
## Key candidates reviewed
|
||||
|
||||
### `ClassNode.getAllInterfaces()` — private `getAllInterfaces(Set)`
|
||||
`src/main/java/org/codehaus/groovy/ast/ClassNode.java` lines 441–452.
|
||||
|
||||
Fixed in GROOVY-11036: now uses `if (set.add(face)) face.getAllInterfaces(set)`.
|
||||
The guard on `set.add()` return value prevents re-traversal of shared ancestors.
|
||||
CLEAN (post-fix).
|
||||
|
||||
### `GeneralUtils.addAllInterfaces()` / `getInterfacesAndSuperInterfaces()`
|
||||
`src/main/java/org/codehaus/groovy/ast/tools/GeneralUtils.java` lines 491–518.
|
||||
|
||||
Uses `if (result.add(in)) { addAllInterfaces(result, in); }` for interface branches.
|
||||
Superclass branch is linear (Java single-inheritance), so no diamond possible there.
|
||||
CLEAN.
|
||||
|
||||
### `Traits.collectAllInterfacesReverseOrder()`
|
||||
`src/main/java/org/codehaus/groovy/transform/trait/Traits.java` lines 307–316.
|
||||
|
||||
Uses `if (interfaces.add(iNode)) collectAllInterfacesReverseOrder(iNode, interfaces)`.
|
||||
Correct guard. CLEAN.
|
||||
|
||||
### `ResolveVisitor` cycle detection
|
||||
`src/main/java/org/codehaus/groovy/control/ResolveVisitor.java` lines 1363–1381.
|
||||
|
||||
Uses BFS with `done.add(next)` as the loop guard. CLEAN.
|
||||
|
||||
### `WideningCategories.lowestUpperBound()`
|
||||
`src/main/java/org/codehaus/groovy/ast/tools/WideningCategories.java` lines 319–447.
|
||||
|
||||
Recursion on superclass chain (linear, no diamond). Interface sets pre-computed via
|
||||
`getInterfacesAndSuperInterfaces()` which is guarded. CLEAN.
|
||||
|
||||
## Verdict
|
||||
|
||||
CLEAN for O(2^D) diamond recursion. Groovy uses `if (set.add(node))` guards on all
|
||||
recursive interface hierarchy traversal. The two existing defects (groovy-0001,
|
||||
groovy-0002) are O(N²) list-membership scans, not diamond recursion.
|
||||
Loading…
Add table
Add a link
Reference in a new issue