Хабрахабр

И снова в космос: как единорог Stellarium посещал

За все время своего существования люди приложили колоссальное количество усилий, чтобы изучить практически всю площадь звездного неба. На сегодняшний день мы рассмотрели сотни тысяч астероидов, комет, туманностей и звезд, галактик и планет. Чтобы увидеть всю эту красоту самостоятельно, не обязательно выходить из дома и покупать себе телескоп. Можно установить на компьютер 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-версию анализатора.

Отвечаю: я являюсь одним из разработчиков PVS-Studio, а единорог — это наш любимый озорной маскот. Новые читатели и пользователи Stellarium возможно задались вопросом: почему в заголовке статьи фигурирует единорог и как он связан с анализом кода? Итак, вверх!

Я надеюсь, что благодаря этой статье читатели узнают для себя что-то новое, а разработчики Stellarium смогут устранить часть ошибок и улучшить качество кода.

Приносите себе кофе с воздушным круассаном и устраивайтесь поудобнее, ведь мы переходим к самому интересному — обзору результатов анализа и разбору ошибок!

Подозрительные условия

Для большего удовольствия от чтения предлагаю не смотреть сразу на предупреждение анализатора, а попробовать здесь и далее найти ошибки самостоятельно.

void QZipReaderPrivate::scanFiles()
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

Если да, то хвалю. Смогли найти ошибку?

Оно всегда верно, так как переменная start_of_directory не меняется в теле цикла. Ошибка кроется в условии цикла while. Скорее всего, цикл не будет вечным, так как он содержит return и break, но выглядит такой код странно.

Тогда и оператор break, пожалуй, лишний. Как мне кажется, в коде забыли сделать присваивание start_of_directory = pos в том месте, где идёт проверка сигнатуры. В этом случае код можно переписать так:

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.

Как правило, такие ошибки связаны с copy-paste. Подобный тип ошибок встречается крайне часто: практически в каждом проверенном проекте мы встречали опечатки, связанные с именами вида имя1 и имя2 и им подобными.

Мой коллега Андрей Карпов назвал его "эффектом последней строки". Данный экземпляр кода является ярким примером ещё одного распространенного паттерна ошибок, по поводу которого мы даже проводили отдельное мини-исследование. Если вы ещё не знакомы с этим материалом, то предлагаю открыть вкладку в браузере, чтобы почитать позже, а пока продолжим.

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. Если один из них оказывается нулевым, то освобождается вся зарезервированная внутри функции память, после чего поток управления покидает функцию.

Поток управления достигнет второго по счету return. Что же произойдет, если память под все три структуры выделится корректно, а функция MakeEdge(&mesh->eHead) вернет NULL?

Но освобождения памяти, которая им принадлежала, не произойдет. Так как указатели newVertex1, newVertex2 и newFace являются локальными переменными, то после выхода из функции они прекратят своё существование. Она останется зарезервированной, но доступа к ней мы больше иметь не будем.

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

Функции MakeVertex и MakeFace передают адреса выделенной памяти в другие структуры данных, тем самым делегируя ответственность за её освобождение. Следует отметить, что в данном примере третий return не является ошибочным.

Для удобства я сократил его, оставив только проблемные места. Следующая недоработка находится в методе, который занимает 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 '

Рассмотрим подробнее выражение (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);
}

Заметили что-то подозрительное? Не каждый сможет 🙂

If you wish to call constructor, 'this->Plane::Plane(....)' should be used. Предупреждение PVS-Studio: V603 The object was created but it is not being 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. Нам, землянам, свойственно ошибаться, и нет ничего постыдного в том, что эти ошибки просачиваются в код. Если вы один из землян — ставьте лайк предлагаю скачать и попробовать самому.

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

Подписывайтесь на наши каналы и следите за новостями из мира программирования!

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: George Gribkov. Into Space Again: how the Unicorn Visited Stellarium

Показать больше

Похожие публикации

Добавить комментарий

Ваш адрес email не будет опубликован. Обязательные поля помечены *

Кнопка «Наверх»