From 56e162c480bfeb9efeef90c6490c4deee486c930 Mon Sep 17 00:00:00 2001 From: Pasukhin Dmitry Date: Fri, 13 Feb 2026 08:48:17 +0000 Subject: [PATCH] Coding - Fix critical CodeQL static analysis warnings (#1074) Interface_ParamSet: - Eliminate use-after-free in Append() by deleting old buffer through a temp variable after reassigning the member pointer (CodeQL #5132/#2684) delabella.cpp: - Fix upcast array pointer arithmetic by parenthesizing cast to ensure pointer arithmetic uses derived class (Vert) size rather than base class (DelaBella_Vertex) size (CodeQL #5131) NCollection_SparseArrayBase: - Rework to replace virtual dispatch (createItem/destroyItem/copyItem) with function pointers passed as arguments to protected methods - Store only DestroyItemFunc in base class to enable safe cleanup in destructor without virtual dispatch - Pass CreateItemFunc and CopyItemFunc as arguments with zero per-instance storage overhead - Move Clear() and UnsetValue() from base public API to protected clearItems()/unsetValue() with function pointer parameters; template class provides public wrappers - Remove vtable entirely (no virtual methods remain) - This eliminates the pure virtual call during base class destruction (CodeQL #5012) AdvApp2Var_MathBase: - Rewrite comparison to avoid potential signed integer overflow: *ncfnew + 1 > ncut becomes *ncfnew >= ncut (CodeQL #2692) --- .../TKXSBase/Interface/Interface_ParamSet.cxx | 7 +- .../NCollection/NCollection_SparseArray.hxx | 37 ++++--- .../NCollection_SparseArrayBase.cxx | 76 +++++++------- .../NCollection_SparseArrayBase.hxx | 98 +++++++++++-------- .../TKMesh/BRepMesh/delabella.cpp | 2 +- .../AdvApp2Var/AdvApp2Var_MathBase.cxx | 2 +- 6 files changed, 118 insertions(+), 104 deletions(-) diff --git a/src/DataExchange/TKXSBase/Interface/Interface_ParamSet.cxx b/src/DataExchange/TKXSBase/Interface/Interface_ParamSet.cxx index 0d22b97338..bc33f7e679 100644 --- a/src/DataExchange/TKXSBase/Interface/Interface_ParamSet.cxx +++ b/src/DataExchange/TKXSBase/Interface/Interface_ParamSet.cxx @@ -86,9 +86,10 @@ int Interface_ParamSet::Append(const char* const val, OFP.SetEntityNumber(onum); } // Confirm the new reservation - delete[] theval; - theval = newval; - thelnres = newres; + char* anOldVal = theval; + theval = newval; + thelnres = newres; + delete[] anOldVal; } // Register this parameter for (i = 0; i < lnval; i++) diff --git a/src/FoundationClasses/TKernel/NCollection/NCollection_SparseArray.hxx b/src/FoundationClasses/TKernel/NCollection/NCollection_SparseArray.hxx index 6b373ab7f6..ffb854a374 100644 --- a/src/FoundationClasses/TKernel/NCollection/NCollection_SparseArray.hxx +++ b/src/FoundationClasses/TKernel/NCollection/NCollection_SparseArray.hxx @@ -49,7 +49,7 @@ class NCollection_SparseArray : public NCollection_SparseArrayBase public: //! Constructor; accepts size of blocks explicit NCollection_SparseArray(size_t theIncrement) noexcept - : NCollection_SparseArrayBase(sizeof(TheItemType), theIncrement) + : NCollection_SparseArrayBase(sizeof(TheItemType), theIncrement, destroyItemImpl) { } @@ -58,7 +58,7 @@ public: { if (this == &theOther) return *this; - this->assign(theOther); + this->assign(theOther, createItemImpl, destroyItemImpl, copyItemImpl); return *this; } @@ -67,8 +67,8 @@ public: //! in a fast way (without creation of duplicated data) void Exchange(NCollection_SparseArray& theOther) noexcept { this->exchange(theOther); } - //! Destructor - ~NCollection_SparseArray() override { Clear(); } + //! Clears all the data + void Clear() { clearItems(destroyItemImpl); } public: //!@name Array-like interface (in addition to inherited methods) @@ -95,9 +95,13 @@ public: //! Set a value at specified index method TheItemType& SetValue(const size_t theIndex, const TheItemType& theValue) { - return *(TheItemType*)this->setValue(theIndex, (void*)&theValue); + return *(TheItemType*)this->setValue(theIndex, (void*)&theValue, createItemImpl, copyItemImpl); } + //! Deletes the item from the array; + //! returns True if that item was defined + bool UnsetValue(const size_t theIndex) { return this->unsetValue(theIndex, destroyItemImpl); } + //!@} public: @@ -126,7 +130,7 @@ public: bool IsBound(const size_t theIndex) const { return this->HasValue(theIndex); } //! Remove the item from array - bool UnBind(const size_t theIndex) { return this->UnsetValue(theIndex); } + bool UnBind(const size_t theIndex) { return UnsetValue(theIndex); } //!@} @@ -190,29 +194,22 @@ public: }; private: - // Implementation of virtual methods providing type-specific behaviour + // Static functions providing type-specific item operations - //! Create new item at the specified address with default constructor - // virtual void createItem (void* theAddress) - // { - // new (theAddress) TheItemType; - // } - - //! Create new item at the specified address with copy constructor - //! from existing item - void createItem(void* theAddress, void* theOther) override + //! Copy-construct a new item from existing item + static void createItemImpl(void* theAddress, void* theOther) { new (theAddress) TheItemType(*(const TheItemType*)theOther); } - //! Call destructor to the item at given address - void destroyItem(void* theAddress) override + //! Call destructor on the item at given address + static void destroyItemImpl(void* theAddress) { ((TheItemType*)theAddress)->TheItemType::~TheItemType(); } - //! Call assignment operator to the item - void copyItem(void* theAddress, void* theOther) override + //! Call assignment operator on the item + static void copyItemImpl(void* theAddress, void* theOther) { (*(TheItemType*)theAddress) = *(const TheItemType*)theOther; } diff --git a/src/FoundationClasses/TKernel/NCollection/NCollection_SparseArrayBase.cxx b/src/FoundationClasses/TKernel/NCollection/NCollection_SparseArrayBase.cxx index 5b8ddb7149..ff9d1544f2 100644 --- a/src/FoundationClasses/TKernel/NCollection/NCollection_SparseArrayBase.cxx +++ b/src/FoundationClasses/TKernel/NCollection/NCollection_SparseArrayBase.cxx @@ -14,13 +14,12 @@ // commercial license or contractual agreement. #include -#include #include #include #include -//================================================================================================= +//================================================================================================== void NCollection_SparseArrayBase::allocData(const size_t iBlock) { @@ -43,9 +42,9 @@ void NCollection_SparseArrayBase::allocData(const size_t iBlock) myNbBlocks = newNbBlocks; } -//================================================================================================= +//================================================================================================== -void NCollection_SparseArrayBase::freeBlock(const size_t iBlock) +void NCollection_SparseArrayBase::freeBlock(const size_t iBlock, DestroyItemFunc theDestroyItem) { void*& anAddr = myData[iBlock]; if (!anAddr) @@ -57,7 +56,7 @@ void NCollection_SparseArrayBase::freeBlock(const size_t iBlock) { if (aBlock.IsSet(anInd)) { - destroyItem(getItem(aBlock, anInd)); + theDestroyItem(getItem(aBlock, anInd)); mySize--; } } @@ -65,16 +64,16 @@ void NCollection_SparseArrayBase::freeBlock(const size_t iBlock) anAddr = nullptr; } -//================================================================================================= +//================================================================================================== -void NCollection_SparseArrayBase::Clear() +void NCollection_SparseArrayBase::clearItems(DestroyItemFunc theDestroyItem) { // free block data for (size_t iBlock = 0; iBlock < myNbBlocks; iBlock++) { if (myData[iBlock]) { - freeBlock(iBlock); + freeBlock(iBlock, theDestroyItem); } } @@ -83,23 +82,21 @@ void NCollection_SparseArrayBase::Clear() myData = nullptr; myNbBlocks = 0; mySize = 0; - - // consistency check - Standard_ProgramError_Raise_if( - mySize != 0, - "NCollection_SparseArrayBase: Implementation error: inconsistent items count") } -//================================================================================================= +//================================================================================================== -void NCollection_SparseArrayBase::assign(const NCollection_SparseArrayBase& theOther) +void NCollection_SparseArrayBase::assign(const NCollection_SparseArrayBase& theOther, + CreateItemFunc theCreateItem, + DestroyItemFunc theDestroyItem, + CopyItemFunc theCopyItem) { if (this == &theOther) return; // if block size is different, clear all data if (myBlockSize != theOther.myBlockSize) - Clear(); + clearItems(theDestroyItem); myBlockSize = theOther.myBlockSize; // iterate by blocks in theOther @@ -110,7 +107,7 @@ void NCollection_SparseArrayBase::assign(const NCollection_SparseArrayBase& theO { // if other block is empty, just make sure to empty that block in "this" if (iBlock < myNbBlocks && myData[iBlock]) - freeBlock(iBlock); + freeBlock(iBlock, theDestroyItem); continue; } @@ -133,7 +130,7 @@ void NCollection_SparseArrayBase::assign(const NCollection_SparseArrayBase& theO aBlock.Set(anInd); (*aBlock.Count)++; mySize++; - createItem(anItem, getItem(anOtherBlock, anInd)); + theCreateItem(anItem, getItem(anOtherBlock, anInd)); } } // else perform copying item-by-item @@ -148,14 +145,14 @@ void NCollection_SparseArrayBase::assign(const NCollection_SparseArrayBase& theO void* anOtherItem = getItem(anOtherBlock, anInd); if (aBlock.IsSet(anInd)) // copy { - copyItem(anItem, anOtherItem); + theCopyItem(anItem, anOtherItem); } else // create { aBlock.Set(anInd); (*aBlock.Count)++; mySize++; - createItem(anItem, getItem(anOtherBlock, anInd)); + theCreateItem(anItem, getItem(anOtherBlock, anInd)); } } else if (aBlock.IsSet(anInd)) // delete @@ -163,7 +160,7 @@ void NCollection_SparseArrayBase::assign(const NCollection_SparseArrayBase& theO aBlock.Set(anInd); (*aBlock.Count)--; mySize--; - destroyItem(anItem); + theDestroyItem(anItem); } } } @@ -172,15 +169,10 @@ void NCollection_SparseArrayBase::assign(const NCollection_SparseArrayBase& theO // clear any remaining blocks in this for (; iBlock < myNbBlocks; iBlock++) if (myData[iBlock]) - freeBlock(iBlock); - - // consistency check - Standard_ProgramError_Raise_if( - mySize != theOther.mySize, - "NCollection_SparseArrayBase: Implementation error: inconsistent items count") + freeBlock(iBlock, theDestroyItem); } -//================================================================================================= +//================================================================================================== void NCollection_SparseArrayBase::exchange(NCollection_SparseArrayBase& theOther) noexcept { @@ -193,11 +185,15 @@ void NCollection_SparseArrayBase::exchange(NCollection_SparseArrayBase& theOther std::swap(myNbBlocks, theOther.myNbBlocks); std::swap(mySize, theOther.mySize); std::swap(myData, theOther.myData); + std::swap(myDestroyItem, theOther.myDestroyItem); } -//================================================================================================= +//================================================================================================== -void* NCollection_SparseArrayBase::setValue(const size_t theIndex, void* const theValue) +void* NCollection_SparseArrayBase::setValue(const size_t theIndex, + void* const theValue, + CreateItemFunc theCreateItem, + CopyItemFunc theCopyItem) { size_t iBlock = theIndex / myBlockSize; @@ -225,18 +221,18 @@ void* NCollection_SparseArrayBase::setValue(const size_t theIndex, void* const t { (*aBlock.Count)++; mySize++; - createItem(anItem, theValue); + theCreateItem(anItem, theValue); } else { // Item already exists, just copy the value - copyItem(anItem, theValue); + theCopyItem(anItem, theValue); } return anItem; } -//================================================================================================= +//================================================================================================== bool NCollection_SparseArrayBase::HasValue(const size_t theIndex) const { @@ -246,9 +242,9 @@ bool NCollection_SparseArrayBase::HasValue(const size_t theIndex) const return getBlock(myData[iBlock]).IsSet(theIndex % myBlockSize) != 0; } -//================================================================================================= +//================================================================================================== -bool NCollection_SparseArrayBase::UnsetValue(const size_t theIndex) +bool NCollection_SparseArrayBase::unsetValue(const size_t theIndex, DestroyItemFunc theDestroyItem) { // check that the item is defined size_t iBlock = theIndex / myBlockSize; @@ -261,18 +257,18 @@ bool NCollection_SparseArrayBase::UnsetValue(const size_t theIndex) return false; // destroy the item - destroyItem(getItem(aBlock, anInd)); + theDestroyItem(getItem(aBlock, anInd)); (*aBlock.Count)--; mySize--; // free block if it becomes empty if (!(*aBlock.Count)) - freeBlock(iBlock); + freeBlock(iBlock, theDestroyItem); return true; } -//================================================================================================= +//================================================================================================== NCollection_SparseArrayBase::Iterator::Iterator(const NCollection_SparseArrayBase* theArray) : myArr((NCollection_SparseArrayBase*)theArray), @@ -284,7 +280,7 @@ NCollection_SparseArrayBase::Iterator::Iterator(const NCollection_SparseArrayBas init(theArray); } -//================================================================================================= +//================================================================================================== void NCollection_SparseArrayBase::Iterator::Next() { @@ -318,7 +314,7 @@ void NCollection_SparseArrayBase::Iterator::Next() } } -//================================================================================================= +//================================================================================================== void NCollection_SparseArrayBase::Iterator::init(const NCollection_SparseArrayBase* theArray) { diff --git a/src/FoundationClasses/TKernel/NCollection/NCollection_SparseArrayBase.hxx b/src/FoundationClasses/TKernel/NCollection/NCollection_SparseArrayBase.hxx index d44b0cdc43..dd1fd8d155 100644 --- a/src/FoundationClasses/TKernel/NCollection/NCollection_SparseArrayBase.hxx +++ b/src/FoundationClasses/TKernel/NCollection/NCollection_SparseArrayBase.hxx @@ -24,16 +24,30 @@ * Base class for NCollection_SparseArray; * provides non-template implementation of general mechanics * of block allocation, items creation / deletion etc. + * + * Type-specific item operations (construction, destruction, copy) + * are provided by the derived template class via function pointers + * passed as arguments to the protected methods. */ class NCollection_SparseArrayBase { public: - //!@name Type-independent public interface + //!@name Function pointer types for type-specific item operations //!@{ - //! Clears all the data - Standard_EXPORT void Clear(); + //! Copy-construct a new item at theAddress from theOther + using CreateItemFunc = void (*)(void* theAddress, void* theOther); + //! Destroy the item at theAddress + using DestroyItemFunc = void (*)(void* theAddress); + //! Copy-assign the item at theAddress from theOther + using CopyItemFunc = void (*)(void* theAddress, void* theOther); + + //!@} + +public: + //!@name Type-independent public interface + //!@{ //! Returns number of currently contained items size_t Size() const noexcept { return mySize; } @@ -41,10 +55,6 @@ public: //! Check whether the value at given index is set Standard_EXPORT bool HasValue(const size_t theIndex) const; - //! Deletes the item from the array; - //! returns True if that item was defined - Standard_EXPORT bool UnsetValue(const size_t theIndex); - //!@} private: @@ -180,18 +190,23 @@ private: protected: // Object life - //! Constructor; initialized by size of item and of block (in items) - NCollection_SparseArrayBase(size_t theItemSize, size_t theBlockSize) noexcept + //! Constructor; initialized by size of item, block size, and item destructor function. + //! @param theDestroyItem is stored to enable proper item destruction in the base destructor + NCollection_SparseArrayBase(size_t theItemSize, + size_t theBlockSize, + DestroyItemFunc theDestroyItem) noexcept : myItemSize(theItemSize), myBlockSize(theBlockSize), myNbBlocks(0), mySize(0), - myData(nullptr) + myData(nullptr), + myDestroyItem(theDestroyItem) { } - //! Destructor - virtual ~NCollection_SparseArrayBase() { Clear(); } + //! Destructor; properly destroys all items and frees all memory. + //! Uses the stored DestroyItemFunc, so no virtual dispatch is needed. + ~NCollection_SparseArrayBase() { clearItems(myDestroyItem); } protected: // Data access interface for descendants @@ -219,48 +234,53 @@ protected: + myItemSize * (theIndex % myBlockSize); } - //! Set a value to the specified item; returns address of the set item - Standard_EXPORT void* setValue(const size_t theIndex, void* const theValue); + //! Clears all items and frees all memory. + //! @param theDestroyItem function to call destructor on each item + Standard_EXPORT void clearItems(DestroyItemFunc theDestroyItem); + + //! Deletes the item at theIndex from the array; + //! returns True if the item was defined. + //! @param theDestroyItem function to call destructor on the item + Standard_EXPORT bool unsetValue(const size_t theIndex, DestroyItemFunc theDestroyItem); + + //! Set a value to the specified item; returns address of the set item. + //! @param theCreateItem function to copy-construct a new item + //! @param theCopyItem function to copy-assign an existing item + Standard_EXPORT void* setValue(const size_t theIndex, + void* const theValue, + CreateItemFunc theCreateItem, + CopyItemFunc theCopyItem); //! Copy contents of theOther to this; - //! assumes that this and theOther have exactly the same type of arguments - Standard_EXPORT void assign(const NCollection_SparseArrayBase& theOther); + //! assumes that this and theOther have exactly the same type of arguments. + //! @param theCreateItem function to copy-construct a new item + //! @param theDestroyItem function to call destructor on an item + //! @param theCopyItem function to copy-assign an existing item + Standard_EXPORT void assign(const NCollection_SparseArrayBase& theOther, + CreateItemFunc theCreateItem, + DestroyItemFunc theDestroyItem, + CopyItemFunc theCopyItem); //! Exchange contents of theOther and this; //! assumes that this and theOther have exactly the same type of arguments Standard_EXPORT void exchange(NCollection_SparseArrayBase& theOther) noexcept; -protected: - // Methods to be provided by descendant - - //! Create new item at the specified address with default constructor - // virtual void createItem (void* theAddress) = 0; - - //! Create new item at the specified address with copy constructor - //! from existing item - virtual void createItem(void* theAddress, void* theOther) = 0; - - //! Call destructor to the item - virtual void destroyItem(void* theAddress) = 0; - - //! Call assignment operator to the item - virtual void copyItem(void* theAddress, void* theOther) = 0; - private: // Implementation of memory allocation/deallocation and access mechanics //! Allocate space for at least iBlock+1 blocks void allocData(const size_t iBlock); - //! Free specified block - void freeBlock(const size_t iBlock); + //! Free specified block, destroying all items via theDestroyItem + void freeBlock(const size_t iBlock, DestroyItemFunc theDestroyItem); protected: - size_t myItemSize; //!< size of item - size_t myBlockSize; //!< block size (in items) - size_t myNbBlocks; //!< allocated size of blocks table - size_t mySize; //!< number of currently defined items - void** myData; //!< array of pointers to data blocks + size_t myItemSize; //!< size of item + size_t myBlockSize; //!< block size (in items) + size_t myNbBlocks; //!< allocated size of blocks table + size_t mySize; //!< number of currently defined items + void** myData; //!< array of pointers to data blocks + DestroyItemFunc myDestroyItem; //!< function to call destructor on items }; #endif diff --git a/src/ModelingAlgorithms/TKMesh/BRepMesh/delabella.cpp b/src/ModelingAlgorithms/TKMesh/BRepMesh/delabella.cpp index 75d2602325..654cbf57d1 100644 --- a/src/ModelingAlgorithms/TKMesh/BRepMesh/delabella.cpp +++ b/src/ModelingAlgorithms/TKMesh/BRepMesh/delabella.cpp @@ -247,7 +247,7 @@ struct CDelaBella : IDelaBella errlog_proc(errlog_file, "[WRN] all input points are colinear, returning single segment!\n"); first_hull_vert = vert_alloc + 0; - vert_alloc[0].next = (DelaBella_Vertex*)vert_alloc + 1; + vert_alloc[0].next = (DelaBella_Vertex*)(vert_alloc + 1); vert_alloc[1].next = nullptr; } else diff --git a/src/ModelingData/TKGeomBase/AdvApp2Var/AdvApp2Var_MathBase.cxx b/src/ModelingData/TKGeomBase/AdvApp2Var/AdvApp2Var_MathBase.cxx index 2242876a19..3ec33abe1e 100644 --- a/src/ModelingData/TKGeomBase/AdvApp2Var/AdvApp2Var_MathBase.cxx +++ b/src/ModelingData/TKGeomBase/AdvApp2Var/AdvApp2Var_MathBase.cxx @@ -535,7 +535,7 @@ int mmaper0_(integer* ncofmx, /* ------ Minimum that can be reached : Stop at 1 or NCFNEW ------ */ ncut = 1; - if (*ncfnew + 1 > ncut) + if (*ncfnew >= ncut) { ncut = *ncfnew + 1; }