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