Geekly articles weekly

分析根代码，科学数据分析框架

图片3
在斯德哥尔摩举办第118届诺贝尔周的时候，我坐在办公室里，我们在那儿开发了PVS-Studio静态分析仪，对ROOT项目（科学研究中使用的大数据处理框架）进行分析审查。 当然，该代码不会赢得任何奖励，但是作者绝对可以指望对最有趣的缺陷进行详细的审查，再加上免费的许可证，可以自己对项目进行彻底检查。

引言


图片1

ROOT是模块化的科学软件工具包。 它提供了处理大数据处理，统计分析，可视化和存储所需的所有功能。 它主要是用C ++编写的。 ROOT出生于CERN ，是高能物理研究的核心。 每天，成千上万的物理学家使用ROOT应用程序分析其数据或执行模拟。

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

新型诊断仪的首次亮相


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 beta版发现的一个奇妙错误。

期望值 SetFunctionList函数遍历迭代器列表。 如果至少一个迭代器无效，则该函数返回false ，否则返回true 。

现实性 即使对于有效的迭代器， SetFunctionList函数也可以返回false 。 让我们找出原因。 AddFunction函数返回fFunctions列表上有效迭代器的数量。 也就是说，添加非null迭代器将导致列表的大小逐渐增加：1、2、3、4，依此类推。 这是该错误起作用的地方：

 ret &= AddFunction(*f);

由于该函数返回的是int类型而不是bool的值 ，因此'＆='操作将对偶数返回false ，因为偶数的最低有效位始终设置为零。 这是一个微妙的错误可以破坏SetFunctionsList的返回值的方式，即使其参数有效。

图片2

条件表达式中的错误


V501 '&&'运算符的左侧和右侧有相同的子表达式：module && module rootcling_impl.cxx 3650

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

让我们从危害最小的错误开始。 模块指针检查两次。 其中一项检查可能是多余的，但对其进行修复以避免将来出现任何混乱仍然是明智的。

V501在'||'的左侧和右侧有相同的子表达式'strchr（fHostAuth-> GetHost（），'*'）' 操作员。 TAuthenticate.cxx 300

 TAuthenticate::TAuthenticate(TSocket *sock, const char *remote, const char *proto, const char *user) { .... // If generic THostAuth (ie 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检测到使用'if（A）{...} else if（A）{...}'模式。 存在逻辑错误的可能性。 检查行：163，165。TProofMonSenderML.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变量与零进行两次比较，因此执行永远不会到达else-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'then'语句等效于'else'语句。 TContainerConverters.cxx 51
  • V523'then'语句等效于'else'语句。 TWebFile.cxx 1310
  • V523'then'语句等效于'else'语句。 方法MLP.cxx 423
  • V523'then'语句等效于'else'语句。 RooAbsCategory.cxx 394

V547表达式'！File_name_value.empty（）'始终为false。 SelectionRules.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 != '(') { .... }

星号字符的检查不会影响条件的结果。 这部分对于'（'）以外的其他任何字符都是正确的。您可以通过绘制真值表轻松地自己检查一下。

关于带有奇怪逻辑的条件的另外两个警告：

  • V590考虑检查此表达式。 表达式过多或打印错误。 TFile.cxx 3963
  • V590考虑检查此表达式。 表达式过多或打印错误。 TStreamerInfoActions.cxx 3084

V593考虑查看“ A = B <C”类型的表达式。 该表达式的计算公式如下：“ A =（B <C）”。 TProofServ.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); } .... }

仅当程序出现错误时，此错误才会显示出来。 ret变量应该存储AddWorkers函数的返回代码，并在出现错误情况时将该值写入日志。 但是它没有按预期工作。 该条件缺少附加的括号，无法强制执行所需的评估顺序。 ret变量实际存储的不是返回码，而是逻辑比较的结果，即0或1。

另一个类似的问题：

  • V593考虑查看“ A = B <C”类型的表达式。 该表达式的计算公式如下：“ A =（B <C）”。 TProofServ.cxx 3897

V768枚举常量'kCostComplexityPruning'用作布尔型变量。 方法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可能会取消引用空指针“ pre”。 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

V595在针对nullptr进行验证之前，已使用了'N'指针。 检查行：484、488。Scanner.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指针在第一次被取消引用之前不会被检查为空。 而且，由于取消引用发生在shouldVisitDecl函数内，因此您看不到它在这里发生。

传统上，此诊断会生成一堆相关的警告。 这里只是几个例子：

  • V595在针对nullptr进行验证之前，已使用“文件”指针。 检查行：141、153。TFileCacheRead.cxx 141
  • V595在对nullptr进行验证之前，已使用了“ fFree”指针。 检查行：2029、2038。TFile.cxx 2029
  • V595在针对nullptr对其进行验证之前，已使用了“ tbuf”指针。 检查行：586、591。TGText.cxx 586
  • V595在针对nullptr对其进行验证之前，已使用了“ fPlayer”指针。 检查行：3425、3430。TProof.cxx 3425
  • V595在针对nullptr对其进行验证之前，已使用了'gProofServ'指针。 检查行：1192、1194。TProofPlayer.cxx 1192
  • V595在针对nullptr对其进行验证之前，已使用了'projDataTmp'指针。 检查行：791、804。RooSimultaneous.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 '行[光标]'变量连续两次被赋值。 也许这是一个错误。 检查行：352，353。Editor.cpp 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; .... }

元素行[Cursor]被分配了一个新值，然后立即被覆盖。 看起来不对劲...

V557阵列可能超限。 “ ivar”索引指向数组界限之外。 BasicMinimizer.cxx 130

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

最近在检查数组索引时犯了这个错误。 我们几乎在每三个项目中都看到它。 虽然在循环内索引到数组很容易-您通常使用'<'运算符将索引与数组的大小进行比较-上面显示的检查要求使用'> ='运算符，而不是'>'。 否则，您可能要为超出数组范围的一个元素建立索引。

此错误已在整个代码中多次克隆：

  • V557阵列可能超限。 “ ivar”索引指向数组界限之外。 BasicMinimizer.cxx 186
  • V557阵列可能超限。 “ ivar”索引指向数组界限之外。 BasicMinimizer.cxx 194
  • V557阵列可能超限。 “ ivar”索引指向数组界限之外。 BasicMinimizer.cxx 209
  • V557阵列可能超限。 “ ivar”索引指向数组界限之外。 BasicMinimizer.cxx 215
  • V557阵列可能超限。 “ ivar”索引指向数组界限之外。 BasicMinimizer.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进行比较 。 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] 。

图片4

内存泄漏


V773在不释放'optionlist'指针的情况下退出了该函数。 可能发生内存泄漏。 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非无效函数应返回一个值。 LogLikelihoodFCN.h 108

 LogLikelihoodFCN & operator = (const LogLikelihoodFCN & rhs) { SetData(rhs.DataPtr() ); SetModelFunction(rhs.ModelFunctionPtr() ); fNEffPoints = rhs.fNEffPoints; fGrad = rhs.fGrad; fIsExtended = rhs.fIsExtended; fWeight = rhs.fWeight; fExecutionPolicy = rhs.fExecutionPolicy; }

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

V596对象已创建，但未使用。 可能没有'throw'关键字：throw runtime_error（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'关键字：throw runtime_error（FOO）; 第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”关键字。 TProofServ.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.find("#") != std::string::npos) { continue; } .... } .... }

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

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

或者，可以将其重写如下：

 while (is) { .... }

V678对象被用作其自身方法的参数。 考虑检查“复制”功能的第一个实际参数。 TFormLeafInfo.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调用Copy函数。

结论


大约一年前，我们检查了NCBI Genome Workbench项目，这是科学研究中用于处理基因组分析的另一个程序。 我之所以这样说是因为科学软件的质量至关重要，但是开发人员往往会低估它。

顺便说一句，macOS 10.15 Catalina是在前一天发布的，他们停止了对32位应用程序的支持。 幸运的是，PVS-Studio提供了大量诊断程序，这些诊断程序专门用于检测与程序移植到64位系统有关的错误。 在PVS-Studio团队的这篇文章中了解更多信息 。

Source: https://habr.com/ru/post/zh-CN472492/

More articles:


All Articles