Проверяем исходный код FreeCAD и его «нехорошие» зависимости

Статья, которая задумывалась как обзор ошибок в открытом проекте FreeCAD, приобрела немного другой характер. Весомая часть предупреждений анализатора пришлась на используемые сторонние библиотеки. Разработка программного обеспечения с активным использованием сторонних библиотек даёт много плюсов, особенно в сфере Open Source. И ошибки в библиотеках не повод отказываться от них. Но надо понимать, что в используемом коде тоже могут жить баги. Их надо быть готовым встретить и по возможности исправить, тем самым улучшив используемые вами библиотеки.

FreeCAD — параметрический трехмерный редактор, позволяющий создавать объемные модели и чертежи их проекций. Разработчик FreeCAD Юрген Ригель, работающий в корпорации DaimlerChrysler, позиционирует свою программу как первый бесплатный инструмент проектирования механики. В среде специалистов ряда отраслей известна проблема создания полноценной САПР в рамках Open Source, и этот проект является кандидатом на такую «полноценность». Проверим же исходный код с помощью PVS-Studio и поможем открытому проекту в этой области стать чуточку лучше. Наверняка вы сталкивались с «глюками» в различных редакторах, когда не удаётся попасть в какую-нибудь точку или выпрямить линию, которая всегда съезжает на один пиксель. Возможно, причиной всего этого являются лишь опечатки в исходном коде.

Что с PVS-Studio?!

Проект FreeCAD является кросс-платформенным, на сайте есть очень хорошая документация по сборке. Мне не составило труда получить проектные файлы для Visual Studio Community 2013 для проверки с помощью установленного плагина PVS-Studio. Но в начале проверка не задалась…

Причиной внутренней ошибки в анализаторе стало наличие бинарной последовательности в текстовом препроцессированном файле с расширением *.i. Анализатор умеет обрабатывать такие ситуации, но тут произошло что-то новое. Проблема в одной из строчек в параметрах компиляции исходных файлов:

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

Флаг компиляции /FI (Name Forced Include File), как и директива #include, служат для включения текстовых заголовочных файлов. Но здесь пытаются включить файл, содержащий информацию в бинарном виде. Это каким-то чудом компилируется. Возможно, в Visual C++ такой файл просто игнорируется.

Если попытаться не компилировать, а именно препроцессировать файлы, то Visual C++ сообщает об ошибке. А вот используемый в PVS-Studio по умолчанию Clang, недолго думая, включил *.i файл бинарный файл. PVS-Studio не ожидал такого подвоха и сошёл с ума.

Чтобы было понятней, о чем идёт речь, вот фрагмент препроцессирпованного с помощью Clang файла:

Проект был аккуратно проверен без этого флага, но хочу обратить внимание разработчиков, что у них здесь ошибка.

FreeCAD

Первые примеры ошибок из проекта получены по известной всем причине.

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;   .... }

По левую сторону оператора неравенства обнаружилась не та переменная «surfaceTwo» вместо «surfaceOne» из-за маленькой опечатки. Осталось посоветовать автору в следующий раз делать copy-paste фрагментами побольше, но и до таких примеров мы тоже дойдём =).

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) {   } }

Чего это мы обратили внимание на функцию, которая ещё пишется? А вот почему: с этим кодом скорее всего будет тоже самое, что и в следующих двух примерах.

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"));     .... }

Будем надеется, что автор случайно не поправил одну скопированную строчку, но в итоге всё равно дописал поиск всех необходимых строк.

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;   }   .... }

А вот, на мой взгляд, серьёзная ошибка для такого приложения. При моделировании много работы выполняется с помощью навигации мышкой, а тут такой ляп: исходный код в последнем условии никогда не получает управление, потому что первое условие такое же и выполняется первым.

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); };

Независимо от условия, всегда выполняется одно действие. Может всё-таки так задумывалось:

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); };

Почему я исправил именно последнюю строку? Возможно, вас заинтересует статья на эту тему: Эффект последней строки. Но возможно, надо исправить первую. Не знаю :).

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

Ещё о «последних строках». Анализатор насторожился, так как в последней строке нет знака минуса. Но тут нельзя однозначно говорить об ошибке, возможно, при реализации такого преобразования, хотели подчеркнуть, что четвёртая компонента не изменяется.

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 );         //<==   .... }

Такой код не имеет смысла. Если указатель нулевой, то можно просто печатать символ новой строки без передачи в функцию неиспользуемых параметров.

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!"); }

В коде создаётся объект типа исключения, но не используется. По всей видимости пропущено ключевое слово «throw»:

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!"); }

Ещё несколько таких мест:

• V596 The object was created but it is not being used. The ‘throw’ keyword could be missing: throw Exception(FOO); application.cpp 274
• V596 The object was created but it is not being used. The ‘throw’ keyword could be missing: throw Exception(FOO); fileinfo.cpp 519
• V596 The object was created but it is not being used. The ‘throw’ keyword could be missing: throw Exception(FOO); waypointpyimp.cpp 244
• V596 The object was created but it is not being used. The ‘throw’ keyword could be missing: throw Exception(FOO); sketch.cpp 185

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(); };

Использование:

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; //<== } 

В базовом классе «Curve» объявлены виртуальные функции, но не объявлен деструктор, который будет создан по умолчанию. И он конечно будет не виртуальным! Это означает, что объекты, наследуемые от этого класса, не будут полностью очищены при таком сценарии использования, когда указатель на дочерний объект сохраняется в указатель на базовый класс. Судя по комментарию, у базового класса наследуемых классов много, например, приведённый класс «Line» в примере.

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); }

Анализатор обнаружил бессмысленное сложение строк. Если приглядеться, то, возможно, тут хотели использовать оператор ‘+=’ вместо простого сложения. Тогда такой код имел бы смысл.

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();   .... }

Один пример с проверкой указателя не в том месте, другие места находятся здесь:

• V595 The ‘cam’ pointer was utilized before it was verified against nullptr. Check lines: 1049, 1056. viewprovider.cpp 1049
• V595 The ‘viewProviderRoot’ pointer was utilized before it was verified against nullptr. Check lines: 187, 188. taskcheckgeometry.cpp 187
• V595 The ‘node’ pointer was utilized before it was verified against nullptr. Check lines: 209, 210. viewproviderrobotobject.cpp 209
• V595 The ‘node’ pointer was utilized before it was verified against nullptr. Check lines: 222, 223. viewproviderrobotobject.cpp 222
• V595 The ‘node’ pointer was utilized before it was verified against nullptr. Check lines: 235, 236. viewproviderrobotobject.cpp 235
• V595 The ‘node’ pointer was utilized before it was verified against nullptr. Check lines: 248, 249. viewproviderrobotobject.cpp 248
• V595 The ‘node’ pointer was utilized before it was verified against nullptr. Check lines: 261, 262. viewproviderrobotobject.cpp 261
• V595 The ‘node’ pointer was utilized before it was verified against nullptr. Check lines: 274, 275. viewproviderrobotobject.cpp 274
• V595 The ‘owner’ pointer was utilized before it was verified against nullptr. 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; }

В данном примере перезаписали значение элемента массива ‘myIndex’ с индексом 1. Мне кажется, хотели сделать так:

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

SALOME Smesh Module

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 );  //<==   .... }

Тут явно не хватает проверки .Z(). Такая функция у класса есть, он даже называется «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;   }   .... }

Указатель не может быть меньше нуля. Даже в самых простых примерах с функцией fopen(), которые можно найти в книгах и интернете, значение функции сравнивают с NULL с помощью == или !=.

Я удивился, как мог появиться такой код. Но мой коллега Андрей Карпов подсказал, что такое часто бывает при рефакторинге кода, где когда-то использовалась функция open(). Эта функция возвращает в случае значение -1 и сравнение <0 вполне уместно. В процессе рефакторинга или портирования программы, эту функцию заменяют на fopen(), но забывают исправить проверку.

Ещё одно такое место:

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

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;   }   .... }

Есть перечисление с именем «SMESHDS_CommandType», в нём много констант. Анализатор обнаружил некорректную проверку: переменная этого типа сравнивается с именованной константой, но что тут делает знак отрицания?? Скорее всего, проверка должна быть такой:

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

К сожалению, такая проверка с выводом сообщения об ошибке была скопирована ещё в 20 мест, вот полный список: 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 ); //<== }

На самом деле анализатор здесь не совсем прав. Неопределённого поведения здесь нет. Но ошибка есть, так что предупреждение выдано не зря. Стандарт C++ не накладывает ограничение, в каком порядке вычисляются фактические аргументы функции. Поэтому не известно, какие значения будут переданы в функцию.

Поясню на простом примере:

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

Этот код может распечатать как «5, 6», так и «6, 6. Результат зависит от компилятора и его настроек.

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;   }   .... }

При работе с классом ‘std::istream’ недостаточно вызова функции ‘eof()’ для завершения цикла. В случае возникновения сбоя при чтении данных, вызов функции ‘eof()’ будет всегда возвращать значение ‘false’. Для завершения цикла в этом случае необходима дополнительная проверка значения, возвращаемого функцией ‘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();   .... }

Указатель „anElem“ разыменовывается на строчку выше, чем проверяется на валидность.

Ещё несколько аналогичных мест в этом проекте:

• V595 The ‘elem’ pointer was utilized before it was verified against nullptr. Check lines: 3989, 3990. smesh_mesheditor.cpp 3989
• V595 The ‘anOldGrp’ pointer was utilized before it was verified against nullptr. Check lines: 1488, 1489. smesh_mesh.cpp 1488
• V595 The ‘aFaceSubmesh’ pointer was utilized before it was verified against nullptr. Check lines: 496, 501. smesh_pattern.cpp 496

Boost C++ Libraries

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()) || ....   {     ....   }   .... }

Неизвестно, какой из операндов оператора %= будет вычислен первым. Соответственно, правильно работает выражение или нет, зависит от везения.

Заключение

Пробуйте и внедряйте статические анализаторы для регулярной проверки своих проектов, а также используемых сторонних библиотек. Это сэкономит время при написании нового кода, а также при поддержке старого.

Эта статья на английском

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Svyatoslav Razmyslov. Analyzing FreeCAD’s Source Code and Its „Sick“ Dependencies.

ссылка на оригинал статьи http://habrahabr.ru/post/257079/