From 395a5977d54f4ab826706207882c2233b020d57d Mon Sep 17 00:00:00 2001 From: msv Date: Wed, 23 May 2018 10:18:49 +0300 Subject: [PATCH] 0029777: Foundation Classes - The methods of moving of one NCollection_Sequence to another are unsafe Make the methods Append, Prepend, InsertBefore and InsertAfter, which take another sequence as an argument, copying the sequence instead of joining if the allocators are different. Add test cases for collection classes. --- src/NCollection/NCollection_List.hxx | 2 - src/NCollection/NCollection_Sequence.hxx | 92 +++++++++++++++++++----- src/QANCollection/QANCollection_Test.cxx | 40 ++++++++++- tests/collections/n/array1 | 3 + tests/collections/n/array2 | 3 + tests/collections/n/arrayMove | 4 ++ tests/collections/n/dblmap | 3 + tests/collections/n/dmap | 3 + tests/collections/n/idmap | 3 + tests/collections/n/imap | 3 + tests/collections/n/list | 3 + tests/collections/n/seq | 3 + tests/collections/n/vector | 3 + 13 files changed, 145 insertions(+), 20 deletions(-) create mode 100644 tests/collections/n/array1 create mode 100644 tests/collections/n/array2 create mode 100644 tests/collections/n/arrayMove create mode 100644 tests/collections/n/dblmap create mode 100644 tests/collections/n/dmap create mode 100644 tests/collections/n/idmap create mode 100644 tests/collections/n/imap create mode 100644 tests/collections/n/list create mode 100644 tests/collections/n/seq create mode 100644 tests/collections/n/vector diff --git a/src/NCollection/NCollection_List.hxx b/src/NCollection/NCollection_List.hxx index 7ce1eaa1ed..58f0de3744 100644 --- a/src/NCollection/NCollection_List.hxx +++ b/src/NCollection/NCollection_List.hxx @@ -274,8 +274,6 @@ public: else { // No - this list has different memory scope - Standard_NoSuchObject_Raise_if (!theIter.More(), "NCollection_List::InsertAfter"); - Iterator anIter; anIter.myPrevious = theIter.myCurrent; anIter.myCurrent = theIter.myCurrent->Next(); diff --git a/src/NCollection/NCollection_Sequence.hxx b/src/NCollection/NCollection_Sequence.hxx index 1162164bb6..55ef23b88c 100644 --- a/src/NCollection/NCollection_Sequence.hxx +++ b/src/NCollection/NCollection_Sequence.hxx @@ -176,14 +176,10 @@ public: //! This method does not change the internal allocator. NCollection_Sequence& Assign (const NCollection_Sequence& theOther) { - if (this == &theOther) - return *this; - Clear (); - Node * pCur = (Node *) theOther.myFirstItem; - while (pCur) { - Node* pNew = new (this->myAllocator) Node (pCur->Value()); - PAppend (pNew); - pCur = (Node *) pCur->Next(); + if (this != &theOther) + { + Clear(); + appendSeq((const Node *)theOther.myFirstItem); } return * this; } @@ -214,8 +210,20 @@ public: //! Append another sequence (making it empty) void Append (NCollection_Sequence& theSeq) { - if (myFirstItem == theSeq.myFirstItem) Assign (theSeq); - PAppend (theSeq); + if (this == &theSeq || theSeq.IsEmpty()) + return; + if (this->myAllocator == theSeq.myAllocator) + { + // Then we take the sequence and glue it to our end - + // deallocation will bring no problem + PAppend(theSeq); + } + else + { + // No - this sequence has different memory scope + appendSeq((const Node *)theSeq.myFirstItem); + theSeq.Clear(); + } } //! Prepend one item @@ -225,8 +233,20 @@ public: //! Prepend another sequence (making it empty) void Prepend (NCollection_Sequence& theSeq) { - if (myFirstItem == theSeq.myFirstItem) Assign (theSeq); - PPrepend (theSeq); + if (this == &theSeq || theSeq.IsEmpty()) + return; + if (this->myAllocator == theSeq.myAllocator) + { + // Then we take the sequence and glue it to our head - + // deallocation will bring no problem + PPrepend(theSeq); + } + else + { + // No - this sequence has different memory scope + prependSeq((const Node *)theSeq.myFirstItem, 1); + theSeq.Clear(); + } } //! InsertBefore theIndex theItem @@ -234,7 +254,7 @@ public: const TheItemType& theItem) { InsertAfter (theIndex-1, theItem); } - //! InsertBefore theIndex another sequence + //! InsertBefore theIndex another sequence (making it empty) void InsertBefore (const Standard_Integer theIndex, NCollection_Sequence& theSeq) { InsertAfter (theIndex-1, theSeq); } @@ -244,12 +264,27 @@ public: const TheItemType& theItem) { PInsertAfter (thePosition, new (this->myAllocator) Node (theItem)); } - //! InsertAfter theIndex theItem + //! InsertAfter theIndex another sequence (making it empty) void InsertAfter (const Standard_Integer theIndex, NCollection_Sequence& theSeq) - { PInsertAfter (theIndex, theSeq); } + { + if (this == &theSeq || theSeq.IsEmpty()) + return; + if (this->myAllocator == theSeq.myAllocator) + { + // Then we take the list and glue it to our head - + // deallocation will bring no problem + PInsertAfter(theIndex, theSeq); + } + else + { + // No - this sequence has different memory scope + prependSeq((const Node *)theSeq.myFirstItem, theIndex + 1); + theSeq.Clear(); + } + } - //! InsertAfter theIndex another sequence + //! InsertAfter theIndex theItem void InsertAfter (const Standard_Integer theIndex, const TheItemType& theItem) { @@ -335,6 +370,31 @@ public: // ---------- FRIEND CLASSES ------------ friend class Iterator; + // ----------- PRIVATE METHODS ----------- + + //! append the sequence headed by the given Node + void appendSeq(const Node * pCur) + { + while (pCur) + { + Node* pNew = new (this->myAllocator) Node(pCur->Value()); + PAppend(pNew); + pCur = (const Node *)pCur->Next(); + } + } + + //! insert the sequence headed by the given Node before the item with the given index + void prependSeq(const Node * pCur, Standard_Integer ind) + { + ind--; + while (pCur) + { + Node* pNew = new (this->myAllocator) Node(pCur->Value()); + PInsertAfter(ind++, pNew); + pCur = (const Node *)pCur->Next(); + } + } + }; #endif diff --git a/src/QANCollection/QANCollection_Test.cxx b/src/QANCollection/QANCollection_Test.cxx index 03e10cb0cc..78715b5123 100644 --- a/src/QANCollection/QANCollection_Test.cxx +++ b/src/QANCollection/QANCollection_Test.cxx @@ -25,6 +25,7 @@ #include #include +#include #define ItemType gp_Pnt #define Key1Type Standard_Real @@ -238,6 +239,23 @@ static void TestList (QANCollection_ListFunc& theL) // Assign AssignCollection (theL, aL); + // Different allocators + { + // The joining of list having different allocator can cause memory error + // if the fact of different allocator is not taken into account. + Handle(NCollection_IncAllocator) anAlloc = new NCollection_IncAllocator; + QANCollection_ListFunc aS2(anAlloc); + aS2.Append(anItem); + theL.Prepend(aS2); + aS2.Append(anItem); + theL.Append(aS2); + aS2.Append(anItem); + QANCollection_ListFunc::Iterator anIter(theL); + theL.InsertBefore(aS2, anIter); + aS2.Append(anItem); + theL.InsertAfter(aS2, anIter); + } + // Clear aL.Clear(); } @@ -294,6 +312,22 @@ static void TestSequence (QANCollection_SequenceFunc& theS) // Assign AssignCollection (theS, aS); + // Different allocators + { + // The joining of sequence having different allocator can cause memory error + // if the fact of different allocator is not taken into account. + Handle(NCollection_IncAllocator) anAlloc = new NCollection_IncAllocator; + QANCollection_SequenceFunc aS2(anAlloc); + aS2.Append(anItem); + theS.Prepend(aS2); + aS2.Append(anItem); + theS.Append(aS2); + aS2.Append(anItem); + theS.InsertBefore(1, aS2); + aS2.Append(anItem); + theS.InsertAfter(1, aS2); + } + // Clear aS.Clear(); } @@ -1113,8 +1147,10 @@ void QANCollection::CommandsTest(Draw_Interpretor& theCommands) { const char *group = "QANCollection"; // from agvCollTest/src/CollectionEXE/FuncTestEXE.cxx - theCommands.Add("QANColTestArray1", "QANColTestArray1", __FILE__, QANColTestArray1, group); - theCommands.Add("QANColTestArray2", "QANColTestArray2", __FILE__, QANColTestArray2, group); + theCommands.Add("QANColTestArray1", "QANColTestArray1 Lower Upper", + __FILE__, QANColTestArray1, group); + theCommands.Add("QANColTestArray2", "QANColTestArray2 LowerRow UpperRow LowerCol UpperCol", + __FILE__, QANColTestArray2, group); theCommands.Add("QANColTestMap", "QANColTestMap", __FILE__, QANColTestMap, group); theCommands.Add("QANColTestDataMap", "QANColTestDataMap", __FILE__, QANColTestDataMap, group); theCommands.Add("QANColTestDoubleMap", "QANColTestDoubleMap", __FILE__, QANColTestDoubleMap, group); diff --git a/tests/collections/n/array1 b/tests/collections/n/array1 new file mode 100644 index 0000000000..5563982979 --- /dev/null +++ b/tests/collections/n/array1 @@ -0,0 +1,3 @@ +puts "Check NCollection_Array1 functionality" + +QANColTestArray1 1 10 diff --git a/tests/collections/n/array2 b/tests/collections/n/array2 new file mode 100644 index 0000000000..15a4c83016 --- /dev/null +++ b/tests/collections/n/array2 @@ -0,0 +1,3 @@ +puts "Check NCollection_Array2 functionality" + +QANColTestArray2 1 5 1 3 diff --git a/tests/collections/n/arrayMove b/tests/collections/n/arrayMove new file mode 100644 index 0000000000..7b825433d9 --- /dev/null +++ b/tests/collections/n/arrayMove @@ -0,0 +1,4 @@ +puts "Check moving of NCollection_Array1 functionality" + +puts "REQUIRED ALL: Error at item" +QANColTestArrayMove diff --git a/tests/collections/n/dblmap b/tests/collections/n/dblmap new file mode 100644 index 0000000000..2c139135d4 --- /dev/null +++ b/tests/collections/n/dblmap @@ -0,0 +1,3 @@ +puts "Check NCollection_DoubleMap functionality" + +QANColTestDoubleMap diff --git a/tests/collections/n/dmap b/tests/collections/n/dmap new file mode 100644 index 0000000000..f08ba4661a --- /dev/null +++ b/tests/collections/n/dmap @@ -0,0 +1,3 @@ +puts "Check NCollection_DataMap functionality" + +QANColTestDataMap diff --git a/tests/collections/n/idmap b/tests/collections/n/idmap new file mode 100644 index 0000000000..901c064c67 --- /dev/null +++ b/tests/collections/n/idmap @@ -0,0 +1,3 @@ +puts "Check NCollection_IndexedDataMap functionality" + +QANColTestIndexedDataMap diff --git a/tests/collections/n/imap b/tests/collections/n/imap new file mode 100644 index 0000000000..7059f56881 --- /dev/null +++ b/tests/collections/n/imap @@ -0,0 +1,3 @@ +puts "Check NCollection_IndexedMap functionality" + +QANColTestIndexedMap diff --git a/tests/collections/n/list b/tests/collections/n/list new file mode 100644 index 0000000000..44f4973e51 --- /dev/null +++ b/tests/collections/n/list @@ -0,0 +1,3 @@ +puts "Check NCollection_List functionality" + +QANColTestList diff --git a/tests/collections/n/seq b/tests/collections/n/seq new file mode 100644 index 0000000000..9db3fd9137 --- /dev/null +++ b/tests/collections/n/seq @@ -0,0 +1,3 @@ +puts "Check NCollection_Sequence functionality" + +QANColTestSequence diff --git a/tests/collections/n/vector b/tests/collections/n/vector new file mode 100644 index 0000000000..208fe63434 --- /dev/null +++ b/tests/collections/n/vector @@ -0,0 +1,3 @@ +puts "Check NCollection_Vector functionality" + +QANColTestVector -- 2.20.1