PVS-Studio спешит на помощь CERN: проверка проекта Geant4

• Введение
• Пикантность
• Пропущенный 'else'
• Некорректная обработка возможной ошибки
• Разыменование нулевого указателя
• Использование нулевого указателя
• Неверная побитовая операция
• Лишнее присваивание
• Множественная проверка одного выражения
• Неаккуратный рефакторинг
• Уже проверенное выражение
• Бесполезное условие
• Избыточное условие
• Неверный вызов конструктора
• Опечатка при инициализации
• Забытый throw
• Ошибка вывода
• Заключение

Проект Geant4 продолжает развиваться, поэтому интересно вновь проверить его с помощью статического анализатора кода PVS-Studio. На этот раз проверке будет подвергнута версия 10.2 (предыдущая проверка относилась к версии 10.0-beta).

Введение

Инструментарий Geant4 разработан в CERN и предназначен для моделирования и исследования поведения частиц при прохождении через вещество, с использованием методов Монте-Карло. Ранние версии проекта была написаны на языке Fortran, а начиная с 4 версии проект был полностью переведён на объектно-ориентированный язык С++.

Более подробно о проекте можно узнать на официальном сайте проекта http://geant4.org.

Пару раз проект Geant4 уже проверялся, и о результатах рассказывают другие статьи. О проверке версии 9.4 можно прочитать в статье "Copy-Paste и мюоны", а о проверке версии 10.0-beta в статье "Продолжение проверки Geant4".

Со времени последней проверки была выпущена новая версия Geant4.10.2. PVS-Studio также обновился с того времени, и для проверки была использована версия анализатора 6.05.

В проекте мне попалось достаточно много ошибок, связанных с условиями и сравнениями. Логические ошибки чаще всего совершаются при оставлении кода на будущую доработку, или же неаккуратной модификации с удалением предыдущих, контролирующих переходы, частей кода. Но и простые опечатки и недостаточная продуманность выражений так же могут привести к ошибкам или избыточному коду.

Пикантность

Дополнительную пикантность очередной проверке проекта придаёт то, что проект Geant4, как я понимаю, регулярно проверяется статическим анализатором Coverity. Об этом свидетельствует множественные записи ReleaseNotes и комментарии в коде, вида:

// Private copy constructor and assigment operator - copying and
// assignment not allowed.  Keeps Coverity happy.

Анализатор Coverity считается лидером рынка, поэтому найти что-то после него является хорошим достижением. И тем не менее PVS-Studio нашел массу интересных ошибок, что говорит, что он тоже стал зрелым и мощным продуктом.

Пропущенный 'else'

G4double G4EmBiasingManager::ApplySecondaryBiasing(....)
{
  ....
  if(0 == nsplit) { 
    ....
  } if(1 == nsplit) { // <=
    ....
  } else {
    ....
  }
  ....
}

V646 Consider inspecting the application's logic. It's possible that 'else' keyword is missing. g4embiasingmanager.cc 299

Это одна из частых ошибок при работе с проверкой нескольких значений одной переменой с помощью if. Конечно это может быть просто неверное форматирование, но в этом примере анализатор скорее всего указывает именно на ошибку.

В результате копирования кода оказалось забыто слово else. Что в данном случае приведёт к выполнению лишнего кода. Например, значение будет равно нулю и будет выполнен код из советующего блока, но из-за ошибки также будет выполнен код блока else после проверки на равенство единице. Для исправления ошибки надо добавить пропущенное else перед условием if(1 == nsplit).

Некорректная обработка возможной ошибки

void G4GenericPolycone::Create( .... )
{
  ....
  G4double rzArea = rz->Area();
  if (rzArea < -kCarTolerance)
    rz->ReverseOrder();

  else if (rzArea < -kCarTolerance)   // <=
  {
    ....
    G4Exception("G4GenericPolycone::Create()", 
                "GeomSolids0002",
                FatalErrorInArgument, message);
  }
  ....
}

V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 102, 105. g4genericpolycone.cc 102

Могу лишь предполагать о назначении кода. Весьма вероятно, что этот фрагмент отвечает за перехват и формирование сообщения об ошибке, но в результате неверного условия сообщения об ошибке не будет. И неизвестно, как потом поведёт себя программа. Возможно, ошибку перехватит обработчик в другом месте, а может быть и такое, что программа продолжит выполнение без ошибки и выдаст некорректный результат работы. Точно сказать, в чём ошибка, достаточно сложно, так как она может быть как в одном из условных выражений, так и в лишнем else. Но судя по форматированию, можно смело предполагать, что оба блока условия верные и для исправления достаточно убрать else перед вторым условным блоком.

Благодаря копированию кода эта же ошибка была размножена и обнаруживается ещё в трёх местах проекта:

• V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 193, 196. g4polycone.cc 193
• V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 219, 222. g4polyhedra.cc 219
• V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 207, 211. g4persistencycentermessenger.cc 207

Разыменование нулевого указателя

G4double * theShells;
G4double * theGammas;

void G4ParticleHPPhotonDist::InitAngular(....)
{
 ....
 if ( theGammas != NULL ) 
 {
   for ( i = 0 ; i < nDiscrete ; i++ )
   {
     vct_gammas_par.push_back( theGammas[ i ] );
     vct_shells_par.push_back( theShells[ i ] );
     ....
   }
 }
 if ( theGammas == NULL ) theGammas = new G4double[nDiscrete2];
 if ( theShells == NULL ) theShells = new G4double[nDiscrete2];
 .... 
}

V595 The 'theShells' pointer was utilized before it was verified against nullptr. Check lines: 147, 156. g4particlehpphotondist.cc 147

Ошибки при работе с указателями встречаются в программах достаточно часто. Здесь приведён случай, когда происходит одновременная работа с двумя объектами, а на корректность проверяется только один из них. Такая ошибка может долго оставаться незамеченной, но в случае, если указатель на объект theShells окажется нулевым, это приведет к неопределённому поведению программы. Для исправления ошибки требуется изменить условие следующим образом:

if ( theGammas != NULL && theShells != NULL) ....

Ещё один фрагмент с отсутствием проверки указателя:

• V595 The 'fCurrentProcess' pointer was utilized before it was verified against nullptr. Check lines: 303, 307. g4steppingmanager2.cc 303

Использование нулевого указателя

G4hhElastic::G4hhElastic(....) 
  : G4HadronElastic("HadrHadrElastic")
{
  ....
  fTarget = target; // later vmg
  fProjectile = projectile;
  ....
  fTarget  = G4Proton::Proton(); // later vmg
  fProjectile  = 0;                        // <=
  fMassTarg   = fTarget->GetPDGMass();
  fMassProj   = fProjectile->GetPDGMass(); // <=
  ....
}

V522 Dereferencing of the null pointer 'fProjectile' might take place. g4hhelastic.cc 184

Этот фрагмент похож на предыдущий. Но здесь указателю явно присваивается нулевое значение, и после переменная используется для инициализации других переменных. Возможно, предполагалось использование значение переменой из первого присваивания, и тогда второе просто лишнее. Возможно, хотели присвоить 0 другой переменной. Истинные причины такого присваивания известны только разработчикам проекта. В любом случае подобная инициализация некорректна, и на такой фрагмент кода стоит обратить внимание.

Неверная побитовая операция

#define dependentAxis 1
#define allowByRegion 2

static enum xDataTOM_interpolationFlag 
  xDataTOM_interpolation_getFromString( .... ) {
    ....
    if( flag | allowByRegion ) {....}  // <=
    if( flag | dependentAxis ) {....}  // <=
    ....
}

• V617 Consider inspecting the condition. The '2' argument of the '|' bitwise operation contains a non-zero value. xdatatom_interpolation.cc 85
• V617 Consider inspecting the condition. The '1' argument of the '|' bitwise operation contains a non-zero value. xdatatom_interpolation.cc 88

Анализатор выдал предупреждение сразу на две соседних строки функции. Внутри условия происходит побитовое ИЛИ с ненулевой константой. Результатом такого выражения всегда будет ненулевое значение, что приводит к неправильной логике работы программы. Чаще всего подобные ошибки происходят в результате опечаток. И в условии вместо побитового ИЛИ должна быть использована другая побитовая операция. Могу предположить, что в данном случае имелось ввиду побитовое И. Исправленный код будет выглядеть так:

if( flag & allowByRegion ) {....}
if( flag & dependentAxis ) {....}

Лишнее присваивание

G4ThreeVector G4GenericTrap::SurfaceNormal(....) const
{
  ....
  if ( noSurfaces == 0 )
  {
    ....
    sumnorm=apprnorm;
  }
  else if ( noSurfaces == 1 )  { sumnorm = sumnorm; } // <=
  else                         { sumnorm = sumnorm.unit(); }
  ....
}

V570 The 'sumnorm' variable is assigned to itself. g4generictrap.cc 515

В этом фрагменте видна логическая ошибка, которая заключается в избыточной ветке условия. Один из вариантов, что хотел сделать программист: при проверке на равенство единице переменой должна была присваиваться другая переменная, название которой схоже с sumnorm. Но так как в проверяемой части кода не было замечено таких переменных, рискну предположить, что это просто лишняя проверка. И для исправления ошибки предлагаю упростить условие, следующим образом:

if ( noSurfaces == 0 )
{
  ....
  sumnorm=apprnorm; 
}
else if ( noSurfaces != 1 ) { sumnorm = sumnorm.unit(); }

Ещё одно подозрительное место.

void G4UImanager::StoreHistory(G4bool historySwitch,....)
{
  if(historySwitch)
  {
    if(saveHistory)
    { historyFile.close(); }
    historyFile.open((char*)fileName);
    saveHistory = true;
  }
  else
  {
    historyFile.close();
    saveHistory = false;
  }
  saveHistory = historySwitch;
}

V519 The 'saveHistory' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 541, 543. g4uimanager.cc 543

Тут так же видна логическая ошибка. Код внутри функции в зависимости от значения historySwitch изменяют значение флага saveHistory и проводят операцию с файлом, о результате которой и сообщает флаг. Но после выполнения всех операций к переменной saveHistory просто присваивается значение historySwitch. Это является странной операцией, так как значение внутри условия уже было установлено, и мы только его испортим. Скорее всего, последнее присваивание лишнее и его нужно удалить.

Похожая ошибка есть в другом месте:

• V519 The 'lvl' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 277, 283. g4iontable.cc 283

Множественная проверка одного выражения

bool parse(....) 
{
 ....           
 if( (word0=="line_pattern") ||
     (word0=="line_pattern") ) { .... } 
 ....
}

V501 There are identical sub-expressions '(word0 == "line_pattern")' to the left and to the right of the '||' operator. style_parser 1172

Чаще всего такая ошибка возникает при проверке множества однотипных переменных внутри одного условия и использования метода Copy-Paste для его формирования.

Пример содержит достаточно маленький фрагмент кода, где ошибка видна явно. В данном случае это просто опечатка и вызвана она скорее всего именно копированием кода. Но это не значит, что её просто обнаружить при обычной проверке. Данное условие было взято из длинного дерева разнообразных проверок. Именно такие конструкции хорошо помогает выявить анализатор, и предотвратить ошибки при рефакторинге кода.

Даже если это не ошибка, код всё равно требуется исправить, чтобы двойная проверка не смутила человека, который будет поддерживать этот код.

Похожие фрагменты были обнаружены и в других местах проекта.

• V501 There are identical sub-expressions to the left and to the right of the '||' operator: ITTU->size() != np || ITTU->size() != np g4peneloperayleighmodel.cc 11563
• V501 There are identical sub-expressions '(ptwXY1->interpolation == ptwXY_interpolationFlat)' to the left and to the right of the '||' operator. ptwxy_binaryoperators.cc 301

Неаккуратный рефакторинг

G4ReactionProduct * G4ParticleHPLabAngularEnergy::Sample(....)
{
  ....
  //if ( it == 0 || it == nEnergies-1 ) 
  if ( it == 0 )
  {
    if(it==0) ....
     ....
  }
  ....
}

V571 Recurring check. The 'if (it == 0)' condition was already verified in line 123. g4particlehplabangularenergy.cc 125

Иногда в процессе рефакторинга кода остаются не до конца изменённые фрагменты. Так произошло и в примере. Старое условие было закомментировано, а новое стало точно соответствовать дополнительной проверке внутри. Для исправления ошибки необходимо лучше продумать исправления блока кода или просто убрать дополнительную проверку внутри условия.

Фрагменты с похожими недоработками:

• V571 Recurring check. The 'if (proj_momentum >= 10.)' condition was already verified in line 809. g4componentgghadronnucleusxsc.cc 815
• V571 Recurring check. The 'if (proj_momentum >= 10.)' condition was already verified in line 869. g4componentgghadronnucleusxsc.cc 875
• V571 Recurring check. The 'if (proj_momentum >= 10.)' condition was already verified in line 568. g4componentggnuclnuclxsc.cc 574
• V571 Recurring check. The 'if (proj_momentum >= 10.)' condition was already verified in line 1868. g4nuclnucldiffuseelastic.cc 1875

Уже проверенное выражение

void GFlashHitMaker::make(....)
{
  ....
  if( gflashSensitive )
  {
    gflashSensitive->Hit(&theSpot);
  }
  else if ( (!gflashSensitive ) && 
           ( pSensitive ) && 
           (....)
          ){....}
  ....
}

V560 A part of conditional expression is always true: (!gflashSensitive). gflashhitmaker.cc 102

В приведённом блоке условие в разделе else является избыточным. Условием входа в блок else итак является ложное значение переменной gflashSensitive и проверять её во второй раз не требуется.

Ещё одно похожее место:

void UseWorkArea( T* newOffset ) 
{
  ....
  if( offset && offset!=newOffset )
  {
    if( newOffset != offset ) {....}
    else {....}
  }
  ....
}

V571 Recurring check. The 'newOffset != offset' condition was already verified in line 154. g4geomsplitter.hh 156

Одна и та же переменная повторно проверяется во внутреннем блоке условия. Эта проверка всегда будет выдавать положительный результат, так как это являлось условием входа во внутренний блок условия. И как следствие, код во внутреннем блоке else никогда не будет выполнятся.

В результате копирования кода эта же лишняя проверка присутствует в других местах проекта. Ох уж этот Copy-Paste:

• V571 Recurring check. The 'newOffset != offset' condition was already verified in line 113. g4pdefsplitter.hh 115
• V571 Recurring check. The 'newOffset != offset' condition was already verified in line 141. g4vuplsplitter.hh 143

Бесполезное условие

void G4XXXStoredViewer::DrawView() {
  ....
  if (kernelVisitWasNeeded) {
    DrawFromStore();
  } else {
    DrawFromStore();
  }
  ....
}

V523 The 'then' statement is equivalent to the 'else' statement. g4xxxstoredviewer.cc 85

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

Подобный фрагмент встречается ещё в одном месте проекта.

• V523 The 'then' statement is equivalent to the 'else' statement. g4xxxsgviewer.cc 84

Избыточное условие

Void G4VTwistSurface::CurrentStatus::ResetfDone(....)
{
  if (validate == fLastValidate && p && *p == fLastp)
  {
     if (!v || (v && *v == fLastv)) return;
  }         
  ....
}

V728 An excessive check can be simplified. The '||' operator is surrounded by opposite expressions '!v' and 'v'. g4vtwistsurface.cc 1198

Данный фрагмент не является ошибкой. Но условие можно упростить следующим образом:

if (!v || *v == fLastv) return;

Похожие фрагменты есть и в других местах.

• V728 An excessive check can be simplified. The '||' operator is surrounded by opposite expressions '!a_cut' and 'a_cut'. array 168
• V728 An excessive check can be simplified. The '||' operator is surrounded by opposite expressions '!a_cut' and 'a_cut'. array 180
• V728 An excessive check can be simplified. The '||' operator is surrounded by opposite expressions '!a_cut' and 'a_cut'. array 240
• V728 An excessive check can be simplified. The '||' operator is surrounded by opposite expressions '!a_cut' and 'a_cut'. array 287
• V728 An excessive check can be simplified. The '||' operator is surrounded by opposite expressions 'p == 0' and 'p != 0'. g4emmodelactivator.cc 216

Неверный вызов конструктора

class G4PhysicsModelCatalog
{
  private:  
  ....
    G4PhysicsModelCatalog();
  ....
  static modelCatalog* catalog;
  ....
};

G4PhysicsModelCatalog::G4PhysicsModelCatalog()
{ if(!catalog) { 
    static modelCatalog catal;
    catalog = &catal; 
  } 
}

G4int G4PhysicsModelCatalog::Register(const G4String& name)
{
  G4PhysicsModelCatalog();
  .... 
}

V603 The object was created but it is not being used. If you wish to call constructor, 'this->G4PhysicsModelCatalog::G4PhysicsModelCatalog(....)' should be used. g4physicsmodelcatalog.cc 51

Вместо обращения к текущему объекту происходит создание нового временного объекта, который будет сразу уничтожен. В результате поля объекта не будут инициализированы. Когда требуется использовать инициализацию полей вне конструктора, то лучше для этого создать отдельную функцию и обращаться к ней. Но если требуется вызвать именно конструктор, необходимо обращается к конструктору с помощью слова this. В случае, если используется C++11, то наиболее красивым решением будет использование делегирующего конструктора. Подробнее подобные ошибки и способ их исправления рассматривается в этой книге (см. раздел 19 "Как правильно вызвать один конструктор из другого").

Опечатка при инициализации

static const G4String name[numberOfMolecula] = {
 ....
 "(CH_3)_2S", "N_2O",       
 "C_5H_10O" "C_8H_6", "(CH_2)_N",
 ....
};

V653 A suspicious string consisting of two parts is used for array initialization. It is possible that a comma is missing. Consider inspecting this literal: "C_5H_10O" "C_8H_6". g4hparametrisedlossmodel.cc 324

Здесь допущена ошибка инициализации массива с помощью констант. В результате опечатки была пропущена запятая. Здесь сразу несколько бед:

• Произойдёт конкатенация двух строковых констант в одну. И мы получим в качестве одной из формул "C_5H_10OC_8H_6". Невиданная разновидность спирта.
• Обращаясь к массиву по индексу, мы можем получать название не той формулы, которой нужно.
• И последнее - возможен выход за границу массива.

Забытый throw

class G4HadronicException : public std::exception {....}
void G4CrossSectionDataStore::ActivateFastPath( ....)
{
  ....
  if ( requests.insert( { key , min_cutoff } ).second ) {
    ....
    G4HadronicException(__FILE__,__LINE__,msg.str());
  }
}

V596 The object was created but it is not being used. The 'throw' keyword could be missing: throw G4HadronicException(FOO); g4crosssectiondatastore.cc 542

Большая часть кода функции занимается формированием сообщения для создания исключения. Но из-за пропущенного throw будет создано неиспользуемое исключение. И программа продолжит работу, что может привести к неопределённому поведению программы или к некорректным расчетам.

Ошибка была повторяется в других местах проекта.

• V596 The object was created but it is not being used. The 'throw' keyword could be missing: throw G4HadronicException(FOO); g4generalphasespacedecay.hh 126
• V596 The object was created but it is not being used. The 'throw' keyword could be missing: throw G4HadronicException(FOO); g4particlehpthermalscattering.cc 515
• V596 The object was created but it is not being used. The 'throw' keyword could be missing: throw G4HadronicException(FOO); g4particlehpthermalscattering.cc 574
• V596 The object was created but it is not being used. The 'throw' keyword could be missing: throw G4HadronicException(FOO); g4particlehpthermalscattering.cc 585
• V596 The object was created but it is not being used. The 'throw' keyword could be missing: throw G4HadronicException(FOO); g4particlehpthermalscattering.cc 687

Ошибка вывода

bool G4GMocrenIO::storeData2() {
  ....
  ofile.write("GRAPE    ", 8);
  ....
}

V666 Consider inspecting second argument of the function 'write'. It is possible that the value does not correspond with the length of a string which was passed with the first argument. g4gmocrenio.cc 1351

Приведённая ошибка связана с несоответствием фактической длинны строки и аргумента, который задаёт длину внутри функции. В данном случае ошибка возникла из-за формирования определённого отступа с помощью пробелов, и на первый взгляд не видно сколько их там на самом деле. Возможно поэтому этой ошибки не было уделено должное внимание, и она находится в проекте ещё со времени первой проверки. Эта ошибка была включена в базу примеров для диагностики V666.

Заключение

Возможно, не все приведённые ошибки являются опасными, но многие на первый взгляд незначительные ошибки могут в будущем привести к более серьёзным последствиям. Поэтому проекты требуется проверять регулярно, чтобы выявлять ошибки на ранних стадиях до того, как они приведут к серьёзным последствиям. Анализатор поможет не только найти и исправить самые заковыристые ошибки, но и обнаружить опасные места в проекте прежде, чем они станут ошибками. Предлагаю скачать и попробовать анализатор PVS-Studio в своём проекте: http://www.viva64.com/ru/pvs-studio/download/.