PVS-Studio поспішає на допомогу CERN: перевірка проекту Geant4

PVS-Studio поспішає на допомогу CERNПроект 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 у своєму проекті.
Джерело: Хабрахабр

0 коментарів

Тільки зареєстровані та авторизовані користувачі можуть залишати коментарі.