PVS-Studio is there to help CERN: analysis of Geant4 project




Geant4 project continues developing, so it's really interesting to recheck it with PVS-Studio static code analyzer. This time we'll do a check of version 10.2 (previously, we checked 10.0 beta-version)

Picture 1

Introduction

Geant4 toolkit is developed in CERN, for the simulation and exploration of particle behavior when passing through matter, using Monte-Carlo methods. Early versions of the project were written in Fortran, and starting with version 4, the project was fully translated into object-oriented language C++.

More details about this project can be found on the official site for the project: http://geant4.org.

This project was already checked a couple of times; you can find the results in other articles. The analysis of version 9.4 is described in the article "Copy-Paste and Muons", and the check of version 10.0-beta is described in the article "Going On with the Check of Geant4"

Since the last time we checked the project, Geant 4 was upgraded to version 10.02. PVS-Studio was also updated to version 6.05, so that was the version we used.

In the project I've encountered quite a number of errors, related to the usage of conditions and comparisons. Logical errors are usually done upon leaving the code for future development, or inaccurate modification, with the removal of previous parts of the code that contain branching statements. At the same time, simple typos and lack of reasoning the expressions may lead to errors or redundant code.

Piquancy of the situation

There was some zest in this check of Geant4, because as far as I understand, the development team already uses a static code analyzer, Coverity, regularly. I drew this conclusion by looking at various Release Notes, and comments in the code like this one:

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

The Coverity analyzer is considered to be a leader in the market of code analyzers, so finding something after the Coverity analysis is already a great achievement. Nonetheless, PVS-Studio found plenty of interesting bugs, which also shows that it has become a powerful and mature product.

Missing '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

This is one of the most common errors when working with checks of several values of one variable using if. Of course, it could just be incorrect formatting, but in this example the analyzer most likely is pointing to a real bug.

As a result of the copying, the else word was left forgotten, which will lead in this case to the execution of excessive code. For example, the value will be zero, and we'll have the code executed from the corresponding block, but because of the error, the code from the else block after the comparison with one. To fix this issue, we should add the missing else before the condition if(1 == nsplit).

Incorrect handling of a potential error

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

We can only assume what this code was meant for. It is very likely that this fragment is intended for catching and forming the error message, but in the cast of an incorrect condition, there will be no error message. It's unclear how the program will behave later on. Perhaps the handler will catch the bug in some different place, but there is a chance that the program will continue working without an error, but will output an incorrect result. It's quite hard to say exactly, what is the cause of this problem, as it can be both in one of the conditional expressions, as well as in the excessive else keyword. But judging by the formatting, we can safely assume that both conditional blocks are correct, and we should just remove else before the second conditional block.

Thanks to copy-paste this error was duplicated and was found in three more fragments:

  • 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

Null pointer dereference

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

We see errors related to pointer handling quite often in programs. In this case we have a situation where two objects are handled simultaneously, but only one is checked for correctness. This error may remain unnoticed for a long time, but if the pointer to the theShells turns out to be a null one, it will lead to undefined program behavior. To fix this, you need to change the condition as follows:

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

One more fragment where the check of the pointer is missing.

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

Null pointer usage

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

This fragment is similar to the previous one. But here's a pointer explicitly assigned with a zero value, and after that the variable is used for the initialization of other variables. The programmer may have intended to use the variable value from the first assignment, so the second is simply unnecessary. Perhaps, 0 was supposed to be assigned to a different variable. The true reasons for this assignment are known only by the developers of the project. In any case such initialization is not correct, and this code fragment is worth reviewing.

Invalid bitwise operation

#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

The analyzer issued a warning for two neighboring strings of a function. We have bitwise OR with a non-zero constant inside a condition. The result of such an expression will always be non-zero, which leads to incorrect logic in the program. Such errors often occur because of typos. Also in the condition, instead of the bitwise OR, another bitwise operation should be used. I suppose that in this case, the author meant to use bitwise AND, so it should look as follows:

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

Extra assignment

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

In this code fragment we see a logic error that is in the redundant condition statment. One of the variants of what was meant to be here: during the verification against one, the variable was to be assigned with a different variable, whose name is also similar with sumnorm. But as in there were no such variables noticed in the checked part of the code, I will hazard a guess that this is just a redundant check. To fix this, let's simplify the condition in the following way:

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

Another suspicious fragment:

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

Here we also see a logic error. The code inside the function, depending on the value of historySwitch, changes the saveHistory flag, and carries out an operation with the file; the result of which is reported by the flag. But after all the operations, the variable saveHistory is just assigned with a value historySwitch. This is strange because the value in the condition was already set, and we have messed it up. Most likely it's a redundant assignment, and it should be removed.

There is a similar error in another fragment:

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

Multiple check of a single expression

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

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

Most often this occurs when testing multiple variables of the same type within the same condition, and using Copy-Paste for its composition.

The example has quite a small code fragment where you can clearly see the error. In this case it is just a typo and it is most likely caused by the code being copied. But this does not mean that it's easy to detect it doing simple check. This condition was taken from a long tree of various checks. The analyzer is especially useful in the detection of such constructions, and prevents errors during code refactoring.

Even if it's not an error, the code requires fixing, so that the double check doesn't confuse the person who will maintain this code.

Similar fragments were found in other parts of the project.

  • 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

Refactoring issue

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

Sometimes during the process of refactoring, you may have fragments that remain unchanged. This is exactly what happened in this example. The old message was commented, the new one became the same as the additional check inside. To fix this you need to more carefully consider the correction of the code block, or just remove the extra check inside conditions.

Fragments with similar issues:

  • 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

An expression that was already checked

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

In the given block, the condition in the else section is redundant. The prerequisite for the entrance to the else block is already a false value of gflashSensitive variable, so it doesn't need to be checked once more.

Another similar fragment:

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

The same variable is checked in the inner condition block. This check will always generate a positive result because it was a condition for the entry to the inner condition block. As a result, the code will never be executed in the inner else block.

The same redundant check was found in several other fragments in the project. Oh, this 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

Useless condition

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

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

The code inside the two branches is identical, which makes the condition useless, as the same code will be executed regardless of it. Analyzer messages of this kind might signal code that wasn't properly catered for, or typos when copying various constants or functions having similar names. In this case it's not clear what this block was made for, but it clearly needs to be reviewed and fixed.

There was another similar fragment:

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

Redundant condition

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

This code fragment doesn't have an error, but it can be simplified in the following way:

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

Several more similar fragments:

  • 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

Incorrect constructor call

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

Instead of accessing the current object, a new temporary object is created and then immediately destroyed. As a result, the fields of the object will not be initialized. If you need to use field initialization outside the constructor, it's better to create a separate function and access it. But if you want to call the constructor, you should access the constructor using the word this. If you use C++11, the most graceful decision would be to use a delegate constructor. More details about these errors, and the ways to fix them, can be found in this book (see section 19, "How to properly call one constructor from another").

A typo during initialization

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

Here we have an error in an array initialization with the constants. As the result of the typo, a comma is missing. There are several troubles at the same time:

  • There will be a concatenation of two string constants in one. And we get one of the formulas as "C_5H_10OC_8H_6". An unprecedented type of alcohol.
  • Accessing the array by index, we can get some unexpected formula.
  • And the last - we may have array index out of bounds.

Forgotten 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

The major part of the function does the forming of a message to create an exception. But because of a missing throw, there will be an unused exception created. The program will continue working, which can lead to undefined behavior, or to incorrect evaluations.

The error was repeated in another parts of the project.

  • 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

Output error

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

This error is caused by the mismatch of actual string length, and the argument that specifies the length inside the function. In this case an error occurred due to the formation of a particular indent created by spaces, at first glance it's hard to say how many of them are there. Perhaps this error wasn't taken into account, as it is still there from the last time we checked the project. This bug was included in the database of examples for V666 diagnostic.

Conclusion

Perhaps not all the listed errors are really dangerous, but a lot of minor bugs can lead to more serious consequences in the future. This is why you should regularly check your projects to detect errors during the early stages, before they lead to serious consequences. The analyzer is of great help in finding and fixing the trickiest bugs, and detecting hazardous places in the project before they turn into bugs. I suggest downloading and trying out PVS-Studio analyzer on your project: http://www.viva64.com/en/pvs-studio-download/.



Use PVS-Studio to search for bugs in C, C++, and C# code

We offer you to check your project code with PVS-Studio. Just one bug found in the project will show you the benefits of the static code analysis methodology better than a dozen of the articles.

goto PVS-Studio;


Do you make errors in the code?

Check your code
with PVS-Studio

Static code analysis
for C, C++, and C#

goto PVS-Studio;
On our website we use a cookie to collect information of a technical nature.
If you do not agree, please leave the site. Learn More →
Do not show again