Geekly Articles each Day
📜 ⬆️ ⬇️

PVS-Studio hurries to the aid of CERN: checking the project Geant4

PVS-Studio hurries to the aid of CERN The Geant4 project continues to evolve, so it is interesting to check it again with the help of the PVS-Studio static code analyzer. This time, version 10.2 will be checked (the previous check was for version 10.0-beta).

Introduction


The Geant4 toolkit was developed at CERN and is designed to simulate and study the behavior of particles passing through a substance, using Monte-Carlo methods. Earlier versions of the project were written in the Fortran language, and since version 4 the project has been completely translated into object-oriented C ++.

More information about the project can be found on the official website of the project http://geant4.org .

A couple of times the project Geant4 has already been tested, and other articles tell about the results. You can read about checking version 9.4 in the article " Copy-Paste and muons ", and about checking version 10.0-beta in the article " Continuing to check Geant4 ".
')
Since the last check, a new version of Geant4.10.2 was released. PVS-Studio has also been updated since that time, and the analyzer version 6.05 was used for testing.

In the project, I came across a lot of errors related to conditions and comparisons. Logical errors are most often made when the code is left for future refinement, or by inaccurate modification with the removal of the previous, controlling transitions, parts of the code. But simple typographical errors and insufficient forethought of expressions can also lead to errors or redundant code.

Piquancy


Additional piquancy of the next project verification is given by the fact that the Geant4 project, as I understand it, is regularly checked by the static Coverity analyzer. This is evidenced by the multiple ReleaseNotes entries and comments in the code, like:
// Private copy constructor and assigment operator - copying and // assignment not allowed. Keeps Coverity happy.

The Coverity analyzer is considered the market leader, so finding something after it is a good achievement. Nevertheless, PVS-Studio found a lot of interesting mistakes, which says that it, too, has become a mature and powerful product.

Missed '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 checking several values ​​of a single variable using if . Of course, this may simply be incorrect formatting, but in this example, the analyzer most likely indicates exactly the error.

As a result of copying the code, the word else was forgotten. What in this case will lead to the execution of extra code. For example, the value will be zero and the code from the advising block will be executed, but due to an error, the code of the else block will also be executed after checking for equality to one. To correct the error, add the missing else before the if (1 == nsplit) condition .

Incorrect handling of possible errors


 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 possibility of logical error presence. Check lines: 102, 105. g4genericpolycone.cc 102

I can only assume about the purpose of the code. It is very likely that this fragment is responsible for intercepting and forming an error message, but as a result of an incorrect condition, an error message will not appear. And it is not known how the program will behave later. It is possible that the error will be intercepted by the handler elsewhere, or it may be that the program will continue to run without error and will produce an incorrect result. It is quite difficult to say exactly what the error is, since it can be either in one of the conditional expressions or in the superfluous else . But judging by the formatting, we can safely assume that both blocks of the conditions are correct and for correction it is enough to remove the else before the second conditional block.

Thanks to copying the code, the same error was multiplied and found in three more places of the project:

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' Check lines: 147, 156. g4particlehpphotondist.cc 147

Errors when working with pointers are found in programs quite often. Here is the case when there is simultaneous work with two objects, and only one of them is checked for correctness. Such an error may go unnoticed for a long time, but if the pointer to the TheShells object turns out to be zero, this will lead to undefined program behavior. To correct the error, you need to change the condition as follows:
 if ( theGammas != NULL && theShells != NULL) ....

Another fragment with no pointer check:

Using the null pointer


 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 the pointer is explicitly assigned a zero value, and after the variable is used to initialize other variables. Perhaps it was supposed to use the value of the variable from the first assignment, and then the second is just superfluous. Perhaps you wanted to assign 0 to another variable. The true reasons for this assignment are known only to the developers of the project. In any case, such initialization is incorrect, and you should pay attention to such a code fragment.

Invalid bitwise operation


 #define dependentAxis 1 #define allowByRegion 2 static enum xDataTOM_interpolationFlag xDataTOM_interpolation_getFromString( .... ) { .... if( flag | allowByRegion ) {....} //<- if( flag | dependentAxis ) {....} //<- .... }

The analyzer issued a warning to immediately two adjacent lines of the function. A bitwise OR occurs with a nonzero constant inside the condition. The result of such an expression will always be a non-zero value, which leads to incorrect logic of the program. Most often, such errors occur as a result of typos. And in the condition, instead of bitwise OR, another bitwise operation should be used. I can assume that in this case there was a bitwise I. meaning. The corrected code will look like this:
 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 fragment, a logical error is visible, which lies in the redundant branch of the condition. One of the options that the programmer wanted to do: when checking for equality to one, the variable should have been assigned another variable, the name of which is similar to sumnorm. But since there were no such variables in the checked part of the code, I would venture to suggest that this is just an extra check. And to correct the error, I propose to simplify the condition as follows:
 if ( noSurfaces == 0 ) { .... sumnorm=apprnorm; } else if ( noSurfaces != 1 ) { sumnorm = sumnorm.unit(); }

Another suspicious place.
 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

There is also a logical error. The code inside the function, depending on the historySwitch value, changes the value of the saveHistory flag and performs an operation with the file, the result of which is reported by the flag. But after performing all the operations, the saveHistory variable is simply assigned the value historySwitch . This is a strange operation, since the value inside the condition has already been established, and we only spoil it. Most likely, the last assignment is superfluous and must be deleted.

A similar error exists elsewhere:

Multiple check of one expression


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

V501 There are identical sub-expressions' (word0 == "line_pattern") operator. style_parser 1172

Most often, this error occurs when checking the set of variables of the same type within a single condition and using the Copy-Paste method to generate it.

The example contains a rather small code snippet, where the error is clearly visible. In this case, it's just a typo and it is most likely caused by the copying of the code. But this does not mean that it is easy to detect during a routine check. This condition was taken from a long tree of various checks. It is such constructions that helps to identify the analyzer and prevent errors when refactoring code.

Even if this is not an error, the code still needs to be fixed so that a double check does not embarrass the person who will support this code.

Similar fragments were found in other places of the project.

Inaccurate refactoring


 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 in the process of refactoring the code, not completely modified fragments remain. It happened in the example. The old condition was commented out, and the new one became exactly the same as an additional check inside. To correct the error, it is necessary to better consider the corrections of the code block or simply remove the additional check inside the condition.

Fragments with similar defects:

Already checked expression


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

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

In the above block, the condition in the else section is redundant. The condition for entering the else block is, therefore, a false value for the variable gflashSensitive and it is not required to check it a second time.

Another similar place:
 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 re-checked in the internal condition block. This test will always produce a positive result, since it was a condition for entering the internal condition block. And as a result, the code in the internal block else will never be executed.

As a result of copying the code, this same extra check is present in other places of the project. Oh, this Copy-Paste:

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 of the condition is identical, which makes the condition itself useless, since the same code will be executed independently of it. Similar messages from the analyzer can signal an unfinished code or typos when copying various constants or functions that are similar in name. In this case, it is not known why this block was made, but it clearly needs to be improved.

A similar fragment is found in another place of the project.

Excess condition


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

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

This snippet is not an error. But the condition can be simplified as follows:
 if (!v || *v == fLastv) return;

There are similar fragments in other places.

Invalid 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 not used. If you wish to call the constructor, 'this-> G4PhysicsModelCatalog :: G4PhysicsModelCatalog (....)' should be used. g4physicsmodelcatalog.cc 51

Instead of referring to the current object, a new temporary object is created, which will be immediately destroyed. As a result, the object fields will not be initialized. When it is required to use field initialization outside the constructor, it is better to create a separate function for this and access it. But if you want to call just the constructor, you must refer to the constructor using the word this . If C ++ 11 is used, the most beautiful solution would be to use a delegating constructor. Such errors and the way to correct them are discussed in more detail in this book (see Section 19 “How to correctly call one constructor from another”).

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

There is an error in initializing the array using constants. As a result of typographical errors, a comma was omitted. There are several troubles here:

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 not used. The 'throw' keyword could be missing: throw G4HadronicException (FOO); g4crosssectiondatastore.cc 542

Most of the function code deals with generating a message to create an exception. But because of a missing throw , an unused exception will be created. And the program will continue to work, which can lead to indefinite program behavior or incorrect calculations.

The error was repeated in other places of the project.

Output error


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

V666 Consider inspecting second argument of the function 'write'. It is possible that it’s not passed on. g4gmocrenio.cc 1351

This error is related to the discrepancy between the actual length of the line and the argument that specifies the length inside the function. In this case, the error occurred due to the formation of a certain indent using spaces, and at first glance it is not visible how many of them there actually are. Perhaps that is why this error has not been given due attention, and it has been in the project since the time of the first check. This error was included in the database of examples for the diagnosis of V666 .

Conclusion


Probably, not all given errors are dangerous, but many seemingly minor errors may lead to more serious consequences in the future. Therefore, projects need to be checked regularly in order to detect errors in the early stages before they lead to serious consequences. The analyzer will help not only to find and fix the most intricate errors, but also to detect dangerous places in the project before they become errors. I suggest to download and try the PVS-Studio analyzer in my project.

Source: https://habr.com/ru/post/304626/

More articles:


All Articles