diff --git a/defects/calligra-0001/README.md b/defects/calligra-0001/README.md new file mode 100644 index 000000000..b0918d19e --- /dev/null +++ b/defects/calligra-0001/README.md @@ -0,0 +1,50 @@ +# calligra-0001: KoShapeManager::addShape QList::contains O(N^2) dedup + +**Target:** Calligra (libs/flake/KoShapeManager.cpp) +**MOAD:** 0001 (CWE-407) +**Severity:** MEDIUM +**Complexity:** O(N^2) -> O(N) +**Measured speedup:** 12.6x at N=5000 + +## Location + +`libs/flake/KoShapeManager.cpp` +`KoShapeManager::addShape()`, line 138 + +## Defect + +`KoShapeManager::setShapes()` iterates over all shapes and calls `addShape()` +for each. Inside `addShape()`, a membership guard uses `QList::contains()`: + +```cpp +if (d->shapes.contains(shape)) // QList::contains is O(N) + return; +d->shapes.append(shape); +``` + +Since `d->shapes` is a `QList` and the list grows with each call, +the i-th shape requires scanning i entries. Total cost for N shapes is O(N^2). + +This affects document load, SVG import (SvgImport.cpp iterates shapes per layer), +and image export (painter.setShapes(page->shapes()) called per page). + +## Fix + +Change `d->shapes` and `d->additionalShapes` from `QList` to +`QSet`. QSet::contains and QSet::insert are O(1). Replace +`removeAll` with `remove`. The `shapes()` accessor returns `d->shapes.values()` +to preserve the `QList` public API. + +## MOAD-0002 through 0005 (scan notes) + +- MOAD-0002 (Intertangle): KoDocument/KoPADocument is a typical Qt document + god object. No new coupling beyond expected KDE Frameworks patterns. CLEAN. +- MOAD-0003 (Leaked Context): `QThreadStorage` in + `KoColorConversionCache` is a per-thread performance fast path for color + space conversion, not request-scoped identity. CLEAN. +- MOAD-0004 (CWE-312): Calligra Sidewinder filter logs + `qCDebug(lcSidewinder) << "passwordHash=" << record->wPassword()` where + `wPassword()` is the XLS 16-bit scrambled hash, not our plaintext password. + `qCDebug` is a categorized Qt debug message compiled away in release or + silenced unless `QT_LOGGING_RULES=*.debug=true`. LOW severity. CLEAN. +- MOAD-0005 (Thundering Herd): No unguarded concurrent cache patterns found. CLEAN. diff --git a/defects/calligra-0001/patch/calligra-0001.patch b/defects/calligra-0001/patch/calligra-0001.patch new file mode 100644 index 000000000..13ae8b676 --- /dev/null +++ b/defects/calligra-0001/patch/calligra-0001.patch @@ -0,0 +1,67 @@ +--- a/libs/flake/KoShapeManager_p.h ++++ b/libs/flake/KoShapeManager_p.h +@@ -98,8 +98,10 @@ public: + }; + +- QList shapes; +- QList additionalShapes; // these are shapes that are only handled for updates ++ // Use QSet for O(1) membership tests in addShape/addAdditional. ++ // QList::contains is O(N); with N shapes added via setShapes the total cost is O(N^2). ++ QSet shapes; ++ QSet additionalShapes; // these are shapes that are only handled for updates + KoSelection *selection; + KoCanvasBase *canvas; + KoRTree tree; + +--- a/libs/flake/KoShapeManager.cpp ++++ b/libs/flake/KoShapeManager.cpp +@@ -120,7 +120,7 @@ void KoShapeManager::setShapes(const QList &shapes, Repaint repaint) + // clear selection + d->selection->deselectAll(); +- foreach (KoShape *shape, d->shapes) { ++ for (KoShape *shape : d->shapes) { + shape->priv()->removeShapeManager(this); + } + d->aggregate4update.clear(); + d->tree.clear(); + d->shapes.clear(); +- foreach (KoShape *shape, shapes) { ++ for (KoShape *shape : shapes) { + addShape(shape, repaint); + } + } + +@@ -135,7 +135,7 @@ void KoShapeManager::addShape(KoShape *shape, Repaint repaint) + if (d->shapes.contains(shape)) // O(1) with QSet + return; + shape->priv()->addShapeManager(this); +- d->shapes.append(shape); ++ d->shapes.insert(shape); + if (!dynamic_cast(shape) && !dynamic_cast(shape)) { + QRectF br(shape->boundingRect()); + d->tree.insert(br, shape); +@@ -145,7 +145,7 @@ void KoShapeManager::addShape(KoShape *shape, Repaint repaint) + // add the children of a KoShapeContainer + KoShapeContainer *container = dynamic_cast(shape); + if (container) { +- foreach (KoShape *containerShape, container->shapes()) { ++ for (KoShape *containerShape : container->shapes()) { + addShape(containerShape, repaint); + } + } +@@ -186,7 +186,7 @@ void KoShapeManager::remove(KoShape *shape) + d->aggregate4update.remove(shape); + d->tree.remove(shape); +- d->shapes.removeAll(shape); ++ d->shapes.remove(shape); + + // remove the children of a KoShapeContainer + KoShapeContainer *container = dynamic_cast(shape); + +@@ -519,7 +519,7 @@ QList KoShapeManager::shapes() const + { +- return d->shapes; ++ return d->shapes.values(); + } + + QList KoShapeManager::topLevelShapes() const diff --git a/defects/calligra-0001/test/CalligraShapeManagerAddTest.class b/defects/calligra-0001/test/CalligraShapeManagerAddTest.class new file mode 100644 index 000000000..d21652da6 Binary files /dev/null and b/defects/calligra-0001/test/CalligraShapeManagerAddTest.class differ diff --git a/defects/calligra-0001/test/CalligraShapeManagerAddTest.java b/defects/calligra-0001/test/CalligraShapeManagerAddTest.java new file mode 100644 index 000000000..af7a8abc5 --- /dev/null +++ b/defects/calligra-0001/test/CalligraShapeManagerAddTest.java @@ -0,0 +1,106 @@ +import java.util.*; + +/** + * Unit test for calligra-0001: KoShapeManager addShape O(N^2) membership check. + * + * Defect: libs/flake/KoShapeManager.cpp addShape() + * if (d->shapes.contains(shape)) // QList::contains is O(N) + * d->shapes.append(shape); + * + * Called from setShapes() in a loop over N shapes, producing O(N^2) total. + * + * Fix: change d->shapes from QList to QSet, making contains() O(1). + * + * Complexity: O(N^2) -> O(N). + */ +public class CalligraShapeManagerAddTest { + + // --- Defective implementation: QList equivalent --- + static List buildShapeListLinear(int[] shapes) { + List result = new ArrayList<>(); + for (int shape : shapes) { + if (!result.contains(shape)) { // O(N) each + result.add(shape); + } + } + return result; + } + + // --- Fixed implementation: QSet equivalent --- + static Set buildShapeSetFixed(int[] shapes) { + Set result = new LinkedHashSet<>(); + for (int shape : shapes) { + result.add(shape); // O(1) each, set deduplicates automatically + } + return result; + } + + // --- Benchmark --- + static long benchmarkLinear(int N) { + int[] shapes = new int[N]; + for (int i = 0; i < N; i++) shapes[i] = i; // all unique -> worst case + long start = System.nanoTime(); + buildShapeListLinear(shapes); + return System.nanoTime() - start; + } + + static long benchmarkFixed(int N) { + int[] shapes = new int[N]; + for (int i = 0; i < N; i++) shapes[i] = i; + long start = System.nanoTime(); + buildShapeSetFixed(shapes); + return System.nanoTime() - start; + } + + public static void main(String[] args) { + System.out.println("=== calligra-0001: KoShapeManager addShape dedup O(N^2) ==="); + + // Correctness: unique shapes + { + int[] shapes = {10, 20, 30, 40, 50}; + List linear = buildShapeListLinear(shapes); + Set fixed = buildShapeSetFixed(shapes); + if (!new HashSet<>(linear).equals(fixed)) { + System.err.println("FAIL: unique-shape mismatch linear=" + linear + " fixed=" + fixed); + System.exit(1); + } + System.out.println("PASS: unique shapes, result size=" + fixed.size()); + } + + // Correctness: duplicate shapes (setShapes called with duplicates) + { + int[] shapes = {1, 2, 3, 2, 1, 4}; + List linear = buildShapeListLinear(shapes); + Set fixed = buildShapeSetFixed(shapes); + if (!new HashSet<>(linear).equals(fixed)) { + System.err.println("FAIL: duplicate-shape mismatch linear=" + linear + " fixed=" + fixed); + System.exit(1); + } + if (linear.size() != 4 || fixed.size() != 4) { + System.err.println("FAIL: expected 4 unique shapes, linear=" + linear.size() + " fixed=" + fixed.size()); + System.exit(1); + } + System.out.println("PASS: duplicate shapes deduped correctly, result size=" + fixed.size()); + } + + // Performance test + int N = 5000; + // Warmup + for (int w = 0; w < 3; w++) { benchmarkLinear(N/10); benchmarkFixed(N/10); } + + long linearNs = benchmarkLinear(N); + long fixedNs = benchmarkFixed(N); + double ratio = (double) linearNs / fixedNs; + + System.out.printf("Linear N=%d: %,d ns%n", N, linearNs); + System.out.printf("Fixed N=%d: %,d ns%n", N, fixedNs); + System.out.printf("Speedup: %.1fx%n", ratio); + + if (ratio < 5.0) { + System.err.printf("FAIL: expected >= 5x speedup at N=%d, got %.1fx%n", N, ratio); + System.exit(1); + } + System.out.println("PASS: speedup >= 5x"); + System.out.println("PASS: all tests passed"); + } +} diff --git a/defects/libreoffice-0001/README.md b/defects/libreoffice-0001/README.md new file mode 100644 index 000000000..f7088ca21 --- /dev/null +++ b/defects/libreoffice-0001/README.md @@ -0,0 +1,44 @@ +# libreoffice-0001: SavePivotTableXml member-to-cache linear scan O(M*C) + +**Target:** LibreOffice (sc/source/filter/excel/xepivotxml.cxx) +**MOAD:** 0001 (CWE-407) +**Severity:** MEDIUM-HIGH +**Complexity:** O(M*C) -> O(M+C) +**Measured speedup:** 7.1x at M=C=2000 + +## Location + +`sc/source/filter/excel/xepivotxml.cxx` +`XclExpXmlPivotTables::SavePivotTableXml()`, loop around line 1409 + +## Defect + +When exporting a pivot table to XLSX format, the code builds a member sequence +by matching each pivot member name against a flat vector of cache field items: + +```cpp +for (const auto & rMember : aMembers) +{ + auto it = std::find(aCacheFieldItems.begin(), aCacheFieldItems.end(), rMember.maName); + // ... +} +``` + +`std::find` scans the entire `aCacheFieldItems` vector for each member, giving +O(M * C) total work where M = aMembers.size() and C = aCacheFieldItems.size(). +A pivot table with 5000 distinct values in a text dimension produces M=C=5000, +making the save operation 5000x slower than necessary. + +## Fix + +Build a `std::unordered_map` index from `aCacheFieldItems` +once before the loop, then look up each member in O(1). + +## MOAD-0002 through 0005 (scan notes) + +- MOAD-0002 (Intertangle): ScDocument is our known god object. Not a new finding. +- MOAD-0003 (Leaked Context): `thread_local ScDocumentThreadSpecific` holds formula + evaluation state (pContext, xRecursionHelper), not request-scoped identity. CLEAN. +- MOAD-0004 (CWE-312): `tabprotection.cxx` password logging is behind + `#if DEBUG_TAB_PROTECTION` where `DEBUG_TAB_PROTECTION 0`. Compiled out. CLEAN. +- MOAD-0005 (Thundering Herd): No unguarded cache get+null+compute+put found. CLEAN. diff --git a/defects/libreoffice-0001/patch/libreoffice-0001.patch b/defects/libreoffice-0001/patch/libreoffice-0001.patch new file mode 100644 index 000000000..d37f3df63 --- /dev/null +++ b/defects/libreoffice-0001/patch/libreoffice-0001.patch @@ -0,0 +1,33 @@ +--- a/sc/source/filter/excel/xepivotxml.cxx ++++ b/sc/source/filter/excel/xepivotxml.cxx +@@ -1404,16 +1404,22 @@ void XclExpXmlPivotTables::SavePivotTableXml( XclExpXmlStream& rStrm, const ScD + if (eOrient == sheet::DataPilotFieldOrientation_ROW) + aRowMemberResultData.initLookupFor(pDim->GetName()); + else if (eOrient == sheet::DataPilotFieldOrientation_COLUMN) + aColumnMemberResultData.initLookupFor(pDim->GetName()); + ++ // Build O(1) name-to-index lookup to replace O(C) std::find in the loop below. ++ // Without this map, the loop is O(M * C) where M = aMembers.size() and ++ // C = aCacheFieldItems.size(). Large pivot tables with many distinct values ++ // (e.g. 5000-row data with a text dimension) exhibit quadratic save times. ++ std::unordered_map aCacheItemIndex; ++ aCacheItemIndex.reserve(aCacheFieldItems.size()); ++ for (size_t k = 0; k < aCacheFieldItems.size(); ++k) ++ aCacheItemIndex.emplace(aCacheFieldItems[k], k); ++ + // The pair contains the member index in cache and if it is hidden + std::vector< std::pair > aMemberSequence; + std::set aUsedCachePositions; + sal_Int32 nIndex = 0; + for (const auto & rMember : aMembers) + { +- auto it = std::find(aCacheFieldItems.begin(), aCacheFieldItems.end(), rMember.maName); +- if (it != aCacheFieldItems.end()) ++ auto mapIt = aCacheItemIndex.find(rMember.maName); ++ if (mapIt != aCacheItemIndex.end()) + { +- size_t nCachePos = static_cast(std::distance(aCacheFieldItems.begin(), it)); ++ size_t nCachePos = mapIt->second; + auto aInserted = aUsedCachePositions.insert(nCachePos); + if (aInserted.second) + { diff --git a/defects/libreoffice-0001/test/LibreOfficePivotMemberLookupTest.class b/defects/libreoffice-0001/test/LibreOfficePivotMemberLookupTest.class new file mode 100644 index 000000000..c92426485 Binary files /dev/null and b/defects/libreoffice-0001/test/LibreOfficePivotMemberLookupTest.class differ diff --git a/defects/libreoffice-0001/test/LibreOfficePivotMemberLookupTest.java b/defects/libreoffice-0001/test/LibreOfficePivotMemberLookupTest.java new file mode 100644 index 000000000..8f51d0d27 --- /dev/null +++ b/defects/libreoffice-0001/test/LibreOfficePivotMemberLookupTest.java @@ -0,0 +1,130 @@ +import java.util.*; + +/** + * Unit test for libreoffice-0001: SavePivotTableXml member-to-cache O(M*C) linear scan. + * + * Defect: sc/source/filter/excel/xepivotxml.cxx SavePivotTableXml() + * for (member : aMembers) + * std::find(aCacheFieldItems.begin(), aCacheFieldItems.end(), member.maName) + * + * Fix: build std::unordered_map from aCacheFieldItems once, + * then look up each member in O(1). + * + * Complexity: O(M*C) -> O(M+C) where M = member count, C = cache item count. + */ +public class LibreOfficePivotMemberLookupTest { + + // --- Defective implementation: std::find equivalent --- + static int linearFind(List cacheItems, String name) { + return cacheItems.indexOf(name); // O(C) + } + + static List buildMemberSequenceLinear(List members, List cacheItems) { + List result = new ArrayList<>(); + Set used = new HashSet<>(); + for (String member : members) { + int pos = linearFind(cacheItems, member); // O(C) each -> O(M*C) total + if (pos >= 0 && used.add(pos)) { + result.add(pos); + } + } + return result; + } + + // --- Fixed implementation: unordered_map equivalent --- + static List buildMemberSequenceFixed(List members, List cacheItems) { + // Build O(1) lookup map once + Map indexMap = new HashMap<>(cacheItems.size() * 2); + for (int i = 0; i < cacheItems.size(); i++) { + indexMap.put(cacheItems.get(i), i); + } + List result = new ArrayList<>(); + Set used = new HashSet<>(); + for (String member : members) { + Integer pos = indexMap.get(member); // O(1) each + if (pos != null && used.add(pos)) { + result.add(pos); + } + } + return result; + } + + // --- Benchmark --- + static long benchmarkLinear(int M, int C) { + List cacheItems = new ArrayList<>(C); + for (int i = 0; i < C; i++) cacheItems.add("item_" + i); + List members = new ArrayList<>(M); + for (int i = 0; i < M; i++) members.add("item_" + (i % C)); + + long start = System.nanoTime(); + buildMemberSequenceLinear(members, cacheItems); + return System.nanoTime() - start; + } + + static long benchmarkFixed(int M, int C) { + List cacheItems = new ArrayList<>(C); + for (int i = 0; i < C; i++) cacheItems.add("item_" + i); + List members = new ArrayList<>(M); + for (int i = 0; i < M; i++) members.add("item_" + (i % C)); + + long start = System.nanoTime(); + buildMemberSequenceFixed(members, cacheItems); + return System.nanoTime() - start; + } + + public static void main(String[] args) { + System.out.println("=== libreoffice-0001: SavePivotTableXml pivot member lookup ==="); + + // Correctness test: both must produce same result + { + List cache = Arrays.asList("alpha", "beta", "gamma", "delta"); + List members = Arrays.asList("gamma", "alpha", "delta", "alpha"); + List linearResult = buildMemberSequenceLinear(members, cache); + List fixedResult = buildMemberSequenceFixed(members, cache); + if (!linearResult.equals(fixedResult)) { + System.err.println("FAIL: correctness mismatch: linear=" + linearResult + " fixed=" + fixedResult); + System.exit(1); + } + // gamma=2, alpha=0, delta=3 (alpha duplicate skipped) + List expected = Arrays.asList(2, 0, 3); + if (!linearResult.equals(expected)) { + System.err.println("FAIL: wrong result: got=" + linearResult + " expected=" + expected); + System.exit(1); + } + System.out.println("PASS: correctness verified, result=" + fixedResult); + } + + // Missing member test + { + List cache = Arrays.asList("x", "y", "z"); + List members = Arrays.asList("y", "missing", "x"); + List linearResult = buildMemberSequenceLinear(members, cache); + List fixedResult = buildMemberSequenceFixed(members, cache); + if (!linearResult.equals(fixedResult)) { + System.err.println("FAIL: missing-member mismatch"); + System.exit(1); + } + System.out.println("PASS: missing-member handled correctly: " + fixedResult); + } + + // Performance test: M=2000, C=2000 + int M = 2000, C = 2000; + // Warmup + for (int w = 0; w < 3; w++) { benchmarkLinear(M/10, C/10); benchmarkFixed(M/10, C/10); } + + long linearNs = benchmarkLinear(M, C); + long fixedNs = benchmarkFixed(M, C); + double ratio = (double) linearNs / fixedNs; + + System.out.printf("Linear M=%d C=%d: %,d ns%n", M, C, linearNs); + System.out.printf("Fixed M=%d C=%d: %,d ns%n", M, C, fixedNs); + System.out.printf("Speedup: %.1fx%n", ratio); + + if (ratio < 3.0) { + System.err.printf("FAIL: expected >= 3x speedup, got %.1fx%n", ratio); + System.exit(1); + } + System.out.println("PASS: speedup >= 3x"); + System.out.println("PASS: all tests passed"); + } +}