Анализ кода ROOT — фреймворка для анализа данных научных исследований22.10.2019 10:18

Пока в Стокгольме проходила 118-я Нобелевская неделя, в офисе разработки статического анализатора кода PVS-Studio готовился обзор кода проекта ROOT, используемого в научных исследованиях для обработки больших данных. Премию за такой код, конечно, не дашь, а вот подробный обзор интересных дефектов кода и лицензию для полной проверки проекта разработчики получат.

Введение

ROOT — набор утилит для работы с данными научных исследований. Он обеспечивает все функциональные возможности, необходимые для обработки больших данных, статистического анализа, визуализации и хранения. В основном написан на языке C++. Разработка началась в CERN (Европейская организация по ядерным исследованиям) для исследований по физике высоких энергий. Каждый день тысячи физиков используют ROOT-приложения для анализа своих данных или для моделирования.
PVS-Studio — это инструмент для выявления ошибок и потенциальных уязвимостей в исходном коде программ, написанных на языках С, C++, C# и Java. Работает в 64-битных системах на Windows, Linux и macOS и может анализировать код, предназначенный для 32-битных, 64-битных и встраиваемых ARM платформ.

Дебют новой диагностики

V1046 Unsafe usage of the bool' and 'int' types together in the operation '&='. 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
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;
}

Бета-версия анализатора, которая использовалась при проверке, нашла вот такую потрясающую ошибку.

Ожидание. Функция SetFunctionList обходит список итераторов. Если хоть один из них будет невалидным, то возвращаемое значение будет false, иначе true.

Реальность. Функция SetFunctionList может возвращать значение false даже для валидных итераторов. Разберёмся в ситуации.Функция AddFunction возвращает количество валидных итераторов в списке fFunctions. Т.е. при добавлении ненулевых итераторов, размер этого списка будет последовательно увеличиваться: 1, 2, 3, 4 и т.д. Вот тут и начнёт проявлять себя ошибка в коде:

ret &= AddFunction(*f);

Т.к. функция возвращает результат типа int, а не bool, то операция '&=' с чётными числами будет давать значение false. Ведь младший бит чётных чисел всегда будет равен нулю. Следовательно, такая неочевидная ошибка будет портить результат функции SetFunctionsList даже для валидных аргументов.

Ошибки в условных выражениях

V501 There are identical sub-expressions to the left and to the right of the '&&' operator: 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);
  ....
}

Начнём с самого безобидного примера. Указатель module проверяется два раза. Скорее всего, одна проверка лишняя. Но код лучше исправить, чтобы не возникало лишних вопросов.

V501 There are identical sub-expressions 'strchr (fHostAuth→GetHost (), '*')' to the left and to the right of the '||' operator. TAuthenticate.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 The use of 'if (A) {…} else if (A) {…}' pattern was detected. There is a probability of logical error presence. Check lines: 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 The 'then' statement is equivalent to the 'else' statement. TKDTree.cxx 805

template 
void TKDTree::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);
  }
  ....
}

Один и тот же copy-paste-код выполняется при любом условии. Возможно, есть опечатка в словах left и right.

Подобного подозрительного кода в проекте немало:

• V523 The 'then' statement is equivalent to the 'else' statement. TContainerConverters.cxx 51
• V523 The 'then' statement is equivalent to the 'else' statement. TWebFile.cxx 1310
• V523 The 'then' statement is equivalent to the 'else' statement. MethodMLP.cxx 423
• V523 The 'then' statement is equivalent to the 'else' statement. RooAbsCategory.cxx 394

V547 Expression '! file_name_value.empty ()' is always 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 Consider inspecting the '! file1 || c <= 0 || c == '*' || c != '('' expression. The expression is excessive or contains a misprint. 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 Consider inspecting this expression. The expression is excessive or contains a misprint. TFile.cxx 3963
• V590 Consider inspecting this expression. The expression is excessive or contains a misprint. TStreamerInfoActions.cxx 3084

V593 Consider reviewing the expression of the 'A = B

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 Consider reviewing the expression of the 'A = B < C' kind. The expression is calculated as following: 'A = (B < C)'. TProofServ.cxx 3897

V768 The enumeration constant 'kCostComplexityPruning' is used as a variable of a Boolean-type. MethodDT.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 Dereferencing of the null pointer 'pre' might take place. 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 Dereferencing of the null pointer 'post' might take place. TSynapse.cxx 74

V595 The 'N' pointer was utilized before it was verified against nullptr. Check lines: 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 The 'file' pointer was utilized before it was verified against nullptr. Check lines: 141, 153. TFileCacheRead.cxx 141
• V595 The 'fFree' pointer was utilized before it was verified against nullptr. Check lines: 2029, 2038. TFile.cxx 2029
• V595 The 'tbuf' pointer was utilized before it was verified against nullptr. Check lines: 586, 591. TGText.cxx 586
• V595 The 'fPlayer' pointer was utilized before it was verified against nullptr. Check lines: 3425, 3430. TProof.cxx 3425
• V595 The 'gProofServ' pointer was utilized before it was verified against nullptr. Check lines: 1192, 1194. TProofPlayer.cxx 1192
• V595 The 'projDataTmp' pointer was utilized before it was verified against nullptr. Check lines: 791, 804. RooSimultaneous.cxx 791

Следующий пример не является ошибкой, но в очередной раз демонстрирует, что макросы поощряют написание неправильного или избыточного кода.

V571 Recurring check. The 'if (fCanvasImp)' condition was already verified in line 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 The 'Line[Cursor]' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 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;
  ....
}

Новое значение элемента Line[Cursor] сразу же перезаписывается. Что-то здесь не так…

V557 Array overrun is possible. The 'ivar' index is pointing beyond array bound. BasicMinimizer.cxx 130

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

Так ошибаться в проверке индекса массива — просто массовая проблема в последнее время. Чуть ли не в каждом третьем проекте встречается. Если при индексации массива в цикле всё просто — почти всегда используется оператор '=', а не '>'. Иначе возможен выход за границу массива на один элемент.

Эту ошибку расплодили по файлу несколько раз:

• V557 Array overrun is possible. The 'ivar' index is pointing beyond array bound. BasicMinimizer.cxx 186
• V557 Array overrun is possible. The 'ivar' index is pointing beyond array bound. BasicMinimizer.cxx 194
• V557 Array overrun is possible. The 'ivar' index is pointing beyond array bound. BasicMinimizer.cxx 209
• V557 Array overrun is possible. The 'ivar' index is pointing beyond array bound. BasicMinimizer.cxx 215
• V557 Array overrun is possible. The 'ivar' index is pointing beyond array bound. BasicMinimizer.cxx 230

V621 Consider inspecting the 'for' operator. It’s possible that the loop will be executed incorrectly or won’t be executed at all. TDataMember.cxx 554

Int_t TDataMember::GetArrayDim() const
{
 if (fArrayDim<0 && fInfo) {
    R__LOCKGUARD(gInterpreterMutex);
    TDataMember *dm = const_cast(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 Suspicious access to element of 'current' array by a constant index inside a loop. TClingUtils.cxx 3082

llvm::StringRef ROOT::TMetaUtils::DataMemberInfo__ValidArrayIndex(....)
{
  ....
  while (current!=0) {
    // Check the token
    if (isdigit(current[0])) {
       for(i=0;i

Этот фрагмент кода парсит некую строку и проверяет её корректность. После того, как нулевой символ строки current определяется как число, выполняется обход всех остальных символов, чтобы убедиться, что до конца строки все символы — числа. Ну как выполняется… счётчик цикла i не используется в цикле. Надо было написать current[i], а не current[0] в условии.

Утечка памяти

V773 The function was exited without releasing the 'optionlist' pointer. A memory leak is possible. TDataMember.cxx 355

void TDataMember::Init(bool afterReading)
{
  ....
  TList *optionlist = new TList();       //storage for options strings

  for (i=0;i

Во время выхода из функции не предусмотрено освобождение памяти по указателю optionList. Нужно ли это в данном конкретном случае — мне сложно сказать. Но обычно такие ошибки исправляют в проектах по отчётам PVS-Studio. Всё зависит от того, должна ли программа пытаться продолжить работать в нештатной ситуации или нет. Таких предупреждений в проекте много, разработчикам лучше перепроверить проект самостоятельно и посмотреть полный отчёт анализатора.

Снова про функцию memset

V597 The compiler could delete the 'memset' function call, which is used to flush 'x' buffer. The memset_s () function should be used to erase the private data. 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 Non-void function should return a value. 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 The object was created but it is not being used. The 'throw' keyword could be missing: throw runtime_error (FOO); RTensor.hxx 363

template 
inline RTensor RTensor::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 The object was created but it is not being used. The 'throw' keyword could be missing: throw runtime_error (FOO); Forest.hxx 137

V609 Divide by zero. Denominator range [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 Consider inspecting the application’s logic. It’s possible that 'else' keyword is missing. 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. По коду можно предположить, что код действительно стоит исправить.

И ещё пару мест исправить заодно:

• V646 Consider inspecting the application’s logic. It’s possible that 'else' keyword is missing. TFormula_v5.cxx 3702
• V646 Consider inspecting the application’s logic. It’s possible that 'else' keyword is missing. RooAbsCategory.cxx 604

V663 Infinite loop is possible. The 'cin.eof ()' condition is insufficient to break from the loop. Consider adding the 'cin.fail ()' function call to the conditional expression. MethodKNN.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 An object is used as an argument to its own method. Consider checking the first actual argument of the 'Copy' function. 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;
}

Напоследок вот такая опечаточка. Вместо fSizes надо было передать orig.fSizes в функцию Copy.

Заключение

Примерно год назад делался обзор кода проекта NCBI Genome Workbench. Этот проект тоже используется в научных исследованиях, но генома. К чему я веду, программное обеспечение в этой сфере очень важно, но его качеству не уделяют должного внимания.

Кстати, на днях вышла macOS 10.15 Catalina, в которой отказались от поддержки 32-х битных приложений. И в PVS-Studio есть большой набор правил для выявления проблем при портировании программ на 64-х битные системы. Подробнее об этом в посте блога анализатора.

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Svyatoslav Razmyslov. Analyzing the Code of ROOT, Scientific Data Analysis Framework.

© Habrahabr.ru