在斯德哥尔摩举办第118届诺贝尔周时,我坐在我们的办公室里,在那里我们开发了PVS-Studio静态分析仪,对ROOT项目(一个用于科学研究的大数据处理框架)进行分析审查。当然,这个代码不会赢得奖品,但作者绝对可以指望对最有趣的缺陷的详细审查,再加上一个免费的许可证来彻底检查项目本身。

介绍

ROOT是一个模块化的科学软件工具包。它提供了处理大数据处理、统计分析、可视化和存储所需的所有功能。它主要写在C++。ROOT诞生于欧洲核子研究中心,是高能物理研究的核心。每天,成千上万的物理学家使用ROOT应用程序来分析他们的数据或进行模拟。

PVS-Studio是一种工具,用于检测以 C、C++、C++ 和 Java 编写的程序源代码中的软件错误和潜在漏洞。它在64位Windows、Linux和macOS上运行,可以分析为32位、64位和嵌入式ARM平台编写的源代码。

您可能还喜欢:使用 PVS-Studio 静态分析器开始,用于在 Linux 下C++开发。

新诊断的德布特

V1046在操作”&_”中一起不安全地使用 bool 和”int”类型。GSLMultiRootFinder.h 175

int AddFunction(const ROOT::Math::IMultiGenFunction & func) {
  ROOT::Math::IMultiGenFunction * f = func.Clone();
  if (!f) return 0;
  fFunctions.push_back(f);
  return fFunctions.size();
}

template<class FuncIterator>
bool SetFunctionList( FuncIterator begin, FuncIterator end) {
  bool ret = true;
  for (FuncIterator itr = begin; itr != end; ++itr) {
    const ROOT::Math::IMultiGenFunction * f = *itr;
    ret &= AddFunction(*f);
  }
  return ret;
}

首先,下面是一个精彩的错误,由 PVS-Studio 的测试版找到,我正用它来进行此审查。

期望值。函数 SetFunctionList 遍历迭代器列表。如果至少有一个迭代器无效,则函数将返回 falsetrue 否则。

现实。SetFunctionList即使对于有效的迭代器,该函数也可以返回 false。让我们找出原因。该 AddFunction 函数返回 fFunctions 列表中的有效迭代器数。也就是说,添加非空迭代器将导致列表的大小逐渐增长:1、2、3、4,等等。这是 Bug 发挥作用的地方:

ret &= AddFunction(*f);

由于函数返回类型 int 而不是 bool&= 因此该操作将返回 false 偶数值,因为偶数中最低显著位始终设置为零。这就是一个微妙的 bug 如何破坏 的返回值, SetFunctionsList 即使其参数有效cheeli.com.cn/wp-content/uploads/2019/10/image3.png”宽度=”311″/*

条件表达式中的错误

V501“&amp;”运算符的左侧和右侧有相同的子表达式:模块和模块 rootcling_impl.cxx 3650

virtual void HandleDiagnostic(....) override
{
  ....
  bool isROOTSystemModuleDiag = module && ....;
  bool isSystemModuleDiag = module && module && module->IsSystem;
  if (!isROOTSystemModuleDiag && !isSystemModuleDiag)
    fChild->HandleDiagnostic(DiagLevel, Info);
  ....
}

让我们从危害最小的 Bug 开始。模块指针被检查两次。其中一个检查可能是多余的,但修复它仍然是明智的,以避免任何混乱在未来。

V501在”|”运算符的左侧和右侧有相同的子表达式”strchr(fHostAuth->GetHost()、”+”)。T 验证.cxx 300

TAuthenticate::TAuthenticate(TSocket *sock, const char *remote,
                             const char *proto, const char *user)
{
  ....
  // If generic THostAuth (i.e. with wild card or user == any)
  // make a personalized memory copy of this THostAuth
  if (strchr(fHostAuth->GetHost(),'*') || strchr(fHostAuth->GetHost(),'*') ||
     fHostAuth->GetServer() == -1 ) {
    fHostAuth = new THostAuth(*fHostAuth);
    fHostAuth->SetHost(fqdn);
    fHostAuth->SetUser(checkUser);
    fHostAuth->SetServer(servtype);
  }
  ....
}

fHostAuth->GetHost() 字符串扫描 * 该字符两次。这些检查之一可能是为了查找 ? 字符,因为这两个字符通常是用于指定各种通配符掩码。

V517如果检测到 (A) =…] 模式,则使用 “如果 (A) =…] ” 模式。存在逻辑错误存在的可能性。检查线: 163, 165。防腐蒙森ML.cxx 163

Int_t TProofMonSenderML::SendSummary(TList *recs, const char *id)
{
  ....
  if (fSummaryVrs == 0) {
    if ((dsn = recs->FindObject("dataset"))) recs->Remove(dsn);
  } else if (fSummaryVrs == 0) {
    // Only the first records
    xrecs = new TList;
    xrecs->SetOwner(kFALSE);
    TIter nxr(recs);
    TObject *o = 0;
    while ((o = nxr())) {
       if (!strcmp(o->GetName(), "vmemmxw")) break;
       xrecs->Add(o);
    }
  }
  ....
}

变量 fSummaryVrs 被比较为零两次,因此执行永远不会到达其他 if 分支中的代码。那里有很多代码…

V523“then”语句等效于”else”语句。TKDTree.cxx 805

template <typename  Index, typename Value>
void TKDTree<Index, Value>::UpdateRange(....)
{
  ....
  if (point[fAxis[inode]]<=fValue[inode]){
    //first examine the node that contains the point
    UpdateRange(GetLeft(inode),point, range, res);
    UpdateRange(GetRight(inode),point, range, res);
  } else {
    UpdateRange(GetLeft(inode),point, range, res);
    UpdateRange(GetRight(inode),point, range, res);
  }
  ....
}

无论条件是什么,都执行相同的代码块(复制粘贴克隆)。我想词和词之间有一种混淆。

该项目充满了这样的可疑点:

  • V523 “然后”语句等效于”else”语句

cxx 51

  • V523 “then”语句等效于”else”语句。TWebFile.cxx 1310
  • V523 “then”语句等效于”else”语句。方法MLP.cxx 423
  • V523 “then”语句等效于”else”语句。RooAbsCategory.cxx 394
  • V547表达式”!文件_name_value.empty()”始终为 false。选择规则.cxx 1423

    bool SelectionRules::AreAllSelectionRulesUsed() const {
      for(auto&& rule : fClassSelectionRules){
        ....
        std::string file_name_value;
        if (!rule.GetAttributeValue("file_name", file_name_value))
         file_name_value.clear();
    
        if (!file_name_value.empty()) {                  // <=
          // don't complain about defined_in rules
          continue;
        }
    
        const char* attrName = nullptr;
        const char* attrVal = nullptr;
        if (!file_name_value.empty()) {                  // <=
          attrName = "file name";
          attrVal = file_name_value.c_str();
        } else {
          attrName = "class";
          if (!name.empty()) attrVal = name.c_str();
        }
        ROOT::TMetaUtils::Warning(0,"Unused %s rule: %s\n", attrName, attrVal);
      }
      ....
    }

    这可能不是一个错误;分析器刚刚发现了一些可以简化的代码。由于 的返回值 file_name_value.empty() 已在循环开始时进行了检查,因此可以删除第二个重复检查,从而丢弃大量不必要的代码。

    V590考虑检查”!file1|c <= 0 |c = “\”||c != ‘(” 表达式。表达式过多或包含打印错误。TTabCom.cxx 840

    TString TTabCom::DetermineClass(const char varName[])
    {
      ....
      c = file1.get();
      if (!file1 || c <= 0 || c == '*' || c != '(') {
        Error("TTabCom::DetermineClass", "variable \"%s\" not defined?",
            varName);
        goto cleanup;
      }
      ....
    }

    下面是分析器报告的条件表达式的问题部分:

    if (.... || c == '*' || c != '(') {
      ....
    }

    对星号字符的检查不会影响条件的结果。此部分对于 除了 以外的任何字符始终为 ( true。通过绘制真表,您可以轻松地自己检查它。

    关于具有奇怪逻辑的条件的两个警告:

    • V590 请考虑检查此表达式。表达式过多或包含打印错误。TFile.cxx 3963
    • V590 请考虑检查此表达式。表达式过多或包含打印错误。TStreamerInfo操作.cxx 3084

    V593请考虑查看”A = B;C”类型的表达式。表达式的计算如下:”A = (B < C)”。防腐服务.cxx 1903

    Int_t TProofServ::HandleSocketInput(TMessage *mess, Bool_t all)
    {
      ....
      if (Int_t ret = fProof->AddWorkers(workerList) < 0) {
        Error("HandleSocketInput:kPROOF_GETSLAVEINFO",
              "adding a list of worker nodes returned: %d", ret);
      }
      ....
    }

    此 Bug 仅在程序的错误行为的情况下才会显示自身。ret 变量应存储函数的返回代码, AddWorkers 并在出现错误情况时将该值写入日志。但它没有按预期工作。该条件缺少强制要求计算顺序的附加括号。ret 变量实际存储的不是返回代码,而是逻辑比较的结果(即 0 或 1)。

    另一个类似的问题:

    • V593 考虑查看”A = B < C”类型的表达式

    防腐服务.cxx 3897

    V768枚举常数”kCost复杂性”用作布尔型的变量。方法DT.cxx 283

    enum EPruneMethod {kExpectedErrorPruning=0, kCostComplexityPruning, kNoPruning};
    
    void TMVA::MethodDT::ProcessOptions()
    {
      ....
      if (fPruneStrength < 0) fAutomatic = kTRUE;
      else fAutomatic = kFALSE;
      if (fAutomatic && fPruneMethod==!DecisionTree::kCostComplexityPruning){
        Log() << kFATAL
              <<  "Sorry automatic pruning strength determination is ...." << Endl;
      }
      ....
    }

    嗯…为什么要否定常量 kCostComplexityPruning 值?我怀疑否定字符是一个拼写错误,现在扭曲了执行逻辑。

    指针处理错误

    V522可能会出现取消引用空指针”预”。TSynapse.cxx 61

    void TSynapse::SetPre(TNeuron * pre)
    {
      if (pre) {
        Error("SetPre","this synapse is already assigned to a pre-neuron.");
        return;
      }
      fpre = pre;
      pre->AddPost(this);
    }

    我尽了最大的努力去理解这个奇怪的代码,而且似乎这个想法是为了避免为 fpre 这个领域分配一个新的值。如果是这样,程序员会意外地检查错误的指针。如果将 nullptr 值传递给 SetPre 函数,则当前实现会导致取消引用空指针。

    我认为此代码段应固定如下:

    void TSynapse::SetPre(TNeuron * pre)
    {
      if (fpre) {
        Error("SetPre","this synapse is already assigned to a pre-neuron.");
        return;
      }
      fpre = pre;
      pre->AddPost(this);
    }

    但是,这不会阻止将空指针传递给函数,但至少此版本在逻辑上比原始版本更一致。

    此代码稍加修改的克隆可以在另一个位置找到:

    • V522 可能取消引用空指针”post”。TSynapse.cxx 74

    V595N指针在验证其针对空头之前已使用。检查线: 484, 488。扫描仪.cxx 484

    bool RScanner::shouldVisitDecl(clang::NamedDecl *D)
    {
       if (auto M = D->getOwningModule()) {                      // <= 2
          return fInterpreter.getSema().isModuleVisible(M);
       }
       return true;
    }
    
    bool RScanner::VisitNamespaceDecl(clang::NamespaceDecl* N)
    {
     if (fScanType == EScanType::kOnePCM)
      return true;
    
     if (!shouldVisitDecl(N))                                    // <= 1
      return true;
    
     if((N && N->isImplicit()) || !N){                           // <= 3
        return true;
     }
     ....
    }

    这是一个极其危险的代码!在 N 首次取消引用指针之前,不会检查该指针为 null。此外,您无法在此处看到它发生,因为取消引用发生在 shouldVisitDecl 函数内部。

    此诊断通常生成一组相关警告。下面是一些示例:

    • V595 “文件”指针在验证空头之前已使用。检查线: 141, 153。TFileCacheRead.cxx 141
    • V595 “fFree”指针在验证空头之前被使用。检查线: 2029, 2038.TFile.cxx 2029
    • V595 “tbuf”指针在验证空头之前被使用

    TGText.cxx 586

  • V595 “fPlayer”指针在验证空头之前已使用。检查线: 3425, 3430。TProof.cxx 3425
  • V595 “gProofServ”指针在验证空头之前被使用。检查线: 1192, 1194。TProofPlayer.cxx 1192
  • V595 “projDataTmp”指针在验证空头之前被使用。检查线: 791, 804。Roo同步.cxx 791
  • 下一个不是 bug,但它是宏如何鼓励编写错误代码或冗余代码的另一个示例。

    V571重复检查。”if (fCanvasImp)”条件已在行 799 中验证。TCanvas.cxx 800

    #define SafeDelete(p) { if (p) { delete p; p = 0; } }
    
    void TCanvas::Close(Option_t *option)
    {
      ....
      if (fCanvasImp)
        SafeDelete(fCanvasImp);
      ....
    }

    fCanvasImp 指针被检查两次,其中一个检查已在 SafeDelete 宏中实现。宏的一个问题是它们很难从代码内部导航,这就是为什么许多程序员在使用之前不检查其内容的原因。

    阵列处理错误

    V519“线[Cursor]”变量连续分配两次值。也许这是个错误。检查线: 352, 353。编者按353

    size_t find_last_non_alnum(const std::string &str,
                                 std::string::size_type index = std::string::npos) {
      ....
      char tmp = Line.GetText()[Cursor];
      Line[Cursor] = Line[Cursor - 1];
      Line[Cursor] = tmp;
      ....
    }

    为元素 Line[Cursor] 分配一个新值,然后立即覆盖该值。看起来不对…

    V557阵列溢出是可能的。”ivar”索引指向超出数组绑定的范围。基本最小化.cxx 130

    bool BasicMinimizer::SetVariableValue(unsigned int ivar, double val) {
      if (ivar > fValues.size() ) return false;
      fValues[ivar] = val;
      return true;
    }

    在检查数组索引时犯此错误是最近的趋势;我们几乎每三个项目就看到它。虽然在循环中索引到数组很容易 – 通常使用 < 运算符将索引与数组的大小进行比较 – 如上所示的检查需要 >= 运算符,而不是 > 。否则,可能会为超出数组绑定的一个元素编制索引。

    此 Bug 在整个代码中克隆了几次:

    • V557 阵列溢出是可能的。”ivar”索引指向超出数组绑定的范围。基本最小化.cxx 186
    • V557 阵列溢出是可能的。”ivar”索引指向超出数组绑定的范围。基本最小化.cxx 194
    • V557 阵列溢出是可能的。”ivar”索引指向超出数组绑定的范围。基本最小化.cxx 209
    • V557 阵列溢出是可能的。”ivar”索引指向超出数组绑定的范围。基本最小化.cxx 215
    • V557 阵列溢出是可能的。”ivar”索引指向超出数组绑定的范围。基本最小化.cxx 230

    V621请考虑检查”for”运算符。循环可能会执行不正确或根本不执行。TDataMEMBER.cxx 554

    Int_t TDataMember::GetArrayDim() const
    {
     if (fArrayDim<0 && fInfo) {
        R__LOCKGUARD(gInterpreterMutex);
        TDataMember *dm = const_cast<TDataMember*>(this);
        dm->fArrayDim = gCling->DataMemberInfo_ArrayDim(fInfo);
        // fArrayMaxIndex should be zero
        if (dm->fArrayDim) {
           dm->fArrayMaxIndex = new Int_t[fArrayDim];
           for(Int_t dim = 0; dim < fArrayDim; ++dim) {
              dm->fArrayMaxIndex[dim] = gCling->DataMemberInfo_MaxIndex(fInfo,dim);
           }
        }
     }
     return fArrayDim;
    }

    在 for 循环中,开发人员显然要比较 dim dm->fArrayDim 变量,而不是fArrayDim因此,此循环永远不会执行。

    V767通过循环内的常量索引对”当前”数组元素的可疑访问。TClingUtils.cxx 3082

    llvm::StringRef ROOT::TMetaUtils::DataMemberInfo__ValidArrayIndex(....)
    {
      ....
      while (current!=0) {
        // Check the token
        if (isdigit(current[0])) {
           for(i=0;i<strlen(current);i++) {
              if (!isdigit(current[0])) {
                 if (errstr) *errstr = current;
                 if (errnum) *errnum = NOT_INT;
                 return llvm::StringRef();
              }
           }
        } else { // current token is not a digit
          ....
        }
        ....
      }
      ....
    

    此代码正在分析和检查字符串。如果当前字符串的第一个字符(即索引 0)已识别为数字,则循环将遍历所有其余字符,以确保所有字符都是数字。至少是这样问题是,在循环中不使用 i 计数器。应重写条件,以便检查当前 [i] 而不是当前 {0}。

    内存泄漏

    V773退出函数时没有释放”选项列表”指针。内存泄漏是可能的。TDataMEMBER.cxx 355

    void TDataMember::Init(bool afterReading)
    {
      ....
      TList *optionlist = new TList();       //storage for options strings
    
      for (i=0;i<token_cnt;i++) {
         if (strstr(tokens[i],"Items")) {
            ptr1 = R__STRTOK_R(tokens[i], "()", &rest);
            if (ptr1 == 0) {
               Fatal("TDataMember","Internal error, found \"Items....",GetTitle());
               return;
            }
            ptr1 = R__STRTOK_R(nullptr, "()", &rest);
            if (ptr1 == 0) {
               Fatal("TDataMember","Internal error, found \"Items....",GetTitle());
               return;
            }
            ....
         }
         ....
      }
      ....
      // dispose of temporary option list...
      delete optionlist;
      ....
    }

    optionList在从函数返回之前,不会释放指针。我不知道这种释放在这种特定情况下是否必要,但是当我们报告这样的错误时,开发人员通常会修复它们。这完全取决于您是否希望程序在出现错误情况时继续运行。ROOT 有很多这样的缺陷,因此我建议作者自己重新检查项目。

    梅姆塞特再次

    V597编译器可以删除”memset”函数调用,该函数调用用于刷新”x”缓冲区。应使用 memset_s() 函数擦除私有数据。TMD5.cxx 366

    void TMD5::Transform(UInt_t buf[4], const UChar_t in[64])
    {
      UInt_t a, b, c, d, x[16];
      ....
      // Zero out sensitive information
      memset(x, 0, sizeof(x));
    }

    许多人认为,在编译后,注释不会进入二进制文件,并且它们绝对正确:D。有些人可能不知道的是,编译器也将删除 memset 函数。这肯定会发生。如果有问题的缓冲区不再在代码中进一步使用,编译器将优化函数调用。从技术上讲,这是一个合理的决定,但如果缓冲区存储任何私有数据,则该数据将保持不变。这是一个典型的安全漏洞CWE-14。

    杂项

    V591非 void 函数应返回一个值。日志纳森格FCN.h 108

    LogLikelihoodFCN & operator = (const LogLikelihoodFCN & rhs) {
      SetData(rhs

    模型功能Ptr() ;
    fNeffPoints = rhs.fNeffpoints;
    fGrad = rhs.fGrad;
    fIs 扩展 = rhs.fIs 扩展;
    f体重 = rhs.f 重量;
    f 执行策略 = rhs.f 执行策略;
    }

    重载运算符没有返回值。这是最近的另一个趋势。

    V596对象已创建,但未使用。”throw”关键字可能丢失:引发运行时错误 (FOO);RTensor.hxx 363

    template <typename Value_t, typename Container_t>
    inline RTensor<Value_t, Container_t> RTensor<Value_t, Container_t>::Transpose()
    {
      if (fLayout == MemoryLayout::RowMajor) {
        fLayout = MemoryLayout::ColumnMajor;
      } else if (fLayout == MemoryLayout::ColumnMajor) {
        fLayout = MemoryLayout::RowMajor;
      } else {
        std::runtime_error("Memory layout is not known.");
      }
      ....
    }

    问题是程序员不小心遗漏了 throw 关键字,从而防止在出现错误的情况下引发异常。

    只有两个此类型的警告。下面是第二个:

    • V596 对象已创建,但未使用。”throw”关键字可能丢失:引发运行时错误 (FOO);森林.hxx 137。

    V609除以零。分母范围 [0..100]。TGHtmlImage.cxx 340

    const char *TGHtml::GetPctWidth(TGHtmlElement *p, char *opt, char *ret)
    {
      int n, m, val;
      ....
      if (n < 0 || n > 100) return z;
      if (opt[0] == 'h') {
        val = fCanvas->GetHeight() * 100;
      } else {
        val = fCanvas->GetWidth() * 100;
      }
      if (!fInTd) {
        snprintf(ret, 15, "%d", val / n);  // <=
      } else {
        ....
      }
      ....
    }

    这一个类似于前面讨论的数组处理示例。n 变量限制为从 0 到 100 的范围。但是,有一个分支执行除除 n 变量,其值可能为 0。我认为 n 的范围限制应固定如下:

    if (n <= 0 || n > 100) return z;

    V646请考虑检查应用程序的逻辑。可能缺少”else”关键字。防腐服务.cxx 729

    TProofServ::TProofServ(Int_t *argc, char **argv, FILE *flog)
           : TApplication("proofserv", argc, argv, 0, -1)
    {
      ....
      if (!logmx.IsDigit()) {
        if (logmx.EndsWith("K")) {
          xf = 1024;
          logmx.Remove(TString::kTrailing, 'K');
        } else if (logmx.EndsWith("M")) {
          xf = 1024*1024;
          logmx.Remove(TString::kTrailing, 'M');
        } if (logmx.EndsWith("G")) {
          xf = 1024*1024*1024;
          logmx.Remove(TString::kTrailing, 'G');
        }
      }
      ....
    }

    分析器报告具有缺少 else 关键字的格式奇怪的 if 语句。此代码的外观表明它确实需要修复。

    此类型的几个警告:

    • V646 请考虑检查应用程序的逻辑。可能缺少”else”关键字。TFormula_v5.cxx 3702
    • V646 请考虑检查应用程序的逻辑。可能缺少”else”关键字。RooAbsCategory.cxx 604

    V663无限循环是可能的。”cin.eof()”条件不足以脱离循环。请考虑将”cin.fail()”函数调用添加到条件表达式。方法KNN.cxx 602

    void TMVA::MethodKNN::ReadWeightsFromStream(std::istream& is)
    {
      ....
      while (!is.eof()) {
        std::string line;
        std::getline(is, line);
    
        if (line.empty() || line


    }
    ….
    }

    使用 std::istream 类时,调用 eof() 函数不足以终止循环。eof()如果无法读取数据,并且此代码中没有其他终止点,则函数将始终返回 false。为了保证循环的终止,需要对函数返回的值进行额外 fail() 检查:

    while (!is.eof() && !is.fail())
    { 
    ....
    }

    作为替代方法,可以重写如下:

    while (is)
    { 
    ....
    }

    V678对象用作其自己方法的参数。请考虑检查”复制”函数的第一个实际参数。TForm叶信息.cxx 2414

    TFormLeafInfoMultiVarDim::TFormLeafInfoMultiVarDim(
      const TFormLeafInfoMultiVarDim& orig) : TFormLeafInfo(orig)
    {
       fNsize = orig.fNsize;
       fSizes.Copy(fSizes);   // <=
       fCounter2 = orig.fCounter2?orig.fCounter2->DeepCopy():0;
       fSumOfSizes = orig.fSumOfSizes;
       fDim = orig.fDim;
       fVirtDim = orig.fVirtDim;
       fPrimaryIndex = orig.fPrimaryIndex;
       fSecondaryIndex = orig.fSecondaryIndex;
    }

    让我们用这个漂亮的小拼写错误来结束这篇文章。复制函数应用 调用 orig.fSizes ,而不是 fSizes

    结论

    大约一年前,我们检查了NCBI基因组工作台项目,这是用于科学研究的另一个涉及基因组分析的项目。我之所以提到这一点,是因为科学软件的质量至关重要,但开发人员往往低估了它。

    顺便说一下,macOS 10.15 Catalina 在前几天发布,他们停止支持 32 位应用程序。幸运的是,PVS-Studio 提供了一组专为检测程序移植到 64 位系统时附带的错误而设计的大量诊断。PVS-Studio 团队在这篇文章中了解更多信息。

    进一步阅读

    Comments are closed.