sc/qa/unit/types/SortOrderReverserTest.cxx | 79 +++++++++++++++++++++++++++++ sc/source/core/data/SheetView.cxx | 4 - 2 files changed, 81 insertions(+), 2 deletions(-)
New commits: commit 48e8dcec7d96a7ae04c6df4a8e25d9a3b0083743 Author: Andras Timar <[email protected]> AuthorDate: Tue Mar 3 15:25:04 2026 +0100 Commit: Miklos Vajna <[email protected]> CommitDate: Wed Mar 4 08:33:51 2026 +0100 Fix SIGABRT in SortOrderReverser::addOrderIndices with mismatched key states The std::equal call in addOrderIndices used begin() for both iterators of the first range (begin, begin) instead of (begin, end), so it always compared zero elements and returned true. This made bKeyStatesEqual unconditionally true, causing sort orders to be merged via mergeOrder() even when the key states differed. Merging incompatible order arrays then triggered an out-of-bounds vector access in mergeOrder(), aborting with a std::vector::operator[] assertion failure. Fix by using the 4-iterator std::equal overload so the sizes and contents of both key state vectors are properly compared. Add unit tests that exercise the replace-vs-merge decision with non-empty key states, which was previously untested (all existing tests used empty maKeyStates). Regression from 7257b091a19f "sc: introduce SortOrderInfo instead of multiple input params". Change-Id: I5a3f8c2d9e4b7a1d0c6f3e8b5a2d7c4f9e1b6a3d Reviewed-on: https://gerrit.libreoffice.org/c/core/+/200890 Tested-by: Jenkins CollaboraOffice <[email protected]> Reviewed-by: Miklos Vajna <[email protected]> diff --git a/sc/qa/unit/types/SortOrderReverserTest.cxx b/sc/qa/unit/types/SortOrderReverserTest.cxx index 532d24857edb..310004e6aab8 100644 --- a/sc/qa/unit/types/SortOrderReverserTest.cxx +++ b/sc/qa/unit/types/SortOrderReverserTest.cxx @@ -92,6 +92,85 @@ CPPUNIT_TEST_FIXTURE(SortOrderReverserTest, testCombiningSortOrder) aReverser.maSortInfo.maOrder.begin())); } +CPPUNIT_TEST_FIXTURE(SortOrderReverserTest, testCombiningDifferentKeyStatesReplaces) +{ + ScSortKeyState aKeyAscending; + aKeyAscending.nField = 0; + aKeyAscending.bDoSort = true; + aKeyAscending.bAscending = true; + aKeyAscending.aColorSortMode = ScColorSortMode::None; + + ScSortKeyState aKeyDescending; + aKeyDescending.nField = 0; + aKeyDescending.bDoSort = true; + aKeyDescending.bAscending = false; + aKeyDescending.aColorSortMode = ScColorSortMode::None; + + sc::SortOrderReverser aReverser; + + // First sort: ascending on column 0 + aReverser.addOrderIndices({ 0, 0, 5, 8, { 3, 1, 4, 2 }, { aKeyAscending } }); + + // Second sort: descending on column 0 - different key state, should replace + aReverser.addOrderIndices({ 0, 0, 5, 8, { 4, 3, 2, 1 }, { aKeyDescending } }); + + // The order should be replaced, not merged - so it's the second sort's order + std::vector<SCCOLROW> aExpectedOrder{ 4, 3, 2, 1 }; + CPPUNIT_ASSERT_EQUAL(aExpectedOrder.size(), aReverser.maSortInfo.maOrder.size()); + CPPUNIT_ASSERT(std::equal(aExpectedOrder.begin(), aExpectedOrder.end(), + aReverser.maSortInfo.maOrder.begin())); + + // And key state should be the second sort's + CPPUNIT_ASSERT_EQUAL(size_t(1), aReverser.maSortInfo.maKeyStates.size()); + CPPUNIT_ASSERT_EQUAL(false, aReverser.maSortInfo.maKeyStates[0].bAscending); +} + +CPPUNIT_TEST_FIXTURE(SortOrderReverserTest, testCombiningSameKeyStatesMerges) +{ + ScSortKeyState aKeyAscending; + aKeyAscending.nField = 0; + aKeyAscending.bDoSort = true; + aKeyAscending.bAscending = true; + aKeyAscending.aColorSortMode = ScColorSortMode::None; + + sc::SortOrderReverser aReverser; + + // First sort: ascending on column 0 + aReverser.addOrderIndices({ 0, 0, 5, 8, { 3, 1, 4, 2 }, { aKeyAscending } }); + + // Second sort: same key state, should merge + aReverser.addOrderIndices({ 0, 0, 5, 8, { 4, 3, 2, 1 }, { aKeyAscending } }); + + // The order should be merged (same as testCombiningSortOrder) + std::vector<SCCOLROW> aExpectedOrder{ 2, 4, 1, 3 }; + CPPUNIT_ASSERT_EQUAL(aExpectedOrder.size(), aReverser.maSortInfo.maOrder.size()); + CPPUNIT_ASSERT(std::equal(aExpectedOrder.begin(), aExpectedOrder.end(), + aReverser.maSortInfo.maOrder.begin())); + + // Key state should be preserved + CPPUNIT_ASSERT_EQUAL(size_t(1), aReverser.maSortInfo.maKeyStates.size()); + CPPUNIT_ASSERT_EQUAL(true, aReverser.maSortInfo.maKeyStates[0].bAscending); +} + +CPPUNIT_TEST_FIXTURE(SortOrderReverserTest, testCombiningDifferentRowRangeReplaces) +{ + sc::SortOrderReverser aReverser; + + // First sort on rows 5-8 + aReverser.addOrderIndices({ 0, 0, 5, 8, { 3, 1, 4, 2 }, {} }); + + // Second sort on rows 10-13 - different range, should replace + aReverser.addOrderIndices({ 0, 0, 10, 13, { 4, 3, 2, 1 }, {} }); + + // The order should be replaced with the second sort's values + std::vector<SCCOLROW> aExpectedOrder{ 4, 3, 2, 1 }; + CPPUNIT_ASSERT_EQUAL(aExpectedOrder.size(), aReverser.maSortInfo.maOrder.size()); + CPPUNIT_ASSERT(std::equal(aExpectedOrder.begin(), aExpectedOrder.end(), + aReverser.maSortInfo.maOrder.begin())); + CPPUNIT_ASSERT_EQUAL(SCROW(10), aReverser.maSortInfo.mnFirstRow); + CPPUNIT_ASSERT_EQUAL(SCROW(13), aReverser.maSortInfo.mnLastRow); +} + CPPUNIT_TEST_FIXTURE(SortOrderReverserTest, testResort) { { diff --git a/sc/source/core/data/SheetView.cxx b/sc/source/core/data/SheetView.cxx index bb2d78303fd6..b9874bf51ab6 100644 --- a/sc/source/core/data/SheetView.cxx +++ b/sc/source/core/data/SheetView.cxx @@ -33,8 +33,8 @@ std::vector<SCCOLROW> mergeOrder(std::vector<SCCOLROW> const& rExistingOrder, void SortOrderReverser::addOrderIndices(SortOrderInfo const& rSortInfo) { - bool bKeyStatesEqual = std::equal(rSortInfo.maKeyStates.begin(), rSortInfo.maKeyStates.begin(), - maSortInfo.maKeyStates.begin()); + bool bKeyStatesEqual = std::equal(rSortInfo.maKeyStates.begin(), rSortInfo.maKeyStates.end(), + maSortInfo.maKeyStates.begin(), maSortInfo.maKeyStates.end()); if (bKeyStatesEqual && maSortInfo.mnFirstRow == rSortInfo.mnFirstRow && maSortInfo.mnLastRow == rSortInfo.mnLastRow)
