sw/inc/bparr.hxx | 3 +++ sw/inc/node.hxx | 3 +++ sw/qa/core/Test-BigPtrArray.cxx | 26 ++++++++++++++++++++------ sw/source/core/bastyp/bparr.cxx | 30 ++++++++++++++++++++++++++++-- sw/source/core/fields/reffld.cxx | 6 ++++-- 5 files changed, 58 insertions(+), 10 deletions(-)
New commits: commit 91abc3df612f287c13dc4ae827be3325014e8dcb Author: Noel Grandin <[email protected]> AuthorDate: Sat Jan 11 16:46:23 2025 +0200 Commit: Noel Grandin <[email protected]> CommitDate: Mon Jan 13 09:06:24 2025 +0100 prevent stale SwNode objects from reporting wrong value in GetIndex Sometimes we end up in situations where a SwNode(BigPtrEntry) is still alive, but has been removed from the SwNodes(BigPtrArray) data structure. And then we call SwNode::GetIndex and it reports a bogus value. So clear those back-pointers to avoid reading incorrect data. Change-Id: I95d4f99bf169eeb1c3290432ff8b64ca0aa69275 Reviewed-on: https://gerrit.libreoffice.org/c/core/+/180126 Tested-by: Jenkins Reviewed-by: Noel Grandin <[email protected]> diff --git a/sw/inc/bparr.hxx b/sw/inc/bparr.hxx index 4f831c8bab19..d50eba9f9507 100644 --- a/sw/inc/bparr.hxx +++ b/sw/inc/bparr.hxx @@ -41,6 +41,7 @@ public: inline sal_Int32 GetPos() const; inline BigPtrArray& GetArray() const; + bool IsDisconnected() const { return m_pBlock == nullptr; } }; // 1000 entries per Block = a bit less than 4K @@ -76,6 +77,8 @@ protected: BlockInfo* InsBlock( sal_uInt16 ); ///< insert block void BlockDel( sal_uInt16 ); ///< some blocks were deleted void UpdIndex( sal_uInt16 ); ///< recalculate indices + void ImplRemove( sal_Int32 pos, sal_Int32 n, bool bClearElement ); + void ImplReplace( sal_Int32 idx, BigPtrEntry* pElem, bool bClearElement ); // fill all blocks sal_uInt16 Compress(); diff --git a/sw/inc/node.hxx b/sw/inc/node.hxx index 12a156908136..13bbda8a81cb 100644 --- a/sw/inc/node.hxx +++ b/sw/inc/node.hxx @@ -116,6 +116,9 @@ public: } void SetRedlineMergeFlag(Merge const eMerge) { m_eMerge = eMerge; } Merge GetRedlineMergeFlag() const { return m_eMerge; } + + using BigPtrEntry::IsDisconnected; // make this public + private: Merge m_eMerge; diff --git a/sw/qa/core/Test-BigPtrArray.cxx b/sw/qa/core/Test-BigPtrArray.cxx index 6754cda04007..00e782701095 100644 --- a/sw/qa/core/Test-BigPtrArray.cxx +++ b/sw/qa/core/Test-BigPtrArray.cxx @@ -237,8 +237,9 @@ public: { sal_Int32 oldCount = bparr.Count(); - delete bparr[0]; // release content + auto p = bparr[0]; bparr.Remove(0); // remove item from container + delete p; // release content CPPUNIT_ASSERT_EQUAL_MESSAGE ( @@ -272,8 +273,9 @@ public: for (int i = NUM_ENTRIES - 1; i >= 0; i--) { sal_Int32 oldCount = bparr.Count(); - delete bparr[i]; + auto p = bparr[i]; bparr.Remove(i); + delete p; CPPUNIT_ASSERT_EQUAL_MESSAGE ( @@ -309,8 +311,9 @@ public: sal_Int32 oldCount = bparr.Count(); sal_Int32 oldElement = static_cast<BigPtrEntryMock*>(bparr[bparr.Count() / 2])->getCount(); - delete bparr[bparr.Count() / 2]; + auto p = bparr[bparr.Count() / 2]; bparr.Remove(bparr.Count() / 2); + delete p; CPPUNIT_ASSERT_EQUAL_MESSAGE ( @@ -346,11 +349,15 @@ public: sal_Int32 nRemove = std::min<sal_Int32>(bparr.Count(), 3); sal_Int32 oldCount = bparr.Count(); + std::vector<BigPtrEntry*> vToDelete; for (sal_Int32 i = 0; i < nRemove; i++) - delete bparr[i]; + vToDelete.push_back(bparr[i]); bparr.Remove(0, nRemove); + for (auto p : vToDelete) + delete p; + CPPUNIT_ASSERT_EQUAL_MESSAGE ( "test_remove_multiple_elements_at_once failed", @@ -371,9 +378,15 @@ public: fillBigPtrArray(bparr, NUM_ENTRIES); - releaseBigPtrArrayContent(bparr); + std::vector<BigPtrEntry*> vToDelete; + for (sal_Int32 i = 0; i < bparr.Count(); i++) + vToDelete.push_back(bparr[i]); + bparr.Remove(0, bparr.Count()); + for (auto p : vToDelete) + delete p; + CPPUNIT_ASSERT_EQUAL_MESSAGE ( "test_remove_all_elements_at_once failed", @@ -488,8 +501,9 @@ public: for (sal_Int32 i = 0, j = NUM_ENTRIES - 1; i < NUM_ENTRIES; i++, j--) { - delete bparr[i]; + auto p = bparr[i]; bparr.Replace(i, new BigPtrEntryMock(j)); + delete p; } for (sal_Int32 i = 0; i < NUM_ENTRIES; i++) diff --git a/sw/source/core/bastyp/bparr.cxx b/sw/source/core/bastyp/bparr.cxx index 03e035c69621..cae512d7af10 100644 --- a/sw/source/core/bastyp/bparr.cxx +++ b/sw/source/core/bastyp/bparr.cxx @@ -75,7 +75,7 @@ void BigPtrArray::Move( sal_Int32 from, sal_Int32 to ) BlockInfo* p = m_ppInf[ cur ]; BigPtrEntry* pElem = p->mvData[ from - p->nStart ]; Insert( pElem, to ); // insert first, then delete! - Remove( ( to < from ) ? ( from + 1 ) : from ); + ImplRemove( ( to < from ) ? ( from + 1 ) : from, 1, /*bClearElement*/false ); } } @@ -304,6 +304,11 @@ void BigPtrArray::Insert( BigPtrEntry* pElem, sal_Int32 pos ) } void BigPtrArray::Remove( sal_Int32 pos, sal_Int32 n ) +{ + ImplRemove(pos, n, true); +} + +void BigPtrArray::ImplRemove( sal_Int32 pos, sal_Int32 n, bool bClearElement ) { CHECKIDX( m_ppInf.get(), m_nBlock, m_nSize, m_nCur ); @@ -320,6 +325,13 @@ void BigPtrArray::Remove( sal_Int32 pos, sal_Int32 n ) sal_uInt16 nel = p->nElem - sal_uInt16(pos); if( sal_Int32(nel) > nElem ) nel = sal_uInt16(nElem); + // clear the back-pointers from the node back to node array. helps to flush out stale accesses. + if (bClearElement) + for(sal_uInt16 i=0; i < nel; ++i) + { + p->mvData[pos+i]->m_pBlock = nullptr; + p->mvData[pos+i]->m_nOffset = 0; + } // move elements if needed if( ( pos + nel ) < sal_Int32(p->nElem) ) { @@ -388,12 +400,26 @@ void BigPtrArray::Remove( sal_Int32 pos, sal_Int32 n ) } void BigPtrArray::Replace( sal_Int32 idx, BigPtrEntry* pElem) +{ + ImplReplace(idx, pElem, /*bClearElement*/ true); +} + +void BigPtrArray::ImplReplace( sal_Int32 idx, BigPtrEntry* pElem, bool bClearElement) { assert(idx < m_nSize); // Index out of bounds m_nCur = Index2Block( idx ); BlockInfo* p = m_ppInf[ m_nCur ]; pElem->m_nOffset = sal_uInt16(idx - p->nStart); pElem->m_pBlock = p; + + // clear the back-pointers from the old element back to element array. helps to flush out stale accesses. + if (bClearElement) + { + p->mvData[idx - p->nStart]->m_pBlock = nullptr; + p->mvData[idx - p->nStart]->m_nOffset = 0; + } + + // update with new element p->mvData[ idx - p->nStart ] = pElem; } @@ -419,7 +445,7 @@ BigPtrEntry* BigPtrArray::ReplaceTheOneAfter( BigPtrEntry* pNotTheOne, BigPtrEnt else { // slow path - BigPtrArray::Replace( pNotTheOne->GetPos()+1, pNewEntry ); + BigPtrArray::ImplReplace( pNotTheOne->GetPos()+1, pNewEntry, /*bClearElement*/false ); } // if the previous node is inside the current block diff --git a/sw/source/core/fields/reffld.cxx b/sw/source/core/fields/reffld.cxx index 75b2f5db7b6a..244b1404b4f3 100644 --- a/sw/source/core/fields/reffld.cxx +++ b/sw/source/core/fields/reffld.cxx @@ -1604,12 +1604,14 @@ SwTextNode* SwGetRefFieldType::FindAnchorRefStyleOther(SwDoc* pDoc, // For references, styleref acts from the position of the reference not the field // Happily, the previous code saves either one into pReference, so the following is generic for both - SwNodeOffset nReference = pReference->GetIndex(); const SwNodes& nodes = pDoc->GetNodes(); // It is possible to end up here, with a pReference pointer which points to a node which has already been // removed from the nodes array, which means that calling GetIndex() returns an incorrect index. - if (nReference >= nodes.Count() || nodes[nReference] != pReference) + SwNodeOffset nReference; + if (!pReference->IsDisconnected()) + nReference = pReference->GetIndex(); + else nReference = nodes.Count() - 1; SwTextNode* pTextNd = nullptr;
