在斯德哥尔摩举办第118届诺贝尔周时，我坐在我们的办公室里，在那里我们开发了PVS-Studio静态分析仪，对ROOT项目（一个用于科学研究的大数据处理框架）进行分析审查。当然，这个代码不会赢得奖品，但作者绝对可以指望对最有趣的缺陷的详细审查，再加上一个免费的许可证来彻底检查项目本身。
介绍
ROOT是一个模块化的科学软件工具包。它提供了处理大数据处理、统计分析、可视化和存储所需的所有功能。它主要写在C++。ROOT诞生于欧洲核子研究中心，是高能物理研究的核心。每天，成千上万的物理学家使用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 的测试版找到，我正用它来进行此审查。
期望值。函数 SetFunctionList 遍历迭代器列表。如果至少有一个迭代器无效，则函数将返回 false ， true 否则。
现实。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“&”运算符的左侧和右侧有相同的子表达式：模块和模块 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
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
下一个不是 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 团队在这篇文章中了解更多信息。