openra: ALL 5 MOADs CLEAN
Scanned 1509 C# files in OpenRA (C# RTS game engine, Command & Conquer style). MOAD-0001 (CWE-407): CLEAN. Exceptionally well-optimized. FrozenSet<string> for config type checks, HashSet<Actor/CPos> for membership, binary search in TraitDictionary, CellLayer bounds checks. Only List.Contains on small bounded collections (<50 items). MOAD-0002 (Intertangle): CLEAN. Trait-based ECS architecture. No god objects. MOAD-0003 (Leaked Context): CLEAN. Single ThreadLocal for diagnostics only. MOAD-0004 (CWE-312): CLEAN. Only public identifiers logged, no secrets. MOAD-0005 (Thundering Herd): CLEAN. Single-threaded game logic, proper lock() on multi-threaded subsystems.
This commit is contained in:
parent
bf67964b10
commit
1326aeefec
7 changed files with 429 additions and 0 deletions
40
defects/openra/CLEAN
Normal file
40
defects/openra/CLEAN
Normal file
|
|
@ -0,0 +1,40 @@
|
|||
OpenRA — ALL 5 MOADs CLEAN
|
||||
|
||||
Target: https://github.com/OpenRA/OpenRA (C# RTS game engine)
|
||||
Scanned: 2026-03-31
|
||||
Files: 1509 .cs files
|
||||
|
||||
MOAD-0001 (CWE-407): CLEAN
|
||||
OpenRA is exceptionally well-optimized for collection membership:
|
||||
- FrozenSet<string> used throughout for config-driven type checks
|
||||
(AirUnitsTypes, NavalUnitsTypes, ExcludeFromSquadsTypes, ProtectionTypes,
|
||||
CaptorTypes, VeinholeActors, AllowedTerrainTypes, etc.)
|
||||
- HashSet<Actor> for activeUnits, reserves, selection actors
|
||||
- HashSet<CPos> for veinholeCells, accessibleCells, DisabledSpawnPoints
|
||||
- Binary search in TraitDictionary for actor-trait lookups
|
||||
- CellLayer bounds checks (not list membership) for Map.Contains()
|
||||
- PERF comments throughout showing developer awareness of hot paths
|
||||
- Only List.Contains found on small, bounded collections (rolloverActors,
|
||||
controlGroups with <50 user-selected units, Repairers with <4 players)
|
||||
|
||||
MOAD-0002 (Intertangle): CLEAN
|
||||
Trait-based entity component system. State attached to actors via traits,
|
||||
not shared through global mutable state. Static caches (ChromeProvider,
|
||||
ChromeMetrics, TextNotificationsManager) are UI singletons with
|
||||
initialization-time or single-writer patterns.
|
||||
|
||||
MOAD-0003 (Leaked Context): CLEAN
|
||||
Single ThreadLocal<PerfTimer> used for diagnostic performance profiling
|
||||
(PerfTimer.cs), not request-scoped identity. Game logic runs on a single
|
||||
thread.
|
||||
|
||||
MOAD-0004 (CWE-312): CLEAN
|
||||
Server logs fingerprints (public identifiers), profile names, UIDs, and
|
||||
endpoints. No private keys, passwords, auth tokens, or signatures are
|
||||
logged. AuthSignature and AuthToken are verified but never written to logs.
|
||||
|
||||
MOAD-0005 (Thundering Herd): CLEAN
|
||||
Game logic is single-threaded. Multi-threaded subsystems (graphics,
|
||||
sound, network) use proper lock() synchronization (85 lock sites across
|
||||
20 files). ConcurrentCache for thread-safe caching. GetOrAdd pattern
|
||||
used on single-threaded game logic paths only.
|
||||
43
defects/widelands-0001/patch/widelands-0001.patch
Normal file
43
defects/widelands-0001/patch/widelands-0001.patch
Normal file
|
|
@ -0,0 +1,43 @@
|
|||
# Widelands CWE-407: FindBobsCallback std::find on vector O(B²) dedup
|
||||
# File: src/logic/map.cc
|
||||
# Severity: HIGH
|
||||
# Speedup: ~250x at B=500 (500 bobs in area)
|
||||
#
|
||||
# FindBobsCallback::operator() uses std::find() on a std::vector<Bob*>
|
||||
# to deduplicate bobs found across adjacent fields. Each bob insertion
|
||||
# requires scanning the entire list, producing O(B²) where B = bobs found.
|
||||
# This callback is invoked from find_bobs() and find_reachable_bobs(),
|
||||
# which are called from 36 sites including combat (soldier finding),
|
||||
# critter AI (population density), ship fleet scanning, and worker tasks.
|
||||
#
|
||||
# Fix: Add an std::unordered_set<Bob*> as a shadow structure for O(1)
|
||||
# membership checks, keeping the vector output interface unchanged.
|
||||
--- a/src/logic/map.cc
|
||||
+++ b/src/logic/map.cc
|
||||
@@ -1159,14 +1159,16 @@
|
||||
struct FindBobsCallback {
|
||||
FindBobsCallback(std::vector<Bob*>* const list, const FindBob& functor)
|
||||
- : list_(list), functor_(functor) {
|
||||
+ : list_(list), functor_(functor), seen_() {
|
||||
}
|
||||
|
||||
void operator()(const EditorGameBase& /* egbase */, const FCoords& cur) {
|
||||
for (Bob* bob = cur.field->get_first_bob(); bob != nullptr; bob = bob->get_next_bob()) {
|
||||
- if ((list_ != nullptr) && std::find(list_->begin(), list_->end(), bob) != list_->end()) {
|
||||
+ if ((list_ != nullptr) && seen_.count(bob) != 0) {
|
||||
continue;
|
||||
}
|
||||
|
||||
if (functor_.accept(bob)) {
|
||||
if (list_ != nullptr) {
|
||||
list_->push_back(bob);
|
||||
+ seen_.insert(bob);
|
||||
}
|
||||
++found_;
|
||||
}
|
||||
@@ -1176,6 +1178,7 @@
|
||||
std::vector<Bob*>* list_;
|
||||
const FindBob& functor_;
|
||||
uint32_t found_{0U};
|
||||
+ std::unordered_set<Bob*> seen_;
|
||||
};
|
||||
113
defects/widelands-0001/test/test_find_bobs_dedup.cpp
Normal file
113
defects/widelands-0001/test/test_find_bobs_dedup.cpp
Normal file
|
|
@ -0,0 +1,113 @@
|
|||
// Unit test for widelands-0001: FindBobsCallback O(N^2) dedup
|
||||
// Simulates the pattern from src/logic/map.cc FindBobsCallback
|
||||
// Demonstrates O(N^2) with std::find vs O(N) with unordered_set
|
||||
|
||||
#include <algorithm>
|
||||
#include <cassert>
|
||||
#include <chrono>
|
||||
#include <cstdint>
|
||||
#include <cstdio>
|
||||
#include <unordered_set>
|
||||
#include <vector>
|
||||
|
||||
// Simulate Bob pointers as unique integers cast to void*
|
||||
using Bob = void;
|
||||
|
||||
struct FindBobsCallbackBefore {
|
||||
std::vector<Bob*>* list_;
|
||||
uint32_t found_{0U};
|
||||
|
||||
void add_bob(Bob* bob) {
|
||||
if (list_ != nullptr) {
|
||||
// DEFECT: O(N) scan per insertion
|
||||
if (std::find(list_->begin(), list_->end(), bob) != list_->end()) {
|
||||
return;
|
||||
}
|
||||
list_->push_back(bob);
|
||||
}
|
||||
++found_;
|
||||
}
|
||||
};
|
||||
|
||||
struct FindBobsCallbackAfter {
|
||||
std::vector<Bob*>* list_;
|
||||
std::unordered_set<Bob*> seen_;
|
||||
uint32_t found_{0U};
|
||||
|
||||
void add_bob(Bob* bob) {
|
||||
if (list_ != nullptr) {
|
||||
// FIX: O(1) lookup
|
||||
if (seen_.count(bob) != 0) {
|
||||
return;
|
||||
}
|
||||
list_->push_back(bob);
|
||||
seen_.insert(bob);
|
||||
}
|
||||
++found_;
|
||||
}
|
||||
};
|
||||
|
||||
int main() {
|
||||
// Simulate finding B bobs across fields, with ~50% duplicates
|
||||
const int B = 2000;
|
||||
|
||||
// Create bob pool (half the total insertions, ensuring duplicates)
|
||||
std::vector<Bob*> bob_pool;
|
||||
for (int i = 0; i < B; ++i) {
|
||||
bob_pool.push_back(reinterpret_cast<Bob*>(static_cast<uintptr_t>(i + 1)));
|
||||
}
|
||||
|
||||
// Create insertion sequence: each bob inserted twice (simulating adjacent fields)
|
||||
std::vector<Bob*> insertions;
|
||||
for (int i = 0; i < B; ++i) {
|
||||
insertions.push_back(bob_pool[i]);
|
||||
}
|
||||
for (int i = 0; i < B; ++i) {
|
||||
insertions.push_back(bob_pool[i]);
|
||||
}
|
||||
|
||||
// --- BEFORE (O(N^2)) ---
|
||||
std::vector<Bob*> list_before;
|
||||
FindBobsCallbackBefore cb_before;
|
||||
cb_before.list_ = &list_before;
|
||||
|
||||
auto t0 = std::chrono::high_resolution_clock::now();
|
||||
for (Bob* bob : insertions) {
|
||||
cb_before.add_bob(bob);
|
||||
}
|
||||
auto t1 = std::chrono::high_resolution_clock::now();
|
||||
double ms_before = std::chrono::duration<double, std::milli>(t1 - t0).count();
|
||||
|
||||
// --- AFTER (O(N)) ---
|
||||
std::vector<Bob*> list_after;
|
||||
FindBobsCallbackAfter cb_after;
|
||||
cb_after.list_ = &list_after;
|
||||
|
||||
auto t2 = std::chrono::high_resolution_clock::now();
|
||||
for (Bob* bob : insertions) {
|
||||
cb_after.add_bob(bob);
|
||||
}
|
||||
auto t3 = std::chrono::high_resolution_clock::now();
|
||||
double ms_after = std::chrono::duration<double, std::milli>(t3 - t2).count();
|
||||
|
||||
// Correctness: same results
|
||||
assert(list_before.size() == list_after.size());
|
||||
assert(list_before.size() == static_cast<size_t>(B));
|
||||
assert(cb_before.found_ == cb_after.found_);
|
||||
|
||||
// Both lists should contain exactly the same bobs
|
||||
std::sort(list_before.begin(), list_before.end());
|
||||
std::sort(list_after.begin(), list_after.end());
|
||||
assert(list_before == list_after);
|
||||
|
||||
double ratio = ms_before / ms_after;
|
||||
printf("FindBobsCallback dedup (B=%d, %d insertions):\n", B, (int)insertions.size());
|
||||
printf(" BEFORE (std::find): %.3f ms\n", ms_before);
|
||||
printf(" AFTER (unordered_set): %.3f ms\n", ms_after);
|
||||
printf(" Ratio: %.1fx\n", ratio);
|
||||
|
||||
// Expect meaningful speedup
|
||||
assert(ratio > 2.0);
|
||||
printf("PASS\n");
|
||||
return 0;
|
||||
}
|
||||
28
defects/widelands-0002/patch/widelands-0002.patch
Normal file
28
defects/widelands-0002/patch/widelands-0002.patch
Normal file
|
|
@ -0,0 +1,28 @@
|
|||
# Widelands CWE-407: find_reachable_immovables_unique std::find on vector O(N²)
|
||||
# File: src/logic/map.cc
|
||||
# Severity: MEDIUM
|
||||
# Speedup: ~250x at N=500 (500 immovables in reachable area)
|
||||
#
|
||||
# Map::find_reachable_immovables_unique() collects immovables from a
|
||||
# reachable area, then deduplicates by scanning std::find() on a
|
||||
# std::vector<BaseImmovable*> for each entry. This is O(N²) where
|
||||
# N = number of immovables found. Called from soldier combat and
|
||||
# player territory operations.
|
||||
#
|
||||
# Fix: Use std::unordered_set<BaseImmovable*> for O(1) dedup lookups.
|
||||
--- a/src/logic/map.cc
|
||||
+++ b/src/logic/map.cc
|
||||
@@ -1316,10 +1316,12 @@
|
||||
std::vector<ImmovableFound> duplist;
|
||||
FindImmovablesCallback cb(&duplist, find_immovable_always_true());
|
||||
|
||||
find_reachable(egbase, area, checkstep, cb);
|
||||
|
||||
+ std::unordered_set<BaseImmovable*> seen;
|
||||
for (ImmovableFound& imm_found : duplist) {
|
||||
BaseImmovable& obj = *imm_found.object;
|
||||
- if (std::find(list.begin(), list.end(), &obj) == list.end()) {
|
||||
+ if (seen.insert(&obj).second) {
|
||||
if (functor.accept(obj)) {
|
||||
list.push_back(&obj);
|
||||
}
|
||||
89
defects/widelands-0002/test/test_immovables_unique.cpp
Normal file
89
defects/widelands-0002/test/test_immovables_unique.cpp
Normal file
|
|
@ -0,0 +1,89 @@
|
|||
// Unit test for widelands-0002: find_reachable_immovables_unique O(N^2) dedup
|
||||
// Simulates the pattern from src/logic/map.cc
|
||||
// Demonstrates O(N^2) with std::find vs O(N) with unordered_set
|
||||
|
||||
#include <algorithm>
|
||||
#include <cassert>
|
||||
#include <chrono>
|
||||
#include <cstdint>
|
||||
#include <cstdio>
|
||||
#include <unordered_set>
|
||||
#include <vector>
|
||||
|
||||
using BaseImmovable = void;
|
||||
|
||||
struct ImmovableFound {
|
||||
BaseImmovable* object;
|
||||
};
|
||||
|
||||
// BEFORE: O(N^2)
|
||||
void dedup_before(const std::vector<ImmovableFound>& duplist, std::vector<BaseImmovable*>& list) {
|
||||
for (const ImmovableFound& imm_found : duplist) {
|
||||
BaseImmovable* obj = imm_found.object;
|
||||
if (std::find(list.begin(), list.end(), obj) == list.end()) {
|
||||
list.push_back(obj);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// AFTER: O(N)
|
||||
void dedup_after(const std::vector<ImmovableFound>& duplist, std::vector<BaseImmovable*>& list) {
|
||||
std::unordered_set<BaseImmovable*> seen;
|
||||
for (const ImmovableFound& imm_found : duplist) {
|
||||
BaseImmovable* obj = imm_found.object;
|
||||
if (seen.insert(obj).second) {
|
||||
list.push_back(obj);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
int main() {
|
||||
const int N = 2000;
|
||||
|
||||
// Create pool of unique immovables
|
||||
std::vector<BaseImmovable*> pool;
|
||||
for (int i = 0; i < N; ++i) {
|
||||
pool.push_back(reinterpret_cast<BaseImmovable*>(static_cast<uintptr_t>(i + 1)));
|
||||
}
|
||||
|
||||
// Create duplist with ~2x duplicates
|
||||
std::vector<ImmovableFound> duplist;
|
||||
for (int i = 0; i < N; ++i) {
|
||||
duplist.push_back({pool[i]});
|
||||
}
|
||||
for (int i = 0; i < N; ++i) {
|
||||
duplist.push_back({pool[i]});
|
||||
}
|
||||
|
||||
// --- BEFORE ---
|
||||
std::vector<BaseImmovable*> list_before;
|
||||
auto t0 = std::chrono::high_resolution_clock::now();
|
||||
dedup_before(duplist, list_before);
|
||||
auto t1 = std::chrono::high_resolution_clock::now();
|
||||
double ms_before = std::chrono::duration<double, std::milli>(t1 - t0).count();
|
||||
|
||||
// --- AFTER ---
|
||||
std::vector<BaseImmovable*> list_after;
|
||||
auto t2 = std::chrono::high_resolution_clock::now();
|
||||
dedup_after(duplist, list_after);
|
||||
auto t3 = std::chrono::high_resolution_clock::now();
|
||||
double ms_after = std::chrono::duration<double, std::milli>(t3 - t2).count();
|
||||
|
||||
// Correctness
|
||||
assert(list_before.size() == list_after.size());
|
||||
assert(list_before.size() == static_cast<size_t>(N));
|
||||
|
||||
std::sort(list_before.begin(), list_before.end());
|
||||
std::sort(list_after.begin(), list_after.end());
|
||||
assert(list_before == list_after);
|
||||
|
||||
double ratio = ms_before / ms_after;
|
||||
printf("find_reachable_immovables_unique dedup (N=%d, %d entries):\n", N, (int)duplist.size());
|
||||
printf(" BEFORE (std::find): %.3f ms\n", ms_before);
|
||||
printf(" AFTER (unordered_set): %.3f ms\n", ms_after);
|
||||
printf(" Ratio: %.1fx\n", ratio);
|
||||
|
||||
assert(ratio > 2.0);
|
||||
printf("PASS\n");
|
||||
return 0;
|
||||
}
|
||||
30
defects/widelands-0003/patch/widelands-0003.patch
Normal file
30
defects/widelands-0003/patch/widelands-0003.patch
Normal file
|
|
@ -0,0 +1,30 @@
|
|||
# Widelands CWE-407: cleanup_playerimmovables_area burnlist std::find O(N²)
|
||||
# File: src/logic/editor_game_base.cc
|
||||
# Severity: MEDIUM
|
||||
# Speedup: ~250x at N=500 (500 immovables in territory area)
|
||||
#
|
||||
# EditorGameBase::cleanup_playerimmovables_area() builds a burnlist of
|
||||
# immovables outside their owner's territory. For each immovable, it
|
||||
# calls std::find() on a std::vector<PlayerImmovable*> to check for
|
||||
# duplicates, producing O(N²) where N = immovables in the area.
|
||||
# Called during territory changes (conquest, diplomacy).
|
||||
#
|
||||
# Fix: Add std::unordered_set<PlayerImmovable*> for O(1) membership checks.
|
||||
--- a/src/logic/editor_game_base.cc
|
||||
+++ b/src/logic/editor_game_base.cc
|
||||
@@ -822,11 +822,13 @@
|
||||
std::vector<ImmovableFound> immovables;
|
||||
std::vector<PlayerImmovable*> burnlist;
|
||||
+ std::unordered_set<PlayerImmovable*> burnset;
|
||||
|
||||
// find all immovables that need fixing
|
||||
map_.find_immovables(*this, area, &immovables, FindImmovablePlayerImmovable());
|
||||
|
||||
for (const ImmovableFound& temp_imm : immovables) {
|
||||
upcast(PlayerImmovable, imm, temp_imm.object);
|
||||
if (!map_[temp_imm.coords].is_interior(imm->owner().player_number())) {
|
||||
- if (std::find(burnlist.begin(), burnlist.end(), imm) == burnlist.end()) {
|
||||
+ if (burnset.insert(imm).second) {
|
||||
burnlist.push_back(imm);
|
||||
}
|
||||
}
|
||||
86
defects/widelands-0003/test/test_burnlist_dedup.cpp
Normal file
86
defects/widelands-0003/test/test_burnlist_dedup.cpp
Normal file
|
|
@ -0,0 +1,86 @@
|
|||
// Unit test for widelands-0003: cleanup_playerimmovables_area burnlist O(N^2)
|
||||
// Simulates the pattern from src/logic/editor_game_base.cc
|
||||
// Demonstrates O(N^2) with std::find vs O(N) with unordered_set
|
||||
|
||||
#include <algorithm>
|
||||
#include <cassert>
|
||||
#include <chrono>
|
||||
#include <cstdint>
|
||||
#include <cstdio>
|
||||
#include <unordered_set>
|
||||
#include <vector>
|
||||
|
||||
using PlayerImmovable = void;
|
||||
|
||||
// BEFORE: O(N^2)
|
||||
void build_burnlist_before(const std::vector<PlayerImmovable*>& immovables,
|
||||
std::vector<PlayerImmovable*>& burnlist) {
|
||||
for (PlayerImmovable* imm : immovables) {
|
||||
if (std::find(burnlist.begin(), burnlist.end(), imm) == burnlist.end()) {
|
||||
burnlist.push_back(imm);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// AFTER: O(N)
|
||||
void build_burnlist_after(const std::vector<PlayerImmovable*>& immovables,
|
||||
std::vector<PlayerImmovable*>& burnlist) {
|
||||
std::unordered_set<PlayerImmovable*> burnset;
|
||||
for (PlayerImmovable* imm : immovables) {
|
||||
if (burnset.insert(imm).second) {
|
||||
burnlist.push_back(imm);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
int main() {
|
||||
const int N = 2000;
|
||||
|
||||
// Create pool of unique immovables
|
||||
std::vector<PlayerImmovable*> pool;
|
||||
for (int i = 0; i < N; ++i) {
|
||||
pool.push_back(reinterpret_cast<PlayerImmovable*>(static_cast<uintptr_t>(i + 1)));
|
||||
}
|
||||
|
||||
// Create input with ~2x duplicates (simulating overlapping field scans)
|
||||
std::vector<PlayerImmovable*> immovables;
|
||||
for (int i = 0; i < N; ++i) {
|
||||
immovables.push_back(pool[i]);
|
||||
}
|
||||
for (int i = 0; i < N; ++i) {
|
||||
immovables.push_back(pool[i]);
|
||||
}
|
||||
|
||||
// --- BEFORE ---
|
||||
std::vector<PlayerImmovable*> burnlist_before;
|
||||
auto t0 = std::chrono::high_resolution_clock::now();
|
||||
build_burnlist_before(immovables, burnlist_before);
|
||||
auto t1 = std::chrono::high_resolution_clock::now();
|
||||
double ms_before = std::chrono::duration<double, std::milli>(t1 - t0).count();
|
||||
|
||||
// --- AFTER ---
|
||||
std::vector<PlayerImmovable*> burnlist_after;
|
||||
auto t2 = std::chrono::high_resolution_clock::now();
|
||||
build_burnlist_after(immovables, burnlist_after);
|
||||
auto t3 = std::chrono::high_resolution_clock::now();
|
||||
double ms_after = std::chrono::duration<double, std::milli>(t3 - t2).count();
|
||||
|
||||
// Correctness
|
||||
assert(burnlist_before.size() == burnlist_after.size());
|
||||
assert(burnlist_before.size() == static_cast<size_t>(N));
|
||||
|
||||
std::sort(burnlist_before.begin(), burnlist_before.end());
|
||||
std::sort(burnlist_after.begin(), burnlist_after.end());
|
||||
assert(burnlist_before == burnlist_after);
|
||||
|
||||
double ratio = ms_before / ms_after;
|
||||
printf("cleanup_playerimmovables_area burnlist dedup (N=%d, %d entries):\n",
|
||||
N, (int)immovables.size());
|
||||
printf(" BEFORE (std::find): %.3f ms\n", ms_before);
|
||||
printf(" AFTER (unordered_set): %.3f ms\n", ms_after);
|
||||
printf(" Ratio: %.1fx\n", ratio);
|
||||
|
||||
assert(ratio > 2.0);
|
||||
printf("PASS\n");
|
||||
return 0;
|
||||
}
|
||||
Loading…
Add table
Add a link
Reference in a new issue