Checking FreeCAD source code and its “bad” dependencies

The article, which was conceived as a review of errors in the open draft FreeCAD, acquired a slightly different character. A significant part of the analyzer warnings fell on used third-party libraries. Software development with the active use of third-party libraries gives a lot of advantages, especially in the field of Open Source. And errors in libraries are not a reason to abandon them. But you have to understand that bugs can also live in the code used. They should be ready to be met and, if possible, corrected, thereby improving the libraries you use.

FreeCAD is a parametric three-dimensional editor that allows you to create three-dimensional models and drawings of their projections. FreeCAD developer Jürgen Riegel, who works for DaimlerChrysler Corporation, positions his program as the first free mechanic design tool. Among specialists of a number of industries, the problem of creating a full-fledged CAD system within the framework of Open Source is known, and this project is a candidate for such a “usefulness”. Let's check the source code using PVS-Studio and help an open project in this area to become a little better. Surely you are faced with "glitches" in various editors, when you can not get to any point or straighten the line, which always moves down one pixel. Perhaps the reason for all this is only typos in the source code.

What about PVS-Studio ?!

The FreeCAD project is cross-platform, the site has very good build documentation. It was not difficult for me to get project files for Visual Studio Community 2013 for verification using the installed PVS-Studio plugin. But at the beginning the check did not work ...
')


The cause of the internal error in the analyzer was the presence of a binary sequence in a text preprocessed file with the extension * .i. The analyzer can handle such situations, but something new has happened here. The problem is in one of the lines in the compilation parameters of the source files:

/FI"Drawing.dir/Debug//Drawing_d.pch" 

The compilation flag / FI (Name Forced Include File) , like the #include directive, serves to include text header files. But here they are trying to include a file containing information in binary form. It is miraculously compiled. Perhaps in Visual C ++, such a file is simply ignored.

If you try not to compile, namely preprocess files, then Visual C ++ reports an error. But the default Clang used in PVS-Studio, without thinking twice, included the * .i file of the binary file. PVS-Studio did not expect such a trick and went crazy.

To make it clearer what is being said, here is a fragment of a file preprocessing using Clang:



The project was carefully checked without this flag, but I want to draw the attention of the developers that they have a mistake here.

Freecad

The first examples of errors from the project are obtained for all known reasons.



V501 There are identical sub-expressions' surfaceTwo-> IsVRational () '=' operator. modelrefine.cpp 780

 bool FaceTypedBSpline::isEqual(const TopoDS_Face &faceOne, const TopoDS_Face &faceTwo) const { .... if (surfaceOne->IsURational() != surfaceTwo->IsURational()) return false; if (surfaceTwo->IsVRational() != surfaceTwo->IsVRational())//<= return false; if (surfaceOne->IsUPeriodic() != surfaceTwo->IsUPeriodic()) return false; if (surfaceOne->IsVPeriodic() != surfaceTwo->IsVPeriodic()) return false; if (surfaceOne->IsUClosed() != surfaceTwo->IsUClosed()) return false; if (surfaceOne->IsVClosed() != surfaceTwo->IsVClosed()) return false; if (surfaceOne->UDegree() != surfaceTwo->UDegree()) return false; if (surfaceOne->VDegree() != surfaceTwo->VDegree()) return false; .... } 

On the left side of the inequality operator, the wrong variable “surfaceTwo” instead of “surfaceOne” was found due to a small typo. It remains to advise the author next time to make copy-paste with larger fragments, but we will also get to such examples =).

V517 The use of if (A) {...} else if (A) {...} 'pattern was detected. There is a possibility of logical error presence. Check lines: 162, 164. taskpanelview.cpp 162

 /// @cond DOXERR void TaskPanelView::OnChange(....) { std::string temp; if (Reason.Type == SelectionChanges::AddSelection) { } else if (Reason.Type == SelectionChanges::ClrSelection) { } else if (Reason.Type == SelectionChanges::RmvSelection) { } else if (Reason.Type == SelectionChanges::RmvSelection) { } } 

What we have paid attention to the function that is still being written? And here's why: with this code, it’s likely to be the same as in the following two examples.

V517 The use of if (A) {...} else if (A) {...} 'pattern was detected. There is a possibility of logical error presence. Check lines: 1465, 1467. application.cpp 1465

 pair<string, string> customSyntax(const string& s) { #if defined(FC_OS_MACOSX) if (s.find("-psn_") == 0) return make_pair(string("psn"), s.substr(5)); #endif if (s.find("-display") == 0) return make_pair(string("display"), string("null")); else if (s.find("-style") == 0) return make_pair(string("style"), string("null")); .... else if (s.find("-button") == 0) //<== return make_pair(string("button"), string("null")); //<== else if (s.find("-button") == 0) //<== return make_pair(string("button"), string("null")); //<== else if (s.find("-btn") == 0) return make_pair(string("btn"), string("null")); .... } 

Let's hope that the author did not accidentally correct one copied line, but in the end, he still finished searching for all the necessary lines.

V517 The use of if (A) {...} else if (A) {...} 'pattern was detected. There is a possibility of logical error presence. Check lines: 191, 199. blendernavigationstyle.cpp 191

 SbBool BlenderNavigationStyle::processSoEvent(....) { .... else if (!press && (this->currentmode == NavigationStyle::DRAGGING)) { //<== SbTime tmp = (ev->getTime() - this->centerTime); float dci = (float)QApplication::....; if (tmp.getValue() < dci) { newmode = NavigationStyle::ZOOMING; } processed = TRUE; } else if (!press && (this->currentmode == NavigationStyle::DRAGGING)) { //<== this->setViewing(false); processed = TRUE; } .... } 

But, in my opinion, a serious mistake for such an application. When modeling, a lot of work is done with the help of mouse navigation, and here is such a blooper: the source code in the last condition never gets control, because the first condition is the same and is executed first.

V523 The 'then' statement is equivalent to the 'else' statement. viewproviderfemmesh.cpp 695

 inline void insEdgeVec(std::map<int,std::set<int> > &map, int n1, int n2) { if(n1<n2) map[n2].insert(n1); else map[n2].insert(n1); }; 

Regardless of the condition, one action is always performed. Maybe all the same thought so:

 inline void insEdgeVec(std::map<int,std::set<int> > &map, int n1, int n2) { if(n1<n2) map[n2].insert(n1); else map[n1].insert(n2); }; 

Why did I correct the last line? You might be interested in an article on this topic: Last line effect . But perhaps you need to fix the first one. I do not know :).

V570 The 'this-> quat [3]' variable is assigned to itself. rotation.cpp 260

 Rotation & Rotation::invert(void) { this->quat[0] = -this->quat[0]; this->quat[1] = -this->quat[1]; this->quat[2] = -this->quat[2]; this->quat[3] = this->quat[3]; //<== return *this; } 

More about the “last lines”. The analyzer became alert, as there is no minus sign in the last line. But here it is impossible to unequivocally talk about an error, perhaps, when implementing such a transformation, we wanted to emphasize that the fourth component does not change.

V576 Incorrect format. A different number of actual arguments is expected while calling the 'fprintf' function. Expected: 2. Present: 3. memdebug.cpp 222

 int __cdecl MemDebug::sAllocHook(....) { .... if ( pvData != NULL ) fprintf( logFile, " at %p\n", pvData ); else fprintf( logFile, "\n", pvData ); //<== .... } 

Such code does not make sense. If the pointer is zero, then you can simply print a newline character without passing unused parameters to the function.

V596 The object was not used. The 'throw' keyword could be missing: throw Exception (FOO); waypointpyimp.cpp 231

 void WaypointPy::setTool(Py::Int arg) { if((int)arg.operator long() > 0) getWaypointPtr()->Tool = (int)arg.operator long(); else Base::Exception("negativ tool not allowed!"); } 

An exception type object is created in the code, but is not used. Most likely the “throw” keyword is missing:

 void WaypointPy::setTool(Py::Int arg) { if((int)arg.operator long() > 0) getWaypointPtr()->Tool = (int)arg.operator long(); else throw Base::Exception("negativ tool not allowed!"); } 

A few more places like this:

• V596 The object was not used. The 'throw' keyword could be missing: throw Exception (FOO); application.cpp 274
• V596 The object was not used. The 'throw' keyword could be missing: throw Exception (FOO); fileinfo.cpp 519
• V596 The object was not used. The 'throw' keyword could be missing: throw Exception (FOO); waypointpyimp.cpp 244
• V596 The object was not used. The 'throw' keyword could be missing: throw Exception (FOO); sketch.cpp 185

V599 The virtual class contains not only virtual functions. constraints.cpp 1442

 class Curve { //a base class for all curve-based //objects (line, circle/arc, ellipse/arc) //<== public: virtual DeriVector2 CalculateNormal(....) = 0; virtual int PushOwnParams(VEC_pD &pvec) = 0; virtual void ReconstructOnNewPvec (....) = 0; virtual Curve* Copy() = 0; }; class Line: public Curve //<== { public: Line(){} Point p1; Point p2; DeriVector2 CalculateNormal(Point &p, double* derivparam = 0); virtual int PushOwnParams(VEC_pD &pvec); virtual void ReconstructOnNewPvec (VEC_pD &pvec, int &cnt); virtual Line* Copy(); }; 

Using:

 class ConstraintAngleViaPoint : public Constraint { private: inline double* angle() { return pvec[0]; }; Curve* crv1; //<== Curve* crv2; //<== .... }; ConstraintAngleViaPoint::~ConstraintAngleViaPoint() { delete crv1; crv1 = 0; //<== delete crv2; crv2 = 0; //<== } 

In the base class “Curve” virtual functions are declared, but the destructor is not declared, which will be created by default. And it certainly will not be virtual! This means that objects inherited from this class will not be completely cleared in such a use case, when the pointer to the child is stored in the pointer to the base class. Judging by the commentary, the base class has many inheritable classes, for example, the “Line” class in the example.

V655 The strings were concatenated but are not utilized. Consider inspecting the expression. propertyitem.cpp 1013

 void PropertyVectorDistanceItem::setValue(const QVariant& variant) { if (!variant.canConvert<Base::Vector3d>()) return; const Base::Vector3d& value = variant.value<Base::Vector3d>(); Base::Quantity q = Base::Quantity(value.x, Base::Unit::Length); QString unit = QString::fromLatin1("('%1 %2'").arg(....; q = Base::Quantity(value.y, Base::Unit::Length); unit + QString::fromLatin1("'%1 %2'").arg(....; //<== setPropertyValue(unit); } 

The analyzer detected meaningless row addition. If you look closely, you might want to use the '+ =' operator instead of simple addition. Then such code would make sense.

V595 Check lines: 293, 294. view3dinventorexamples.cpp 293

 void LightManip(SoSeparator * root) { SoInput in; in.setBuffer((void *)scenegraph, std::strlen(scenegraph)); SoSeparator * _root = SoDB::readAll( &in ); root->addChild(_root); //<== if ( root == NULL ) return; //<== root->ref(); .... } 

One example of checking the pointer in the wrong place, other places are here:

• It was verified against nullptr. Check lines: 1049, 1056. viewprovider.cpp 1049
• V595 The 'viewProviderRoot' pointer was used before it was verified against nullptr. Check lines: 187, 188. taskcheckgeometry.cpp 187
• V595 Check lines: 209, 210. viewproviderrobotobject.cpp 209
• V595 Check lines: 222, 223. viewproviderrobotobject.cpp 222
• V595 Check lines: 235, 236. viewproviderrobotobject.cpp 235
• V595 Check lines: 248, 249. viewproviderrobotobject.cpp 248
• V595 Check lines: 261, 262. viewproviderrobotobject.cpp 261
• V595 Check lines: 274, 275. viewproviderrobotobject.cpp 274
• V595 Check lines: 991, 995. propertysheet.cpp 991

Open CASCADE library

V519 The 'myIndex [1]' variable is assigned values ​​twice successively. Perhaps this is a mistake. Check lines: 60, 61. brepmesh_pairofindex.hxx 61

 //! Prepends index to the pair. inline void Prepend(const Standard_Integer theIndex) { if (myIndex[1] >= 0) Standard_OutOfRange::Raise ("BRepMesh_PairOfIndex...."); myIndex[1] = myIndex[0]; myIndex[1] = theIndex; } 

In this example, rewrote the value of the array element 'myIndex' with index 1. It seems to me that they wanted to do this:

 myIndex[1] = myIndex[0]; myIndex[0] = theIndex; 

SALOME Smesh Module

V501 There are identical sub-expressions '0 <= theParamsHint.Y ()' the operator. smesh_block.cpp 661

 bool SMESH_Block::ComputeParameters(const gp_Pnt& thePoint, gp_XYZ& theParams, const int theShapeID, const gp_XYZ& theParamsHint) { .... bool hasHint = ( 0 <= theParamsHint.X() && theParamsHint.X() <= 1 && 0 <= theParamsHint.Y() && theParamsHint.Y() <= 1 && 0 <= theParamsHint.Y() && theParamsHint.Y() <= 1 ); //<== .... } 

There is clearly not enough .Z () checking. The class has such a function, it is even called “gp_XYZ”.

V503 This is a nonsensical comparison: pointer <0. driverdat_r_smds_mesh.cpp 55

 Driver_Mesh::Status DriverDAT_R_SMDS_Mesh::Perform() { .... FILE* aFileId = fopen(file2Read, "r"); if (aFileId < 0) { fprintf(stderr, "....", file2Read); return DRS_FAIL; } .... } 

The pointer can not be less than zero. Even in the simplest examples with the fopen () function, which can be found in books and on the Internet, the value of the function is compared to NULL using == or! =.

I was surprised how such a code could appear. But my colleague Andrei Karpov suggested that this often happens when refactoring code, where the open () function was once used. This function returns the value -1 if the comparison <0 is appropriate. In the process of refactoring or porting a program, this function is replaced with fopen (), but they forget to correct the check.

Another such place:

• V503 This is a nonsensical comparison: pointer <0. driverdat_w_smds_mesh.cpp 41

V562 It's a bit to compare the value of 12:! MyType == SMESHDS_MoveNode. smeshds_command.cpp 75

 class SMESHDS_EXPORT SMESHDS_Command { .... private: SMESHDS_CommandType myType; .... }; enum SMESHDS_CommandType { SMESHDS_AddNode, SMESHDS_AddEdge, SMESHDS_AddTriangle, SMESHDS_AddQuadrangle, .... }; void SMESHDS_Command::MoveNode(....) { if (!myType == SMESHDS_MoveNode) //<== { MESSAGE("SMESHDS_Command::MoveNode : Bad Type"); return; } .... } 

There is an enumeration named "SMESHDS_CommandType", there are many constants in it. The analyzer detected an incorrect check: a variable of this type is compared with a named constant, but what does the negation sign do here ?? Most likely, the check should be like this:

 if (myType != SMESHDS_MoveNode) //<== { MESSAGE("SMESHDS_Command::MoveNode : Bad Type"); return; } 

Unfortunately, this check with the error message was copied to 20 more places, here is the complete list: FreeCAD_V562.txt .

V567 Undefined behavior. Evaluation order is not defined for 'splice' function. The 'outerBndPos' variable is modified while it is being used twice between the sequence points. smesh_pattern.cpp 4260

 void SMESH_Pattern::arrangeBoundaries (....) { .... if ( outerBndPos != boundaryList.begin() ) boundaryList.splice( boundaryList.begin(), boundaryList, outerBndPos, //<== ++outerBndPos ); //<== } 

In fact, the analyzer is not quite right. Uncertain behavior is not here. But there is an error, so the warning was issued not in vain. The C ++ standard does not impose a restriction on the order in which the actual arguments of the function are calculated. Therefore, it is not known which values ​​will be passed to the function.

Let me explain with a simple example:

 int a = 5; printf("%i, %i", a, ++a); 

This code can print both "5, 6" and "6, 6. The result depends on the compiler and its settings.

V663 Infinite loop is possible. The 'cin.eof ()' condition is not a loop from the loop. Consider adding the 'cin.fail ()' function call to the conditional expression. unv_utilities.hxx 63

 inline bool beginning_of_dataset(....) { .... while( ((olds != "-1") || (news == "-1") ) && !in_file.eof() ){ olds = news; in_file >> news; } .... } 

When working with the 'std :: istream' class, it is not enough to call the 'eof ()' function to end the loop. In case of a failure in reading data, the call to the 'eof () function will always return the value' false '. To complete the loop in this case, additional verification of the value returned by the 'fail ()' function is necessary.

V595 The 'anElem' pointer was used before it was verified against nullptr. Check lines: 1950, 1951. smesh_controls.cpp 1950

 bool ElemGeomType::IsSatisfy( long theId ) { if (!myMesh) return false; const SMDS_MeshElement* anElem = myMesh->FindElement( theId ); const SMDSAbs_ElementType anElemType = anElem->GetType(); if (!anElem || (myType != SMDSAbs_All && anElemType != myType)) return false; const int aNbNode = anElem->NbNodes(); .... } 

The anElem pointer is derefered a line higher than is checked for validity.

Some more similar places in this project:

• V595 'ele m pointer pointer pointer pointer pointer pointer Check lines: 3989, 3990. smesh_mesheditor.cpp 3989
• V595 The 'anOldGrp' pointer was used before it was verified against nullptr. Check lines: 1488, 1489. smesh_mesh.cpp 1488
• V595 The 'aFaceSubmesh' pointer was used before it was verified against nullptr. Check lines: 496, 501. smesh_pattern.cpp 496

Boost C ++ Libraries

V567 Undefined behavior. This is a variable. regex_token_iterator.hpp 63

 template<typename BidiIter> struct regex_token_iterator_impl : counted_base<regex_token_iterator_impl<BidiIter> > { .... if(0 != (++this->n_ %= (int)this->subs_.size()) || .... { .... } .... } 

It is not known which of the operands of the operator% = will be calculated first. Accordingly, the expression works correctly or not, depends on luck.

Conclusion

Try and implement static analyzers to regularly check your projects, as well as used third-party libraries. This will save time when writing new code, as well as with the support of the old.


If you want to share this article with an English-speaking audience, then please use the link to the translation: Svyatoslav Razmyslov. Analyzing FreeCAD's “Sick“ Dependencies .