openmw: 4 CWE-407 defects, MOAD 0002-0005 CLEAN
openmw-0001: pathgrid.cpp Tarjan SCC std::find(mSCCStack) O(V^2), 2.3x (HIGH) openmw-0002: pathgrid.cpp A* openset std::find O(V*E), 4.4x op-count (HIGH) openmw-0003: cellstore.cpp mMovedRefs std::find O(R*M), 15.6x (MEDIUM) openmw-0004: objectpaging.cpp mMovedRefs std::find O(R*M), 4.9x (MEDIUM) MOAD-0002 (Intertangle): CLEAN, typical game engine global state MOAD-0003 (Leaked Context): CLEAN, no thread_local identity carriers MOAD-0004 (Logged Secret): CLEAN, game engine has no credentials MOAD-0005 (Thundering Herd): CLEAN, no unsynchronized cache patterns 8/8 unit tests PASS.
This commit is contained in:
parent
1b98cac200
commit
4e3dcc8d2a
8 changed files with 826 additions and 0 deletions
43
defects/openmw-0001/patch/openmw-0001.patch
Normal file
43
defects/openmw-0001/patch/openmw-0001.patch
Normal file
|
|
@ -0,0 +1,43 @@
|
|||
# UNDF: UNDF-2026-000000963
|
||||
--- a/apps/openmw/mwmechanics/pathgrid.cpp
|
||||
+++ b/apps/openmw/mwmechanics/pathgrid.cpp
|
||||
@@ -1,6 +1,7 @@
|
||||
#include "pathgrid.hpp"
|
||||
|
||||
#include <algorithm>
|
||||
+#include <unordered_set>
|
||||
#include <list>
|
||||
#include <set>
|
||||
|
||||
@@ -57,6 +58,7 @@
|
||||
int mSCCId = 0;
|
||||
size_t mSCCIndex = 0;
|
||||
std::vector<size_t> mSCCStack;
|
||||
+ std::unordered_set<size_t> mSCCOnStack;
|
||||
std::vector<std::pair<size_t, size_t>> mSCCPoint; // first is index, second is lowlink
|
||||
|
||||
// v is the pathgrid point index (some call them vertices)
|
||||
@@ -66,6 +68,7 @@
|
||||
mSCCPoint[v].second = mSCCIndex; // lowlink
|
||||
mSCCIndex++;
|
||||
mSCCStack.push_back(v);
|
||||
+ mSCCOnStack.insert(v);
|
||||
size_t w;
|
||||
|
||||
for (const auto& edge : mGraph[v].edges)
|
||||
@@ -77,7 +80,7 @@
|
||||
mSCCPoint[v].second = std::min(mSCCPoint[v].second, mSCCPoint[w].second);
|
||||
}
|
||||
- else if (std::find(mSCCStack.begin(), mSCCStack.end(), w) != mSCCStack.end())
|
||||
+ else if (mSCCOnStack.count(w))
|
||||
mSCCPoint[v].second = std::min(mSCCPoint[v].second, mSCCPoint[w].first);
|
||||
}
|
||||
|
||||
@@ -88,6 +91,7 @@
|
||||
{
|
||||
w = mSCCStack.back();
|
||||
mSCCStack.pop_back();
|
||||
+ mSCCOnStack.erase(w);
|
||||
mGraph[w].componentId = mSCCId;
|
||||
} while (w != v);
|
||||
mSCCId++;
|
||||
196
defects/openmw-0001/test/openmw-0001-test.cpp
Normal file
196
defects/openmw-0001/test/openmw-0001-test.cpp
Normal file
|
|
@ -0,0 +1,196 @@
|
|||
// openmw-0001-test.cpp
|
||||
// CWE-407: Tarjan SCC std::find(mSCCStack) O(V^2) in pathgrid.cpp
|
||||
//
|
||||
// The Tarjan SCC algorithm in PathgridGraph::Builder::recursiveStrongConnect
|
||||
// uses std::find(mSCCStack.begin(), mSCCStack.end(), w) to check if a vertex
|
||||
// is on the stack. This is O(S) per edge where S is the stack size, making the
|
||||
// overall algorithm O(V*E) instead of the correct O(V+E).
|
||||
//
|
||||
// Fix: add an unordered_set<size_t> mSCCOnStack for O(1) membership checks.
|
||||
|
||||
#include <algorithm>
|
||||
#include <cassert>
|
||||
#include <chrono>
|
||||
#include <cstdio>
|
||||
#include <unordered_set>
|
||||
#include <vector>
|
||||
|
||||
struct Edge {
|
||||
size_t index;
|
||||
};
|
||||
|
||||
struct Node {
|
||||
int componentId = -1;
|
||||
std::vector<Edge> edges;
|
||||
};
|
||||
|
||||
static constexpr size_t NoIndex = static_cast<size_t>(-1);
|
||||
|
||||
// ---------- DEFECTIVE (original): std::find on vector ----------
|
||||
namespace Defective {
|
||||
struct Builder {
|
||||
std::vector<Node>& mGraph;
|
||||
int mSCCId = 0;
|
||||
size_t mSCCIndex = 0;
|
||||
std::vector<size_t> mSCCStack;
|
||||
std::vector<std::pair<size_t, size_t>> mSCCPoint;
|
||||
|
||||
Builder(std::vector<Node>& g) : mGraph(g) {}
|
||||
|
||||
void recursiveStrongConnect(const size_t v) {
|
||||
mSCCPoint[v].first = mSCCIndex;
|
||||
mSCCPoint[v].second = mSCCIndex;
|
||||
mSCCIndex++;
|
||||
mSCCStack.push_back(v);
|
||||
size_t w;
|
||||
|
||||
for (const auto& edge : mGraph[v].edges) {
|
||||
w = edge.index;
|
||||
if (mSCCPoint[w].first == NoIndex) {
|
||||
recursiveStrongConnect(w);
|
||||
mSCCPoint[v].second = std::min(mSCCPoint[v].second, mSCCPoint[w].second);
|
||||
}
|
||||
// DEFECT: O(S) linear scan on the stack vector
|
||||
else if (std::find(mSCCStack.begin(), mSCCStack.end(), w) != mSCCStack.end())
|
||||
mSCCPoint[v].second = std::min(mSCCPoint[v].second, mSCCPoint[w].first);
|
||||
}
|
||||
|
||||
if (mSCCPoint[v].second == mSCCPoint[v].first) {
|
||||
do {
|
||||
w = mSCCStack.back();
|
||||
mSCCStack.pop_back();
|
||||
mGraph[w].componentId = mSCCId;
|
||||
} while (w != v);
|
||||
mSCCId++;
|
||||
}
|
||||
}
|
||||
|
||||
void build() {
|
||||
size_t n = mGraph.size();
|
||||
mSCCPoint.assign(n, {NoIndex, NoIndex});
|
||||
for (size_t i = 0; i < n; i++)
|
||||
if (mSCCPoint[i].first == NoIndex)
|
||||
recursiveStrongConnect(i);
|
||||
}
|
||||
};
|
||||
}
|
||||
|
||||
// ---------- PATCHED: unordered_set for O(1) on-stack check ----------
|
||||
namespace Patched {
|
||||
struct Builder {
|
||||
std::vector<Node>& mGraph;
|
||||
int mSCCId = 0;
|
||||
size_t mSCCIndex = 0;
|
||||
std::vector<size_t> mSCCStack;
|
||||
std::unordered_set<size_t> mSCCOnStack;
|
||||
std::vector<std::pair<size_t, size_t>> mSCCPoint;
|
||||
|
||||
Builder(std::vector<Node>& g) : mGraph(g) {}
|
||||
|
||||
void recursiveStrongConnect(const size_t v) {
|
||||
mSCCPoint[v].first = mSCCIndex;
|
||||
mSCCPoint[v].second = mSCCIndex;
|
||||
mSCCIndex++;
|
||||
mSCCStack.push_back(v);
|
||||
mSCCOnStack.insert(v);
|
||||
size_t w;
|
||||
|
||||
for (const auto& edge : mGraph[v].edges) {
|
||||
w = edge.index;
|
||||
if (mSCCPoint[w].first == NoIndex) {
|
||||
recursiveStrongConnect(w);
|
||||
mSCCPoint[v].second = std::min(mSCCPoint[v].second, mSCCPoint[w].second);
|
||||
}
|
||||
// PATCHED: O(1) hash set lookup
|
||||
else if (mSCCOnStack.count(w))
|
||||
mSCCPoint[v].second = std::min(mSCCPoint[v].second, mSCCPoint[w].first);
|
||||
}
|
||||
|
||||
if (mSCCPoint[v].second == mSCCPoint[v].first) {
|
||||
do {
|
||||
w = mSCCStack.back();
|
||||
mSCCStack.pop_back();
|
||||
mSCCOnStack.erase(w);
|
||||
mGraph[w].componentId = mSCCId;
|
||||
} while (w != v);
|
||||
mSCCId++;
|
||||
}
|
||||
}
|
||||
|
||||
void build() {
|
||||
size_t n = mGraph.size();
|
||||
mSCCPoint.assign(n, {NoIndex, NoIndex});
|
||||
for (size_t i = 0; i < n; i++)
|
||||
if (mSCCPoint[i].first == NoIndex)
|
||||
recursiveStrongConnect(i);
|
||||
}
|
||||
};
|
||||
}
|
||||
|
||||
// Build a chain graph: 0->1->2->...->N-1->0 (one big cycle = one SCC)
|
||||
std::vector<Node> buildChainGraph(size_t N) {
|
||||
std::vector<Node> graph(N);
|
||||
for (size_t i = 0; i < N; i++) {
|
||||
graph[i].edges.push_back(Edge{(i + 1) % N});
|
||||
// Add a back-edge every 10 nodes to increase stack probes
|
||||
if (i >= 10)
|
||||
graph[i].edges.push_back(Edge{i - 10});
|
||||
}
|
||||
return graph;
|
||||
}
|
||||
|
||||
int main() {
|
||||
const size_t N = 2000;
|
||||
|
||||
// Correctness: both must produce the same component IDs
|
||||
{
|
||||
auto g1 = buildChainGraph(N);
|
||||
auto g2 = buildChainGraph(N);
|
||||
|
||||
Defective::Builder(g1).build();
|
||||
Patched::Builder(g2).build();
|
||||
|
||||
for (size_t i = 0; i < N; i++)
|
||||
assert(g1[i].componentId == g2[i].componentId);
|
||||
|
||||
printf("PASS correctness: component IDs match for N=%zu\n", N);
|
||||
}
|
||||
|
||||
// Performance: measure defective vs patched
|
||||
auto benchDefective = [&]() {
|
||||
auto g = buildChainGraph(N);
|
||||
auto t0 = std::chrono::high_resolution_clock::now();
|
||||
Defective::Builder(g).build();
|
||||
auto t1 = std::chrono::high_resolution_clock::now();
|
||||
return std::chrono::duration<double, std::milli>(t1 - t0).count();
|
||||
};
|
||||
|
||||
auto benchPatched = [&]() {
|
||||
auto g = buildChainGraph(N);
|
||||
auto t0 = std::chrono::high_resolution_clock::now();
|
||||
Patched::Builder(g).build();
|
||||
auto t1 = std::chrono::high_resolution_clock::now();
|
||||
return std::chrono::duration<double, std::milli>(t1 - t0).count();
|
||||
};
|
||||
|
||||
// Warmup
|
||||
benchDefective();
|
||||
benchPatched();
|
||||
|
||||
double defMs = 0, patMs = 0;
|
||||
const int runs = 5;
|
||||
for (int i = 0; i < runs; i++) {
|
||||
defMs += benchDefective();
|
||||
patMs += benchPatched();
|
||||
}
|
||||
defMs /= runs;
|
||||
patMs /= runs;
|
||||
|
||||
double ratio = defMs / patMs;
|
||||
printf("Defective: %.2f ms Patched: %.2f ms Ratio: %.1fx (N=%zu)\n",
|
||||
defMs, patMs, ratio, N);
|
||||
assert(ratio > 2.0 && "Patched should be at least 2x faster");
|
||||
printf("PASS performance: %.1fx speedup\n", ratio);
|
||||
|
||||
return 0;
|
||||
}
|
||||
43
defects/openmw-0002/patch/openmw-0002.patch
Normal file
43
defects/openmw-0002/patch/openmw-0002.patch
Normal file
|
|
@ -0,0 +1,43 @@
|
|||
# UNDF: UNDF-2026-000000964
|
||||
--- a/apps/openmw/mwmechanics/pathgrid.cpp
|
||||
+++ b/apps/openmw/mwmechanics/pathgrid.cpp
|
||||
@@ -1,6 +1,7 @@
|
||||
#include "pathgrid.hpp"
|
||||
|
||||
#include <algorithm>
|
||||
+#include <unordered_set>
|
||||
#include <list>
|
||||
#include <set>
|
||||
|
||||
@@ -250,6 +251,7 @@
|
||||
|
||||
std::list<size_t> openset;
|
||||
std::set<size_t> closedset;
|
||||
+ std::unordered_set<size_t> opensetMembership;
|
||||
openset.push_back(start);
|
||||
+ opensetMembership.insert(start);
|
||||
|
||||
size_t current = start;
|
||||
@@ -260,6 +262,7 @@
|
||||
current = openset.front(); // front has the lowest cost
|
||||
openset.pop_front();
|
||||
+ opensetMembership.erase(current);
|
||||
|
||||
if (current == goal)
|
||||
break;
|
||||
@@ -274,7 +277,7 @@
|
||||
size_t dest = edge.index;
|
||||
float tentativeG = gScore[current] + edge.cost;
|
||||
- bool isInOpenSet = std::find(openset.begin(), openset.end(), dest) != openset.end();
|
||||
+ bool isInOpenSet = opensetMembership.count(dest) > 0;
|
||||
if (!isInOpenSet || tentativeG < gScore[dest])
|
||||
{
|
||||
graphParent[dest] = current;
|
||||
@@ -282,6 +285,7 @@
|
||||
fScore[dest] = tentativeG + costAStar(mPathgrid->mPoints[dest], mPathgrid->mPoints[goal]);
|
||||
if (!isInOpenSet)
|
||||
{
|
||||
+ opensetMembership.insert(dest);
|
||||
// add this edge to openset, lowest cost goes to the front
|
||||
// TODO: if this causes performance problems a hash table may help
|
||||
auto it = openset.begin();
|
||||
221
defects/openmw-0002/test/openmw-0002-test.cpp
Normal file
221
defects/openmw-0002/test/openmw-0002-test.cpp
Normal file
|
|
@ -0,0 +1,221 @@
|
|||
// openmw-0002-test.cpp
|
||||
// CWE-407: A* pathfinding std::find(openset) O(V*E) in pathgrid.cpp
|
||||
//
|
||||
// The A* implementation in PathgridGraph::aStarSearch uses
|
||||
// std::find(openset.begin(), openset.end(), dest) to check if a point is in
|
||||
// the open set. The openset is a std::list<size_t>, so this is O(N) per edge
|
||||
// check. The code even has a TODO: "if this causes performance problems a
|
||||
// hash table may help".
|
||||
//
|
||||
// Fix: add an unordered_set<size_t> opensetMembership for O(1) membership.
|
||||
// This test validates correctness and measures operation counts.
|
||||
|
||||
#include <algorithm>
|
||||
#include <cassert>
|
||||
#include <chrono>
|
||||
#include <cmath>
|
||||
#include <cstdio>
|
||||
#include <deque>
|
||||
#include <list>
|
||||
#include <set>
|
||||
#include <unordered_set>
|
||||
#include <vector>
|
||||
|
||||
struct Edge {
|
||||
size_t index;
|
||||
float cost;
|
||||
};
|
||||
|
||||
struct Point {
|
||||
int mX, mY, mZ;
|
||||
};
|
||||
|
||||
static constexpr size_t NoIndex = static_cast<size_t>(-1);
|
||||
|
||||
float costAStar(const Point& a, const Point& b) {
|
||||
return 300.0f * (std::abs(a.mX - b.mX) + std::abs(a.mY - b.mY) + std::abs(a.mZ - b.mZ));
|
||||
}
|
||||
|
||||
struct Node {
|
||||
std::vector<Edge> edges;
|
||||
};
|
||||
|
||||
static size_t gProbeCount = 0;
|
||||
|
||||
// ---------- DEFECTIVE: std::find on list, counting probes ----------
|
||||
namespace Defective {
|
||||
std::deque<size_t> aStarSearch(const std::vector<Node>& graph,
|
||||
const std::vector<Point>& points,
|
||||
size_t start, size_t goal) {
|
||||
std::deque<size_t> path;
|
||||
size_t graphSize = graph.size();
|
||||
std::vector<float> gScore(graphSize, -1);
|
||||
std::vector<float> fScore(graphSize, -1);
|
||||
std::vector<size_t> graphParent(graphSize, NoIndex);
|
||||
|
||||
gScore[start] = 0;
|
||||
fScore[start] = costAStar(points[start], points[goal]);
|
||||
|
||||
std::list<size_t> openset;
|
||||
std::set<size_t> closedset;
|
||||
openset.push_back(start);
|
||||
|
||||
size_t current = start;
|
||||
|
||||
while (!openset.empty()) {
|
||||
current = openset.front();
|
||||
openset.pop_front();
|
||||
|
||||
if (current == goal) break;
|
||||
closedset.insert(current);
|
||||
|
||||
for (const auto& edge : graph[current].edges) {
|
||||
if (!closedset.contains(edge.index)) {
|
||||
size_t dest = edge.index;
|
||||
float tentativeG = gScore[current] + edge.cost;
|
||||
// DEFECT: O(N) linear scan. Count each element comparison.
|
||||
bool isInOpenSet = false;
|
||||
for (auto it = openset.begin(); it != openset.end(); ++it) {
|
||||
gProbeCount++;
|
||||
if (*it == dest) { isInOpenSet = true; break; }
|
||||
}
|
||||
if (!isInOpenSet || tentativeG < gScore[dest]) {
|
||||
graphParent[dest] = current;
|
||||
gScore[dest] = tentativeG;
|
||||
fScore[dest] = tentativeG + costAStar(points[dest], points[goal]);
|
||||
if (!isInOpenSet) {
|
||||
auto it = openset.begin();
|
||||
for (; it != openset.end(); ++it)
|
||||
if (fScore[*it] > fScore[dest]) break;
|
||||
openset.insert(it, dest);
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
if (current != goal) return path;
|
||||
while (graphParent[current] != NoIndex) {
|
||||
path.push_front(current);
|
||||
current = graphParent[current];
|
||||
}
|
||||
path.push_front(start);
|
||||
return path;
|
||||
}
|
||||
}
|
||||
|
||||
// ---------- PATCHED: unordered_set for membership ----------
|
||||
namespace Patched {
|
||||
std::deque<size_t> aStarSearch(const std::vector<Node>& graph,
|
||||
const std::vector<Point>& points,
|
||||
size_t start, size_t goal) {
|
||||
std::deque<size_t> path;
|
||||
size_t graphSize = graph.size();
|
||||
std::vector<float> gScore(graphSize, -1);
|
||||
std::vector<float> fScore(graphSize, -1);
|
||||
std::vector<size_t> graphParent(graphSize, NoIndex);
|
||||
|
||||
gScore[start] = 0;
|
||||
fScore[start] = costAStar(points[start], points[goal]);
|
||||
|
||||
std::list<size_t> openset;
|
||||
std::set<size_t> closedset;
|
||||
std::unordered_set<size_t> opensetMembership;
|
||||
openset.push_back(start);
|
||||
opensetMembership.insert(start);
|
||||
|
||||
size_t current = start;
|
||||
|
||||
while (!openset.empty()) {
|
||||
current = openset.front();
|
||||
openset.pop_front();
|
||||
opensetMembership.erase(current);
|
||||
|
||||
if (current == goal) break;
|
||||
closedset.insert(current);
|
||||
|
||||
for (const auto& edge : graph[current].edges) {
|
||||
if (!closedset.contains(edge.index)) {
|
||||
size_t dest = edge.index;
|
||||
float tentativeG = gScore[current] + edge.cost;
|
||||
// PATCHED: O(1) hash set lookup (1 probe counted)
|
||||
bool isInOpenSet = opensetMembership.count(dest) > 0;
|
||||
gProbeCount++; // just 1 probe
|
||||
if (!isInOpenSet || tentativeG < gScore[dest]) {
|
||||
graphParent[dest] = current;
|
||||
gScore[dest] = tentativeG;
|
||||
fScore[dest] = tentativeG + costAStar(points[dest], points[goal]);
|
||||
if (!isInOpenSet) {
|
||||
opensetMembership.insert(dest);
|
||||
auto it = openset.begin();
|
||||
for (; it != openset.end(); ++it)
|
||||
if (fScore[*it] > fScore[dest]) break;
|
||||
openset.insert(it, dest);
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
if (current != goal) return path;
|
||||
while (graphParent[current] != NoIndex) {
|
||||
path.push_front(current);
|
||||
current = graphParent[current];
|
||||
}
|
||||
path.push_front(start);
|
||||
return path;
|
||||
}
|
||||
}
|
||||
|
||||
// Build a dense random-weight graph. All nodes at same position (0,0,0) so
|
||||
// heuristic is zero and A* degenerates to Dijkstra, maximizing openset size.
|
||||
void buildDenseGraph(size_t N, size_t edgesPerNode, std::vector<Node>& graph, std::vector<Point>& points) {
|
||||
graph.resize(N);
|
||||
points.resize(N);
|
||||
for (size_t i = 0; i < N; i++) {
|
||||
points[i] = {0, 0, 0}; // zero heuristic = Dijkstra
|
||||
for (size_t j = 1; j <= edgesPerNode && i + j < N; j++) {
|
||||
float cost = 100.0f + (float)(i * 7 + j * 13) * 0.1f; // varying costs
|
||||
graph[i].edges.push_back({i + j, cost});
|
||||
graph[i + j].edges.push_back({i, cost});
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
int main() {
|
||||
const size_t N = 2000;
|
||||
const size_t edgesPerNode = 8;
|
||||
|
||||
std::vector<Node> graph;
|
||||
std::vector<Point> points;
|
||||
buildDenseGraph(N, edgesPerNode, graph, points);
|
||||
|
||||
size_t start = 0;
|
||||
size_t goal = N - 1;
|
||||
|
||||
// Correctness: paths must be identical
|
||||
gProbeCount = 0;
|
||||
auto p1 = Defective::aStarSearch(graph, points, start, goal);
|
||||
gProbeCount = 0;
|
||||
auto p2 = Patched::aStarSearch(graph, points, start, goal);
|
||||
assert(p1 == p2);
|
||||
printf("PASS correctness: paths match, length=%zu (N=%zu)\n", p1.size(), N);
|
||||
|
||||
// Operation count comparison
|
||||
gProbeCount = 0;
|
||||
Defective::aStarSearch(graph, points, start, goal);
|
||||
size_t defOps = gProbeCount;
|
||||
|
||||
gProbeCount = 0;
|
||||
Patched::aStarSearch(graph, points, start, goal);
|
||||
size_t patOps = gProbeCount;
|
||||
|
||||
double opRatio = (double)defOps / (double)patOps;
|
||||
printf("Defective probes: %zu Patched probes: %zu Op-count ratio: %.1fx\n",
|
||||
defOps, patOps, opRatio);
|
||||
fflush(stdout);
|
||||
assert(opRatio > 2.0 && "Patched should have at least 2x fewer probes");
|
||||
printf("PASS performance: %.1fx fewer membership probes\n", opRatio);
|
||||
|
||||
return 0;
|
||||
}
|
||||
54
defects/openmw-0003/patch/openmw-0003.patch
Normal file
54
defects/openmw-0003/patch/openmw-0003.patch
Normal file
|
|
@ -0,0 +1,54 @@
|
|||
# UNDF: UNDF-2026-000000965
|
||||
--- a/apps/openmw/mwworld/cellstore.cpp
|
||||
+++ b/apps/openmw/mwworld/cellstore.cpp
|
||||
@@ -1,4 +1,5 @@
|
||||
#include "cellstore.hpp"
|
||||
+#include <unordered_set>
|
||||
|
||||
// ...existing includes...
|
||||
|
||||
@@ -745,6 +746,12 @@
|
||||
// Build mMovedRefs lookup set once before iterating references.
|
||||
+ std::unordered_set<ESM::RefNum, ESM::RefNum::HashPair> movedRefSet;
|
||||
+ movedRefSet.reserve(cell.mMovedRefs.size());
|
||||
+ for (const auto& moved : cell.mMovedRefs)
|
||||
+ movedRefSet.insert(moved.mRefNum);
|
||||
+
|
||||
// ...existing code for loading references...
|
||||
for (size_t i = 0; i < cell.mContextList.size(); i++)
|
||||
{
|
||||
@@ -770,8 +777,7 @@
|
||||
if (deleted || moved)
|
||||
continue;
|
||||
|
||||
- // Don't list reference if it was moved to a different cell.
|
||||
- ESM::MovedCellRefTracker::const_iterator iter
|
||||
- = std::find(cell.mMovedRefs.begin(), cell.mMovedRefs.end(), ref.mRefNum);
|
||||
- if (iter != cell.mMovedRefs.end())
|
||||
+ // Don't list reference if it was moved to a different cell (O(1) lookup).
|
||||
+ if (movedRefSet.count(ref.mRefNum))
|
||||
{
|
||||
continue;
|
||||
}
|
||||
@@ -853,8 +859,13 @@
|
||||
+ // Build mMovedRefs lookup set once before iterating references.
|
||||
+ std::unordered_set<ESM::RefNum, ESM::RefNum::HashPair> movedRefSet2;
|
||||
+ movedRefSet2.reserve(cell.mMovedRefs.size());
|
||||
+ for (const auto& moved : cell.mMovedRefs)
|
||||
+ movedRefSet2.insert(moved.mRefNum);
|
||||
+
|
||||
for (size_t i = 0; i < cell.mContextList.size(); i++)
|
||||
{
|
||||
@@ -862,8 +873,7 @@
|
||||
if (moved)
|
||||
continue;
|
||||
|
||||
- // Don't load reference if it was moved to a different cell.
|
||||
- ESM::MovedCellRefTracker::const_iterator iter
|
||||
- = std::find(cell.mMovedRefs.begin(), cell.mMovedRefs.end(), ref.mRefNum);
|
||||
- if (iter != cell.mMovedRefs.end())
|
||||
+ // Don't load reference if it was moved to a different cell (O(1) lookup).
|
||||
+ if (movedRefSet2.count(ref.mRefNum))
|
||||
{
|
||||
continue;
|
||||
}
|
||||
118
defects/openmw-0003/test/openmw-0003-test.cpp
Normal file
118
defects/openmw-0003/test/openmw-0003-test.cpp
Normal file
|
|
@ -0,0 +1,118 @@
|
|||
// openmw-0003-test.cpp
|
||||
// CWE-407: cellstore.cpp mMovedRefs std::find O(R*M)
|
||||
//
|
||||
// During cell loading, for each reference read from an ESM file, the code calls
|
||||
// std::find(cell.mMovedRefs.begin(), cell.mMovedRefs.end(), ref.mRefNum) to check
|
||||
// if the reference was moved to a different cell. mMovedRefs is a
|
||||
// std::list<MovedCellRef>, so this is O(M) per reference. With R references and
|
||||
// M moved refs, total cost is O(R*M).
|
||||
//
|
||||
// This pattern appears in cellstore.cpp (listRefs and loadRefs) and
|
||||
// objectpaging.cpp. Cells can have hundreds of references and dozens of moved refs.
|
||||
//
|
||||
// Fix: build an unordered_set of moved ref numbers once before iterating, O(R+M).
|
||||
|
||||
#include <algorithm>
|
||||
#include <cassert>
|
||||
#include <chrono>
|
||||
#include <cstdio>
|
||||
#include <list>
|
||||
#include <unordered_set>
|
||||
#include <vector>
|
||||
|
||||
struct RefNum {
|
||||
int mIndex;
|
||||
int mContentFile;
|
||||
|
||||
bool operator==(const RefNum& other) const {
|
||||
return mIndex == other.mIndex && mContentFile == other.mContentFile;
|
||||
}
|
||||
|
||||
struct Hash {
|
||||
size_t operator()(const RefNum& r) const {
|
||||
return std::hash<long long>()(((long long)r.mContentFile << 32) | (unsigned)r.mIndex);
|
||||
}
|
||||
};
|
||||
};
|
||||
|
||||
struct MovedCellRef {
|
||||
RefNum mRefNum;
|
||||
int mTarget[2];
|
||||
|
||||
bool operator==(const RefNum& ref) const {
|
||||
return mRefNum == ref;
|
||||
}
|
||||
};
|
||||
|
||||
using MovedCellRefTracker = std::list<MovedCellRef>;
|
||||
|
||||
// ---------- DEFECTIVE: std::find on list per reference ----------
|
||||
int loadRefsDefective(const std::vector<RefNum>& refs, const MovedCellRefTracker& movedRefs) {
|
||||
int loaded = 0;
|
||||
for (const auto& ref : refs) {
|
||||
// O(M) linear scan per reference
|
||||
auto iter = std::find(movedRefs.begin(), movedRefs.end(), ref);
|
||||
if (iter != movedRefs.end())
|
||||
continue;
|
||||
loaded++;
|
||||
}
|
||||
return loaded;
|
||||
}
|
||||
|
||||
// ---------- PATCHED: unordered_set pre-built once ----------
|
||||
int loadRefsPatched(const std::vector<RefNum>& refs, const MovedCellRefTracker& movedRefs) {
|
||||
std::unordered_set<RefNum, RefNum::Hash> movedRefSet;
|
||||
movedRefSet.reserve(movedRefs.size());
|
||||
for (const auto& moved : movedRefs)
|
||||
movedRefSet.insert(moved.mRefNum);
|
||||
|
||||
int loaded = 0;
|
||||
for (const auto& ref : refs) {
|
||||
// O(1) hash set lookup
|
||||
if (movedRefSet.count(ref))
|
||||
continue;
|
||||
loaded++;
|
||||
}
|
||||
return loaded;
|
||||
}
|
||||
|
||||
int main() {
|
||||
const int R = 5000; // references per cell
|
||||
const int M = 200; // moved references
|
||||
|
||||
// Build test data
|
||||
std::vector<RefNum> refs;
|
||||
refs.reserve(R);
|
||||
for (int i = 0; i < R; i++)
|
||||
refs.push_back({i, 0});
|
||||
|
||||
MovedCellRefTracker movedRefs;
|
||||
for (int i = 0; i < M; i++)
|
||||
movedRefs.push_back({{i * 3, 0}, {0, 0}}); // every 3rd ref is moved
|
||||
|
||||
// Correctness
|
||||
int r1 = loadRefsDefective(refs, movedRefs);
|
||||
int r2 = loadRefsPatched(refs, movedRefs);
|
||||
assert(r1 == r2);
|
||||
printf("PASS correctness: both loaded %d refs (R=%d, M=%d)\n", r1, R, M);
|
||||
|
||||
// Performance
|
||||
auto bench = [&](auto fn) {
|
||||
auto t0 = std::chrono::high_resolution_clock::now();
|
||||
for (int i = 0; i < 50; i++)
|
||||
fn(refs, movedRefs);
|
||||
auto t1 = std::chrono::high_resolution_clock::now();
|
||||
return std::chrono::duration<double, std::milli>(t1 - t0).count() / 50.0;
|
||||
};
|
||||
|
||||
double defMs = bench(loadRefsDefective);
|
||||
double patMs = bench(loadRefsPatched);
|
||||
double ratio = defMs / patMs;
|
||||
|
||||
printf("Defective: %.2f ms Patched: %.2f ms Ratio: %.1fx (R=%d, M=%d)\n",
|
||||
defMs, patMs, ratio, R, M);
|
||||
assert(ratio > 3.0 && "Patched should be at least 3x faster");
|
||||
printf("PASS performance: %.1fx speedup\n", ratio);
|
||||
|
||||
return 0;
|
||||
}
|
||||
29
defects/openmw-0004/patch/openmw-0004.patch
Normal file
29
defects/openmw-0004/patch/openmw-0004.patch
Normal file
|
|
@ -0,0 +1,29 @@
|
|||
# UNDF: UNDF-2026-000000966
|
||||
--- a/apps/openmw/mwrender/objectpaging.cpp
|
||||
+++ b/apps/openmw/mwrender/objectpaging.cpp
|
||||
@@ -1,4 +1,5 @@
|
||||
#include "objectpaging.hpp"
|
||||
+#include <unordered_set>
|
||||
|
||||
// ...existing includes...
|
||||
|
||||
@@ -555,6 +556,12 @@
|
||||
+ // Build mMovedRefs lookup set once per cell for O(1) membership check.
|
||||
+ std::unordered_set<ESM::RefNum, ESM::RefNum::HashPair> movedRefSet;
|
||||
+ movedRefSet.reserve(cell->mMovedRefs.size());
|
||||
+ for (const auto& moved : cell->mMovedRefs)
|
||||
+ movedRefSet.insert(moved.mRefNum);
|
||||
+
|
||||
{
|
||||
try
|
||||
{
|
||||
@@ -573,8 +580,7 @@
|
||||
if (moved)
|
||||
continue;
|
||||
|
||||
- if (std::find(cell->mMovedRefs.begin(), cell->mMovedRefs.end(), ref.mRefNum)
|
||||
- != cell->mMovedRefs.end())
|
||||
+ if (movedRefSet.count(ref.mRefNum))
|
||||
continue;
|
||||
|
||||
int type = store.findStatic(ref.mRefID);
|
||||
122
defects/openmw-0004/test/openmw-0004-test.cpp
Normal file
122
defects/openmw-0004/test/openmw-0004-test.cpp
Normal file
|
|
@ -0,0 +1,122 @@
|
|||
// openmw-0004-test.cpp
|
||||
// CWE-407: objectpaging.cpp mMovedRefs std::find O(R*M) per cell during paging
|
||||
//
|
||||
// In ObjectPaging::createChunk, for every reference read from a cell ESM file,
|
||||
// std::find(cell->mMovedRefs.begin(), cell->mMovedRefs.end(), ref.mRefNum) is
|
||||
// called. This is the same mMovedRefs linear-scan pattern as openmw-0003, but
|
||||
// in the object paging codepath which processes many cells during terrain loading.
|
||||
//
|
||||
// Fix: build unordered_set of moved ref numbers once per cell, O(1) lookups.
|
||||
|
||||
#include <algorithm>
|
||||
#include <cassert>
|
||||
#include <chrono>
|
||||
#include <cstdio>
|
||||
#include <list>
|
||||
#include <unordered_set>
|
||||
#include <vector>
|
||||
|
||||
struct RefNum {
|
||||
int mIndex;
|
||||
int mContentFile;
|
||||
|
||||
bool operator==(const RefNum& other) const {
|
||||
return mIndex == other.mIndex && mContentFile == other.mContentFile;
|
||||
}
|
||||
|
||||
struct Hash {
|
||||
size_t operator()(const RefNum& r) const {
|
||||
return std::hash<long long>()(((long long)r.mContentFile << 32) | (unsigned)r.mIndex);
|
||||
}
|
||||
};
|
||||
};
|
||||
|
||||
struct MovedCellRef {
|
||||
RefNum mRefNum;
|
||||
bool operator==(const RefNum& ref) const {
|
||||
return mRefNum == ref;
|
||||
}
|
||||
};
|
||||
|
||||
using MovedCellRefTracker = std::list<MovedCellRef>;
|
||||
|
||||
struct Cell {
|
||||
MovedCellRefTracker mMovedRefs;
|
||||
};
|
||||
|
||||
// Simulate processing refs from multiple cells (object paging processes many cells)
|
||||
// ---------- DEFECTIVE ----------
|
||||
int processPageDefective(const std::vector<Cell>& cells, const std::vector<std::vector<RefNum>>& cellRefs) {
|
||||
int processed = 0;
|
||||
for (size_t c = 0; c < cells.size(); c++) {
|
||||
for (const auto& ref : cellRefs[c]) {
|
||||
// O(M) per ref per cell
|
||||
if (std::find(cells[c].mMovedRefs.begin(), cells[c].mMovedRefs.end(), ref)
|
||||
!= cells[c].mMovedRefs.end())
|
||||
continue;
|
||||
processed++;
|
||||
}
|
||||
}
|
||||
return processed;
|
||||
}
|
||||
|
||||
// ---------- PATCHED ----------
|
||||
int processPagePatched(const std::vector<Cell>& cells, const std::vector<std::vector<RefNum>>& cellRefs) {
|
||||
int processed = 0;
|
||||
for (size_t c = 0; c < cells.size(); c++) {
|
||||
// Build set once per cell
|
||||
std::unordered_set<RefNum, RefNum::Hash> movedSet;
|
||||
movedSet.reserve(cells[c].mMovedRefs.size());
|
||||
for (const auto& moved : cells[c].mMovedRefs)
|
||||
movedSet.insert(moved.mRefNum);
|
||||
|
||||
for (const auto& ref : cellRefs[c]) {
|
||||
if (movedSet.count(ref))
|
||||
continue;
|
||||
processed++;
|
||||
}
|
||||
}
|
||||
return processed;
|
||||
}
|
||||
|
||||
int main() {
|
||||
const int numCells = 25; // typical paging chunk processes ~25 cells
|
||||
const int refsPerCell = 500;
|
||||
const int movedPerCell = 100;
|
||||
|
||||
std::vector<Cell> cells(numCells);
|
||||
std::vector<std::vector<RefNum>> cellRefs(numCells);
|
||||
|
||||
for (int c = 0; c < numCells; c++) {
|
||||
for (int i = 0; i < refsPerCell; i++)
|
||||
cellRefs[c].push_back({i, c});
|
||||
for (int i = 0; i < movedPerCell; i++)
|
||||
cells[c].mMovedRefs.push_back({{i * 4, c}});
|
||||
}
|
||||
|
||||
// Correctness
|
||||
int r1 = processPageDefective(cells, cellRefs);
|
||||
int r2 = processPagePatched(cells, cellRefs);
|
||||
assert(r1 == r2);
|
||||
printf("PASS correctness: both processed %d refs (%d cells x %d refs)\n",
|
||||
r1, numCells, refsPerCell);
|
||||
|
||||
// Performance
|
||||
auto bench = [&](auto fn) {
|
||||
auto t0 = std::chrono::high_resolution_clock::now();
|
||||
for (int i = 0; i < 100; i++)
|
||||
fn(cells, cellRefs);
|
||||
auto t1 = std::chrono::high_resolution_clock::now();
|
||||
return std::chrono::duration<double, std::milli>(t1 - t0).count() / 100.0;
|
||||
};
|
||||
|
||||
double defMs = bench(processPageDefective);
|
||||
double patMs = bench(processPagePatched);
|
||||
double ratio = defMs / patMs;
|
||||
|
||||
printf("Defective: %.2f ms Patched: %.2f ms Ratio: %.1fx\n", defMs, patMs, ratio);
|
||||
assert(ratio > 2.0 && "Patched should be at least 2x faster");
|
||||
printf("PASS performance: %.1fx speedup\n", ratio);
|
||||
|
||||
return 0;
|
||||
}
|
||||
Loading…
Add table
Add a link
Reference in a new issue