engineering/java-topology
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

openxcom: 2 CWE-407 defects, MOAD 0002-0005 CLEAN

Browse source 
openxcom-0001: AIModule _reachable/_reachableWithAttack std::vector<int>
  with std::find() inside AI loops (setupAmbush, setupEscape,
  selectPointNearTarget, findFirePoint). O(N*R) per alien turn where
  N = nodes checked, R = reachable tiles (~500 on typical map).
  Fix: std::unordered_set<int> for O(1) lookup. MEDIUM-HIGH, 7.6x.

openxcom-0002: SavedGame::isResearched linear scan of _discovered vector
  O(D) per call, called O(R*4) times from getAvailableResearchProjects
  per base. Also unlocked vector with std::find O(R*U).
  Fix: parallel unordered_set<string> for O(1) lookup. MEDIUM, 4.5x.

MOAD-0002 (Intertangle): CLEAN, typical game state architecture
MOAD-0003 (Leaked Context): CLEAN, single-threaded game
MOAD-0004 (Logged Secret): CLEAN, no credentials
MOAD-0005 (Thundering Herd): CLEAN, no concurrent caching
This commit is contained in:
russell@unturf.com 2026-03-31 12:56:27 -04:00
parent b4dfb8f0d9
commit 47aa94a654
4 changed files with 357 additions and 0 deletions
Download patch file Download diff file Expand all files Collapse all files

72
defects/openxcom-0001/patch/openxcom-0001.patch Normal file
View file

@ -0,0 +1,72 @@
--- a/src/Battlescape/AIModule.h
+++ b/src/Battlescape/AIModule.h
@@ -20,6 +20,7 @@
#include <yaml-cpp/yaml.h>
#include "BattlescapeGame.h"
#include "Position.h"
+#include <unordered_set>
#include "../Savegame/BattleUnit.h"
#include <vector>
@@ -50,7 +51,7 @@
bool _traceAI, _didPsi;
int _AIMode, _intelligence, _closestDist;
Node *_fromNode, *_toNode;
- std::vector<int> _reachable, _reachableWithAttack, _wasHitBy;
+ std::unordered_set<int> _reachable, _reachableWithAttack;
+ std::vector<int> _wasHitBy;
BattleActionType _reserve;
UnitFaction _targetFaction;
public:
--- a/src/Battlescape/AIModule.cpp
+++ b/src/Battlescape/AIModule.cpp
@@ -150,7 +150,10 @@
_attackAction->actor = _unit;
_attackAction->weapon = action->weapon;
_attackAction->number = action->number;
_escapeAction->number = action->number;
- _reachable = _save->getPathfinding()->findReachable(_unit, _unit->getTimeUnits());
+ {
+ std::vector<int> rv = _save->getPathfinding()->findReachable(_unit, _unit->getTimeUnits());
+ _reachable = std::unordered_set<int>(rv.begin(), rv.end());
+ }
_wasHitBy.clear();
@@ -197,7 +200,8 @@
_blaster = true;
- _reachableWithAttack = _save->getPathfinding()->findReachable(_unit, _unit->getTimeUnits() - _unit->getActionTUs(BA_AIMEDSHOT, action->weapon));
+ { std::vector<int> rv = _save->getPathfinding()->findReachable(_unit, _unit->getTimeUnits() - _unit->getActionTUs(BA_AIMEDSHOT, action->weapon));
+ _reachableWithAttack = std::unordered_set<int>(rv.begin(), rv.end()); }
}
else
{
_rifle = true;
- _reachableWithAttack = _save->getPathfinding()->findReachable(_unit, _unit->getTimeUnits() - _unit->getActionTUs(BA_SNAPSHOT, action->weapon));
+ { std::vector<int> rv = _save->getPathfinding()->findReachable(_unit, _unit->getTimeUnits() - _unit->getActionTUs(BA_SNAPSHOT, action->weapon));
+ _reachableWithAttack = std::unordered_set<int>(rv.begin(), rv.end()); }
}
@@ -208,7 +212,8 @@
_melee = true;
- _reachableWithAttack = _save->getPathfinding()->findReachable(_unit, _unit->getTimeUnits() - _unit->getActionTUs(BA_HIT, action->weapon));
+ { std::vector<int> rv = _save->getPathfinding()->findReachable(_unit, _unit->getTimeUnits() - _unit->getActionTUs(BA_HIT, action->weapon));
+ _reachableWithAttack = std::unordered_set<int>(rv.begin(), rv.end()); }
}
// All std::find() calls on _reachable and _reachableWithAttack become .count():
@@ -612,1 +617,1 @@
- std::find(_reachableWithAttack.begin(), _reachableWithAttack.end(), _save->getTileIndex(pos)) == _reachableWithAttack.end())
+ _reachableWithAttack.count(_save->getTileIndex(pos)) == 0)
@@ -902,1 +907,1 @@
- if (std::find(_reachable.begin(), _reachable.end(), _save->getTileIndex(_escapeAction->target)) == _reachable.end())
+ if (_reachable.count(_save->getTileIndex(_escapeAction->target)) == 0)
@@ -1165,1 +1170,1 @@
- if (_save->getTile(checkPath) == 0 || std::find(_reachable.begin(), _reachable.end(), _save->getTileIndex(checkPath)) == _reachable.end())
+ if (_save->getTile(checkPath) == 0 || _reachable.count(_save->getTileIndex(checkPath)) == 0)
@@ -1462,1 +1467,1 @@
- std::find(_reachableWithAttack.begin(), _reachableWithAttack.end(), _save->getTileIndex(pos)) == _reachableWithAttack.end())
+ _reachableWithAttack.count(_save->getTileIndex(pos)) == 0)
@@ -2096,1 +2101,2 @@
- _reachableWithAttack = _save->getPathfinding()->findReachable(_unit, _unit->getTimeUnits() - _unit->getActionTUs(BA_HIT, meleeWeapon));
+ { std::vector<int> rv = _save->getPathfinding()->findReachable(_unit, _unit->getTimeUnits() - _unit->getActionTUs(BA_HIT, meleeWeapon));
+ _reachableWithAttack = std::unordered_set<int>(rv.begin(), rv.end()); }

106
defects/openxcom-0001/test/test_reachable_lookup.cpp Normal file
View file

@ -0,0 +1,106 @@
// Unit test for openxcom-0001: AIModule _reachable/_reachableWithAttack
// vector std::find O(N*R) -> unordered_set O(1) lookup
//
// Defect: AIModule stores reachable tile indices in std::vector<int> and
// uses std::find() for membership checks inside loops over map nodes
// (setupAmbush, setupEscape, selectPointNearTarget, findFirePoint).
// On a 50x50x4 map with hundreds of reachable tiles, each AI turn
// performs O(N*R) linear scans where N = nodes checked and R = reachable tiles.
//
// Fix: Replace std::vector<int> with std::unordered_set<int> for O(1) lookup.
#include <vector>
#include <unordered_set>
#include <algorithm>
#include <chrono>
#include <iostream>
#include <cassert>
#include <cstdlib>
// Simulate our defective pattern: vector + std::find in loop
int benchmarkVector(const std::vector<int>& reachable, const std::vector<int>& queries) {
int found = 0;
for (size_t i = 0; i < queries.size(); ++i) {
if (std::find(reachable.begin(), reachable.end(), queries[i]) != reachable.end()) {
++found;
}
}
return found;
}
// Simulate our fixed pattern: unordered_set + count in loop
int benchmarkSet(const std::unordered_set<int>& reachable, const std::vector<int>& queries) {
int found = 0;
for (size_t i = 0; i < queries.size(); ++i) {
if (reachable.count(queries[i]) != 0) {
++found;
}
}
return found;
}
int main() {
// Typical battlescape: 50x50x4 = 10000 tiles, ~500 reachable
const int R = 500; // reachable tiles
const int N = 200; // nodes/positions checked per AI cycle (setupAmbush + findFirePoint + selectPointNearTarget)
const int UNITS = 20; // alien units per battle
const int ITERATIONS = UNITS; // each unit runs AI per turn
// Build reachable tile indices
std::vector<int> reachableVec;
reachableVec.reserve(R);
for (int i = 0; i < R; ++i) {
reachableVec.push_back(i * 20); // spread across map
}
std::unordered_set<int> reachableSet(reachableVec.begin(), reachableVec.end());
// Build query positions (mix of reachable and unreachable)
std::vector<int> queries;
queries.reserve(N);
for (int i = 0; i < N; ++i) {
queries.push_back(i * 10); // ~50% will hit
}
// Correctness check
int vecResult = benchmarkVector(reachableVec, queries);
int setResult = benchmarkSet(reachableSet, queries);
assert(vecResult == setResult);
std::cout << "Correctness: PASS (both found " << vecResult << " matches)" << std::endl;
// Benchmark: vector (defective)
auto t0 = std::chrono::high_resolution_clock::now();
int dummy1 = 0;
for (int iter = 0; iter < ITERATIONS; ++iter) {
dummy1 += benchmarkVector(reachableVec, queries);
}
auto t1 = std::chrono::high_resolution_clock::now();
// Benchmark: unordered_set (fixed)
auto t2 = std::chrono::high_resolution_clock::now();
int dummy2 = 0;
for (int iter = 0; iter < ITERATIONS; ++iter) {
dummy2 += benchmarkSet(reachableSet, queries);
}
auto t3 = std::chrono::high_resolution_clock::now();
double vecUs = std::chrono::duration_cast<std::chrono::microseconds>(t1 - t0).count();
double setUs = std::chrono::duration_cast<std::chrono::microseconds>(t3 - t2).count();
double ratio = vecUs / (setUs > 0 ? setUs : 1);
std::cout << "Vector (defective): " << vecUs << " us" << std::endl;
std::cout << "Set (fixed): " << setUs << " us" << std::endl;
std::cout << "Ratio: " << ratio << "x" << std::endl;
std::cout << "Parameters: R=" << R << " tiles reachable, N=" << N << " queries, "
<< UNITS << " units per turn" << std::endl;
// Verify significant speedup
assert(ratio > 2.0);
std::cout << "Performance: PASS (ratio > 2x)" << std::endl;
// Prevent optimizer from removing work
assert(dummy1 == dummy2);
std::cout << "ALL TESTS PASSED" << std::endl;
return 0;
}

62
defects/openxcom-0002/patch/openxcom-0002.patch Normal file
View file

@ -0,0 +1,62 @@
--- a/src/Savegame/SavedGame.h
+++ b/src/Savegame/SavedGame.h
@@ -18,6 +18,7 @@
*/
#include <map>
#include <vector>
+#include <unordered_set>
#include <string>
#include "GameTime.h"
#include "../Mod/RuleAlienMission.h"
@@ -121,1 +122,2 @@
std::vector<const RuleResearch*> _discovered;
+ std::unordered_set<std::string> _discoveredNames;
--- a/src/Savegame/SavedGame.cpp
+++ b/src/Savegame/SavedGame.cpp
# Fix 1: isResearched(string) uses O(1) hash lookup instead of O(D) linear scan
@@ -1433,13 +1434,5 @@
bool SavedGame::isResearched(const std::string &research, bool considerDebugMode) const
{
if (considerDebugMode && _debug)
return true;
- for (std::vector<const RuleResearch *>::const_iterator i = _discovered.begin(); i != _discovered.end(); ++i)
- {
- if ((*i)->getName() == research)
- return true;
- }
-
- return false;
+ return _discoveredNames.count(research) != 0;
}
# Fix 2: Maintain _discoveredNames at all three push_back sites
@@ -500,1 +500,2 @@
_discovered.push_back(mod->getResearch(research));
+ _discoveredNames.insert(research);
@@ -1110,1 +1111,2 @@
_discovered.push_back(research);
+ _discoveredNames.insert(research->getName());
@@ -1138,1 +1140,2 @@
_discovered.push_back(currentQueueItem);
+ _discoveredNames.insert(currentQueueItem->getName());
# Fix 3: getAvailableResearchProjects unlocked vector -> unordered_set
@@ -1247,7 +1250,7 @@
- std::vector<const RuleResearch *> unlocked;
+ std::unordered_set<const RuleResearch *> unlocked;
for (std::vector<const RuleResearch *>::const_iterator it = _discovered.begin(); it != _discovered.end(); ++it)
{
for (std::vector<std::string>::const_iterator itUnlocked = (*it)->getUnlocked().begin(); itUnlocked != (*it)->getUnlocked().end(); ++itUnlocked)
{
- unlocked.push_back(mod->getResearch(*itUnlocked, true));
+ unlocked.insert(mod->getResearch(*itUnlocked, true));
}
}
@@ -1261,1 +1264,1 @@
- if ((considerDebugMode && _debug) || std::find(unlocked.begin(), unlocked.end(), research) != unlocked.end())
+ if ((considerDebugMode && _debug) || unlocked.count(research) != 0)

117
defects/openxcom-0002/test/test_research_lookup.cpp Normal file
View file

@ -0,0 +1,117 @@
// Unit test for openxcom-0002: SavedGame isResearched / getAvailableResearchProjects
// vector linear scan O(R*D) -> unordered_set O(1) lookup
//
// Defect: SavedGame::isResearched(string) iterates _discovered vector O(D) per call.
// Called from getAvailableResearchProjects in a loop over all research topics (R),
// multiple times per topic (dependencies, requirements, name check, getOneFree).
// With 100+ research topics and 50+ discovered, this is O(R*D) = O(5000+) string
// comparisons per base per geoscape tick.
//
// Additionally, getAvailableResearchProjects builds an "unlocked" vector and uses
// std::find for membership, adding O(R*U) where U = unlocked topics.
//
// Fix: Maintain parallel unordered_set<string> _discoveredNames for O(1) lookup.
// Use unordered_set for unlocked collection.
#include <vector>
#include <unordered_set>
#include <algorithm>
#include <chrono>
#include <iostream>
#include <cassert>
#include <string>
// Simulate research name strings
std::string makeResearchName(int i) {
return "STR_RESEARCH_TOPIC_" + std::to_string(i);
}
// Defective: linear scan of discovered vector
bool isResearchedDefective(const std::vector<std::string>& discovered, const std::string& name) {
for (size_t i = 0; i < discovered.size(); ++i) {
if (discovered[i] == name)
return true;
}
return false;
}
// Fixed: hash set lookup
bool isResearchedFixed(const std::unordered_set<std::string>& discovered, const std::string& name) {
return discovered.count(name) != 0;
}
int main() {
// Typical late-game: 150 research topics, 80 discovered
const int TOTAL_RESEARCH = 150;
const int DISCOVERED = 80;
const int CALLS_PER_TOPIC = 4; // deps + reqs + name + getOneFree
const int BASES = 8; // max bases
// Build discovered sets
std::vector<std::string> discoveredVec;
std::unordered_set<std::string> discoveredSet;
for (int i = 0; i < DISCOVERED; ++i) {
std::string name = makeResearchName(i);
discoveredVec.push_back(name);
discoveredSet.insert(name);
}
// Build query names (all research topics)
std::vector<std::string> allTopics;
for (int i = 0; i < TOTAL_RESEARCH; ++i) {
allTopics.push_back(makeResearchName(i));
}
// Correctness check
for (int i = 0; i < TOTAL_RESEARCH; ++i) {
bool a = isResearchedDefective(discoveredVec, allTopics[i]);
bool b = isResearchedFixed(discoveredSet, allTopics[i]);
assert(a == b);
}
std::cout << "Correctness: PASS" << std::endl;
// Benchmark: vector (defective) -- simulates getAvailableResearchProjects across all bases
auto t0 = std::chrono::high_resolution_clock::now();
int dummy1 = 0;
for (int base = 0; base < BASES; ++base) {
for (int topic = 0; topic < TOTAL_RESEARCH; ++topic) {
for (int call = 0; call < CALLS_PER_TOPIC; ++call) {
dummy1 += isResearchedDefective(discoveredVec, allTopics[topic]) ? 1 : 0;
}
}
}
auto t1 = std::chrono::high_resolution_clock::now();
// Benchmark: unordered_set (fixed)
auto t2 = std::chrono::high_resolution_clock::now();
int dummy2 = 0;
for (int base = 0; base < BASES; ++base) {
for (int topic = 0; topic < TOTAL_RESEARCH; ++topic) {
for (int call = 0; call < CALLS_PER_TOPIC; ++call) {
dummy2 += isResearchedFixed(discoveredSet, allTopics[topic]) ? 1 : 0;
}
}
}
auto t3 = std::chrono::high_resolution_clock::now();
double vecUs = std::chrono::duration_cast<std::chrono::microseconds>(t1 - t0).count();
double setUs = std::chrono::duration_cast<std::chrono::microseconds>(t3 - t2).count();
double ratio = vecUs / (setUs > 0 ? setUs : 1);
std::cout << "Vector (defective): " << vecUs << " us" << std::endl;
std::cout << "Set (fixed): " << setUs << " us" << std::endl;
std::cout << "Ratio: " << ratio << "x" << std::endl;
std::cout << "Parameters: R=" << TOTAL_RESEARCH << " topics, D=" << DISCOVERED
<< " discovered, " << CALLS_PER_TOPIC << " calls/topic, "
<< BASES << " bases" << std::endl;
// Verify significant speedup
assert(ratio > 2.0);
std::cout << "Performance: PASS (ratio > 2x)" << std::endl;
assert(dummy1 == dummy2);
std::cout << "ALL TESTS PASSED" << std::endl;
return 0;
}