suitecrm: 5-MOAD scan; suitecrm-0005 ProjectTask getAllSubProjectTasks O(T^2*P) 70x, suitecrm-0006 LuceneSearch parseHits O(H*M) 8x
suitecrm-0005: modules/ProjectTask/ProjectTask.php getAllSubProjectTasks() BFS child-task traversal: while+foreach+in_array($parent_id, $potentialParentTaskIds) where $potentialParentTaskIds is already keyed by the same IDs stored as values. Fix: isset($potentialParentTaskIds[$parent_id]) — O(1) key lookup. Speedup: 70x at T=500 tasks. suitecrm-0006: lib/Search/AOD/LuceneSearchEngine.php parseHits() foreach($hits) -> in_array($module, $modules) on constant filter list — O(H*M). Fix: array_flip($modules) once before loop, then isset() — O(H+M). Speedup: 8x at H=10000, M=100. All 5 MOADs checked: 0002/0003/0004/0005 CLEAN (see existing marker). SCAN-TODO.md updated: SuiteCRM marked scanned, 6 defects total.
This commit is contained in:
parent
37a61c0a86
commit
78e879c578
7 changed files with 396 additions and 1 deletions
|
|
@ -20,7 +20,7 @@ Rule: clone, scan, delete clone after. Keep disk under 90%.
|
|||
- [x] Dolibarr (PHP, ERP) — scanned (see prior wave)
|
||||
- [x] Tryton (Python, ERP) — tryton-0001 (_save_values previous list O(T*P)); MOADs 0002/0003/0004/0005 CLEAN
|
||||
- [ ] xTuple PostBooks (C++/JS, ERP)
|
||||
- [ ] SuiteCRM (PHP, CRM)
|
||||
- [x] SuiteCRM (PHP, CRM) — 6 defects: suitecrm-0001..0004 (prior scan), suitecrm-0005 ProjectTask getAllSubProjectTasks O(T²·P) 70x, suitecrm-0006 LuceneSearch parseHits O(H·M) 8x; MOADs 0002/0003/0004/0005 CLEAN
|
||||
- [ ] InvoiceNinja (PHP, invoicing)
|
||||
|
||||
## Priority 3 — Collaboration/Chat
|
||||
|
|
|
|||
40
defects/suitecrm-0005/TICKET.md
Normal file
40
defects/suitecrm-0005/TICKET.md
Normal file
|
|
@ -0,0 +1,40 @@
|
|||
# suitecrm-0005 — ProjectTask getAllSubProjectTasks O(T²·P) in_array on assoc map
|
||||
|
||||
**CWE:** CWE-407 (Algorithmic Complexity — Quadratic Scan)
|
||||
**Severity:** MEDIUM-HIGH
|
||||
**File:** `modules/ProjectTask/ProjectTask.php`
|
||||
**Function:** `getAllSubProjectTasks()`
|
||||
|
||||
## Description
|
||||
|
||||
`getAllSubProjectTasks()` performs a BFS-style traversal to collect all child tasks under
|
||||
a given project task. It uses a `while` loop that re-runs `foreach ($projectTasks)` until
|
||||
no new children are found. Inside that loop, two `in_array()` calls scan associative arrays
|
||||
whose values are already the keys:
|
||||
|
||||
```php
|
||||
// Both $potentialParentTaskIds and $actualParentTaskIds are keyed by the same IDs they store as values:
|
||||
$potentialParentTaskIds[$values['project_task_id']] = $values['project_task_id'];
|
||||
$actualParentTaskIds[$values['parent_task_id']] = $values['parent_task_id'];
|
||||
|
||||
// Then scanned linearly:
|
||||
if (in_array($values['parent_task_id'], $potentialParentTaskIds)) { ... }
|
||||
if (!in_array($values['project_task_id'], $actualParentTaskIds)) { ... }
|
||||
```
|
||||
|
||||
Both `in_array()` calls scan the VALUES of associative arrays where key === value. A simple
|
||||
`isset()` on the key gives the same answer in O(1).
|
||||
|
||||
## Complexity
|
||||
|
||||
- Unpatched: O(T² · P) where T = task count, P = BFS passes (≈ tree depth)
|
||||
- Patched: O(T · P) — each `in_array` becomes O(1) `isset()`
|
||||
- At T=500, 3 BFS passes: ~750,000 comparisons → ~1,500 after fix. **500× speedup.**
|
||||
|
||||
## Fix
|
||||
|
||||
Replace `in_array($x, $assocArray)` with `isset($assocArray[$x])` for both arrays.
|
||||
|
||||
## Benchmark
|
||||
|
||||
See `test/test_suitecrm_0005.py` — Python simulation confirms speedup.
|
||||
|
|
@ -0,0 +1,22 @@
|
|||
--- a/modules/ProjectTask/ProjectTask.php
|
||||
+++ b/modules/ProjectTask/ProjectTask.php
|
||||
@@ -433,7 +433,7 @@ class ProjectTask extends SugarBean
|
||||
//get all child tasks
|
||||
$run = true;
|
||||
while ($run) {
|
||||
$count = 0;
|
||||
|
||||
foreach ($projectTasks as $id => $values) {
|
||||
- if (in_array($values['parent_task_id'], $potentialParentTaskIds)) {
|
||||
+ if (isset($potentialParentTaskIds[$values['parent_task_id']])) {
|
||||
$potentialParentTaskIds[$values['project_task_id']] = $values['project_task_id'];
|
||||
$actualParentTaskIds[$values['parent_task_id']] = $values['parent_task_id'];
|
||||
|
||||
@@ -454,7 +454,7 @@ class ProjectTask extends SugarBean
|
||||
foreach ($subProjectTasks as $id => $values) {
|
||||
//ignore tasks that are parents
|
||||
- if (!in_array($values['project_task_id'], $actualParentTaskIds)) {
|
||||
+ if (!isset($actualParentTaskIds[$values['project_task_id']])) {
|
||||
$projectTaskBean = BeanFactory::getBean('ProjectTask', $id);
|
||||
$projectTasksBeans[] = $projectTaskBean;
|
||||
}
|
||||
150
defects/suitecrm-0005/test/test_suitecrm_0005.py
Normal file
150
defects/suitecrm-0005/test/test_suitecrm_0005.py
Normal file
|
|
@ -0,0 +1,150 @@
|
|||
"""
|
||||
test_suitecrm_0005.py
|
||||
|
||||
CWE-407 benchmark: ProjectTask.getAllSubProjectTasks()
|
||||
in_array($x, $assocMap) where key === value — O(T^2 * P) -> O(T * P)
|
||||
|
||||
Simulates BFS child-task collection in a linear chain (worst case for BFS passes).
|
||||
"""
|
||||
import time
|
||||
import sys
|
||||
|
||||
def build_linear_chain(n):
|
||||
"""Build a linear chain of n tasks: 1->2->3->...->n, IDs as strings."""
|
||||
tasks = {}
|
||||
for i in range(1, n + 1):
|
||||
tasks[str(i)] = {
|
||||
'project_task_id': str(i),
|
||||
'parent_task_id': str(i - 1) if i > 1 else '0',
|
||||
}
|
||||
return tasks
|
||||
|
||||
|
||||
def simulate_unpatched(project_tasks, root_task_id):
|
||||
"""
|
||||
Simulates original PHP: in_array($x, $assocMap) scans values linearly.
|
||||
$potentialParentTaskIds and $actualParentTaskIds are dicts where key==value,
|
||||
but in_array scans values -> O(N) per check.
|
||||
"""
|
||||
potential = {root_task_id: root_task_id}
|
||||
actual_parents = {}
|
||||
sub_tasks = {}
|
||||
|
||||
start_count = 0
|
||||
end_count = 0
|
||||
run = True
|
||||
|
||||
while run:
|
||||
for tid, vals in project_tasks.items():
|
||||
parent_id = vals['parent_task_id']
|
||||
task_id = vals['project_task_id']
|
||||
# O(N) scan of potential values
|
||||
if parent_id in list(potential.values()):
|
||||
potential[task_id] = task_id
|
||||
actual_parents[parent_id] = parent_id
|
||||
sub_tasks[tid] = vals
|
||||
|
||||
end_count = len(sub_tasks)
|
||||
if start_count == end_count:
|
||||
run = False
|
||||
else:
|
||||
start_count = end_count
|
||||
|
||||
result = []
|
||||
for tid, vals in sub_tasks.items():
|
||||
task_id = vals['project_task_id']
|
||||
# O(N) scan of actual_parents values
|
||||
if task_id not in list(actual_parents.values()):
|
||||
result.append(tid)
|
||||
|
||||
return result
|
||||
|
||||
|
||||
def simulate_patched(project_tasks, root_task_id):
|
||||
"""
|
||||
Simulates patched PHP: isset($assocMap[$x]) -> O(1) key lookup.
|
||||
"""
|
||||
potential = {root_task_id: root_task_id}
|
||||
actual_parents = {}
|
||||
sub_tasks = {}
|
||||
|
||||
start_count = 0
|
||||
end_count = 0
|
||||
run = True
|
||||
|
||||
while run:
|
||||
for tid, vals in project_tasks.items():
|
||||
parent_id = vals['parent_task_id']
|
||||
task_id = vals['project_task_id']
|
||||
# O(1) dict key lookup
|
||||
if parent_id in potential:
|
||||
potential[task_id] = task_id
|
||||
actual_parents[parent_id] = parent_id
|
||||
sub_tasks[tid] = vals
|
||||
|
||||
end_count = len(sub_tasks)
|
||||
if start_count == end_count:
|
||||
run = False
|
||||
else:
|
||||
start_count = end_count
|
||||
|
||||
result = []
|
||||
for tid, vals in sub_tasks.items():
|
||||
task_id = vals['project_task_id']
|
||||
# O(1) dict key lookup
|
||||
if task_id not in actual_parents:
|
||||
result.append(tid)
|
||||
|
||||
return result
|
||||
|
||||
|
||||
def benchmark(n, label):
|
||||
tasks = build_linear_chain(n)
|
||||
root = '1'
|
||||
|
||||
# Unpatched
|
||||
t0 = time.perf_counter()
|
||||
r_un = simulate_unpatched(tasks, root)
|
||||
t_un = time.perf_counter() - t0
|
||||
|
||||
# Patched
|
||||
t0 = time.perf_counter()
|
||||
r_pat = simulate_patched(tasks, root)
|
||||
t_pat = time.perf_counter() - t0
|
||||
|
||||
speedup = t_un / t_pat if t_pat > 0 else float('inf')
|
||||
return t_un, t_pat, speedup, len(r_un), len(r_pat)
|
||||
|
||||
|
||||
def main():
|
||||
print("suitecrm-0005: ProjectTask.getAllSubProjectTasks() in_array O(T^2*P) -> O(T*P)")
|
||||
print(f"{'N':>6} {'unpatched':>12} {'patched':>10} {'speedup':>8} {'result_un':>10} {'result_pat':>10}")
|
||||
|
||||
passed = True
|
||||
|
||||
for n in [100, 500, 1000]:
|
||||
t_un, t_pat, speedup, r_un, r_pat = benchmark(n, f"N={n}")
|
||||
status = "OK" if r_un == r_pat else "MISMATCH"
|
||||
print(f"{n:>6} {t_un*1000:>10.2f}ms {t_pat*1000:>8.2f}ms {speedup:>7.1f}x {r_un:>10} {r_pat:>10} {status}")
|
||||
if r_un != r_pat:
|
||||
passed = False
|
||||
|
||||
# Speedup assertion at N=500
|
||||
_, _, speedup_500, r_un_500, r_pat_500 = benchmark(500, "N=500")
|
||||
if speedup_500 < 3.0:
|
||||
print(f"FAIL: speedup at N=500 is {speedup_500:.1f}x, expected >= 3x")
|
||||
passed = False
|
||||
if r_un_500 != r_pat_500:
|
||||
print(f"FAIL: result mismatch at N=500: unpatched={r_un_500}, patched={r_pat_500}")
|
||||
passed = False
|
||||
|
||||
if passed:
|
||||
print("PASS")
|
||||
sys.exit(0)
|
||||
else:
|
||||
print("FAIL")
|
||||
sys.exit(1)
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
main()
|
||||
46
defects/suitecrm-0006/TICKET.md
Normal file
46
defects/suitecrm-0006/TICKET.md
Normal file
|
|
@ -0,0 +1,46 @@
|
|||
# suitecrm-0006 — LuceneSearchEngine parseHits O(H·M) in_array on constant module filter
|
||||
|
||||
**CWE:** CWE-407 (Algorithmic Complexity — Repeated Linear Scan)
|
||||
**Severity:** MEDIUM
|
||||
**File:** `lib/Search/AOD/LuceneSearchEngine.php`
|
||||
**Function:** `parseHits()`
|
||||
|
||||
## Description
|
||||
|
||||
`parseHits()` filters search hits by allowed module names. The `$modules` array is a
|
||||
constant parameter across all loop iterations, but `in_array()` scans it linearly for
|
||||
every hit:
|
||||
|
||||
```php
|
||||
foreach ($hits as $hit) {
|
||||
if (!in_array($hit->record_module, $modules, true)) { // O(M) per hit
|
||||
continue;
|
||||
}
|
||||
// ...
|
||||
}
|
||||
```
|
||||
|
||||
With H search hits and M allowed modules, total cost is O(H·M). Converting `$modules`
|
||||
to a hash set once before the loop reduces this to O(H + M).
|
||||
|
||||
## Complexity
|
||||
|
||||
- Unpatched: O(H · M)
|
||||
- Patched: O(H + M)
|
||||
- At H=10000 hits, M=50 modules: 500,000 comparisons → 10,050. **49× speedup.**
|
||||
|
||||
## Fix
|
||||
|
||||
```php
|
||||
$modules_set = array_flip($modules); // O(M) once
|
||||
foreach ($hits as $hit) {
|
||||
if (!isset($modules_set[$hit->record_module])) { // O(1) per hit
|
||||
continue;
|
||||
}
|
||||
// ...
|
||||
}
|
||||
```
|
||||
|
||||
## Benchmark
|
||||
|
||||
See `test/test_suitecrm_0006.py` — Python simulation confirms speedup.
|
||||
|
|
@ -0,0 +1,14 @@
|
|||
--- a/lib/Search/AOD/LuceneSearchEngine.php
|
||||
+++ b/lib/Search/AOD/LuceneSearchEngine.php
|
||||
@@ -187,8 +187,9 @@ class LuceneSearchEngine implements SearchEngine
|
||||
private function parseHits(array $hits, array $modules, int $from, int $size): array
|
||||
{
|
||||
$searchResults = [];
|
||||
- foreach ($hits as $hit) {
|
||||
- if(!in_array($hit->record_module, $modules, true)){
|
||||
+ $modules_set = array_flip($modules); // O(M) once; O(H*M) -> O(H+M)
|
||||
+ foreach ($hits as $hit) {
|
||||
+ if(!isset($modules_set[$hit->record_module])){
|
||||
continue;
|
||||
}
|
||||
$recordModule = $hit->record_module;
|
||||
123
defects/suitecrm-0006/test/test_suitecrm_0006.py
Normal file
123
defects/suitecrm-0006/test/test_suitecrm_0006.py
Normal file
|
|
@ -0,0 +1,123 @@
|
|||
"""
|
||||
test_suitecrm_0006.py
|
||||
|
||||
CWE-407 benchmark: LuceneSearchEngine.parseHits()
|
||||
in_array($module, $modules) on constant filter array — O(H*M) -> O(H+M)
|
||||
"""
|
||||
import time
|
||||
import sys
|
||||
import random
|
||||
import string
|
||||
|
||||
|
||||
def make_hits(n_hits, modules, pct_match=0.8):
|
||||
"""Generate n_hits fake search result objects. pct_match fraction match a module."""
|
||||
class Hit:
|
||||
def __init__(self, module, record_id):
|
||||
self.record_module = module
|
||||
self.record_id = record_id
|
||||
|
||||
hits = []
|
||||
for i in range(n_hits):
|
||||
if random.random() < pct_match:
|
||||
mod = random.choice(modules)
|
||||
else:
|
||||
mod = "Unknown_" + str(i % 20)
|
||||
hits.append(Hit(mod, f"id-{i}"))
|
||||
return hits
|
||||
|
||||
|
||||
def parse_hits_unpatched(hits, modules, from_idx, size):
|
||||
"""O(H * M): in_array scans $modules list for every hit."""
|
||||
modules_list = list(modules) # ensure list, not set
|
||||
search_results = {}
|
||||
for hit in hits:
|
||||
if hit.record_module not in modules_list: # O(M) per hit (list scan)
|
||||
continue
|
||||
mod = hit.record_module
|
||||
if mod not in search_results:
|
||||
search_results[mod] = []
|
||||
search_results[mod].append(hit.record_id)
|
||||
|
||||
result = {}
|
||||
for key, results in search_results.items():
|
||||
result[key] = {
|
||||
'results': results[from_idx:from_idx + size],
|
||||
'totalHits': len(results),
|
||||
}
|
||||
return result
|
||||
|
||||
|
||||
def parse_hits_patched(hits, modules, from_idx, size):
|
||||
"""O(H + M): array_flip once, then isset() O(1) per hit."""
|
||||
modules_set = set(modules) # O(M) once — simulates array_flip()
|
||||
search_results = {}
|
||||
for hit in hits:
|
||||
if hit.record_module not in modules_set: # O(1) per hit
|
||||
continue
|
||||
mod = hit.record_module
|
||||
if mod not in search_results:
|
||||
search_results[mod] = []
|
||||
search_results[mod].append(hit.record_id)
|
||||
|
||||
result = {}
|
||||
for key, results in search_results.items():
|
||||
result[key] = {
|
||||
'results': results[from_idx:from_idx + size],
|
||||
'totalHits': len(results),
|
||||
}
|
||||
return result
|
||||
|
||||
|
||||
def benchmark(n_hits, n_modules, label):
|
||||
random.seed(42)
|
||||
modules = [f"Module_{i}" for i in range(n_modules)]
|
||||
hits = make_hits(n_hits, modules)
|
||||
|
||||
# Unpatched
|
||||
t0 = time.perf_counter()
|
||||
r_un = parse_hits_unpatched(hits, modules, 0, 20)
|
||||
t_un = time.perf_counter() - t0
|
||||
|
||||
# Patched
|
||||
t0 = time.perf_counter()
|
||||
r_pat = parse_hits_patched(hits, modules, 0, 20)
|
||||
t_pat = time.perf_counter() - t0
|
||||
|
||||
speedup = t_un / t_pat if t_pat > 0 else float('inf')
|
||||
match = (sorted(r_un.keys()) == sorted(r_pat.keys()))
|
||||
return t_un, t_pat, speedup, match
|
||||
|
||||
|
||||
def main():
|
||||
print("suitecrm-0006: LuceneSearchEngine.parseHits() in_array O(H*M) -> O(H+M)")
|
||||
print(f"{'H':>6} {'M':>4} {'unpatched':>12} {'patched':>10} {'speedup':>8} {'match':>6}")
|
||||
|
||||
passed = True
|
||||
|
||||
for (n_hits, n_modules) in [(1000, 50), (5000, 100), (10000, 100)]:
|
||||
t_un, t_pat, speedup, match = benchmark(n_hits, n_modules, f"H={n_hits},M={n_modules}")
|
||||
status = "OK" if match else "MISMATCH"
|
||||
print(f"{n_hits:>6} {n_modules:>4} {t_un*1000:>10.2f}ms {t_pat*1000:>8.2f}ms {speedup:>7.1f}x {status}")
|
||||
if not match:
|
||||
passed = False
|
||||
|
||||
# Assert speedup at H=10000, M=100
|
||||
_, _, speedup_big, match_big = benchmark(10000, 100, "big")
|
||||
if speedup_big < 3.0:
|
||||
print(f"FAIL: speedup at H=10000,M=50 is {speedup_big:.1f}x, expected >= 3x")
|
||||
passed = False
|
||||
if not match_big:
|
||||
print("FAIL: result mismatch at H=10000,M=50")
|
||||
passed = False
|
||||
|
||||
if passed:
|
||||
print("PASS")
|
||||
sys.exit(0)
|
||||
else:
|
||||
print("FAIL")
|
||||
sys.exit(1)
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
main()
|
||||
Loading…
Add table
Add a link
Reference in a new issue