fix undefined behavior found by clang static analyzer

interpretml · Apr 12, 2024 · 43319de · 43319de
1 parent fe50e13
commit 43319de
Show file tree

Hide file tree

Showing 7 changed files with 21 additions and 8 deletions.
diff --git a/python/interpret-core/interpret/visual/_inline.py b/python/interpret-core/interpret/visual/_inline.py
@@ -143,7 +143,9 @@ def _build_javascript(viz_obj, id_str=None, default_key=-1, js_url=None):
         else:
             # if no root directory then fallback to the git source location
             root_path = os.path.join(interpret_path, "..", "..", "..")
-            js_path = os.path.join(root_path, "shared", "vis", "dist", "interpret-inline.js")
+            js_path = os.path.join(
+                root_path, "shared", "vis", "dist", "interpret-inline.js"
+            )
         js_last_modified = time.ctime(os.path.getmtime(js_path))
 
         with open(js_path, "r", encoding="utf-8") as f:

diff --git a/shared/libebm/GenerateTermUpdate.cpp b/shared/libebm/GenerateTermUpdate.cpp
@@ -714,7 +714,7 @@ EBM_API_BODY ErrorEbm EBM_CALLING_CONVENTION GenerateTermUpdate(void* rng,
       // are going to remain having 0 splits.
       pBoosterShell->GetInnerTermUpdate()->Reset();
 
-      if(IntEbm{0} == lastDimensionLeavesMax || 1 != cRealDimensions && MONOTONE_NONE != significantDirection) {
+      if(IntEbm{0} == lastDimensionLeavesMax || (1 != cRealDimensions && MONOTONE_NONE != significantDirection)) {
          // this is kind of hacky where if any one of a number of things occurs (like we have only 1 leaf)
          // we sum everything into a single bin. The alternative would be to always sum into the tensor bins
          // but then collapse them afterwards into a single bin, but that's more work.

diff --git a/shared/libebm/PartitionOneDimensionalBoosting.cpp b/shared/libebm/PartitionOneDimensionalBoosting.cpp
@@ -312,7 +312,6 @@ static int FindBestSplitGain(RandomDeterministic* const pRng,
 
    EBM_ASSERT(FloatCalc{0} <= k_gainMin);
    FloatCalc bestGain = k_gainMin; // it must at least be this, and maybe it needs to be more
-   EBM_ASSERT(0 <= cSamplesLeafMin);
    EBM_ASSERT(0.0 < hessianMin);
    EBM_ASSERT(pBinLast != pBinCur); // then we would be non-splitable and would have exited above
    do {

diff --git a/shared/libebm/PartitionTwoDimensionalBoosting.cpp b/shared/libebm/PartitionTwoDimensionalBoosting.cpp
@@ -89,7 +89,6 @@ static FloatCalc SweepMultiDimensional(const size_t cRuntimeScores,
    auto* const aGradientPairsHigh =
          bUseStackMemory ? binHigh.GetGradientPairs() : p_DO_NOT_USE_DIRECTLY_High->GetGradientPairs();
 
-   EBM_ASSERT(0 <= cSamplesLeafMin);
    EBM_ASSERT(0.0 < hessianMin);
 
    const bool bUseLogitBoost = bHessian && !(TermBoostFlags_DisableNewtonGain & flags);
@@ -301,7 +300,6 @@ template<bool bHessian, size_t cCompilerScores> class PartitionTwoDimensionalBoo
       auto* pTotals1HighLowBest = IndexBin(aAuxiliaryBins, cBytesPerBin * 2);
       auto* pTotals1HighHighBest = IndexBin(aAuxiliaryBins, cBytesPerBin * 3);
 
-      EBM_ASSERT(0 <= cSamplesLeafMin);
       EBM_ASSERT(0.0 < hessianMin);
 
       LOG_0(Trace_Verbose, "PartitionTwoDimensionalBoostingInternal Starting FIRST bin sweep loop");

diff --git a/shared/libebm/PartitionTwoDimensionalInteraction.cpp b/shared/libebm/PartitionTwoDimensionalInteraction.cpp
@@ -98,7 +98,6 @@ template<bool bHessian, size_t cCompilerScores> class PartitionTwoDimensionalInt
       auto* const aGradientPairs11 =
             bUseStackMemory ? bin11.GetGradientPairs() : p_DO_NOT_USE_DIRECTLY_11->GetGradientPairs();
 
-      EBM_ASSERT(0 <= cSamplesLeafMin);
       EBM_ASSERT(0.0 < hessianMin);
 
 #ifndef NDEBUG

diff --git a/shared/libebm/compute/BinSumsBoosting.hpp b/shared/libebm/compute/BinSumsBoosting.hpp
@@ -947,6 +947,7 @@ struct BitPack final {
    INLINE_ALWAYS static void Func(BinSumsBoostingBridge* const pParams) {
 
       static_assert(!bCollapsed, "Cannot be bCollapsed since there would be no bitpacking");
+      static_assert(cCompilerPack <= COUNT_BITS(typename TFloat::TInt::T), "cCompilerPack must fit into the bitpack");
 
       if(cCompilerPack == pParams->m_cPack) {
          size_t cSamples = pParams->m_cSamples;
@@ -967,11 +968,15 @@ struct BitPack final {
             }
             pParams->m_cSamples = cSamples;
             if(bWeight) {
+               EBM_ASSERT(nullptr != pParams->m_aWeights);
                pParams->m_aWeights = IndexByte(pParams->m_aWeights, sizeof(typename TFloat::T) * cRemnants);
+            } else {
+               EBM_ASSERT(nullptr == pParams->m_aWeights);
             }
 
             const size_t cScores = GET_COUNT_SCORES(cCompilerScores, pParams->m_cScores);
 
+            EBM_ASSERT(nullptr != pParams->m_aGradientsAndHessians);
             pParams->m_aGradientsAndHessians = IndexByte(pParams->m_aGradientsAndHessians,
                   sizeof(typename TFloat::T) * (bHessian ? size_t{2} : size_t{1}) * cScores * cRemnants);
          }
@@ -984,7 +989,7 @@ struct BitPack final {
                bHessian,
                bWeight,
                cCompilerScores,
-               GetNextBitPack<TFloat>(cCompilerPack, k_cItemsPerBitPackBoostingMin)>::Func(pParams);
+               GetNextBitPack<typename TFloat::TInt::T>(cCompilerPack, k_cItemsPerBitPackBoostingMin)>::Func(pParams);
       }
    }
 };
@@ -1020,7 +1025,8 @@ INLINE_RELEASE_TEMPLATED static void BitPackBoosting(BinSumsBoostingBridge* cons
          bHessian,
          bWeight,
          cCompilerScores,
-         GetFirstBitPack<TFloat>(k_cItemsPerBitPackBoostingMax, k_cItemsPerBitPackBoostingMin)>::Func(pParams);
+         GetFirstBitPack<typename TFloat::TInt::T>(
+               k_cItemsPerBitPackBoostingMax, k_cItemsPerBitPackBoostingMin)>::Func(pParams);
 }
 template<typename TFloat,
       bool bParallel,

diff --git a/shared/libebm/compute/Objective.hpp b/shared/libebm/compute/Objective.hpp
@@ -132,8 +132,11 @@ struct BitPackObjective final {
             pData->m_cSamples = cSamples;
 
             if(bWeight) {
+               EBM_ASSERT(nullptr != pData->m_aWeights);
                pData->m_aWeights =
                      IndexByte(pData->m_aWeights, sizeof(typename TObjective::TFloatInternal::T) * cRemnants);
+            } else {
+               EBM_ASSERT(nullptr == pData->m_aWeights);
             }
 
             const size_t cScores = GET_COUNT_SCORES(cCompilerScores, pData->m_cScores);
@@ -149,6 +152,9 @@ struct BitPackObjective final {
             }
 
             if(!TObjective::k_bRmse) {
+               EBM_ASSERT(nullptr != pData->m_aTargets);
+               EBM_ASSERT(nullptr != pData->m_aSampleScores);
+
                constexpr bool bClassification = Task_GeneralClassification == TObjective::k_task;
                if(bClassification) {
                   // targets are integers
@@ -161,6 +167,9 @@ struct BitPackObjective final {
                }
                pData->m_aSampleScores = IndexByte(
                      pData->m_aSampleScores, sizeof(typename TObjective::TFloatInternal::T) * cScores * cRemnants);
+            } else {
+               EBM_ASSERT(nullptr == pData->m_aTargets);
+               EBM_ASSERT(nullptr == pData->m_aSampleScores);
             }
          }
          DoneBitpacking<TObjective,