За все время своего существования люди приложили колоссальное количество усилий, чтобы изучить практически всю площадь звездного неба. На сегодняшний день мы рассмотрели сотни тысяч астероидов, комет, туманностей и звезд, галактик и планет. Чтобы увидеть всю эту красоту самостоятельно, не обязательно выходить из дома и покупать себе телескоп. Можно установить на компьютер Stellarium — виртуальный планетарий, и посмотреть на ночное небо, с комфортом лежа на диване… Но с комфортом ли? Чтобы выяснить ответ на этот вопрос, проверим Stellarium на наличие ошибок в компьютерном коде.
Немного о проекте...
Согласно описанию на сайте Wikipedia, Stellarium — это виртуальный планетарий с открытым исходным кодом, доступный для платформ Linux, Mac OS X, Microsoft Windows, Symbian, Android и iOS, а также MeeGo. Программа использует технологии OpenGL и Qt, чтобы создавать реалистичное небо в режиме реального времени. Со Stellarium возможно увидеть то, что можно видеть средним и даже крупным телескопом. Также программа предоставляет наблюдения за солнечными затмениями и движением комет.
Stellarium создан французским программистом Фабианом Шеро, который запустил проект летом 2001 года. Другие видные разработчики включают Роберта Спирмана, Джохэйннса Гадждозика, Мэтью Гейтса, Тимоти Ривза, Богдана Маринова и Джохана Меериса, который является ответственным за художественные работы.
… и об анализаторе
Анализ проекта проводился с помощью статического анализатора кода PVS-Studio. Это инструмент для выявления ошибок и потенциальных уязвимостей в исходном коде программ, написанных на языках С, C++ и C# (в скором времени и на Java!). Работает в среде Windows, Linux и macOS. Он разработан для тех, кому важно повышать качество своего кода.
Провести анализ было достаточно просто. Сначала я скачал проект Stellarium с GitHub, после чего установил все необходимые для сборки пакеты. Так как проект собирается с помощью Qt Creator, я использовал систему отслеживания запуска компиляторов, которая является встроенной в Standalone-версию анализатора. Там же можно просмотреть готовый отчёт об анализе.
Новые читатели и пользователи Stellarium возможно задались вопросом: почему в заголовке статьи фигурирует единорог и как он связан с анализом кода? Отвечаю: я являюсь одним из разработчиков PVS-Studio, а единорог — это наш любимый озорной маскот. Итак, вверх!
Я надеюсь, что благодаря этой статье читатели узнают для себя что-то новое, а разработчики Stellarium смогут устранить часть ошибок и улучшить качество кода.
Приносите себе кофе с воздушным круассаном и устраивайтесь поудобнее, ведь мы переходим к самому интересному — обзору результатов анализа и разбору ошибок!
Подозрительные условия
Для большего удовольствия от чтения предлагаю не смотреть сразу на предупреждение анализатора, а попробовать здесь и далее найти ошибки самостоятельно.
void QZipReaderPrivate::scanFiles()
{
....
// find EndOfDirectory header
int i = 0;
int start_of_directory = -1;
EndOfDirectory eod;
while (start_of_directory == -1) {
const int pos = device->size()
- int(sizeof(EndOfDirectory)) - i;
if (pos < 0 || i > 65535) {
qWarning() << "QZip: EndOfDirectory not found";
return;
}
device->seek(pos);
device->read((char *)&eod, sizeof(EndOfDirectory));
if (readUInt(eod.signature) == 0x06054b50)
break;
++i;
}
....
}Предупреждение PVS-Studio: V654 The condition 'start_of_directory == — 1' of loop is always true. qzip.cpp 617
Смогли найти ошибку? Если да, то хвалю.
Ошибка кроется в условии цикла while. Оно всегда верно, так как переменная start_of_directory не меняется в теле цикла. Скорее всего, цикл не будет вечным, так как он содержит return и break, но выглядит такой код странно.
Как мне кажется, в коде забыли сделать присваивание start_of_directory = pos в том месте, где идёт проверка сигнатуры. Тогда и оператор break, пожалуй, лишний. В этом случае код можно переписать так:
int i = 0;
int start_of_directory = -1;
EndOfDirectory eod;
while (start_of_directory == -1) {
const int pos = device->size()
- int(sizeof(EndOfDirectory)) - i;
if (pos < 0 || i > 65535) {
qWarning() << "QZip: EndOfDirectory not found";
return;
}
device->seek(pos);
device->read((char *)&eod, sizeof(EndOfDirectory));
if (readUInt(eod.signature) == 0x06054b50)
start_of_directory = pos;
++i;
}Впрочем, я не уверен, что код должен быть именно таким. Лучше всего, чтобы сами разработчики проекта проанализировали эту часть программы и внесли нужные правки.
Еще одно странное условие:
class StelProjectorCylinder : public StelProjector
{
public:
....
protected:
....
virtual bool
intersectViewportDiscontinuityInternal(const Vec3d& capN,
double capD) const
{
static const SphericalCap cap1(1,0,0);
static const SphericalCap cap2(-1,0,0);
static const SphericalCap cap3(0,0,-1);
SphericalCap cap(capN, capD);
return cap.intersects(cap1)
&& cap.intersects(cap2)
&& cap.intersects(cap2);
}
};Предупреждение PVS-Studio: V501 There are identical sub-expressions 'cap.intersects(cap2)' to the left and to the right of the '&&' operator. StelProjectorClasses.hpp 175
Как вы уже, наверное, догадались, ошибка кроется в последней строчке функции: программист допустил опечатку, и в итоге получилось, что функция возвращает результат независимо от значения cap3.
Подобный тип ошибок встречается крайне часто: практически в каждом проверенном проекте мы встречали опечатки, связанные с именами вида имя1 и имя2 и им подобными. Как правило, такие ошибки связаны с copy-paste.
Данный экземпляр кода является ярким примером ещё одного распространенного паттерна ошибок, по поводу которого мы даже проводили отдельное мини-исследование. Мой коллега Андрей Карпов назвал его "эффектом последней строки". Если вы ещё не знакомы с этим материалом, то предлагаю открыть вкладку в браузере, чтобы почитать позже, а пока продолжим.
void BottomStelBar::updateText(bool updatePos)
{
....
updatePos = true;
....
if (location->text() != newLocation || updatePos)
{
updatePos = true;
....
}
....
if (fov->text() != str)
{
updatePos = true;
....
}
....
if (fps->text() != str)
{
updatePos = true;
....
}
if (updatePos)
{
....
}
}Предупреждения PVS-Studio:
- V560 A part of conditional expression is always true: updatePos. StelGuiItems.cpp 732
- V547 Expression 'updatePos' is always true. StelGuiItems.cpp 831
- V763 Parameter 'updatePos' is always rewritten in function body before being used. StelGuiItems.cpp 690
Значение параметра updatePos всегда перезаписывается до того, как будет использовано, т.е. функция отработает одинаково, независимо от переданного ей значения.
Выглядит странно, не так ли? Во всех местах, где участвует параметр updatePos, он имеет значение true. Это значит, что условия if (location->text() != newLocation || updatePos) и if (updatePos) будут всегда истинны.
Еще один фрагмент:
void LandscapeMgr::onTargetLocationChanged(StelLocation loc)
{
....
if (pl && flagEnvironmentAutoEnabling)
{
QSettings* conf = StelApp::getInstance().getSettings();
setFlagAtmosphere(pl->hasAtmosphere()
& conf->value("landscape/flag_atmosphere", true).toBool());
setFlagFog(pl->hasAtmosphere()
& conf->value("landscape/flag_fog", true).toBool());
setFlagLandscape(true);
}
....
}Предупреждения PVS-Studio:
- V792 The 'toBool' function located to the right of the operator '&' will be called regardless of the value of the left operand. Perhaps, it is better to use '&&'. LandscapeMgr.cpp 782
- V792 The 'toBool' function located to the right of the operator '&' will be called regardless of the value of the left operand. Perhaps, it is better to use '&&'. LandscapeMgr.cpp 783
Анализатор выявил подозрительное выражение в аргументах функций setFlagAtmosphere и setFlagFog. Действительно: по обеим сторонам от битового оператора & находятся значения, имеющие тип bool. Вместо оператора & стоит использовать оператор &&, и сейчас я объясню почему.
Да, результат этого выражения будет всегда корректен. Перед применением побитового «и» оба операнда будут повышаться до типа int. В языке C++ такая конвертация однозначна: false конвертируется в 0, а true конвертируется в 1. Поэтому результат у данного выражения будет таким же, как если бы использовался оператор &&.
Но есть нюанс. При подсчете результата операции && используется так называемое «ленивое вычисление». Если значение левого операнда является false, то правое значение даже не вычисляется, ведь логическое «и» в любом случае вернет false. Это сделано для экономии вычислительных ресурсов и позволяет писать более сложные конструкции. Например, можно проверить, что указатель не нулевой, и, если это так, разыменовать его для выполнения дополнительной проверки. Пример: if (ptr && ptr->foo()).
Такое «ленивое вычисление» не производится при использовании побитового оператора &: выражения conf->value("...", true).toBool() будут вычисляться каждый раз, независимо от значения pl->hasAtmosphere().
В редких случаях это бывает сделано специально. Например, если вычисление правого операнда имеет «побочные эффекты», результат которых используется позже. Так тоже делать не очень хорошо, потому что это затрудняет понимание кода и усложняет уход за ним. К тому же, порядок вычисления операндов & не определен, поэтому в некоторых случаях использования таких «трюков» можно получить неопределенное поведение.
Если нужно сохранить побочные эффекты — сделайте это в отдельной строке и сохраните результат в отдельную переменную. Люди, которые будут работать с этим кодом в дальнейшем, будут вам благодарны :)
Переходим к следующей теме.
Неправильная работа с памятью
Начнем тему динамической памяти с вот такого фрагмента:
/************ Basic Edge Operations ****************/
/* __gl_meshMakeEdge creates one edge,
* two vertices, and a loop (face).
* The loop consists of the two new half-edges.
*/
GLUEShalfEdge* __gl_meshMakeEdge(GLUESmesh* mesh)
{
GLUESvertex* newVertex1 = allocVertex();
GLUESvertex* newVertex2 = allocVertex();
GLUESface* newFace = allocFace();
GLUEShalfEdge* e;
/* if any one is null then all get freed */
if ( newVertex1 == NULL
|| newVertex2 == NULL
|| newFace == NULL)
{
if (newVertex1 != NULL)
{
memFree(newVertex1);
}
if (newVertex2 != NULL)
{
memFree(newVertex2);
}
if (newFace != NULL)
{
memFree(newFace);
}
return NULL;
}
e = MakeEdge(&mesh->eHead);
if (e == NULL)
{
return NULL;
}
MakeVertex(newVertex1, e, &mesh->vHead);
MakeVertex(newVertex2, e->Sym, &mesh->vHead);
MakeFace(newFace, e, &mesh->fHead);
return e;
}Предупреждения PVS-Studio:
- V773 The function was exited without releasing the 'newVertex1' pointer. A memory leak is possible. mesh.c 312
- V773 The function was exited without releasing the 'newVertex2' pointer. A memory leak is possible. mesh.c 312
- V773 The function was exited without releasing the 'newFace' pointer. A memory leak is possible. mesh.c 312
Функция выделяет память для нескольких структур и передает её указателям newVertex1, newVertex2 (интересные имена, правда?) и newFace. Если один из них оказывается нулевым, то освобождается вся зарезервированная внутри функции память, после чего поток управления покидает функцию.
Что же произойдет, если память под все три структуры выделится корректно, а функция MakeEdge(&mesh->eHead) вернет NULL? Поток управления достигнет второго по счету return.
Так как указатели newVertex1, newVertex2 и newFace являются локальными переменными, то после выхода из функции они прекратят своё существование. Но освобождения памяти, которая им принадлежала, не произойдет. Она останется зарезервированной, но доступа к ней мы больше иметь не будем.
Такие ситуации называются «утечка памяти». Типичный сценарий при такой ошибке: при продолжительной работе программы она начинает потреблять все больше и больше оперативной памяти, вплоть до полного её исчерпания.
Следует отметить, что в данном примере третий return не является ошибочным. Функции MakeVertex и MakeFace передают адреса выделенной памяти в другие структуры данных, тем самым делегируя ответственность за её освобождение.
Следующая недоработка находится в методе, который занимает 90 строк. Для удобства я сократил его, оставив только проблемные места.
void AstroCalcDialog::drawAngularDistanceGraph()
{
....
QVector<double> xs, ys;
....
}Осталась всего одна строчка. Дам подсказку: это единственное упоминание объектов xs и ys.
Предупреждения PVS-Studio:
- V808 'xs' object of 'QVector' type was created but was not utilized. AstroCalcDialog.cpp 5329
- V808 'ys' object of 'QVector' type was created but was not utilized. AstroCalcDialog.cpp 5329
Векторы xs и ys создаются, но нигде не используются. Получается, что при каждом использовании метода drawAngularDistanceGraph происходит лишнее создание и удаление пустого контейнера. Думаю, это объявление осталось в коде после рефакторинга. Это, конечно, не ошибка, но стоит удалить лишний код.
Странные приведения типов
Еще один пример после небольшого форматирования выглядит вот так:
void SatellitesDialog::updateSatelliteData()
{
....
// set default
buttonColor = QColor(0.4, 0.4, 0.4);
....
}Для того, чтобы понять, в чем ошибка, вам придется посмотреть на прототипы конструкторов класса Qcolor:
Предупреждения PVS-Studio:
- V674 The literal '0.4' of 'double' type is being implicitly cast to 'int' type while calling the 'QColor' function. Inspect the first argument. SatellitesDialog.cpp 413
- V674 The literal '0.4' of 'double' type is being implicitly cast to 'int' type while calling the 'QColor' function. Inspect the second argument. SatellitesDialog.cpp 413
- V674 The literal '0.4' of 'double' type is being implicitly cast to 'int' type while calling the 'QColor' function. Inspect the third argument. SatellitesDialog.cpp 413
У класса Qcolor не существует конструкторов, принимающих тип double, поэтому аргументы в примере будут неявно преобразовываться в int. Это приводит к тому, что поля r, g, b объекта buttonColor будут иметь значения 0.
Если программист намеревался создать объект из значений типа double, ему следовало использовать другой конструктор.
Например, можно было использовать конструктор, принимающий Qrgb, написав:
buttonColor = QColor(QColor::fromRgbF(0.4, 0.4, 0.4));Можно было сделать и по-другому. В Qt для обозначения RGB-цветов используются вещественные значения в диапазоне [0.0, 1.0] или целочисленные в диапазоне [0, 255].
Поэтому программист мог перевести значения из вещественных в целочисленные, написав вот так:
buttonColor = QColor((int)(255 * 0.4),
(int)(255 * 0.4),
(int)(255 * 0.4));или просто
buttonColor = QColor(102, 102, 102);Заскучали? Не переживайте: впереди нас ждут более интересные ошибки.
«Единорог в космосе». Вид из Stellarium.
Прочие ошибки
Напоследок я оставил вам еще несколько вкусняшек :) Приступим к одной из них.
HipsTile* HipsSurvey::getTile(int order, int pix)
{
....
if (order == orderMin && !allsky.isNull())
{
int nbw = sqrt(12 * 1 << (2 * order));
int x = (pix % nbw) * allsky.width() / nbw;
int y = (pix / nbw) * allsky.width() / nbw;
int s = allsky.width() / nbw;
QImage image = allsky.copy(x, y, s, s);
....
}
....
}Предупреждение PVS-Studio: V634 The priority of the '*' operation is higher than that of the '<<' operation. It's possible that parentheses should be used in the expression. StelHips.cpp 271
Ну что, смогли обнаружить ошибку? Рассмотрим подробнее выражение (12 * 1 << (2 * order)). Анализатор напоминает, что операция '*' имеет более высокий приоритет, чем операция битового сдвига '<<'. Легко понять, что умножение 12 на 1 бессмысленно, а скобки вокруг 2 * order не нужны.
Скорее всего, программист собирался написать так:
int nbw = sqrt(12 * (1 << 2 * order));
В таком случае значение <i>12 </i> будет умножаться на корректное число.Примечание. Дополнительно хочу отметить, что если значение правого операнда '<<' больше или равно количеству битов левого операнда, то результат не определен. Так как численные литералы по умолчанию имеют тип int, который занимает 32 бита, значение параметра order не должно превышать 15. Иначе вычисление выражения может закончиться неопределенным поведением.
Продолжаем. Приведенный ниже метод является весьма запутанным, но я уверен, что искушенный читатель справится с обнаружением ошибки :)
/* inherits documentation from base class */
QCPRange QCPStatisticalBox::
getKeyRange(bool& foundRange, SignDomain inSignDomain) const
{
foundRange = true;
if (inSignDomain == sdBoth)
{
return QCPRange(mKey - mWidth * 0.5, mKey + mWidth * 0.5);
}
else if (inSignDomain == sdNegative)
{
if (mKey + mWidth * 0.5 < 0)
return QCPRange(mKey - mWidth * 0.5, mKey + mWidth * 0.5);
else if (mKey < 0)
return QCPRange(mKey - mWidth * 0.5, mKey);
else
{
foundRange = false;
return QCPRange();
}
}
else if (inSignDomain == sdPositive)
{
if (mKey - mWidth * 0.5 > 0)
return QCPRange(mKey - mWidth * 0.5, mKey + mWidth * 0.5);
else if (mKey > 0)
return QCPRange(mKey, mKey + mWidth * 0.5);
else
{
foundRange = false;
return QCPRange();
}
}
foundRange = false;
return QCPRange();
}Предупреждение PVS-Studio: V779 Unreachable code detected. It is possible that an error is present. qcustomplot.cpp 19512.
Дело в том, что все ветви if...else имеют return. Поэтому поток управления никогда не дойдет до последних двух строк.
По большому счету, данный пример будет нормально выполняться и работать корректно. Но присутствие недостижимого кода само по себе является сигналом. В данном случае оно указывает на неправильную структуру метода, которая сильно усложняет читабельность и понятность кода.
Данный фрагмент кода следует подвергнуть рефакторингу, получив на выходе более опрятную функцию. Например, так:
/* inherits documentation from base class */
QCPRange QCPStatisticalBox::
getKeyRange(bool& foundRange, SignDomain inSignDomain) const
{
foundRange = true;
switch (inSignDomain)
{
case sdBoth:
{
return QCPRange(mKey - mWidth * 0.5, mKey + mWidth * 0.5);
break;
}
case sdNegative:
{
if (mKey + mWidth * 0.5 < 0)
return QCPRange(mKey - mWidth * 0.5, mKey + mWidth * 0.5);
else if (mKey < 0)
return QCPRange(mKey - mWidth * 0.5, mKey);
break;
}
case sdPositive: {
if (mKey - mWidth * 0.5 > 0)
return QCPRange(mKey - mWidth * 0.5, mKey + mWidth * 0.5);
else if (mKey > 0)
return QCPRange(mKey, mKey + mWidth * 0.5);
break;
}
}
foundRange = false;
return QCPRange();
}Последней в нашем обзоре будет ошибка, которая понравилась мне больше всего. Код проблемного места краток и прост:
Plane::Plane(Vec3f &v1, Vec3f &v2, Vec3f &v3)
: distance(0.0f), sDistance(0.0f)
{
Plane(v1, v2, v3, SPolygon::CCW);
}Заметили что-то подозрительное? Не каждый сможет :)
Предупреждение PVS-Studio: V603 The object was created but it is not being used. If you wish to call constructor, 'this->Plane::Plane(....)' should be used. Plane.cpp 29
Программист рассчитывал, что часть полей объекта будет инициализирована внутри вложенного конструктора, но получилось так: когда вызывается конструктор Plane(Vec3f &v1, Vec3f &v2, Vec3f &v3), внутри него создается безымянный временный объект, который сразу же удаляется. В результате часть послей объекта остается неинициализированной.
Чтобы код заработал правильно, следует использовать удобную и безопасную фичу C++11 — делегирующий конструктор:
Plane::Plane(Vec3f& v1, Vec3f& v2, Vec3f& v3)
: Plane(v1, v2, v3, SPolygon::CCW)
{
distance = 0.0f;
sDistance = 0.0f;
}Но если вы используете компилятор для более старых версий языка, то можно написать так:
Plane::Plane(Vec3f &v1, Vec3f &v2, Vec3f &v3)
: distance(0.0f), sDistance(0.0f)
{
this->Plane::Plane(v1, v2, v3, SPolygon::CCW);
}Или так:
Plane::Plane(Vec3f &v1, Vec3f &v2, Vec3f &v3)
: distance(0.0f), sDistance(0.0f)
{
new (this) Plane(v1, v2, v3, SPolygon::CCW);
}Замечу, что последние два способа являются весьма опасными. Поэтому следует быть очень осторожным и хорошо понимать, как такие способы работают.
Заключение
Какие выводы можно сделать о качестве кода Stellarium? Честно говоря, ошибок было не очень много. Также во всем проекте я не обнаружил ни одной ошибки, в которой код завязан на неопределенном поведении. Для opensource-проекта качество кода оказалось на высоком уровне, за что я снимаю шляпу перед разработчиками. Ребята, вы молодцы! Мне было приятно и интересно делать обзор на ваш проект.
Что же насчет самого планетария — я пользуюсь им достаточно часто. К сожалению, живя в городе, я очень редко могу насладиться ясным ночным небом, а Stellarium позволяет мне оказаться в любой точке планеты, не вставая с дивана. Это действительно комфортно!
Особенно мне нравится режим «Constellation art». От вида огромных фигур, застилающих все небо в странном танце, захватывает дух.
«Странный танец». Вид из Stellarium.
Нам, землянам, свойственно ошибаться, и нет ничего постыдного в том, что эти ошибки просачиваются в код. Для этого и разрабатываются инструменты анализа кода, такие, как PVS-Studio. Если вы один из землян — ставьте лайк предлагаю скачать и попробовать самому.
Надеюсь, что вам было интересно читать мою статью, и вы узнали для себя что-нибудь новое и полезное. А разработчикам желаю скорейшего исправления найденных ошибок.
Подписывайтесь на наши каналы и следите за новостями из мира программирования!
- Vk: @pvsstudio_rus
- Telegram: @pvsstudio_rus
- Instagram: @pvsstudio_rus
- Twitter: @pvsstudio_rus
- YouTube: @PVSStudioTool
Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: George Gribkov. Into Space Again: how the Unicorn Visited Stellarium
Автор: GGribkov
