Проверяем исходный код 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
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
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: 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 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
Заключение Пробуйте и внедряйте статические анализаторы для регулярной проверки своих проектов, а также используемых сторонних библиотек. Это сэкономит время при написании нового кода, а также при поддержке старого.Эта статья на английском Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Svyatoslav Razmyslov. Analyzing FreeCAD’s Source Code and Its «Sick» Dependencies.
