Андрей Карпов

23 Апр 2012

Теги:

#Cpp

Проверка проекта Blender с помощью PVS-Studio

23 Апр 2012

Автор: Андрей Карпов

Введение
О проекте Blender
Ложные предупреждения
Замеченные дефекты и странные участки кода
Выводы
Дополнительные ресурсы

Мы продолжаем проверять open source проекты и делать мир программ лучше. На этот раз проверке подвергся пакет Blender 2.62 для создания трёхмерной компьютерной графики.

Введение

Мы регулярно проверяем различные open source проекты на языке Си/Си++ и делаем отчёт о результатах проверки. Это позволяет миру open source программ стать лучше, а нам рассказать об инструменте PVS-Studio программистам. Отчеты, как правило, содержат не все найденные дефекты. Мы не знакомы с проектами и нам бывает сложно понять, действительно мы нашли ошибку или просто хитрый код. Это не страшно. Мы всегда предоставляем авторам бесплатных open source проектов ключ на некоторое время, чтобы они могли более подробно проверить исходный код. Если проект маленький, то для его проверки вообще будет достаточно триальной версии PVS-Studio, в которой доступна вся функциональность.

Читатели часто оставляют комментарии, что проверка open source проектов это только реклама нашего продукта. Также они ставят нам в пример Coverity, который значительно активнее поддерживает open source проекты.

Подобное сравнение не честно. Улучшение качества кода открытых продуктов стало результатом реализации программы Vulnerability Discovery and Remediation Open Source Hardening Project. В рамках данной инициативы компании Coverity был выделен грант в размере 297,000 долларов для поддержки проектов с открытым кодом [1]. Конечно, это сумма маленькая, но если бы нас тоже хоть немного спонсировали, мы бы смогли гораздо активнее заниматься проверкой открытых проектов.

О проекте Blender

Blender - свободный пакет для создания трёхмерной компьютерной графики, включающий в себя средства моделирования, анимации, рендеринга, постобработки видео, а также создания интерактивных игр. Начиная с 2002 года Blender является проектом с открытым исходным кодом (GNU GPL) и развивается при активной поддержке Blender Foundation [2].

Пакет Blender написан на языке Си, Си++ и Python. Естественно мы проверяли части, написанные на Си и Си++. Объем исходного кода вместе с дополнительными библиотеками составляет 68 мегабайт (2105 KLOC).

Кстати, здесь я, кажется, повстречал функцию с самом большой цикломатической сложностью, из видимых мною. Эту функция fast9_corner_score(), которую можно посмотреть в файле fast_9.c. Её цикломатическая сложность равна 1767. Но на самом деле функция относительно проста, так что ничего невероятного вы не увидите.

Для проверки использовался статический анализатор PVS-Studio версии 4.60.

Ложные предупреждения

Используемый в проекте Blender стиль написания кода приводит к тому, что анализатор PVS-Studio выдает много ложных срабатываний, среди которых теряются полезные сообщения. В результате с проектом Blender невозможно начать работу, не проведя настройку статического анализатора. Однако, всё не так страшно, как может показаться в первый момент. Приложив даже минимальные усилия, можно колоссально облегчить себе работу по анализу отчёта.

Поясню вышесказанное, используя числовые данные. Всего PVS-Studio выдаёт 574 предупреждения первого уровня, относящиеся к диагностическим правилам общего назначения. Уже после беглого просмотра отчета, становится ясно, что большинство ложных сообщений относится к макросам BLI_array_append, BLI_array_growone и другим макросам с именем, начинающимся на "BLI_array_".

Эти макросы безопасны, но используются часто. Анализатор выдает в местах использования этих макросов предупреждения V514 и V547. Чтобы избавиться от этих предупреждений, можно вписать специальный комментарий в файл BLI_array.h, в котором определены все эти макросы:

//-V:BLI_array_:514,547

Вписать его можно в любом месте. После этого придется перезапустить анализ, но зато результат будет весьма существенен. Исчезнет около 280 ложных предупреждений.

Итого, после добавления одного единственного комментария, количество сообщений первого уровня сократится с 574 до 294! Этот пример хорошо показывает, что наличие большого количества ложных срабатываний не означает, что отчёт будет сложно анализировать. Часто совсем небольшими усилиями можно убрать большую часть шума.

Более подробно с методами подавления ложных срабатываний можно познакомиться в соответствующем разделе документации про подавление ложных предупреждений.

Замеченные дефекты и странные участки кода

Ошибка в макросе

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

Например, рассмотрим макрос DEFAULT_STREAM, который неоднократно используется в проекте Blender. Он длинный, поэтому здесь приводится только его часть:

#define  DEFAULT_STREAM  \
  m[dC] = RAC(ccel,dC); \
  \
  if((!nbored & CFBnd)) { \
  \
  ....

Предупреждение PVS-Studio: V564 The '&' operator is applied to bool type value. You've probably forgotten to include parentheses or intended to use the '&&' operator. bf_intern_elbeem solver_main.cpp 567

Здесь неверно расставлены круглые скобки. В результате, вначале вычисляется "!nbored", а уже затем к значению булева типа применяется операция &. Корректный код должен был выглядеть так:

if(!(nbored & CFBnd)) { \

Ошибка использования макроса

Здесь ошибка возникает не из-за макроса, а из-за опечатки при его использовании:

#define MAX2(x,y) ( (x)>(y) ? (x) : (y) )
static Scene *preview_prepare_scene(....)
{
  ...
  int actcol = MAX2(base->object->actcol > 0, 1) - 1;
  ...
}

Предупреждение PVS-Studio: V562 It's odd to compare 0 or 1 with a value of 1: (base->object->actcol > 0) > (1). bf_editor_render render_preview.c 361

Если раскрыть макрос, то получится:

int actcol = ( ( (base->object->actcol > 0) > (1) ) ?
  (base->object->actcol > 0) : (1) ) - 1;

Выражение "base->object->actcol > 0" всегда даёт 0 или 1. Условие "[0..1] > 1" всегда ложно. Это значит, что высказывание можно упросить до:

int actcol = 0;

Это явно не то, что хотелось. По всей видимости, при копировании фрагмента "base->object->actcol" случайно захватили "> 0".

Корректный вариант кода:

int actcol = MAX2(base->object->actcol, 1) - 1;

Разыменовывание нулевого указателя

static int render_new_particle_system(...)
{
  ParticleSettings *part, *tpart=0;
  ...
  // tpart don't used
  ...
  psys_particle_on_emitter(psmd,tpart->from,
    tpa->num,pa->num_dmcache,tpa->fuv,
    tpa->foffset,co,nor,0,0,sd.orco,0);
  ...
}

Предупреждение PVS-Studio: V522 Dereferencing of the null pointer 'tpart' might take place. bf_render convertblender.c 1788

В функции render_new_particle_system() указатель 'tpart' инициализируется нулём и нигде не изменяется до момента разыменования. Функция достаточно сложная и содержит переменные с похожими именами. Скорее всего, это опечатка и использован должен быть другой указатель.

Одинаковые функции

Анализатор нашёл множество функций с одинаковым телом. Внимательно к этим сообщениям я не присматривался, но одну ошибку, кажется, я нашел. Возможно, воспользовавшись PVS-Studio, авторы Blender смогут найти и другие подобные места.

float uiLayoutGetScaleX(uiLayout *layout)
{
  return layout->scale[0];
}

float uiLayoutGetScaleY(uiLayout *layout)
{
  return layout->scale[0];
}

Предупреждение PVS-Studio: V524 It is odd that the body of 'uiLayoutGetScaleY' function is fully equivalent to the body of 'uiLayoutGetScaleX' function (interface_layout.c, line 2410). bf_editor_interface interface_layout.c 2415

Интуиция подсказывает, что функция uiLayoutGetScaleY() должна возвращать второй элемент массива 'scale':

float uiLayoutGetScaleY(uiLayout *layout)
{
  return layout->scale[1];
}

Опечатка в однородном блоке кода

void tcd_malloc_decode(....) {
  ...
  x0 = j == 0 ? tilec->x0 :
    int_min(x0, (unsigned int) tilec->x0);
  y0 = j == 0 ? tilec->y0 :
    int_min(y0, (unsigned int) tilec->x0);
  x1 = j == 0 ? tilec->x1 :
    int_max(x1, (unsigned int) tilec->x1);      
  y1 = j == 0 ? tilec->y1 :
    int_max(y1, (unsigned int) tilec->y1);
  ...
}

Предупреждение PVS-Studio: V537 Consider reviewing the correctness of 'x0' item's usage. extern_openjpeg tcd.c 650

Если внимательно присмотреться, то можно заметить ошибку при присваивании нового значения переменной 'y0'. В самом конце строки используется член класса 'tilec->x0', а не 'tilec->y0'.

Скорее всего, этот код создавался с помощью технологии Copy-Paste и в процессе правки имя одной переменной забыли изменить. Корректный вариант:

y0 = j == 0 ? tilec->y0 :
  int_min(y0, (unsigned int) tilec->y0);

Неуточнённое поведение

#define cpack(x) \
  glColor3ub( ((x)&0xFF), (((x)>>8)&0xFF), (((x)>>16)&0xFF) )
static void star_stuff_init_func(void)
{
  cpack(-1);
  glPointSize(1.0);
  glBegin(GL_POINTS);
}

Предупреждение PVS-Studio: V610 Unspecified behavior. Check the shift operator '>>. The left operand '(- 1)' is negative. bf_editor_space_view3d view3d_draw.c 101

Согласно стандарту языка Си++ сдвиг отрицательного значения вправо приводит к неуточненному поведению программы. На практике, такой приём часто используют, но лучше так не делать. Нет никакой гарантии, что код всегда будет работать ожидаемым образом. Подробнее данный вопрос рассматривается в статье "Не зная брода, не лезь в воду. Часть третья".

Можно предложить переписать код следующим образом:

cpack(UINT_MAX);

Аналогичные опасные места присутствуют и в других функциях:

V610 Undefined behavior. Check the shift operator '<<. The left operand '-1' is negative. bf_intern_ghost ghost_ndofmanager.cpp 289

V610 Undefined behavior. Check the shift operator '<<. The left operand '(~0)' is negative. extern_bullet btquantizedbvh.h 82

V610 Undefined behavior. Check the shift operator '<<. The left operand '(~0)' is negative. extern_bullet btsoftbodyconcavecollisionalgorithm.h 48

Странные сравнения

static PyObject *bpy_bmlayercollection_subscript_slice(
  BPy_BMLayerCollection *self,
  Py_ssize_t start, Py_ssize_t stop)
{
  ...
  if (start >= start) start = len - 1;
  if (stop >= stop)   stop  = len - 1;
  ...
}

Предупреждения PVS-Studio:

V501 There are identical sub-expressions to the left and to the right of the '>=' operator: start >= start bf_python_bmesh bmesh_py_types_customdata.c 442

V501 There are identical sub-expressions to the left and to the right of the '>=' operator: stop >= stop bf_python_bmesh bmesh_py_types_customdata.c 443

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

if (start >= len) start = len - 1;
if (stop >= len)   stop  = len - 1;

Вот ещё одно странное сравнение:

typedef struct opj_pi_resolution {
  int pdx, pdy;
  int pw, ph;
} opj_pi_resolution_t;

static bool pi_next_rpcl(opj_pi_iterator_t * pi) {
  ...
  if ((res->pw==0)||(res->pw==0)) continue;
  ...
}

Предупреждение PVS-Studio: V501 There are identical sub-expressions to the left and to the right of the '||' operator: (res->pw == 0) || (res->pw == 0) extern_openjpeg pi.c 219

Скорее всего, здесь должна проверяться не только переменная 'pw', но и 'ph':

if ((res->pw==0)||(res->ph==0)) continue;

Аналогичные неправильные проверки также присутствуют здесь:

V501 There are identical sub-expressions to the left and to the right of the '||' operator: (res->pw == 0) || (res->pw == 0) extern_openjpeg pi.c 300

V501 There are identical sub-expressions to the left and to the right of the '||' operator: (res->pw == 0) || (res->pw == 0) extern_openjpeg pi.c 379

Одинаковые действия

EIGEN_DONT_INLINE static void run(....)
{
  ...
  if ((size_t(lhs0+alignedStart)%sizeof(LhsPacket))==0)
    for (Index i = alignedStart;i<alignedSize;
         i+=ResPacketSize)
      pstore(&res[i],
             pcj.pmadd(ploadu<LhsPacket>(&lhs0[i]),
                       ptmp0, pload<ResPacket>(&res[i])));
  else
    for (Index i = alignedStart;i<alignedSize;
         i+=ResPacketSize)
      pstore(&res[i],
             pcj.pmadd(ploadu<LhsPacket>(&lhs0[i]),
                       ptmp0, pload<ResPacket>(&res[i])));
  ...
}

Предупреждение PVS-Studio: V523 The 'then' statement is equivalent to the 'else' statement. bf_ikplugin generalmatrixvector.h 268

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

Некорректное заполнение массива

static int imb_read_tiff_pixels(....)
{
  float *fbuf=NULL;
  ...
  memset(fbuf, 1.0, sizeof(fbuf));
  ...
}

Предупреждение PVS-Studio: V579 The memset function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third argument. bf_imbuf tiff.c 442

Анализатор выдаёт одно предупреждение, но на самом деле здесь удалось сделать в одной строчке сразу 2 ошибки. Записали себе реализовать поиск второй ошибки. Это должно быть не сложно.

Первая ошибка. Переменная 'fbuf' это указатель, а значит sizeof(fbuf) вернет размер указателя, а не массива. В результате функция memset() заполнит только несколько первых байт в массиве.

Вторая ошибка. Массив, состоящий из элементов типа float, хотели заполнить единицами. Но функция memset работает с байтами. Массив будет заполнен мусором.

Аналогичная ошибка присутствует также здесь:

V579 The memset function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third argument. bf_imbuf tiff.c 450

Опечатка в коде для очистки массива

int ntlGeometryObjModel::initModel(....)
{
  ...
  ntlSetVec3f averts; averts.mVerts.clear();
  ntlSetVec3f anorms; averts.mVerts.clear();
  ...
}

Предупреждение PVS-Studio: V586 The 'clear' function is called twice for deallocation of the same resource. Check lines: 176, 177. bf_intern_elbeem ntl_geometrymodel.cpp 177

Мне кажется, что очищать массив в только что созданных объектах нет смысла. Но я не знаком с проектом и возможно в этом действии есть смысл. Из-за опечатки оба раза очищается один и тот же массив. Корректный код должен выглядеть так:

ntlSetVec3f averts; averts.mVerts.clear();
ntlSetVec3f anorms; anorms.mVerts.clear();

Двойная проверка

В коде Blender встретились две одинаковые проверки написанные рядом. Возможно, второе условие другим. А возможно код корректен и вторая проверка просто лишняя.

static void fcurve_add_to_list (....)
{
  ...
  if (agrp == NULL) {
    if (agrp == NULL) {
  ...
}

Предупреждение PVS-Studio: V571 Recurring check. The 'if (agrp == ((void *) 0))' condition was already verified in line 1108. bf_blenkernel ipo.c 1110

Странный код

void CcdPhysicsController::RelativeRotate(
  const float rotval[9], bool local)
{
  ...
  btMatrix3x3 drotmat(
    rotval[0],rotval[4],rotval[8],
    rotval[1],rotval[5],rotval[9],
    rotval[2],rotval[6],rotval[10]);
  ...
}

Предупреждения PVS-Studio:

V557 Array overrun is possible. The '9' index is pointing beyond array bound. ge_phys_bullet ccdphysicscontroller.cpp 867

V557 Array overrun is possible. The '10' index is pointing beyond array bound. ge_phys_bullet ccdphysicscontroller.cpp 868

Указатель 'rotval' может ссылаться на массив любого размера. Код возможно корректен. Число [9] это просто подсказка человеку.

Есть здесь ошибка или нет, мне судить сложно. Если массив rotval действительно состоит из 9 элементов, то произойдёт выход за границы массива.

Несозданный файл

void LogFileObject::Write(....) {
  ...
  // If there's no destination file, make one before outputting
  if (file_ == NULL) {
    ...
    // file_ don't used
    ...
    fwrite(file_header_string, 1, header_len, file_);
    ...
}

Предупреждение PVS-Studio: V575 The null pointer is passed into 'fwrite' function. Inspect the fourth argument. extern_libmv logging.cc 870

Согласно комментарию, если дескриптор файла равен NULL, то мы должны создать новый файл. Однако, до того момента как будет вызвана функция fwrite(), переменная 'filxe_' нигде не используется. В результате в качестве дескриптора в функцию fwrite() будет предан нулевой указатель.

Использование указателя до проверки на нулевое значение

PVS-Studio имеет интересную проверку V595. Кратко диагностическое правило можно сформулировать так:

V595 выдается, если:

1) указатель разыменовывается;

2) далее указатель нигде не меняется;

3) указатель сравнивается с 0.

Есть ряд исключений, но в подробности вдаваться не будем.

У этого правила есть как преимущества, так и недостатки. Преимущества - можно найти интересные ошибки. Недостатки - достаточно много ложных срабатываний.

Ложные срабатывания в большинстве своём связаны с наличием ненужных проверок в макросах. И с эти пока не получается пока бороться. Типовой пример, где выдается ложное срабатывание:

#define SAFE_RELEASE(p) { if (p) { Release(p); delete p; } }
X *p = ....;
p->Foo(); // <= V595
SAFE_RELEASE(p);

Указатель 'p' всегда неравен NULL. Но есть проверка, и анализатор подозревает неладное.

Такое длинное вступление связано с тем, что в Blender предупреждение V595 выдается очень часто. Всего PVS-Studio выдал 119 таких предупреждений. Скорее всего, больше половины из них являются ложными срабатываниями. Однако авторам лучше самим изучить отчёт, выдаваемый PVS-Studio.

Приведу только один пример:

static struct DerivedMesh *dynamicPaint_Modifier_apply(....)
{
  ...
  for (; surface; surface=surface->next) {
    PaintSurfaceData *sData = surface->data;
    if (surface &&
        surface->format !=
          MOD_DPAINT_SURFACE_F_IMAGESEQ &&
        sData)
    {
      ...
}

Предупреждение PVS-Studio: V595 The 'surface' pointer was utilized before it was verified against nullptr. Check lines: 1585, 1587. bf_blenkernel dynamicpaint.c 1585

Указатель 'surface' в начале используется, чтобы инициализировать переменную 'sData'. И только затем указатель 'surface' проверяется на нулевое значение.

Выводы

1) Статические анализаторы полезны. И не забывайте, что максимально они полезны при регулярном использовании. Они помогают обнаружить множество ошибок на самом раннем этапе и тем самым избежать множества мучительных отладок, баг репортов от тестировщиков и жалоб пользователей.

2) PVS-Studio иногда выдаёт много ложных срабатываний. Но, как правило, приложив минимальные усилия, можно отфильтровать большинство из них.

3) Триальная версия PVS-Studio, которую можно скачать с сайта, обладает полной функциональностью. Её вполне хватит для проверки небольших проектов. Для разработчиков больших бесплатных open source мы выдаем на время бесплатный ключ.