Статический анализ наиболее полезен при регулярных проверках. Особенно для таких активно развивающихся проектов как Blender. Пришло время проверить его вновь, и узнать, какие подозрительные места удастся найти на этот раз.
Blender - профессиональный пакет для создания трёхмерной компьютерной графики, включающий в себя средства моделирования, анимации, рендеринга, постобработки и монтажа видео со звуком, также использующийся для создания интерактивных игр.
Проект уже проверялся ранее. Результаты проверки версии 2.62 изложены в статье "Проверка проекта Blender с помощью PVS-Studio".
Со времени прошлой проверки размер исходного кода вместе с дополнительными библиотеками увеличился до 77 мегабайт. А его объём вырос до 2206 KLOC. На момент предыдущей поверки размер проекта составлял 68 мегабайт (2105 KLOC).
Рассчитать объём кода мне помогла утилита SourceMonitor. Эта утилита умеет анализировать код на языках C++, C, C#, VB.NET, Java, Delphi и рассчитывает различные метрики. Например, она может определить цикломатическую сложность ваших проектов, а также сформировать подробную статистику для каждого из файлов проекта. И показать результаты в виде таблицы или диаграмм.
Итак, эта статья расскажет про ошибки и подозрительные места, найденные в версии Blender 2.77a. Для проверки использовался анализатор PVS-Studio версии 6.05.
При активном использовании механизмов копирования и автоматического дополнения кода очень часто могут возникнуть ошибки в наименованиях различных переменных и констант. Такие ошибки могут привести к неверным результатам вычислений или непредвиденному поведению программы. В проекте Blender анализатор нашёл сразу несколько примеров. Рассмотрим их подробно.
CurvePoint::CurvePoint(CurvePoint *iA, CurvePoint *iB, float t3)
{
....
if ((iA->getPoint2D() - // <=
iA->getPoint2D()).norm() < 1.0e-6) { // <=
....
}
....
}V501 There are identical sub-expressions to the left and to the right of the '-' operator: iA->getPoint2D() - iA->getPoint2D() curve.cpp 136
Внутри функции CurvePoint происходит работа с двумя близкими по имени объектами iA и iB. Разные методы этих объектов постоянно пересекаются в различных операциях в довольно длинном дереве условий. В одном из условных блоков допущена опечатка. В результате происходит операция вычитания между свойством одного и того же объекта. Не зная особенностей кода, точно сказать, в каком из двух операндов допущена ошибка, невозможно. Предлагаю два возможных варианта её исправления:
if ((iA->getPoint2D()-iB->getPoint2D()).norm()<1.0e-6)....или
if ((iB->getPoint2D()-iA->getPoint2D()).norm()<1.0e-6)....Следующая ошибка тоже спряталась внутри условного оператора.
template<typename MatrixType, int QRPreconditioner>
void JacobiSVD<MatrixType, QRPreconditioner>::allocate(....)
{
....
if(m_cols>m_rows)m_qr_precond_morecols.allocate(*this);
if(m_rows>m_cols)m_qr_precond_morerows.allocate(*this);
if(m_cols!=m_cols)m_scaledMatrix.resize(rows,cols); // <=
}V501 There are identical sub-expressions to the left and to the right of the '!=' operator: m_cols != m_cols jacobisvd.h 819
В приведённом фрагменте кода происходит уравнение количества строк и столбцов внутри некой матрицы. Если количество соответственно не равно, происходит выделение памяти для новых элементов или их создание. А после, если были добавлены новые ячейки, происходит операция изменения размера матрицы. Но в результате ошибки в выражении условного оператора операция не произойдёт никогда, так как условие m_cols!=m_cols всегда ложно. В данном случае не имеет значения, какую из двух частей выражения требуется изменить, поэтому предлагаю такой вариант:
if(m_cols!=m_rows) m_scaledMatrix.resize(rows,cols)Ещё несколько проблемных мест, обнаруженных диагностикой V501:
Здесь опечатка в наименовании привела к более серьёзной ошибке.
int QuantitativeInvisibilityF1D::operator()(....)
{
ViewEdge *ve = dynamic_cast<ViewEdge*>(&inter);
if (ve) {
result = ve->qi();
return 0;
}
FEdge *fe = dynamic_cast<FEdge*>(&inter);
if (fe) {
result = ve->qi(); // <=
return 0;
}
....
}V522 Dereferencing of the null pointer 've' might take place. functions1d.cpp 107
Приведённая функция достаточно короткая, но опечатки могут подстерегать нас даже в простых функциях. Из кода видно, что последовательно создаются и проверяются два объекта. Но после проверки второго объекта возникла ошибка, и если fe успешно создан, то вместо него в результат записывается результат работы функции из первого объекта, который согласно ранним условиям не был создан. Это скорее всего приведёт к аварийному завершению программы, если исключение не перехватит обработчик более высокого уровня.
По всей видимости второй фрагмент кода писался с помощью Copy-Paste. И случайно в одном месте забыли поменять имя переменной ve. Правильный же код, скорее всего, должен был быть таким:
FEdge *fe = dynamic_cast<FEdge*>(&inter);
if (fe) {
result = fe->qi();
return 0;
}static ImBuf *accessor_get_ibuf(....)
{
ImBuf *ibuf, *orig_ibuf, *final_ibuf;
....
/* First try to get fully processed image from the cache. */
ibuf = accesscache_get(accessor,
clip_index,
frame,
input_mode,
downscale,
transform_key);
if (ibuf != NULL) {
return ibuf;
}
/* And now we do postprocessing of the original frame. */
orig_ibuf = accessor_get_preprocessed_ibuf(accessor,
clip_index,
frame);
if (orig_ibuf == NULL) {
return NULL;
}
....
if (downscale > 0) {
if (final_ibuf == orig_ibuf) {
final_ibuf = IMB_dupImBuf(orig_ibuf);
}
IMB_scaleImBuf(final_ibuf,
ibuf->x / (1 << downscale), // <=
ibuf->y / (1 << downscale)); // <=
}
....
if (input_mode == LIBMV_IMAGE_MODE_RGBA) {
BLI_assert(ibuf->channels == 3 || // <=
ibuf->channels == 4); // <=
}
....
return final_ibuf;
}Предупреждения:
В приведённом фрагменте видно, что проверка переменной ibuf в случае, если объект создан, прерывает функцию гораздо раньше, чем эта переменная используется. Возможно на этом можно было остановится и подтвердить факт разыменования указателя. Однако при более внимательном изучении кода и комментариев к нему, выясняется истинная причина ошибки. И на самом деле здесь опять допущена опечатка. В местах, указанных анализатором, на самом деле должна была использоваться переменная orig_ibuf вместо ibuf.
typedef enum eOutlinerIdOpTypes {
OUTLINER_IDOP_INVALID = 0,
OUTLINER_IDOP_UNLINK,
OUTLINER_IDOP_LOCAL,
....
} eOutlinerIdOpTypes;
typedef enum eOutlinerLibOpTypes {
OL_LIB_INVALID = 0,
OL_LIB_RENAME,
OL_LIB_DELETE,
} eOutlinerLibOpTypes;
static int outliner_lib_operation_exec(....)
{
....
eOutlinerIdOpTypes event; // <=
....
event = RNA_enum_get(op->ptr, "type");
switch (event) {
case OL_LIB_RENAME: // <=
{
....
}
case OL_LIB_DELETE: // <=
{
....
}
default:
/* invalid - unhandled */
break;
}
....
}Предупреждения:
В примере показаны два типа, являющиеся перечислениями. И то, что при написании кода в типе была допущена опечатка, вполне ожидаемо для таких, почти неотличимых по имени, типов.
Фактически код работает корректно. Но при этом он вводит в заблуждение несоответствием типов. Переменная получает значение одного перечисления и сравнивается с константами другого. Для исправления ошибки в данном случае достаточно изменить тип переменой event на eOutlinerLibOpTypes.
static void blf_font_draw_buffer_ex(....)
{
....
cbuf[3] = (unsigned char)((alphatest = ((int)cbuf[3] +
(int)(a * 255)) < 255) ? alphatest : 255);
....
}V593 Consider reviewing the expression of the 'A = B < C' kind. The expression is calculated as following: 'A = (B < C)'. blf_font.c 414
Несоблюдение приоритета операций - одна из часто встречающихся ошибок при работе со сложными выражениями. В данном случае это всего лишь опечатка, но она привела к нарушению логики работы тернарного оператора. Из-за неверно поставленной скобки возникла ошибка с приоритетом операций. Вдобавок портится значение переменной alphatest. Вместо значения, которое вычисляет тернарный оператор, переменной alphatest присваивается значение bool-типа, полученное в результате выполнения операции сравнения (<). А уже после этого тернарный оператор работает со значением переменой alphatest, и результат его работы не сохраняется. Для исправления ошибки необходимо изменить выражение следующим образом:
cbuf[3] = (unsigned char)(alphatest = (((int)cbuf[3] +
(int)(a * 255)) < 255) ? alphatest : 255);bool BKE_ffmpeg_alpha_channel_is_supported(RenderData *rd)
{
int codec = rd->ffcodecdata.codec;
if (codec == AV_CODEC_ID_QTRLE)
return true;
if (codec == AV_CODEC_ID_PNG)
return true;
if (codec == AV_CODEC_ID_PNG)
return true;
....
}V649 There are two 'if' statements with identical conditional expressions. The first 'if' statement contains function return. This means that the second 'if' statement is senseless. Check lines: 1672, 1675. writeffmpeg.c 1675
В функции проводится последовательная проверка значения переменой на соответствие флагу с помощью однострочных условий. В результате опечатки один из флагов проверяется дважды. Наверняка, вместо повторной проверки должна быть проверена другая константа. Но вариантов этих констант очень много, поэтому я не могу предположить, как именно надо исправить этот код.
bool BM_face_exists_overlap_subset(...., const int len)
{
int i;
....
for (i = 0; i < len; i++) {
BM_ITER_ELEM (f, &viter, varr[i], BM_FACES_OF_VERT) {
if ((f->len <= len) && (....)) {
BMLoop *l_iter, *l_first;
if (is_init == false) {
is_init = true;
for (i = 0; i < len; i++) { // <=
BM_ELEM_API_FLAG_ENABLE(varr[i], _FLAG_OVERLAP);
}
}
....
}
}
}
}V535 The variable 'i' is being used for this loop and for the outer loop. Check lines: 2204, 2212. bmesh_queries.c 2212
Использование одной и той же переменой во внешнем и вложенном цикле может привести к некорректному выполнению внешнего цикла. В данном случае это скорее всего не является ошибкой, так как цикл видимо ищет нужный элемент и завершается, и второй цикл срабатывает только в этом случае. Но всё равно использование единой переменой является опасным местом, и может привести к реальным ошибкам, если понадобится оптимизировать этот фрагмент кода.
Лишние фрагменты кода можно встретить в любой программе. Иногда это старый код, оставшийся после рефакторинга. А бывает, что лишние фрагменты служат для поддержания стиля кода проекта. Иногда такие части кода могут быть опасными. Другими словами, дублирующийся код часто указывает на наличие логических ошибок.
static void knife_add_single_cut(....)
{
....
if ((lh1->v && lh2->v) && // <=
(lh1->v->v && lh2->v && lh2->v->v) && // <=
(e_base = BM_edge_exists(lh1->v->v, lh2->v->v)))
{
....
return;
}
....
}V501 There are identical sub-expressions 'lh2->v' to the left and to the right of the '&&' operator. editmesh_knife.c 781
Это один из вариантов непродуманного условия. Это, конечно, не ошибка, а лишняя проверка, но это не значит, что код не нуждается в доработке. Условие состоит из нескольких выражений. При этом часть второго выражения полностью соответствует проверке одной из переменных в первом и является лишней. Для исправления требуется убрать лишнюю проверку lh2->v из второго выражения. После этого код станет гораздо наглядней.
Другой пример:
static int edbm_rip_invoke__vert(....)
{
....
if (do_fill) {
if (do_fill) {
....
}
}
....
}V571 Recurring check. The 'if (do_fill)' condition was already verified in line 751. editmesh_rip.c 752
Ещё один вариант логической ошибки. Здесь два абсолютно одинаковых выражения проверяются внутри внешнего и вложенного условия. Повторная проверка всегда будет выдавать одинаковый результат и не имеет смысла. Конечно, данный код никак не влияет на работу программы. Но неизвестно, как будет изменятся код со временем, и лишние проверки могут сбить с толку.
Излишние проверки встречаются далеко не в одном месте проекта. Вот ещё несколько мест, обнаруженных анализатором:
А третий пример является явным избыточным кодом:
static void writedata_do_write(....)
{
if ((wd == NULL) || wd->error ||
(mem == NULL) || memlen < 1) return;
if (wd->error) return;
....
}V649 There are two 'if' statements with identical conditional expressions. The first 'if' statement contains function return. This means that the second 'if' statement is senseless. Check lines: 331, 332. writefile.c 332
Здесь строка if (wd->error) return; просто лишняя, и функция завершится раньше, чем будет обработано это условие. Поэтому её требуется просто убрать.
static int select_less_exec(....)
{
....
if ((lastsel==0)&&(bp->hide==0)&&(bp->f1 & SELECT)){
if (lastsel != 0) sel = 1;
else sel = 0;
....
}
....
}V637 Two opposite conditions were encountered. The second condition is always false. Check lines: 938, 939. editcurve_select.c 938
Из фрагмента верно, что внутри внешнего условного блока расположено дополнительное условие. Вложенное условие противоположно основному и всегда выдаёт одинаковый результат, а переменная sel никогда не получит значение 1. Поэтому достаточно просто прописать sel = 0 без повторной проверки. Хотя, возможно ошибка должна быть исправлена изменением одного из условий. Я не участвую в разработке проекта и мне сложно судить, что здесь к чему.
DerivedMesh *fluidsimModifier_do(....)
{
....
if (!fluidmd || (fluidmd && !fluidmd->fss))
return dm;
....
}V728 An excessive check can be simplified. The '||' operator is surrounded by opposite expressions '!fluidmd' and 'fluidmd'. mod_fluidsim_util.c 528
В рамках одного условия проверяются противоположные значения одной и той же переменной. Такие условия часто встречаются в разных видах и вариациях. Они не наносят вреда при работе программы, но зря усложняют код. Данное выражение можно упростить и привести к следующему виду:
if (!fluidmd || !fluidmd->fss)) ....Похожие места:
Ещё один вариант такого условия:
void ED_transverts_create_from_obedit(....)
{
....
if ((tipsel && rootsel) || (rootsel)) {....}
....
}V686 A pattern was detected: (rootsel) || ((rootsel) && ...). The expression is excessive or contains a logical error. ed_transverts.c 325
Как и в примере выше, внутри одного выражения проверяется одна и та же переменная дважды. Такое выражение не является ошибочным, но явно перегружено лишней проверкой. Для придания коду большей компактности и наглядности его требуется упростить.
if ((tipsel || rootsel) {....}Подобные ошибки встретились и в других местах проекта.
static bool find_prev_next_keyframes(....)
{
....
do {
aknext = (ActKeyColumn *)BLI_dlrbTree_search_next(
&keys, compare_ak_cfraPtr, &cfranext);
if (aknext) {
if (CFRA == (int)aknext->cfra) {
cfranext = aknext->cfra; // <=
}
else {
if (++nextcount == U.view_frame_keyframes)
donenext = true;
}
cfranext = aknext->cfra; // <=
}
} while ((aknext != NULL) && (donenext == false));
....
}V519 The 'cfranext' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 447, 454. anim_draw.c 454
Присвоение внутри условных блоков не имеет смысла, так как его значение всё равно присваивается заново в конце цикла без какого-либо условия. Сделать вывод, что лишняя строка расположена именно сверху помогает цикл, находящийся в коде следом за приведённым фрагментом. Он отличается только prev переменными и отсутствием этой строки в условии. К тому же, если предположить, что лишняя строка снизу и условие CFRA == (int)aknext->cfra окажется ложным, то цикл превратится в бесконечный. Этот фрагмент явно нуждается в корректировке, но как именно его изменить знают только разработчики проекта.
Подобных фрагментов с инициализированными, но не используемыми в итоге переменными, встретилось очень много. Некоторые из них относятся к примерам логических ошибок и излишних перепроверок, о подобных фрагментах уже много раз говорилось выше. Встречаются также константы, которые вполне возможно должны были изменятся внутри функций. Но в итоге остались лишь в виде проверки, всегда возвращающей одинаковый результат. Пример подобного фрагмента:
static int rule_avoid_collision(....)
{
....
int n, neighbors = 0, nearest = 0; // <=
....
if (ptn && nearest==0) // <=
MEM_freeN(ptn);
return ret;
}V560 A part of conditional expression is always true: nearest == 0. boids.c 361
Остальные фрагменты я приведу просто списком. Возможно, многие из них являются спорными, но на них стоит обратить внимание.
int TileManager::gen_tiles(bool sliced)
{
....
state.tiles.clear(); // <=
....
int tile_index = 0;
state.tiles.clear();
state.tiles.resize(num);
....
}V586 The 'clear' function is called twice for deallocation of the same resource. Check lines: 149, 156. tile.cpp 156
В данном случае это может быть просто лишняя строка. Возможно, раньше между двумя очистками списка располагался ещё какой-то код, но в данном случае это ещё один лишний фрагмент, который не несёт пользы и его надо удалить, дабы не загромождать код. Эта строка может быть и следствием того, что в ней должен был очищаться какой-то другой объект, который не видно при беглом осмотре кода. В таком случае фрагмент будет явной ошибкой, которая может привести к неизвестным последствиям для программы.
Очень часто такой, на первый взгляд избыточный, код может привести в итоге к действительно серьёзным ошибкам или поможет избежать их появление в будущем при доработке кода. Именно поэтому стоит обратить внимание на подобные сообщения анализатора, а не отметать их как незначительные.
Команда PVS-Studio сейчас активно работает над новым направлением. А я прикрываю тылы, заполняя информационное поле статьями о повторной проверке некоторых открытых проектов. Что это за направление? Не могу сказать. Я только оставлю картинку, которую каждый волен трактовать на свой лад.
Анализатор указал достаточно много проблемных мест в проекте. Впрочем, в Blender иногда используется странный стиль кода, и нельзя точно трактовать такие фрагменты как ошибки. Я считаю, опасные ошибки часто возникают из-за опечаток. Именно с их нахождением хорошо справляется анализатор PVS-Studio. Но ошибки, отражённые в статье, это исключительно мнение автора, которое достаточно субъективно. И для полной оценки возможностей анализатора его стоит скачать и попробовать самостоятельно.