From ec50768d5c1e72b238ab06a78012690f227eb609 Mon Sep 17 00:00:00 2001 From: "russell@unturf.com" Date: Tue, 31 Mar 2026 10:07:48 -0400 Subject: [PATCH] monogame-0001/0002/0003: MonoGame CWE-407 scan, 3 defects monogame-0001: IntermediateWriter.WriteSharedResources writtenSharedResources List.Contains in while loop O(R^2), fix: HashSet MEDIUM monogame-0002: IntermediateSerializer._scannedObjects List.Contains per object during scan O(N^2), fix: HashSet MEDIUM monogame-0003: OpenAssetImporter._bones List.Contains in recursive tree import O(N*B), fix: HashSet MEDIUM MOAD-0002 through 0005: CLEAN (single-threaded game framework, no secrets, no leaked context, no thundering herd) --- .../monogame-0001/patch/monogame-0001.patch | 22 ++++ .../monogame-0001/test/monogame-0001-test.cs | 81 ++++++++++++ .../monogame-0002/patch/monogame-0002.patch | 23 ++++ .../monogame-0002/test/monogame-0002-test.cs | 81 ++++++++++++ .../monogame-0003/patch/monogame-0003.patch | 24 ++++ .../monogame-0003/test/monogame-0003-test.cs | 116 ++++++++++++++++++ 6 files changed, 347 insertions(+) create mode 100644 defects/monogame-0001/patch/monogame-0001.patch create mode 100644 defects/monogame-0001/test/monogame-0001-test.cs create mode 100644 defects/monogame-0002/patch/monogame-0002.patch create mode 100644 defects/monogame-0002/test/monogame-0002-test.cs create mode 100644 defects/monogame-0003/patch/monogame-0003.patch create mode 100644 defects/monogame-0003/test/monogame-0003-test.cs diff --git a/defects/monogame-0001/patch/monogame-0001.patch b/defects/monogame-0001/patch/monogame-0001.patch new file mode 100644 index 000000000..59f5af4ad --- /dev/null +++ b/defects/monogame-0001/patch/monogame-0001.patch @@ -0,0 +1,22 @@ +--- a/MonoGame.Framework.Content.Pipeline/Serialization/Intermediate/IntermediateWriter.cs ++++ b/MonoGame.Framework.Content.Pipeline/Serialization/Intermediate/IntermediateWriter.cs +@@ -217,13 +217,13 @@ namespace Microsoft.Xna.Framework.Content.Pipeline.Serialization.Intermediate + internal void WriteSharedResources() + { +- if (!_sharedResources.Any()) ++ if (_sharedResources.Count == 0) + return; + + Xml.WriteStartElement("Resources"); + + // Loop like this because we might create more shared resources while we're serializing. +- var writtenSharedResources = new List(); +- while (_sharedResources.Any(x => !writtenSharedResources.Contains(x.Value))) ++ var writtenSharedResources = new HashSet(); ++ while (_sharedResources.Any(x => !writtenSharedResources.Contains(x.Value))) + { +- var sharedResource = _sharedResources.First(x => !writtenSharedResources.Contains(x.Value)); ++ var sharedResource = _sharedResources.First(x => !writtenSharedResources.Contains(x.Value)); + writtenSharedResources.Add(sharedResource.Value); + + WriteSharedResource(sharedResource.Value, sharedResource.Key); diff --git a/defects/monogame-0001/test/monogame-0001-test.cs b/defects/monogame-0001/test/monogame-0001-test.cs new file mode 100644 index 000000000..23f12b94e --- /dev/null +++ b/defects/monogame-0001/test/monogame-0001-test.cs @@ -0,0 +1,81 @@ +// monogame-0001-test: IntermediateWriter.WriteSharedResources List.Contains O(R^2) +// The writtenSharedResources list uses List.Contains() inside a while loop +// that iterates through all shared resources. With a HashSet, Contains is O(1). +// +// Defect: IntermediateWriter.cs line 226 +// var writtenSharedResources = new List(); +// while (_sharedResources.Any(x => !writtenSharedResources.Contains(x.Value))) +// +// Fix: HashSet for O(1) lookup instead of O(N) list scan. + +using System; +using System.Collections.Generic; +using System.Diagnostics; +using System.Linq; + +public class MonoGame0001Test +{ + // Simulate the defective pattern: List.Contains in a while loop + static long SimulateDefective(int resourceCount) + { + var sharedResources = new Dictionary(); + for (int i = 0; i < resourceCount; i++) + sharedResources[new object()] = "#Resource" + (i + 1); + + var sw = Stopwatch.StartNew(); + var writtenSharedResources = new List(); + while (sharedResources.Any(x => !writtenSharedResources.Contains(x.Value))) + { + var sharedResource = sharedResources.First(x => !writtenSharedResources.Contains(x.Value)); + writtenSharedResources.Add(sharedResource.Value); + } + sw.Stop(); + return sw.ElapsedTicks; + } + + // Simulate the fixed pattern: HashSet.Contains in a while loop + static long SimulateFixed(int resourceCount) + { + var sharedResources = new Dictionary(); + for (int i = 0; i < resourceCount; i++) + sharedResources[new object()] = "#Resource" + (i + 1); + + var sw = Stopwatch.StartNew(); + var writtenSharedResources = new HashSet(); + while (sharedResources.Any(x => !writtenSharedResources.Contains(x.Value))) + { + var sharedResource = sharedResources.First(x => !writtenSharedResources.Contains(x.Value)); + writtenSharedResources.Add(sharedResource.Value); + } + sw.Stop(); + return sw.ElapsedTicks; + } + + static void Main() + { + // Warmup + SimulateDefective(10); + SimulateFixed(10); + + int N = 2000; + int trials = 3; + + long defectiveTotal = 0; + long fixedTotal = 0; + + for (int t = 0; t < trials; t++) + { + defectiveTotal += SimulateDefective(N); + fixedTotal += SimulateFixed(N); + } + + double ratio = (double)defectiveTotal / fixedTotal; + + Console.WriteLine($"monogame-0001: IntermediateWriter writtenSharedResources"); + Console.WriteLine($" N={N} shared resources, {trials} trials"); + Console.WriteLine($" Defective (List): {defectiveTotal / trials} ticks avg"); + Console.WriteLine($" Fixed (HashSet): {fixedTotal / trials} ticks avg"); + Console.WriteLine($" Ratio: {ratio:F1}x"); + Console.WriteLine(ratio > 2.0 ? " PASS: defect confirmed" : " FAIL: ratio too low"); + } +} diff --git a/defects/monogame-0002/patch/monogame-0002.patch b/defects/monogame-0002/patch/monogame-0002.patch new file mode 100644 index 000000000..e3a2f4435 --- /dev/null +++ b/defects/monogame-0002/patch/monogame-0002.patch @@ -0,0 +1,23 @@ +--- a/MonoGame.Framework.Content.Pipeline/Serialization/Intermediate/IntermediateSerializer.cs ++++ b/MonoGame.Framework.Content.Pipeline/Serialization/Intermediate/IntermediateSerializer.cs +@@ -59,7 +59,7 @@ namespace Microsoft.Xna.Framework.Content.Pipeline.Serialization.Intermediate + private IntermediateSerializer() + { +- _scannedObjects = new List(); ++ _scannedObjects = new HashSet(ReferenceEqualityComparer.Instance); + _namespaceAliasHelper = new NamespaceAliasHelper(this); + } + +@@ -75,9 +75,9 @@ namespace Microsoft.Xna.Framework.Content.Pipeline.Serialization.Intermediate + +- private readonly List _scannedObjects; ++ private readonly HashSet _scannedObjects; + + internal bool AlreadyScanned(object value) + { +- if (_scannedObjects.Contains(value)) +- return true; +- _scannedObjects.Add(value); +- return false; ++ return !_scannedObjects.Add(value); + } diff --git a/defects/monogame-0002/test/monogame-0002-test.cs b/defects/monogame-0002/test/monogame-0002-test.cs new file mode 100644 index 000000000..208ef4a8d --- /dev/null +++ b/defects/monogame-0002/test/monogame-0002-test.cs @@ -0,0 +1,81 @@ +// monogame-0002-test: IntermediateSerializer._scannedObjects List.Contains O(N^2) +// Every object scanned during serialization does a List.Contains check, +// which is O(N) per call, making the total scan O(N^2). +// +// Defect: IntermediateSerializer.cs line 375 +// private readonly List _scannedObjects; +// if (_scannedObjects.Contains(value)) return true; +// _scannedObjects.Add(value); +// +// Fix: HashSet with ReferenceEqualityComparer for O(1) lookup. + +using System; +using System.Collections.Generic; +using System.Diagnostics; + +public class MonoGame0002Test +{ + // Simulate the defective pattern + static long SimulateDefective(int objectCount) + { + var scannedObjects = new List(); + var objects = new object[objectCount]; + for (int i = 0; i < objectCount; i++) + objects[i] = new object(); + + var sw = Stopwatch.StartNew(); + foreach (var obj in objects) + { + if (scannedObjects.Contains(obj)) + continue; + scannedObjects.Add(obj); + } + sw.Stop(); + return sw.ElapsedTicks; + } + + // Simulate the fixed pattern + static long SimulateFixed(int objectCount) + { + var scannedObjects = new HashSet(ReferenceEqualityComparer.Instance); + var objects = new object[objectCount]; + for (int i = 0; i < objectCount; i++) + objects[i] = new object(); + + var sw = Stopwatch.StartNew(); + foreach (var obj in objects) + { + scannedObjects.Add(obj); + } + sw.Stop(); + return sw.ElapsedTicks; + } + + static void Main() + { + // Warmup + SimulateDefective(100); + SimulateFixed(100); + + int N = 10000; + int trials = 3; + + long defectiveTotal = 0; + long fixedTotal = 0; + + for (int t = 0; t < trials; t++) + { + defectiveTotal += SimulateDefective(N); + fixedTotal += SimulateFixed(N); + } + + double ratio = (double)defectiveTotal / fixedTotal; + + Console.WriteLine($"monogame-0002: IntermediateSerializer _scannedObjects"); + Console.WriteLine($" N={N} objects, {trials} trials"); + Console.WriteLine($" Defective (List): {defectiveTotal / trials} ticks avg"); + Console.WriteLine($" Fixed (HashSet): {fixedTotal / trials} ticks avg"); + Console.WriteLine($" Ratio: {ratio:F1}x"); + Console.WriteLine(ratio > 2.0 ? " PASS: defect confirmed" : " FAIL: ratio too low"); + } +} diff --git a/defects/monogame-0003/patch/monogame-0003.patch b/defects/monogame-0003/patch/monogame-0003.patch new file mode 100644 index 000000000..36d21acf5 --- /dev/null +++ b/defects/monogame-0003/patch/monogame-0003.patch @@ -0,0 +1,24 @@ +--- a/MonoGame.Framework.Content.Pipeline/OpenAssetImporter.cs ++++ b/MonoGame.Framework.Content.Pipeline/OpenAssetImporter.cs +@@ -217,7 +217,7 @@ namespace Microsoft.Xna.Framework.Content.Pipeline + private Dictionary _deformationBones; // The names and offset matrices of all deformation bones. + private Node _rootBone; // The node that represents the root bone. +- private List _bones = new List(); // All nodes attached to the root bone. ++ private HashSet _bones = new HashSet(); // All nodes attached to the root bone. + private Dictionary _pivots; // The transformation pivots. + + // XNA content +@@ -1115,7 +1115,7 @@ namespace Microsoft.Xna.Framework.Content.Pipeline +- private static void GetSubtree(Node node, List list) ++ private static void GetSubtree(Node node, ICollection collection) + { + Debug.Assert(node != null); +- Debug.Assert(list != null); ++ Debug.Assert(collection != null); + +- list.Add(node); ++ collection.Add(node); + foreach (var child in node.Children) +- GetSubtree(child, list); ++ GetSubtree(child, collection); + } diff --git a/defects/monogame-0003/test/monogame-0003-test.cs b/defects/monogame-0003/test/monogame-0003-test.cs new file mode 100644 index 000000000..cf3f5968c --- /dev/null +++ b/defects/monogame-0003/test/monogame-0003-test.cs @@ -0,0 +1,116 @@ +// monogame-0003-test: OpenAssetImporter._bones List.Contains O(N*B) +// During recursive node import, _bones.Contains(aiNode) is called for every +// non-mesh, non-pivot node. With _bones as a List, this is O(B) per call, +// making the full tree traversal O(N*B) where N=total nodes, B=bone count. +// +// Defect: OpenAssetImporter.cs line 583, 818 +// private List _bones = new List(); +// else if (!_bones.Contains(aiNode)) // O(B) per node +// +// Fix: HashSet for O(1) Contains. + +using System; +using System.Collections.Generic; +using System.Diagnostics; + +public class MonoGame0003Test +{ + class FakeNode + { + public string Name; + public List Children = new List(); + } + + // Simulate the defective pattern: List.Contains in tree traversal + static long SimulateDefective(FakeNode root, List bones) + { + var sw = Stopwatch.StartNew(); + TraverseDefective(root, bones); + sw.Stop(); + return sw.ElapsedTicks; + } + + static void TraverseDefective(FakeNode node, List bones) + { + // Simulates the check at line 583 + if (!bones.Contains(node)) + { + // would create NodeContent + } + foreach (var child in node.Children) + TraverseDefective(child, bones); + } + + // Simulate the fixed pattern: HashSet.Contains in tree traversal + static long SimulateFixed(FakeNode root, HashSet bones) + { + var sw = Stopwatch.StartNew(); + TraverseFixed(root, bones); + sw.Stop(); + return sw.ElapsedTicks; + } + + static void TraverseFixed(FakeNode node, HashSet bones) + { + if (!bones.Contains(node)) + { + // would create NodeContent + } + foreach (var child in node.Children) + TraverseFixed(child, bones); + } + + static FakeNode BuildTree(int depth, int breadth, List allNodes) + { + var node = new FakeNode { Name = "node_" + allNodes.Count }; + allNodes.Add(node); + if (depth > 0) + { + for (int i = 0; i < breadth; i++) + { + var child = BuildTree(depth - 1, breadth, allNodes); + node.Children.Add(child); + } + } + return node; + } + + static void Main() + { + // Build a tree with ~1000 nodes (depth=3, breadth=10 = 1111 nodes) + var allNodes = new List(); + var root = BuildTree(3, 10, allNodes); + + // Half the nodes are bones + var bonesList = new List(); + var bonesSet = new HashSet(); + for (int i = 0; i < allNodes.Count; i += 2) + { + bonesList.Add(allNodes[i]); + bonesSet.Add(allNodes[i]); + } + + // Warmup + SimulateDefective(root, bonesList); + SimulateFixed(root, bonesSet); + + int trials = 5; + long defectiveTotal = 0; + long fixedTotal = 0; + + for (int t = 0; t < trials; t++) + { + defectiveTotal += SimulateDefective(root, bonesList); + fixedTotal += SimulateFixed(root, bonesSet); + } + + double ratio = (double)defectiveTotal / fixedTotal; + + Console.WriteLine($"monogame-0003: OpenAssetImporter _bones"); + Console.WriteLine($" Nodes={allNodes.Count}, Bones={bonesList.Count}, {trials} trials"); + Console.WriteLine($" Defective (List): {defectiveTotal / trials} ticks avg"); + Console.WriteLine($" Fixed (HashSet): {fixedTotal / trials} ticks avg"); + Console.WriteLine($" Ratio: {ratio:F1}x"); + Console.WriteLine(ratio > 2.0 ? " PASS: defect confirmed" : " FAIL: ratio too low"); + } +}