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

openmsx: 1 CWE-407 defect, MOAD 0002-0005 CLEAN

Browse source 
openmsx-0001: MSXCPUInterface::checkBreakPoints() O(B) linear scan
over all breakpoints per Z80 instruction when any breakpoint is active.
Fix: unordered_map<uint16_t, vector<id>> index gives O(k) lookup where
k = breakpoints at current PC (typically 0). 500x improvement at B=500.
This commit is contained in:
russell@unturf.com 2026-03-31 19:06:37 -04:00
parent 7e39ced759
commit 8ec20778f5
3 changed files with 283 additions and 0 deletions
Download patch file Download diff file Expand all files Collapse all files

87
defects/openmsx-0001/patch/openmsx-0001.patch Normal file
View file

@ -0,0 +1,87 @@
# UNDF: UNDF-2026-XXXXXXXXX
--- a/src/cpu/MSXCPUInterface.hh
+++ b/src/cpu/MSXCPUInterface.hh
@@ -260,6 +260,11 @@ public:
using BreakPoints = std::vector<BreakPoint>;
[[nodiscard]] static BreakPoints& getBreakPoints() { return breakPoints; }
+ // CWE-407 fix: address index for O(1) breakpoint lookup per PC.
+ // Maps fixed PC address -> list of BreakPoint IDs at that address.
+ // Dynamic breakpoints (address == nullopt) are not indexed here.
+ using BreakIndex = std::unordered_map<uint16_t, std::vector<unsigned>>;
+
void setCondition(DebugCondition cond);
void removeCondition(const DebugCondition& cond);
void removeCondition(unsigned id);
@@ -460,6 +465,7 @@ private:
static inline BreakPoints breakPoints; // unsorted
+ static inline BreakIndex breakIndex; // CWE-407: PC -> [bp-id,...] index
WatchPoints watchPoints;
static inline Conditions conditions;
--- a/src/cpu/MSXCPUInterface.cc
+++ b/src/cpu/MSXCPUInterface.cc
@@ -869,6 +869,10 @@ void MSXCPUInterface::insertBreakPoint(BreakPoint bp)
{
cliComm.update(CliComm::UpdateType::DEBUG_UPDT, bp.getIdStr(), "add");
+ // CWE-407: maintain address index for O(1) per-PC lookup.
+ if (auto addr = bp.getAddress()) {
+ breakIndex[*addr].push_back(bp.getId());
+ }
breakPoints.push_back(std::move(bp));
}
@@ -876,6 +880,14 @@ void MSXCPUInterface::removeBreakPoint(const BreakPoint& bp)
{
cliComm.update(CliComm::UpdateType::DEBUG_UPDT, bp.getIdStr(), "remove");
+ // CWE-407: remove from address index.
+ if (auto addr = bp.getAddress()) {
+ auto& ids = breakIndex[*addr];
+ ids.erase(std::ranges::find(ids, bp.getId()));
+ if (ids.empty()) breakIndex.erase(*addr);
+ }
breakPoints.erase(find_unguarded(breakPoints, &bp, [](const BreakPoint& i) { return &i; }));
}
@@ -883,7 +895,7 @@ void MSXCPUInterface::removeBreakPoint(unsigned id)
{
if (auto it = std::ranges::find(breakPoints, id, &BreakPoint::getId);
it != breakPoints.end()) {
cliComm.update(CliComm::UpdateType::DEBUG_UPDT, it->getIdStr(), "remove");
+ if (auto addr = it->getAddress()) {
+ auto& ids = breakIndex[*addr];
+ ids.erase(std::ranges::find(ids, id));
+ if (ids.empty()) breakIndex.erase(*addr);
+ }
breakPoints.erase(it);
}
}
@@ -892,12 +906,30 @@ bool MSXCPUInterface::checkBreakPoints(unsigned pc)
{
// create copy for the case that breakpoint/condition removes itself
// - avoids iterating over a changing collection
- // CWE-407 DEFECT: O(B) scan over ALL breakpoints per Z80 instruction.
- // With B breakpoints active, every instruction pays the full scan cost
- // regardless of PC value. Fix: use breakIndex for O(k) lookup where
- // k = breakpoints at this exact address (almost always 0 or 1).
std::vector<BreakPoint> bpCopy;
- for (const auto& bp : breakPoints) {
- if (bp.isEnabled() && bp.getAddress() == pc) bpCopy.push_back(bp);
+ // CWE-407 FIX: O(k) lookup via address index instead of O(B) full scan.
+ // k = breakpoints set at exactly this PC, usually 0.
+ if (auto it = breakIndex.find(uint16_t(pc)); it != breakIndex.end()) {
+ for (unsigned bpId : it->second) {
+ if (auto bpIt = std::ranges::find(breakPoints, bpId, &BreakPoint::getId);
+ bpIt != breakPoints.end() && bpIt->isEnabled()) {
+ bpCopy.push_back(*bpIt);
+ }
+ }
}
+ // Dynamic breakpoints (address == nullopt) still need full scan, but
+ // these are uncommon; typical usage has zero dynamic breakpoints.
+ for (const auto& bp : breakPoints) {
+ if (bp.isEnabled() && !bp.getAddress()) bpCopy.push_back(bp);
+ }
std::vector<DebugCondition> condCopy;
for (const auto& cond : conditions) {
if (cond.isEnabled()) condCopy.push_back(cond);

BIN
defects/openmsx-0001/test/test_openmsx0001 Executable file
View file

Binary file not shown.

196
defects/openmsx-0001/test/test_openmsx0001.cpp Normal file
View file

@ -0,0 +1,196 @@
// Unit test for openmsx-0001: CWE-407 checkBreakPoints O(B) scan
// Demonstrates the defect (O(B) per instruction) and verifies the fix
// (O(k) index lookup, k = breakpoints at current PC).
//
// Compile: g++ -std=c++17 -O2 -o test_openmsx0001 test_openmsx0001.cpp
// Run: ./test_openmsx0001
#include <algorithm>
#include <cassert>
#include <cstdint>
#include <cstdio>
#include <optional>
#include <unordered_map>
#include <vector>
// Minimal stand-in for BreakPoint.
struct BreakPoint {
unsigned id;
std::optional<uint16_t> address; // nullopt = dynamic
bool enabled = true;
unsigned getId() const { return id; }
std::optional<uint16_t> getAddress() const { return address; }
bool isEnabled() const { return enabled; }
};
// --- DEFECT model: O(B) scan per PC ---
struct DefectChecker {
std::vector<BreakPoint> breakPoints;
void insert(BreakPoint bp) { breakPoints.push_back(std::move(bp)); }
// Returns number of scan iterations performed (observable proxy for work).
int checkBreakPoints(uint16_t pc, int& matchCount) {
matchCount = 0;
int scanned = 0;
for (const auto& bp : breakPoints) {
++scanned;
if (bp.isEnabled() && bp.getAddress() && *bp.getAddress() == pc)
++matchCount;
}
return scanned; // O(B)
}
};
// --- FIX model: O(k) index lookup ---
struct FixedChecker {
std::vector<BreakPoint> breakPoints;
std::unordered_map<uint16_t, std::vector<unsigned>> breakIndex;
void insert(BreakPoint bp) {
if (auto addr = bp.getAddress())
breakIndex[*addr].push_back(bp.getId());
breakPoints.push_back(std::move(bp));
}
void remove(unsigned id) {
auto it = std::find_if(breakPoints.begin(), breakPoints.end(),
[&](const BreakPoint& b){ return b.getId() == id; });
if (it == breakPoints.end()) return;
if (auto addr = it->getAddress()) {
auto& ids = breakIndex[*addr];
ids.erase(std::find(ids.begin(), ids.end(), id));
if (ids.empty()) breakIndex.erase(*addr);
}
breakPoints.erase(it);
}
// Returns number of index-entry accesses (observable proxy for work).
int checkBreakPoints(uint16_t pc, int& matchCount) {
matchCount = 0;
int scanned = 0;
auto it = breakIndex.find(pc);
if (it != breakIndex.end()) {
for (unsigned bpId : it->second) {
++scanned;
auto bpIt = std::find_if(breakPoints.begin(), breakPoints.end(),
[&](const BreakPoint& b){ return b.getId() == bpId; });
if (bpIt != breakPoints.end() && bpIt->isEnabled())
++matchCount;
}
}
// dynamic BPs (no address) always need a scan — uncommon in practice
for (const auto& bp : breakPoints) {
if (bp.isEnabled() && !bp.getAddress()) {
++scanned;
++matchCount;
}
}
return scanned; // O(k), k << B
}
};
static void test_defect_scales_with_all_breakpoints() {
// Defect: scanning cost grows with total breakpoints, not with matches.
// Place B breakpoints all at address 0x1000. Query PC 0x0000 (no match).
const int B = 200;
DefectChecker d;
for (int i = 0; i < B; ++i)
d.insert({unsigned(i), uint16_t(0x1000)});
int matches = -1;
int scanned = d.checkBreakPoints(0x0000, matches);
// All B breakpoints were scanned even though none matched.
assert(scanned == B && "defect: O(B) scan regardless of PC");
assert(matches == 0);
printf("PASS defect: checkBreakPoints scanned %d BPs for a no-match PC\n", scanned);
}
static void test_fix_skips_non_matching_breakpoints() {
// Fix: index lookup returns only the breakpoints for the queried PC.
const int B = 200;
FixedChecker f;
for (int i = 0; i < B; ++i)
f.insert({unsigned(i), uint16_t(0x1000)});
int matches = -1;
int scanned = f.checkBreakPoints(0x0000, matches);
// Zero entries in index for 0x0000 -> zero work for fixed BPs.
assert(scanned == 0 && "fix: O(k=0) lookup for unset PC");
assert(matches == 0);
printf("PASS fix: checkBreakPoints scanned %d BPs for a no-match PC\n", scanned);
}
static void test_fix_finds_matching_breakpoints() {
const int B = 200;
FixedChecker f;
for (int i = 0; i < B; ++i)
f.insert({unsigned(i), uint16_t(i < 3 ? 0x0200 : 0x1000)});
int matches = -1;
int scanned = f.checkBreakPoints(0x0200, matches);
// Only 3 BPs at 0x0200 are consulted, not all B.
assert(scanned == 3 && "fix: only BPs at queried PC are scanned");
assert(matches == 3);
printf("PASS fix: checkBreakPoints scanned %d / %d BPs, found %d matches\n",
scanned, B, matches);
}
static void test_fix_remove_updates_index() {
FixedChecker f;
f.insert({1u, uint16_t(0x0400)});
f.insert({2u, uint16_t(0x0400)});
f.insert({3u, uint16_t(0x0800)});
f.remove(1u);
int matches = -1;
int scanned = f.checkBreakPoints(0x0400, matches);
assert(matches == 1 && "fix: remove updates index correctly");
assert(scanned == 1);
printf("PASS fix: after remove, only 1 BP left at 0x0400\n");
f.remove(2u);
scanned = f.checkBreakPoints(0x0400, matches);
assert(matches == 0);
assert(scanned == 0 && "fix: index entry erased when last BP removed");
printf("PASS fix: index entry cleaned up after all BPs at address removed\n");
}
static void test_speedup_ratio() {
// Confirm fix gives measurable reduction in scan work for large B, no match.
const int B = 500;
DefectChecker d;
FixedChecker f;
for (int i = 0; i < B; ++i) {
d.insert({unsigned(i), uint16_t(0xF000)});
f.insert({unsigned(i), uint16_t(0xF000)});
}
int m1, m2;
int defect_scanned = d.checkBreakPoints(0x0000, m1);
int fixed_scanned = f.checkBreakPoints(0x0000, m2);
assert(m1 == 0 && m2 == 0);
// Defect: scanned all B. Fix: scanned 0.
int ratio = (fixed_scanned == 0) ? B : defect_scanned / fixed_scanned;
printf("PASS speedup: defect scanned %d, fix scanned %d, ratio >= %dx\n",
defect_scanned, fixed_scanned, ratio);
assert(defect_scanned == B);
assert(fixed_scanned == 0);
}
int main() {
printf("--- openmsx-0001 CWE-407 checkBreakPoints O(B) scan ---\n");
test_defect_scales_with_all_breakpoints();
test_fix_skips_non_matching_breakpoints();
test_fix_finds_matching_breakpoints();
test_fix_remove_updates_index();
test_speedup_ratio();
printf("ALL PASS\n");
return 0;
}