- PVSM.RU - https://www.pvsm.ru -
Статический анализ наиболее полезен при регулярных проверках. Особенно для таких активно развивающихся проектов как Blender. Пришло время проверить его вновь, и узнать, какие подозрительные места удастся найти на этот раз.
Blender — профессиональный пакет для создания трёхмерной компьютерной графики, включающий в себя средства моделирования, анимации, рендеринга, постобработки и монтажа видео со звуком, также использующийся для создания интерактивных игр.
Проект уже проверялся ранее. Результаты проверки версии 2.62 изложены в статье "Проверка проекта Blender с помощью PVS-Studio [1]".
Со времени прошлой проверки размер исходного кода вместе с дополнительными библиотеками увеличился до 77 мегабайт. А его объём вырос до 2206 KLOC. На момент предыдущей поверки размер проекта составлял 68 мегабайт (2105 KLOC).
Рассчитать объём кода мне помогла утилита SourceMonitor [2]. Эта утилита умеет анализировать код на языках C++, C, C#, VB.NET, Java, Delphi и рассчитывает различные метрики. Например, она может определить цикломатическую сложность [3] ваших проектов, а также сформировать подробную статистику для каждого из файлов проекта. И показать результаты в виде таблицы или диаграмм.
Итак, эта статья расскажет про ошибки и подозрительные места, найденные в версии 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 [4] 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 [4] 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 [5] 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 [7] 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 [8] 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 [9] 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 [4] 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 [10] 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 [8] 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 [11] 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 [12] 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 [13] 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 [14] 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 [15] 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 [16] The 'clear' function is called twice for deallocation of the same resource. Check lines: 149, 156. tile.cpp 156
В данном случае это может быть просто лишняя строка. Возможно, раньше между двумя очистками списка располагался ещё какой-то код, но в данном случае это ещё один лишний фрагмент, который не несёт пользы и его надо удалить, дабы не загромождать код. Эта строка может быть и следствием того, что в ней должен был очищаться какой-то другой объект, который не видно при беглом осмотре кода. В таком случае фрагмент будет явной ошибкой, которая может привести к неизвестным последствиям для программы.
Очень часто такой, на первый взгляд избыточный, код может привести в итоге к действительно серьёзным ошибкам или поможет избежать их появление в будущем при доработке кода. Именно поэтому стоит обратить внимание на подобные сообщения анализатора, а не отметать их как незначительные.
Команда PVS-Studio сейчас активно работает над новым направлением. А я прикрываю тылы, заполняя информационное поле статьями о повторной проверке некоторых открытых проектов. Что это за направление? Не могу сказать. Я только оставлю картинку, которую каждый волен трактовать на свой лад.
Анализатор указал достаточно много проблемных мест в проекте. Впрочем, в Blender иногда используется странный стиль кода, и нельзя точно трактовать такие фрагменты как ошибки. Я считаю, опасные ошибки часто возникают из-за опечаток. Именно с их нахождением хорошо справляется анализатор PVS-Studio. Но ошибки, отражённые в статье, это исключительно мнение автора, которое достаточно субъективно. И для полной оценки возможностей анализатора его стоит скачать и попробовать самостоятельно.
Автор: PVS-Studio
Источник [17]
Сайт-источник PVSM.RU: https://www.pvsm.ru
Путь до страницы источника: https://www.pvsm.ru/pvs-studio/159690
Ссылки в тексте:
[1] Проверка проекта Blender с помощью PVS-Studio: http://www.viva64.com/ru/b/0145/
[2] SourceMonitor: http://www.campwoodsw.com/sourcemonitor.html
[3] цикломатическую сложность: http://www.viva64.com/ru/t/0010/
[4] V501: http://www.viva64.com/ru/d/0090/
[5] V522: http://www.viva64.com/ru/d/0111/
[6] V556: http://www.viva64.com/ru/d/0147/
[7] V593: http://www.viva64.com/ru/d/0197/
[8] V649: http://www.viva64.com/ru/d/0268/
[9] V535: http://www.viva64.com/ru/d/0124/
[10] V571: http://www.viva64.com/ru/d/0169/
[11] V637: http://www.viva64.com/ru/d/0255/
[12] V728: http://www.viva64.com/ru/d/0375/
[13] V686: http://www.viva64.com/ru/d/0320/
[14] V519: http://www.viva64.com/ru/d/0108/
[15] V560: http://www.viva64.com/ru/d/0153/
[16] V586: http://www.viva64.com/ru/d/0189/
[17] Источник: https://habrahabr.ru/post/306104/?utm_source=habrahabr&utm_medium=rss&utm_campaign=best
Нажмите здесь для печати.