Celestia — трехмерный космический симулятор. Симуляция космоса позволяет исследовать нашу вселенную в трех измерениях. Celestia доступна на Windows, Linux и macOS. Проект очень маленький и в нём, с помощью PVS-Studio, обнаруживается совсем небольшое количество дефектов. Однако нам очень хочется уделить ему внимание, так как это популярный образовательный проект, который полезно улучшить. Кстати, программа используется в популярных фильмах, сериалах и передачах для представления космоса. Что тоже повышает требования к качеству кода.
Введение
На официальном сайте проекта Celestia можно найти его подробное описание. Исходный код проекта размещён на GitHub. Исключив библиотеки и тесты, анализатор проверил 166 .cpp файлов. Проект маленький, но найденные дефекты достаточно интересные.
Для анализа кода использовался статический анализатор кода PVS-Studio. И Celestia, и PVS-Studio являются кроссплатформенными. Для анализа была выбрана платформа Windows. Проект было легко собрать, подтянув зависимости с помощью Vcpkg — менеджера библиотек Microsoft. По отзывам он уступает возможностям Conan, но этой программой тоже было удобно пользоваться.
Результаты анализа
Предупреждение 1
V501 There are identical sub-expressions to the left and to the right of the '<' operator: b.nAttributes < b.nAttributes cmodfix.cpp 378
bool operator<(const Mesh::VertexDescription& a,
const Mesh::VertexDescription& b)
{
if (a.stride < b.stride)
return true;
if (b.stride < a.stride)
return false;
if (a.nAttributes < b.nAttributes) // <=
return true;
if (b.nAttributes < b.nAttributes) // <=
return false;
for (uint32_t i = 0; i < a.nAttributes; i++)
{
if (a.attributes[i] < b.attributes[i])
return true;
else if (b.attributes[i] < a.attributes[i])
return false;
}
return false;
}Как же легко ошибиться при копировании кода. Пишем об этом в каждом обзоре. Видимо, только статический анализ кода способен выручить в этой ситуации.
Программист скопировал условное выражение и не до конца его отредактировал. Правильный вариант, скорее всего, такой:
if (a.nAttributes < b.nAttributes)
return true;
if (b.nAttributes < a.nAttributes)
return false;Интересное исследование на эту тему: "Зло живёт в функциях сравнения".
Предупреждение 2
V575 The 'memset' function processes '0' elements. Inspect the third argument. winmain.cpp 2235
static void BuildScriptsMenu(HMENU menuBar, const fs::path& scriptsDir)
{
....
MENUITEMINFO info;
memset(&info, sizeof(info), 0);
info.cbSize = sizeof(info);
info.fMask = MIIM_SUBMENU;
....
}Автор кода перепутал второй и третий аргументы функции memset. Вместо заполнения структуры нулями, указано заполнять 0 байт памяти.
Предупреждение 3
V595 The 'destinations' pointer was utilized before it was verified against nullptr. Check lines: 48, 50. wintourguide.cpp 48
BOOL APIENTRY TourGuideProc(....)
{
....
const DestinationList* destinations = guide->appCore->getDestinations();
Destination* dest = (*destinations)[0];
guide->selectedDest = dest;
if (hwnd != NULL && destinations != NULL)
{
....
}
....
}Указатель destinations разыменовывается на две строки выше, чем сравнивается с NULL. Такой код может потенциально привести к ошибке.
Предупреждение 4
V702 Classes should always be derived from std::exception (and alike) as 'public' (no keyword was specified, so compiler defaults it to 'private'). fs.h 21
class filesystem_error : std::system_error
{
public:
filesystem_error(std::error_code ec, const char* msg) :
std::system_error(ec, msg)
{
}
}; // filesystem_errorАнализатор обнаружил класс, унаследованный от класса std::exception через модификатор private (заданный по умолчанию). Данное наследование опасно тем, что при непубличном наследовании, при попытке поймать исключение std::exception, оно будет пропущено. Как результат, обработчики исключений ведут себя не так, как задумывалось.
Предупреждение 5
V713 The pointer 's' was utilized in the logical expression before it was verified against nullptr in the same logical expression. winmain.cpp 3031
static char* skipUntilQuote(char* s)
{
while (*s != '"' && s != '')
s++;
return s;
}В одном месте условного выражения забыли разыменовать указатель s. Получилось сравнение указателя, а не значения по нему. А это не имеет смысла в данной ситуации.
Предупреждение 6
V773 The function was exited without releasing the 'vertexShader' pointer. A memory leak is possible. modelviewwidget.cpp 1517
GLShaderProgram*
ModelViewWidget::createShader(const ShaderKey& shaderKey)
{
....
auto* glShader = new GLShaderProgram();
auto* vertexShader = new GLVertexShader();
if (!vertexShader->compile(vertexShaderSource.toStdString()))
{
qWarning("Vertex shader error: %s", vertexShader->log().c_str());
std::cerr << vertexShaderSource.toStdString() << std::endl;
delete glShader;
return nullptr;
}
....
}При выходе из функции освобождается память по указателю glShader, но не очищается по указателю vertexShader.
Такое же место ниже по коду:
- V773 The function was exited without releasing the 'fragmentShader' pointer. A memory leak is possible. modelviewwidget.cpp 1526
Предупреждение 7
V547 Expression '!inputFilename.empty()' is always true. makexindex.cpp 128
int main(int argc, char* argv[])
{
if (!parseCommandLine(argc, argv) || inputFilename.empty())
{
Usage();
return 1;
}
istream* inputFile = &cin;
if (!inputFilename.empty())
{
inputFile = new ifstream(inputFilename, ios::in);
if (!inputFile->good())
{
cerr << "Error opening input file " << inputFilename << 'n';
return 1;
}
}
....
}Повторная проверка наличия имени файла. Не является ошибкой, но из-за того, что проверка переменной inputFilename уже есть в начале функции, ниже проверку можно убрать, что сделает код более компактным.
Предупреждение 8
V556 The values of different enum types are compared: switch(ENUM_TYPE_A) { case ENUM_TYPE_B:… }. render.cpp 7457
enum LabelAlignment
{
AlignCenter,
AlignLeft,
AlignRight
};
enum LabelVerticalAlignment
{
VerticalAlignCenter,
VerticalAlignBottom,
VerticalAlignTop,
};
struct Annotation
{
....
LabelVerticalAlignment valign : 3;
....
};
void Renderer::renderAnnotations(....)
{
....
switch (annotations[i].valign)
{
case AlignCenter:
vOffset = -font[fs]->getHeight() / 2;
break;
case VerticalAlignTop:
vOffset = -font[fs]->getHeight();
break;
case VerticalAlignBottom:
vOffset = 0;
break;
}
....
}В операторе switch, перепутали значения перечислений. Из-за этого в одном месте сравниваются перечисления разных типов: LabelVerticalAlignment и AlignCenter.
Предупреждение 9
V581 The conditional expressions of the 'if' statements situated alongside each other are identical. Check lines: 2844, 2850. shadermanager.cpp 2850
GLVertexShader*
ShaderManager::buildParticleVertexShader(const ShaderProperties& props)
{
....
if (props.texUsage & ShaderProperties::PointSprite)
{
source << "uniform float pointScale;n";
source << "attribute float pointSize;n";
}
if (props.texUsage & ShaderProperties::PointSprite)
{
source << DeclareVarying("pointFade", Shader_Float);
}
....
}Анализатор обнаружил два одинаковых условных выражения подряд. Тут либо допущена ошибка, либо два условия можно объединить в одно, и тем самым упростить код.
Предупреждение 10
V668 There is no sense in testing the 'dp' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. windatepicker.cpp 625
static LRESULT
DatePickerCreate(HWND hwnd, CREATESTRUCT& cs)
{
DatePicker* dp = new DatePicker(hwnd, cs);
if (dp == NULL)
return -1;
....
}Значение указателя, возвращённого оператором new, сравнивается с нулём. Если оператор не смог выделить память, то согласно стандарту языка Си++, генерируется исключение std::bad_alloc(). Таким образом, проверять указатель на равенство нулю не имеет смысла.
Ещё три аналогичных проверки:
- V668 There is no sense in testing the 'modes' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. winmain.cpp 2967
- V668 There is no sense in testing the 'dropTarget' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. winmain.cpp 3272
- V668 There is no sense in testing the 'appCore' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. winmain.cpp 3352
Предупреждение 11
V624 The constant 3.14159265 is being utilized. The resulting value could be inaccurate. Consider using the M_PI constant from <math.h>. 3dstocmod.cpp 62
int main(int argc, char* argv[])
{
....
Model* newModel = GenerateModelNormals(*model,
float(smoothAngle * 3.14159265 / 180.0), weldVertices, weldTolerance);
....
}Диагностика рекомендательная, но здесь действительно лучше воспользоваться готовой константой для числа Pi из стандартной библиотеки.
Заключение
В последнее время проект развивается силами энтузиастов, но по-прежнему популярен и востребован в учебных программах. В интернете есть тысячи дополнений с разными космическими объектами. Celestia использовалась в фильме "The Day After Tomorrow" и документальном сериале "Through the Wormhole with Morgan Freeman".
Мы рады, что проверяя многие проекты с открытым исходным кодом, мы не только популяризуем методологию статического анализа кода, но и вносим вклад в развитие мира открытых проектов. Вы, кстати, тоже можете использовать анализатор PVS-Studio не только для проверки своих собственных, но и сторонних проектов в качестве энтузиаста. Для этого можно воспользоваться одним из вариантов бесплатного лицензирования.
Используйте статические анализаторы кода, делайте свои проекты надёжней и качественней!
Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Svyatoslav Razmyslov. Celestia: Bugs' Adventures in Space.
Автор: Святослав
