This article was initially meant as a review of bugs found in the FreeCAD open-source project but eventually took a bit different direction. It happened because a considerable portion of the warnings had been generated for the third-party libraries employed by the project. Extensive use of third-party libraries in software development is highly beneficial, especially in the open-source software domain. And bugs found in these libraries are no good reason to reject them. But we still should keep in mind that third-party code we use in our projects may contain bugs, so we must be prepared to meet and, if possible, fix them, thus improving the libraries.
FreeCAD is a free and open-source general purpose parametric 3D CAD modeler allowing creating 3D models and drawing their projections. FreeCAD's developer Juergen Riegel, working at DaimlerChrysler corporation, positions his program as the first free mechanical engineering and design tool. There is a well-known issue in a number of related areas that deals with the lack of a full-fledged open-source CAD application, and the FreeCAD project is just aiming to become one. So let's check its source code with PVS-Studio to help this open-source project become a bit better. I bet you encounter "glitches" in various modelers every now and then when you can't hit a certain point or align a line which is constantly shifting one pixel away from the desired position. All of that may be just a result of some typos in the source code.
The FreeCAD project is cross-platform and there is a very good collection of documents on building it available at their site. It wasn't difficult to get project files for Visual Studio Community 2013 for further analysis by the PVS-Studio plugin installed on my computer. But for some reason, the check wouldn't go well at first...
As I found out, the cause of the analyzer's internal error had been the presence of a binary sequence in the preprocessed text *.i file. The analyzer can sort out issues like that but it was something unfamiliar this time. The trouble was with one of the lines in the source-file compilation parameters.
/FI"Drawing.dir/Debug//Drawing_d.pch"The /FI (Name Forced Include File) compilation switch, just like the #include directive, serves for including text header files. But in this case, the programmers are trying to include a file with binary data. It even manages to compile somehow - I guess Visual C++ simply ignores it.
But if we try to preprocess those files, instead of compiling them, Visual C++ will display an error message. However, the Clang compiler, used in PVS-Studio by default, included the binary file into the *.i file without much thinking. PVS-Studio never expected such a trap and went crazy.
To make it clearer, here's a fragment of the file preprocessed by Clang:
I carefully checked the project without that switch but the authors ought to know that they have an error there.
The first bug samples to be discussed result from a very well-known issue.
V501 There are identical sub-expressions 'surfaceTwo->IsVRational()' to the left and to the right of the '!=' 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;
....
}Because of a tiny typo, there is the wrong variable "surfaceTwo" instead of "surfaceOne" found to the left of the inequality operator. I can just recommend to copy-paste larger text blocks next time, though we will speak of such samples a bit later, too =).
V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability 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) {
}
}Why are we discussing an incomplete function? Because this code will most likely face the same troubles as in the next two samples.
V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability 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"));
....
}Hopefully, the author forgot to fix only one copy-pasted line but still managed to fully implement the code searching for all the necessary lines.
V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability 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;
}
....
}And now there is what I suppose to be quite a serious bug for such an application. In modeling, a large bulk of work has to be done through mouse navigation, but we've got a problem with that: the source code under 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, there is always only one branch to be executed. I guess what the programmer really intended was the following:
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 is it exactly the last line that I have fixed? Well, probably you will like the following article on this subject: The Last Line Effect. But it is also possible that the first line should be fixed instead - I'm not sure :).
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;
}A bit more of "the last line effect" errors. What the analyzer didn't like about this code is the missing minus sign in the last line. But I can't say for sure if it is a bug or not in this particular case; it may be that the programmer, when implementing this conversion, just wanted to specifically emphasize that the fourth component doesn't get changed.
V576 Incorrect format. A different number of actual arguments is expected while calling '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 ); // <=
....
}This code doesn't make sense. If the pointer is null, you can simply print the character of the new string without passing unused parameters to the function.
V596 The object was created but it is not being 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 this code but not used. I guess the keyword "throw" is missing here:
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 issues of this kind:
V599 The virtual destructor is not present, although the 'Curve' class contains 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();
};The use:
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 to be created as default is not. And of course, it won't be virtual! It means that all the objects derived from this class won't be fully clear if used when a pointer to the child class is saved into a pointer to the base class. As the comment suggests, the base class has a lot of child ones, for example the "Line" class in the example above.
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 has detected a meaningless string summation. If you look close, you will notice that the programmer probably wanted to use the '+=' operator instead of simple addition. If so, this code would make sense.
V595 The 'root' pointer was utilized before it was verified against nullptr. 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 a pointer check in a wrong place, and all the rest issues are found in the following files:
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 sample, the programmer overwrites the value of the 'myIndex' array's item having index 1. I think the code was actually meant to look like this:
myIndex[1] = myIndex[0];
myIndex[0] = theIndex;V501 There are identical sub-expressions '0 <= theParamsHint.Y()' to the left and to the right of 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 ); // <=
....
}A check with .Z() is obviously missing here. And there is such a function in the class indeed: the class itself is even named "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;
}
....
}A pointer can't be less than zero. Even in the plainest examples with the fopen() function, that you can find in books and on the Internet, operators == or != are used to compare a function value to NULL.
I was wondering at how code like that could have appeared at all but my co-worker Andrey Karpov told me that such things often happen when refactoring code where the open() function was previously used. This function returns -1 in this case, so the comparison <0 is quite legal. In the course of program refactoring or porting, programmers replace this function with fopen() but forget to fix the check.
Another issue of this kind:
V562 It's odd to compare a bool type value with a 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;
}
....
}Here we have an enumeration named "SMESHDS_CommandType" containing a lot of constants. The analyzer has detected an incorrect check: a variable of this type is compared to a named constant, but what for is the negation symbol?? I bet the check should actually look like this:
if (myType != SMESHDS_MoveNode) // <=
{
MESSAGE("SMESHDS_Command::MoveNode : Bad Type");
return;
}Unfortunately, this check with message printing was copied to 20 other fragments. See the full list: FreeCAD_V562.txt.
V567 Undefined behavior. The order of argument evaluation is not defined for 'splice' function. The 'outerBndPos' variable is modified while being used twice between sequence points. smesh_pattern.cpp 4260
void SMESH_Pattern::arrangeBoundaries (....)
{
....
if ( outerBndPos != boundaryList.begin() )
boundaryList.splice( boundaryList.begin(),
boundaryList,
outerBndPos, // <=
++outerBndPos ); // <=
}The analyzer is actually not quite correct about this code. There's no undefined behavior here, but there is an error, so the warning was displayed not in vain. The C++ standard doesn't put any restrictions on the evaluation order of a function's actual arguments. So it is unknown which values will be passed into the function.
Let me clarify on it by a simple example:
int a = 5;
printf("%i, %i", a, ++a);This code may print both "5, 6" and "6, 6", which depends on the compiler and its settings.
V663 Infinite loop is possible. The 'cin.eof()' condition is insufficient to break 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's not enough to call the function 'eof()' to terminate the loop. If a failure occurs when reading the data, calling the 'eof()' function will always return 'false'. To terminate the loop in this case, we need an additional check for the value returned by the function 'fail()'.
V595 The 'anElem' pointer was utilized 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 dereferenced one line earlier than being checked for being valid.
Here are a few other similar issues in this project:
V567 Undefined behavior. The 'this->n_' variable is modified while being used twice between sequence points. 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 unknown which of the operands of the %= operator will be evaluated first. Therefore, the expression being correct or incorrect depends on pure chance.
Try and integrate static analyzers into the development process to run regular analysis of your projects and third-party libraries they use. It will help you save plenty of time when writing new code and maintaining old one.