После большой статьи про проверку операционной системы Tizen мне было задано много вопросов о проценте ложных срабатываний и о плотности ошибок (сколько ошибок PVS-Studio выявляет на 1000 строк кода). Мои рассуждения о том, что это сильно зависит от анализируемого проекта и настроек анализатора не выглядят как настоящий ответ. Я решил привести конкретные числа, проведя более тщательное исследование одного из проектов, входящих в состав Tizen. Поскольку в обсуждении статьи активное участие принимал Carsten Haitzler, я решил, что будет интересно взять для эксперимента EFL Core Libraries, в разработке которого он участвует. Надеюсь, эта статья поможет Carsten стать поклонником нашего анализатора :).
Предыстория
Если кто-то из моих читателей пропустил, то сообщаю им, что недавно я написал открытое письмо разработчикам Tizen, а затем монументальную статью "27000 ошибок в операционной системе Tizen".
После этого на некоторых сайтах появились соответствующие новостные заметки, а также развернулось несколько дискуссий. Вот некоторые из них:
- Reddit. PVS-Studio Team Willing to Work on Improving Tizen Project (open letter).
- Reddit. Good news: Samsung's Tizen no longer worst code ever. Bad news: It's still pretty awful.
- Ycombinator. 27000 errors in the Tizen operating system.
- The Register. Good news: Samsung's Tizen no longer worst code ever. Bad news: It's still pretty awful
- The Hacker News. Researcher Claims Samsung's Tizen OS is Poorly Programmed; Contains 27,000 Bugs!
- Lists.tizen.org: 1, 2, 3, 4, 5, 6.
Хочу высказать признательность Carsten Haitzler, который уделил внимание моей публикации и принял активное участие в её обсуждении.
Поднимались разные темы, к некоторым из которых я дал развернутые комментарии в заметке "Tizen: подводим итоги".
Однако меня преследуют два вечных вопроса:
- Какой процент ложных срабатываний?
- Как много ошибок находит PVS-Studio на 1000 строк кода?
Программисты, которые хорошо понимают, что такое методология статического анализа, согласятся со мной, что такие обобщенные вопросы не имеют смысла. Всё зависит от проекта, с которым мы работаем. Задавать такие вопросы, это то же самое, что просить врача ответить на вопрос «так какая всё-таки средняя температура у пациентов в вашей больнице?»
Поэтому я дам ответ на примере конкретного проекта. Я выбрал EFL Core Libraries. Во-первых, этот проект входит в Tizen. А во-вторых, в его разработке принимает участие Carsten Haitzler, и, думаю, ему будет интересно посмотреть на мои результаты.
Можно было ещё проверить Enlightenment, но у меня не хватило сил на это. Я чувствую, что и без этого статья получится очень длинная.
The Enlightenment Foundation Libraries (EFL) это набор графических библиотек, которые появились в результате разработки Enlightenment, менеджера окон и протокола Wayland.
При проверке EFL Core Libraries использовался свежий код, взятый из репозитория https://git.enlightenment.org/.
Отмечу также, что исследуемый проект проверяется с помощью статического анализатора Coverity. Вот комментарий на эту тему:
I will say that we take checking seriously. Coverity reports a bug rate of 0 for Enlightenment upstream (we've fixed all issues Coverity points out or dismissed them as false after taking a good look) and the bug rate for EFL is 0.04 issues per 1k lines of code which is pretty small (finding issues is easy enough i the codebase is large). They are mostly not so big impacting things. Every release we do has our bug rates go down and we tend to go through a bout of «fixing the issues» in the weeks prior to a release.
Что же, давайте посмотрим как продемонстрирует себя анализатор PVS-Studio.
Характеристики
Анализатор PVS-Studio после своей настройки будет выдавать около 10-15% ложных срабатываний при проверке проекта EFL Core Libraries.
Плотность обнаруживаемых на данный момент ошибок в EFL Core Libraries составляет более 0,71 ошибки на 1000 строк кода.
Как проводились расчёты
Проект EFL Core Libraries на момент анализа содержит около 1 616 000 строк кода на языке С и C++. Из них 17,7% — это комментарии. Таким образом, количество строк кода без комментариев — 1 330 000.
После первого запуска я увидел следующее количество сообщений общего назначения (GA):
- Высокий уровень достоверности: 605
- Средний уровень достоверности: 3924
- Низкий уровень достоверности: 1186
Это, конечно, плохой результат. Именно поэтому я не люблю писать абстрактные результаты замеров. Нужна настройка анализатора, и в этот раз я решил уделить ей время.
Почти весь проект написан на C и, как следствие, в нём широко используются макросы. Именно макросы и становятся причиной большинства ложных срабатываний. Я потратил около 40 минут на быстрый просмотр отчёта и составил файл efl_settings.txt.
Файл содержит необходимые настройки. Чтобы их использовать при проверке проекта, необходимо в конфигурационном файле анализатора (например, в PVS-Studio.cfg) указать:
rules-config=/path/to/efl_settings.txtАнализатор может быть запущен так:
pvs-studio-analyzer analyze ... --cfg /path/to/PVS-Studio.cfg ...или так
pvs-studio ... --cfg /patn/to/PVS-Studio.cfg ...в зависимости от используемого способа интеграции.
С помощью настроек я указал анализатору, чтобы он не выдавал некоторые предупреждения для тех строк кода, в которых встречаются названия определённых макросов или выражения. Также я полностью отключил несколько диагностик. Например, я отключил V505. Использовать функцию alloca в циклах нехорошо, но это не является явной ошибкой. Мне не хочется много дискутировать о том, является то или иное предупреждение ложным срабатыванием, и я посчитал, что будет проще что-то отключить.
Да, следует отметить, что я просматривал и настраивал предупреждения только первых двух уровней. В дальнейшем я буду рассматривать только их. Предупреждения низкого уровня достоверности мы не рекомендуем брать в расчет. По крайней мере, начиная использовать анализатор, нерационально работать с этими предупреждениями. Только разобравшись с первыми двумя уровнями, можно заглянуть на третий и выбрать полезные на ваш взгляд типы предупреждений.
Повторный запуск привёл к следующим результатам:
- Высокий уровень достоверности: 189
- Средний уровень достоверности: 1186
- Низкий уровень достоверности: 1186
Два раза повторяется число 1186. Это не опечатка. Действительно числа так случайно совпали.
Итак, потратив 40 минут на настройку, я значительно сократил число ложных срабатываний. Конечно, я имею большой опыт, у стороннего разработчика этот процесс занял бы больше времени, но ничего страшного и сложного в настройке нет.
В сумме я получил 189+1186=1375 сообщений (High+Medium), с которыми и начал работать.
Проанализировав эти сообщения, я считаю, что анализатор обнаружил 950 фрагментов кода, содержащих ошибки. Другими словами, я нашел 950 фрагментов кода, которые следует исправить. Подробнее об этих ошибках я расскажу в следующей главе.
Рассчитаем теперь плотность обнаруживаемых ошибок:
950*1000/1330000 = около 0,71 ошибок на 1000 строк кода.
Теперь давайте подсчитаем процент ложных срабатываний:
((1375-950) / 1375) * 100% = 30%
Стоп, стоп, стоп! Но ведь в начале статьи говорилось про 10-15% ложных срабатываний. А здесь 30%.
Сейчас объясню. Итак, просматривая отчёт из 1375 сообщений, я пришел к выводу, что 950 указывают на ошибки. Осталось 425 сообщений.
Не все из этих оставшихся 425 сообщений являются ложными срабатываниями. Здесь находится много сообщений, про которые я просто не могу сказать, выявлена с их помощью ошибка или нет.
Рассмотрим пример одного из пропущенных мною сообщений.
....
uint64_t callback_mask;
....
static void
_check_event_catcher_add(void *data, const Efl_Event *event)
{
....
Evas_Callback_Type type = EVAS_CALLBACK_LAST;
....
else if ((type = _legacy_evas_callback_type(array[i].desc)) !=
EVAS_CALLBACK_LAST)
{
obj->callback_mask |= (1 << type);
}
....
}Предупреждение PVS-Studio: V629 Consider inspecting the '1 << type' expression. Bit shifting of the 32-bit value with a subsequent expansion to the 64-bit type. evas_callbacks.c 709
Рассмотрим внимательнее строчку:
obj->callback_mask |= (1 << type);Она используется, чтобы записать 1 в нужный бит переменной callback_mask. Обратите внимание, что переменная callback_mask является 64-битной.
Выражение (1 << type) имеет тип int, поэтому с его помощью можно изменять биты только в младшей части переменной callback_mask. Биты [32-63] изменить нельзя.
Чтобы понять, есть здесь ошибка или нет, нужно разобраться, какой диапазон значений может возвращать функция _legacy_evas_callback_type. Может она вернуть значение больше чем 31? Я не знаю и пропускаю это сообщение.
Прошу меня понять. Я первый раз вижу этот код и понятия не имею что он делает. Вдобавок, меня ждут ещё сотни сообщений анализатора. Я просто не могу начать внимательно разбираться с каждым подобным случаем.
Comment by Carsten Haitzler. Above — actually is a bug that's a result of an optimization that setting bits to decide if it should bother trying to map new event types to old ones (we're refactoring huge chunks of our internals around a new object system and so we have to do this to retain compatibility, but like with any refactoring… stuff happens). Yes — it wraps the bitshift and does the extra work of a whole bunch of if's because the same bits in the mask are re-used for now 2 events due to wrap around. As such this doesn't lead to a bug, just slightly less micro optimizations when set as now that bit means «it has an event callback for type A OR B» not just «type A»… the following code actually does the complete check/mapping. It surely was not intended to wrap so this was a catch but the way it's used means it was actually pretty harmless.
Среди оставшихся 425 сообщений найдутся указывающие на ошибки. Я их просто пропустил.
Если речь зайдет о регулярном использовании PVS-Studio, то можно продолжить его настраивать. Как я говорил, я уже потратил только 40 минут на настройку. Но это не значит, что я сделал всё что мог. Можно ещё сократить число ложных срабатываний отключением диагностик для определённых программных конструкций.
После более внимательного изучения оставшихся сообщений и дополнительной настройки как раз и останется 10-15% ложных срабатываний. Хороший результат.
Найденные ошибки
Теперь рассмотрим найденные мною ошибки. Все 950 описать я не могу, поэтому ограничусь разбором пары предупреждений каждого типа. Остальные предупреждения я приведу списком или отдельным файлом.
Также читатель сам может познакомиться со всеми предупреждениями, открыв файл отчета: zip-архив с отчётом. Учтите, что я оставил в файле только предупреждения General высокого и среднего уровня достоверности.
Я просматривал этот отчёт в Windows, открыв его с помощью утилиты PVS-Studio Standalone.
В Linux можно использовать утилиту Plog Converter, которая сконвертирует отчет в один из следующих форматов:
- xml — удобный формат для дополнительной обработки результатов анализа, поддерживается плагином для SonarQube;
- csv — текстовый формат, предназначенный для представления табличных данных;
- errorfile — формат вывода gcc и clang;
- tasklist — формат ошибок, который можно открыть в QtCreator.
Далее для просмотра отчётов можно использовать QtCreator, Vim/gVim, GNU Emacs, LibreOffice Calc. Подробнее это описано в разделе документации "Как запустить PVS-Studio в Linux" (см. «Просмотр и фильтрация отчёта анализатора»).
V501 (1 ошибка)
Диагностика V501 выявила только одну ошибку, зато красивую. Ошибка находится в функции сравнения, что перекликается с темой недавней статьи "Зло живёт в функциях сравнения".
static int
_ephysics_body_evas_stacking_sort_cb(const void *d1,
const void *d2)
{
const EPhysics_Body_Evas_Stacking *stacking1, *stacking2;
stacking1 = (const EPhysics_Body_Evas_Stacking *)d1;
stacking2 = (const EPhysics_Body_Evas_Stacking *)d2;
if (!stacking1) return 1;
if (!stacking2) return -1;
if (stacking1->stacking < stacking2->stacking) return -1;
if (stacking2->stacking > stacking2->stacking) return 1;
return 0;
}Предупреждение PVS-Studio: V501 There are identical sub-expressions 'stacking2->stacking' to the left and to the right of the '>' operator. ephysics_body.cpp 450
Опечатка. Последнее сравнение должно быть таким:
if (stacking1->stacking > stacking2->stacking) return 1;V512 (8 ошибок)
Для начала взглянем на определение структуры Eina_Array.
typedef struct _Eina_Array Eina_Array;
struct _Eina_Array
{
int version;
void **data;
unsigned int total;
unsigned int count;
unsigned int step;
Eina_Magic __magic;
};Внимательно рассматривать её не надо. Просто структура с какими-то полями.
Теперь посмотрим на определение структуры Eina_Accessor_Array:
typedef struct _Eina_Accessor_Array Eina_Accessor_Array;
struct _Eina_Accessor_Array
{
Eina_Accessor accessor;
const Eina_Array *array;
Eina_Magic __magic;
};Обратите внимание, что в структуре Eina_Accessor_Array хранится указатель на структуру Eina_Array. В остальном же эти структуры никак не связаны между собой и имеют разный размер.
Теперь код, который выявил анализатор и который я не понимаю:
static Eina_Accessor *
eina_array_accessor_clone(const Eina_Array *array)
{
Eina_Accessor_Array *ac;
EINA_SAFETY_ON_NULL_RETURN_VAL(array, NULL);
EINA_MAGIC_CHECK_ARRAY(array);
ac = calloc(1, sizeof (Eina_Accessor_Array));
if (!ac) return NULL;
memcpy(ac, array, sizeof(Eina_Accessor_Array));
return &ac->accessor;
}Предупреждение PVS-Studio: V512 A call of the 'memcpy' function will lead to the 'array' buffer becoming out of range. eina_array.c 186
Давайте я уберу все лишние детали, чтобы было проще:
.... eina_array_accessor_clone(const Eina_Array *array)
{
Eina_Accessor_Array *ac = calloc(1, sizeof (Eina_Accessor_Array));
memcpy(ac, array, sizeof(Eina_Accessor_Array));
}Выделяется память для объекта типа Eina_Accessor_Array. Далее происходит странная вещь.
В выделенный буфер памяти копируется объект типа Eina_Array.
Я не знаю, что должна делать рассмотренная функция, но она делает что-то не то.
Во-первых, при копировании происходит выход за границу источника (структуры Eina_Array).
Во-вторых, вообще такое копирование не имеет смысла. Структуры имеют набор полей совершенно разного типа.
Comment by Carsten Haitzler. Function content correct — Type in param is wrong. It didn't actually matter because the function is assigned to a func ptr that does have the correct type and since it's a generic «parent class» the assignment casts to a generic accessor type thus the compiler didn't complain and this seemed to work.
Рассмотрим следующую ошибку.
static Eina_Bool _convert_etc2_rgb8_to_argb8888(....)
{
const uint8_t *in = src;
uint32_t *out = dst;
int out_step, x, y, k;
unsigned int bgra[16];
....
for (k = 0; k < 4; k++)
memcpy(out + x + k * out_step, bgra + k * 16, 16);
....
}Предупреждение PVS-Studio: V512 A call of the 'memcpy' function will lead to overflow of the buffer 'bgra + k * 16'. draw_convert.c 318
Здесь всё просто. Обыкновенный выход за границу буфера.
Массив bgra состоит из 16 элементов типа unsigned int.
Переменная k принимает в цикле значения от 0 до 3.
Обратите внимание на выражение: bgra + k * 16.
Когда переменная k примет значение больше 0, будет вычисляться указатель, указывающий за пределы массива.
Впрочем, некоторые сообщения V512 указывают на код, который не содержит настоящей ошибки. Однако я не считаю такие срабатывания анализатора ложными. Код плох и, на мой взгляд, должен быть изменён. Рассмотрим такой случай.
#define MATRIX_XX(m) (m)->xx
typedef struct _Eina_Matrix4 Eina_Matrix4;
struct _Eina_Matrix4
{
double xx;
double xy;
double xz;
double xw;
double yx;
double yy;
double yz;
double yw;
double zx;
double zy;
double zz;
double zw;
double wx;
double wy;
double wz;
double ww;
};
EAPI void
eina_matrix4_array_set(Eina_Matrix4 *m, const double *v)
{
memcpy(&MATRIX_XX(m), v, sizeof(double) * 16);
}Предупреждение PVS-Studio: V512 A call of the 'memcpy' function will lead to overflow of the buffer '& (m)->xx'. eina_matrix.c 1003
Можно было просто скопировать массив в структуру. Но вместо этого берётся адрес первого члена xx. Наверное, подразумевается, что в дальнейшем в начале структуры появятся другие поля. И, чтобы не сломалось поведение программы, используется такой приём.
Comment by Carsten Haitzler. Above and related memcpy's — not a bug: taking advantage of guaranteed mem layout in structs.
Мне он не нравится. Я рекомендую написать как-то так:
struct _Eina_Matrix4
{
union {
struct {
double xx;
double xy;
double xz;
double xw;
double yx;
double yy;
double yz;
double yw;
double zx;
double zy;
double zz;
double zw;
double wx;
double wy;
double wz;
double ww;
};
double RawArray[16];
};
};
EAPI void
void eina_matrix4_array_set(Eina_Matrix4 *m, const double *v)
{
memcpy(m->RawArray, v, sizeof(double) * 16);
}Это немного длиннее, зато более идеологически верно. Впрочем, если править код не хочется, то можно подавить предупреждение одним из следующих способов.
Первый способ. Добавить комментарий в код:
memcpy(&MATRIX_XX(m), v, sizeof(double) * 16); //-V512Второй способ. Добавить в файл настроек строчку:
//-V:MATRIX_:512Третий способ. Использовать базу разметки.
Другие ошибки:
- V512 A call of the 'memcpy' function will lead to overflow of the buffer '& (m)->xx'. eina_matrix.c 1098
- V512 A call of the 'memcpy' function will lead to overflow of the buffer '& (m)->xx'. eina_matrix.c 1265
- V512 A call of the 'memcpy' function will lead to the '& pd->projection.xx' buffer becoming out of range. evas_canvas3d_camera.c 120
- V512 A call of the 'memcpy' function will lead to the '& pd->projection.xx' buffer becoming out of range. evas_canvas3d_light.c 270
- V512 A call of the 'memcpy' function will lead to overflow of the buffer 'bgra + k * 16'. draw_convert.c 350
V517 (3 ошибки)
static Eina_Bool
evas_image_load_file_head_bmp(void *loader_data,
Evas_Image_Property *prop,
int *error)
{
....
if (header.comp == 0) // no compression
{
// handled
}
else if (header.comp == 3) // bit field
{
// handled
}
else if (header.comp == 4) // jpeg - only printer drivers
goto close_file;
else if (header.comp == 3) // png - only printer drivers
goto close_file;
else
goto close_file;
....
}Предупреждение PVS-Studio: V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 433, 439. evas_image_load_bmp.c 433
Два раза переменная header.comp сравнивается с константой 3.
Другие ошибки:
- V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 1248, 1408. evas_image_load_bmp.c 1248
- V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 426, 432. parser.c 426
V519 (1 ошибка)
EOLIAN static Efl_Object *
_efl_net_ssl_context_efl_object_finalize(....)
{
Efl_Net_Ssl_Ctx_Config cfg;
....
cfg.load_defaults = pd->load_defaults; // <=
cfg.certificates = &pd->certificates;
cfg.private_keys = &pd->private_keys;
cfg.certificate_revocation_lists =
&pd->certificate_revocation_lists;
cfg.certificate_authorities = &pd->certificate_authorities;
cfg.load_defaults = pd->load_defaults; // <=
....
}Предупреждение PVS-Studio: V519 The 'cfg.load_defaults' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 304, 309. efl_net_ssl_context.c 309
Повторяется присваивание. Одно присваивание лишнее или забыли скопировать что-то другое.
Comment by Carsten Haitzler. Not a bug. Just an overzealous copy & paste of the line.
Ещё один простой случай:
EAPI Eina_Bool
edje_edit_size_class_add(Evas_Object *obj, const char *name)
{
Eina_List *l;
Edje_Size_Class *sc, *s;
....
/* set default values for max */
s->maxh = -1;
s->maxh = -1;
....
}Предупреждение PVS-Studio: V519 The 's->maxh' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 8132, 8133. edje_edit.c 8133
Конечно, не все случаи такие очевидные. Тем не менее, я считаю, что предупреждения, перечисленные ниже, скорее всего указывают на ошибки:
- V519 The 'pdata->seat->object.in' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 1519, 1521. evas_events.c 1521
- V519 The 'pdata->seat->object.in' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 2597, 2599. evas_events.c 2599
- V519 The 'b->buffer[r]' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 348, 353. evas_image_load_pmaps.c 353
- V519 The 'attr_amount' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 13891, 13959. edje_edit.c 13959
- V519 The 'async_loader_running' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 152, 165. evas_gl_preload.c 165
- V519 The 'textlen' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 86, 87. elm_code_widget_undo.c 87
- V519 The 'content' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 313, 315. elm_dayselector.c 315
- V519 The 'wd->resize_obj' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 3099, 3105. elm_entry.c 3105
- V519 The 'wd->resize_obj' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 3125, 3131. elm_entry.c 3131
- V519 The 'idata->values' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 128, 129. elm_view_list.c 129
- V519 The 'wd->resize_obj' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 2602, 2608. efl_ui_text.c 2608
- V519 The 'wd->resize_obj' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 2628, 2634. efl_ui_text.c 2634
- V519 The 'finfo' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 706, 743. evas_image_load_gif.c 743
- V519 The 'current_program_lookups' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 15819, 15820. edje_cc_handlers.c 15820
Примечание. Carsten Haitzler, комментируя статью, написал, что предупреждения V519, приведённые в списке, являются ложными срабатываниями. Я не согласен с таким подходом. Возможно, код работает верно, но всё равно он заслуживает внимания и правки. Я решил оставить в статье список, чтобы читатели сами могли оценить, являются с их точки зрения повторы в присваивании переменных ложными срабатываниями или нет. Но раз Carsten говорит, что это не ошибки, то я не буду их учитывать при подсчётах.
V522 (563 ошибки)
В проекте EFL беда с наличием проверок: выделилась память или нет. В целом, такие проверки в проекте есть. Пример:
if (!(el = malloc(sizeof(Evas_Stringshare_El) + slen + 1)))
return NULL;Более того, они есть даже там, где не надо (см. ниже про предупреждение V668).
Но в огромном количестве мест никакой проверки нет. Рассмотрим для примера парочку сообщений анализатора.
Comment by Carsten Haitzler. OK so this is a general acceptance that at least on Linux which was always our primary focus and for a long time was our only target, returns from malloc/calloc/realloc can't be trusted especially for small amounts. Linux overcommits memory by default. That means you get new memory but the kernel has not actually assigned real physical memory pages to it yet. Only virtual space. Not until you touch it. If the kernel cannot service this request your program crashes anyway trying to access memory in what looks like a valid pointer. So all in all the value of checking returns of allocs that are small at least on Linux is low. Sometimes we do it… sometimes not. But the returns cannot be trusted in general UNLESS its for very large amounts of memory and your alloc is never going to be serviced — e.g. your alloc cannot fit in virtual address space at all (happens sometimes on 32bit). Yes overcommit can be tuned but it comes at a cost that most people never want to pay or no one even knows they can tune. Secondly, fi an alloc fails for a small chunk of memory — e.g. a linked list node… realistically if NULL is returned… crashing is about as good as anything you can do. Your memory is so low that you can crash, call abort() like glib does with g_malloc because if you can't allocate 20-40 bytes… your system is going to fall over anyway as you have no working memory left anyway. I'm not talking about tiny embedded systems here, but large machines with virtual memory and a few megabytes of memory etc. which has been our target. I can see why PVS-Studio doesn't like this. Strictly it is actually correct, but in reality code spent on handling this stuff is kind of a waste of code given the reality of the situation. I'll get more into that later.
static Eina_Debug_Session *
_session_create(int fd)
{
Eina_Debug_Session *session = calloc(1, sizeof(*session));
session->dispatch_cb = eina_debug_dispatch;
session->fd = fd;
// start the monitor thread
_thread_start(session);
return session;
}Comment by Carsten Haitzler. This is brand new code that arrived 2 months ago and still is being built out and tested and not ready for prime time. It's part of our live debugging infra where any app using EFL can be controlled by a debugger daemon (if it is run) and controlled (inspect all objects in memory and the object tree and their state with introspection live as it runs), collect execution timeline logs (how much time is spent in what function call tree where while rendering in which thread — what threads are using what cpu time at which slots down to the ms and below level, correlated with function calls, state of animation system and when wakeup events happen and the device timestamp that triggered the wakeup, and so on… so given that scenario… if you can't calloc a tiny session struct while debugging a crash accessing the first page of memory is pretty much about as good as anything… as above on memory and aborts etc.
Комментарий Андрея Карпова. Не очень понятно, причем здесь новый и не оттестированный код. Статические анализаторы как раз и предназначены в первую очередь искать ошибки в новом коде.
Предупреждение PVS-Studio: V522 There might be dereferencing of a potential null pointer 'session'. eina_debug.c 440
Выделили память с помощью функции calloc и сразу её используем.
Ещё пример:
static Reference *
_entry_reference_add(Entry *entry, Client *client,
unsigned int client_entry_id)
{
Reference *ref;
// increase reference for this file
ref = malloc(sizeof(*ref));
ref->client = client;
ref->entry = entry;
ref->client_entry_id = client_entry_id;
ref->count = 1;
entry->references = eina_list_append(entry->references, ref);
return ref;
}Предупреждение PVS-Studio: V522 There might be dereferencing of a potential null pointer 'ref'. evas_cserve2_cache.c 1404
И вот так 563 раза. В статье я их привести не могу. Даю ссылку на файл: EFL_V522.txt.
V547 (39 ошибок)
static void
_ecore_con_url_dialer_error(void *data, const Efl_Event *event)
{
Ecore_Con_Url *url_con = data;
Eina_Error *perr = event->info;
int status;
status =
efl_net_dialer_http_response_status_get(url_con->dialer);
if ((status < 500) && (status > 599))
{
DBG("HTTP error %d reset to 1", status);
status = 1; /* not a real HTTP error */
}
WRN("HTTP dialer error url='%s': %s",
efl_net_dialer_address_dial_get(url_con->dialer),
eina_error_msg_get(*perr));
_ecore_con_event_url_complete_add(url_con, status);
}Предупреждение PVS-Studio: V547 Expression '(status < 500) && (status > 599)' is always false. ecore_con_url.c 351
Правильный вариант проверки должен быть таким:
if ((status < 500) || (status > 599))Фрагмент кода с этой ошибкой скопирован ещё в 2 места:
- V547 Expression '(status < 500) && (status > 599)' is always false. ecore_con_url.c 658
- V547 Expression '(status < 500) && (status > 599)' is always false. ecore_con_url.c 1340
Следующая ошибочная ситуация:
EAPI void
eina_rectangle_pool_release(Eina_Rectangle *rect)
{
Eina_Rectangle *match;
Eina_Rectangle_Alloc *new;
....
match = (Eina_Rectangle *) (new + 1);
if (match)
era->pool->empty = _eina_rectangle_skyline_list_update(
era->pool->empty, match);
....
}Предупреждение PVS-Studio: V547 Expression 'match' is always true. eina_rectangle.c 798
После того, как к указателю прибавлена единица, нет смысла проверять его на равенство NULL.
Указатель match может стать равен нулю, только если при сложении произойдёт переполнение. Однако переполнение указателя считается неопределённым поведением, поэтому такой вариант не стоит рассматривать.
И ещё один случай.
EAPI const void *
evas_object_smart_interface_get(const Evas_Object *eo_obj,
const char *name)
{
Evas_Smart *s;
....
s = evas_object_smart_smart_get(eo_obj);
if (!s) return NULL;
if (s)
....
}Предупреждение PVS-Studio: V547 Expression 's' is always true. evas_object_smart.c 160
Если указатель равен NULL, то происходит выход из функции. Повторная проверка не имеет смысла.
Прочие ошибки: EFL_V547.txt.
Есть предупреждения V547, которые я пропустил и не выписал, так как мне было неинтересно в них разбираться. Среди них могут найтись ещё несколько ошибок.
V556 (8 ошибок)
Все 8 ошибок выданы на один фрагмент кода. Для начала посмотрим на объявление двух перечислений.
typedef enum _Elm_Image_Orient_Type
{
ELM_IMAGE_ORIENT_NONE = 0,
ELM_IMAGE_ORIENT_0 = 0,
ELM_IMAGE_ROTATE_90 = 1,
ELM_IMAGE_ORIENT_90 = 1,
ELM_IMAGE_ROTATE_180 = 2,
ELM_IMAGE_ORIENT_180 = 2,
ELM_IMAGE_ROTATE_270 = 3,
ELM_IMAGE_ORIENT_270 = 3,
ELM_IMAGE_FLIP_HORIZONTAL = 4,
ELM_IMAGE_FLIP_VERTICAL = 5,
ELM_IMAGE_FLIP_TRANSPOSE = 6,
ELM_IMAGE_FLIP_TRANSVERSE = 7
} Elm_Image_Orient;
typedef enum
{
EVAS_IMAGE_ORIENT_NONE = 0,
EVAS_IMAGE_ORIENT_0 = 0,
EVAS_IMAGE_ORIENT_90 = 1,
EVAS_IMAGE_ORIENT_180 = 2,
EVAS_IMAGE_ORIENT_270 = 3,
EVAS_IMAGE_FLIP_HORIZONTAL = 4,
EVAS_IMAGE_FLIP_VERTICAL = 5,
EVAS_IMAGE_FLIP_TRANSPOSE = 6,
EVAS_IMAGE_FLIP_TRANSVERSE = 7
} Evas_Image_Orient;Как вы видите, названия констант в этих перечислениях похожи. Это и подвело программиста.
EAPI void
elm_image_orient_set(Evas_Object *obj, Elm_Image_Orient orient)
{
Efl_Orient dir;
Efl_Flip flip;
EFL_UI_IMAGE_DATA_GET(obj, sd);
sd->image_orient = orient;
switch (orient)
{
case EVAS_IMAGE_ORIENT_0:
....
case EVAS_IMAGE_ORIENT_90:
....
case EVAS_IMAGE_FLIP_HORIZONTAL:
....
case EVAS_IMAGE_FLIP_VERTICAL:
....
}Предупреждения PVS-Studio:
- V556 The values of different enum types are compared: switch(ENUM_TYPE_A) { case ENUM_TYPE_B:… }. efl_ui_image.c 2141
- V556 The values of different enum types are compared: switch(ENUM_TYPE_A) { case ENUM_TYPE_B:… }. efl_ui_image.c 2145
- V556 The values of different enum types are compared: switch(ENUM_TYPE_A) { case ENUM_TYPE_B:… }. efl_ui_image.c 2149
- V556 The values of different enum types are compared: switch(ENUM_TYPE_A) { case ENUM_TYPE_B:… }. efl_ui_image.c 2153
- V556 The values of different enum types are compared: switch(ENUM_TYPE_A) { case ENUM_TYPE_B:… }. efl_ui_image.c 2157
- V556 The values of different enum types are compared: switch(ENUM_TYPE_A) { case ENUM_TYPE_B:… }. efl_ui_image.c 2161
- V556 The values of different enum types are compared: switch(ENUM_TYPE_A) { case ENUM_TYPE_B:… }. efl_ui_image.c 2165
- V556 The values of different enum types are compared: switch(ENUM_TYPE_A) { case ENUM_TYPE_B:… }. efl_ui_image.c 2169
Восемь раз сравниваются сущности, относящиеся к разным перечислениям.
При этом, благодаря везению, сравнения работаю правильно. Константы совпадают:
- ELM_IMAGE_ORIENT_NONE = 0; EVAS_IMAGE_ORIENT_NONE = 0,
- ELM_IMAGE_ORIENT_0 = 0; EVAS_IMAGE_ORIENT_0 = 0
- ELM_IMAGE_ROTATE_90 = 1; EVAS_IMAGE_ORIENT_90 = 1
- и так далее.
Функция будет работать правильно, но всё равно это ошибки.
Comment by Carsten Haitzler. All of the above orient/rotate enum stuff is intentional. We had to cleanup duplication of enums and we ensured they had the same values so they were interchangeable — we moved from rotate to orient and kept the compatibility. It's part of our move over to the new object system and a lot of code auto-generation etc. that is still underway and beta. It's not an error but intended to do this as part of transitioning, so it's a false positive.
Комментарий Андрей Карпова. Здесь и в некоторых других местах я не согласен, что это false positives. По такой логике получается, что предупреждение для неправильного, но по какой-то причине работающего кода, является ложным срабатыванием.
V558 (4 ошибки)
accessor_iterator<T>& operator++(int)
{
accessor_iterator<T> tmp(*this);
++*this;
return tmp;
}Предупреждение PVS-Studio: V558 Function returns the reference to temporary local object: tmp. eina_accessor.hh 519
Чтобы исправить код, надо убрать & из объявления функции:
accessor_iterator<T> operator++(int)Другие ошибки:
- V558 Function returns the reference to temporary local object: tmp. eina_accessor.hh 535
- V558 Function returns the reference to temporary local object: tmp. eina_accessor.hh 678
- V558 Function returns the reference to temporary local object: tmp. eina_accessor.hh 694
V560 (32 ошибок)
static unsigned int read_compressed_channel(....)
{
....
signed char headbyte;
....
if (headbyte >= 0)
{
....
}
else if (headbyte >= -127 && headbyte <= -1) // <=
....
}Предупреждение PVS-Studio: V560 A part of conditional expression is always true: headbyte <= — 1. evas_image_load_psd.c 221
Если переменная headbyte была >= 0, то нет смысла выполнять проверку <= -1.
Рассмотрим другой случай.
static Eeze_Disk_Type
_eeze_disk_type_find(Eeze_Disk *disk)
{
const char *test;
....
test = udev_device_get_property_value(disk->device, "ID_BUS");
if (test)
{
if (!strcmp(test, "ata")) return EEZE_DISK_TYPE_INTERNAL;
if (!strcmp(test, "usb")) return EEZE_DISK_TYPE_USB;
return EEZE_DISK_TYPE_UNKNOWN;
}
if ((!test) && (!filesystem)) // <=
....
}Предупреждение PVS-Studio: V560 A part of conditional expression is always true: (!test). eeze_disk.c 55
Второе условие избыточно. Если указатель test был ненулевым, то функция бы уже завершила свою работу.
Другие ошибки: EFL_V560.txt.
V568 (3 ошибки)
EOLIAN static Eina_Error
_efl_net_server_tcp_efl_net_server_fd_socket_activate(....)
{
....
struct sockaddr_storage *addr;
socklen_t addrlen;
....
addrlen = sizeof(addr);
if (getsockname(fd, (struct sockaddr *)&addr, &addrlen) != 0)
....
}Предупреждение PVS-Studio: V568 It's odd that 'sizeof()' operator evaluates the size of a pointer to a class, but not the size of the 'addr' class object. efl_net_server_tcp.c 192
У меня есть сильное подозрение, что следовало вычислять не размер указателя, а размер структуры. Тогда код должен быть таким:
addrlen = sizeof(*addr);Другие ошибки:
- V568 It's odd that 'sizeof()' operator evaluates the size of a pointer to a class, but not the size of the 'addr' class object. efl_net_server_udp.c 228
- V568 It's odd that 'sizeof()' operator evaluates the size of a pointer to a class, but not the size of the 'addr' class object. efl_net_server_unix.c 198
V571 (6 ошибок)
EAPI void eeze_disk_scan(Eeze_Disk *disk)
{
....
if (!disk->cache.vendor)
if (!disk->cache.vendor)
disk->cache.vendor = udev_device_get_sysattr_value(....);
....
}Предупреждение PVS-Studio: V571 Recurring check. The 'if (!disk->cache.vendor)' condition was already verified in line 298. eeze_disk.c 299
Лишняя или неправильная проверка.
Другие ошибки:
- V571 Recurring check. The 'if (!disk->cache.model)' condition was already verified in line 302. eeze_disk.c 303
- V571 Recurring check. The 'if (priv->last_buffer)' condition was already verified in line 150. emotion_sink.c 152
- V571 Recurring check. The 'if (pd->editable)' condition was already verified in line 892. elm_code_widget.c 894
- V571 Recurring check. The 'if (mnh >= 0)' condition was already verified in line 279. els_box.c 281
- V571 Recurring check. The 'if (mnw >= 0)' condition was already verified in line 285. els_box.c 287
Примечание. Carsten Haitzler не считает это ошибками. Он считает подобные сообщения рекомендациями по микрооптимизации. А я считаю, что этот код неверен и его следует править. С моей точки зрения, это ошибки. Здесь у нас несогласие, как интерпретировать сообщения анализатора.
V575 (126 ошибок)
Диагностика срабатывает, когда функции передают странные фактические аргументы. Рассмотрим несколько вариантов срабатываний этой диагностики.
static void
free_buf(Eina_Evlog_Buf *b)
{
if (!b->buf) return;
b->size = 0;
b->top = 0;
# ifdef HAVE_MMAP
munmap(b->buf, b->size);
# else
free(b->buf);
# endif
b->buf = NULL;
}Предупреждение PVS-Studio: V575 The 'munmap' function processes '0' elements. Inspect the second argument. eina_evlog.c 117
Вначале в переменную b->size записали 0, а затем передали ее в функцию munmap.
Мне кажется, надо было написать так:
static void
free_buf(Eina_Evlog_Buf *b)
{
if (!b->buf) return;
b->top = 0;
# ifdef HAVE_MMAP
munmap(b->buf, b->size);
# else
free(b->buf);
# endif
b->buf = NULL;
b->size = 0;
}Продолжим.
EAPI Eina_Bool
eina_simple_xml_parse(....)
{
....
else if ((itr + sizeof("<!>") - 1 < itr_end) &&
(!memcmp(itr + 2, "", sizeof("") - 1)))
....
}Предупреждение PVS-Studio: V575 The 'memcmp' function processes '0' elements. Inspect the third argument. eina_simple_xml_parser.c 355
Непонятно, зачем сравнивать 0 байт памяти.
Продолжим.
static void
_edje_key_down_cb(....)
{
....
char *compres = NULL, *string = (char *)ev->string;
....
if (compres)
{
string = compres;
free_string = EINA_TRUE;
}
else free(compres);
....
}Предупреждение PVS-Studio: V575 The null pointer is passed into 'free' function. Inspect the first argument. edje_entry.c 2306
Если указатель compress нулевой, то и не надо освобождать память. Строчку
else free(compres);можно удалить.
Comment by Carsten Haitzler. Not a bug but indeed some extra if paranoia like code that isn't needed. Micro optimizations again?
Комментарий Андрей Карпова. Вновь у нас разный взгляд. Я считаю это полезным предупреждением, указывающим на ошибку. Есть вероятность, что надо было освободить другой указатель и совершенно верно, что анализатор указывает на этот код. Даже если ошибки нет, код следует исправить, чтобы он не путал анализатор и программистов.
Аналогично:
- V575 The null pointer is passed into 'free' function. Inspect the first argument. efl_ui_internal_text_interactive.c 1022
- V575 The null pointer is passed into 'free' function. Inspect the first argument. edje_cc_handlers.c 15962
Но больше всего срабатываний диагностики V575 связано с использованием потенциально нулевых указателей. В общем-то эти ошибки аналогичны тем, что мы рассматривали, говоря о диагностике V522.
static void _fill_all_outs(char **outs, const char *val)
{
size_t vlen = strlen(val);
for (size_t i = 0; i < (sizeof(_dexts) / sizeof(char *)); ++i)
{
if (outs[i])
continue;
size_t dlen = strlen(_dexts[i]);
char *str = malloc(vlen + dlen + 1);
memcpy(str, val, vlen);
memcpy(str + vlen, _dexts[i], dlen);
str[vlen + dlen] = '';
outs[i] = str;
}
}Предупреждение PVS-Studio: V575 The potential null pointer is passed into 'memcpy' function. Inspect the first argument. main.c 112
Используем указатель, не проверив, удалось ли выделить память.
Другие ошибки: EFL_V575.txt.
V587 (2 ошибки)
void
_ecore_x_event_handle_focus_in(XEvent *xevent)
{
....
e->time = _ecore_x_event_last_time;
_ecore_x_event_last_time = e->time;
....
}Предупреждение PVS-Studio: V587 An odd sequence of assignments of this kind: A = B; B = A;. Check lines: 1006, 1007. ecore_x_events.c 1007
Comment by Carsten Haitzler. Not bugs as such — looks like just overzealous storing of last timestamp. This is adding a timestamp to an event when no original timestamp exists so we can keep a consistent structure for events with timestamps, but it is code clutter and a micro optimization.
Комментарий Андрей Карпова. Видимо, мы не можем прийти к согласию по ряду вопросов. Я говорю ошибка, Carsten говорит, что это неаккуратность. Я не согласен и по этой причине несколько подобных комментариев я не стал включать в статью.
Ещё одна ошибка: V587 An odd sequence of assignments of this kind: A = B; B = A;. Check lines: 1050, 1051. ecore_x_events.c 1051
V590 (3 ошибки)
static int command(void)
{
....
while (*lptr == ' ' && *lptr != '')
lptr++; /* skip whitespace */
....
}Предупреждение PVS-Studio: V590 Consider inspecting the '* lptr == ' ' && * lptr != ''' expression. The expression is excessive or contains a misprint. embryo_cc_sc2.c 944
Избыточная проверка. Её можно упростить:
while (*lptr == ' ')Ещё два аналогичных предупреждения:
- V590 Consider inspecting the 'sym->ident == 9 || sym->ident != 10' expression. The expression is excessive or contains a misprint. embryo_cc_sc3.c 1782
- V590 Consider inspecting the '* p == 'n' || * p != '"'' expression. The expression is excessive or contains a misprint. cpplib.c 4012
V591 (1 ошибка)
_self_type& operator=(_self_type const& other)
{
_base_type::operator=(other);
}Предупреждение PVS-Studio: V591 Non-void function should return a value. eina_accessor.hh 330
V595 (4 ошибок)
static void
eng_image_size_get(void *engine EINA_UNUSED, void *image,
int *w, int *h)
{
Evas_GL_Image *im;
if (!image)
{
*w = 0; // <=
*h = 0; // <=
return;
}
im = image;
if (im->orient == EVAS_IMAGE_ORIENT_90 ||
im->orient == EVAS_IMAGE_ORIENT_270 ||
im->orient == EVAS_IMAGE_FLIP_TRANSPOSE ||
im->orient == EVAS_IMAGE_FLIP_TRANSVERSE)
{
if (w) *w = im->h;
if (h) *h = im->w;
}
else
{
if (w) *w = im->w;
if (h) *h = im->h;
}
}Предупреждения PVS-Studio:
- V595 The 'w' pointer was utilized before it was verified against nullptr. Check lines: 575, 585. evas_engine.c 575
- V595 The 'h' pointer was utilized before it was verified against nullptr. Check lines: 576, 586. evas_engine.c 576
Проверки if (w) и if (h) подсказывают анализатору, что входные аргументы w и h могут быть равны NULL. Опасно, что в начале функции они используются без проверки.
Если вызвать функцию eng_image_size_get вот так:
eng_image_size_get(NULL, NULL, NULL, NULL);то она окажется к этому не готова и произойдёт разыменование нулевого указателя.
Сообщения, которые, как я думаю, также указывают на ошибки:
- V595 The 'cur->node' pointer was utilized before it was verified against nullptr. Check lines: 9889, 9894. evas_object_textblock.c 9889
- V595 The 'subtype' pointer was utilized before it was verified against nullptr. Check lines: 2200, 2203. eet_data.c 2200
V597 (6 ошибок)
EAPI Eina_Binbuf *
emile_binbuf_decipher(Emile_Cipher_Algorithm algo,
const Eina_Binbuf *data,
const char *key,
unsigned int length)
{
....
Eina_Binbuf *result = NULL;
unsigned int *over;
EVP_CIPHER_CTX *ctx = NULL;
unsigned char ik[MAX_KEY_LEN];
unsigned char iv[MAX_IV_LEN];
....
on_error:
memset(iv, 0, sizeof (iv));
memset(ik, 0, sizeof (ik));
if (ctx)
EVP_CIPHER_CTX_free(ctx);
eina_binbuf_free(result);
return NULL;
}Предупреждения PVS-Studio:
- V597 The compiler could delete the 'memset' function call, which is used to flush 'iv' buffer. The memset_s() function should be used to erase the private data. emile_cipher_openssl.c 293
- V597 The compiler could delete the 'memset' function call, which is used to flush 'ik' buffer. The memset_s() function should be used to erase the private data. emile_cipher_openssl.c 294
Я уже много раз писал в статьях, почему компилятор может удалить в таком коде вызовы функций memset. Не хочется повторяться. Если кто-то ещё не знаком с этим вопросом, предлагаю ознакомиться с описанием диагностики V597.
Comment by Carsten Haitzler. Above 2 — totally familiar with the issue. The big problem is memset_s is not portable or easily available, thus why we don't use it yet. You have to do special checks for it to see if it exists as it does not exist everywhere. Just as a simple example add AC_CHECK_FUNCS([memset_s]) to your configure.ac and memset_s is not found you have to jump through some more hoops like define __STDC_WANT_LIB_EXT1__ 1 before including system headers… and it's still not declared. On my pretty up to date Arch system memset_s is not defined by any system headers, same on debian testing… warning: implicit declaration of function 'memset_s'; did you mean memset'? [-Wimplicit-function-declaration], and then compile failure… no matter what I do. A grep -r of all my system includes shows no memset_s declared… so I think advising people to use memset_s is only a viable advice if its widely available and usable. Be aware of this.
Другие ошибки:
- V597 The compiler could delete the 'memset' function call, which is used to flush 'key_material' buffer. The memset_s() function should be used to erase the private data. emile_cipher_openssl.c 144
- V597 The compiler could delete the 'memset' function call, which is used to flush 'iv' buffer. The memset_s() function should be used to erase the private data. emile_cipher_openssl.c 193
- V597 The compiler could delete the 'memset' function call, which is used to flush 'ik' buffer. The memset_s() function should be used to erase the private data. emile_cipher_openssl.c 194
- V597 The compiler could delete the 'memset' function call, which is used to flush 'key_material' buffer. The memset_s() function should be used to erase the private data. emile_cipher_openssl.c 249
V609 (1 ошибка)
Для начала рассмотрим функцию eina_value_util_type_size.
static inline size_t
eina_value_util_type_size(const Eina_Value_Type *type)
{
if (type == EINA_VALUE_TYPE_INT)
return sizeof(int32_t);
if (type == EINA_VALUE_TYPE_UCHAR)
return sizeof(unsigned char);
if ((type == EINA_VALUE_TYPE_STRING) ||
(type == EINA_VALUE_TYPE_STRINGSHARE))
return sizeof(char*);
if (type == EINA_VALUE_TYPE_TIMESTAMP)
return sizeof(time_t);
if (type == EINA_VALUE_TYPE_ARRAY)
return sizeof(Eina_Value_Array);
if (type == EINA_VALUE_TYPE_DOUBLE)
return sizeof(double);
if (type == EINA_VALUE_TYPE_STRUCT)
return sizeof(Eina_Value_Struct);
return 0;
}Обратите внимание, что функция может вернуть 0. Теперь посмотрим, как эта функция используется:
static inline unsigned int
eina_value_util_type_offset(const Eina_Value_Type *type,
unsigned int base)
{
unsigned size, padding;
size = eina_value_util_type_size(type);
if (!(base % size))
return base;
padding = ( (base > size) ? (base - size) : (size - base));
return base + padding;
}Предупреждение PVS-Studio: V609 Mod by zero. Denominator range [0..24]. eina_inline_value_util.x 60
Потенциальное деление на ноль. Я не знаю возможна ли в реальности ситуация, когда функция eina_value_util_type_size вернёт здесь 0. Но в любом случае код опасен.
Comment by Carsten Haitzler. The 0 return would only happen if you have provided totally invalid input, like again strdup(NULL)… So I call this a false positive as you cant have an eina_value generic value that is not valid without bad stuff happening — validate you passes a proper value in first. eina_value is performance sensitive btw so every check here costs something. it's like adding if() checks to the add opcode.
V610 (1 ошибка)
void fetch_linear_gradient(....)
{
....
if (t + inc*length < (float)(INT_MAX >> (FIXPT_BITS + 1)) &&
t+inc*length > (float)(INT_MIN >> (FIXPT_BITS + 1)))
....
}Предупреждение PVS-Studio: V610 Unspecified behavior. Check the shift operator '>>'. The left operand '(- 0x7fffffff — 1)' is negative. ector_software_gradient.c 412
V614 (1 ошибка)
extern struct tm *gmtime (const time_t *__timer)
__attribute__ ((__nothrow__ , __leaf__));
static void
_set_headers(Evas_Object *obj)
{
static char part[] = "ch_0.text";
int i;
struct tm *t;
time_t temp;
ELM_CALENDAR_DATA_GET(obj, sd);
elm_layout_freeze(obj);
sd->filling = EINA_TRUE;
t = gmtime(&temp); // <=
....
}Предупреждение PVS-Studio: V614 Uninitialized variable 'temp' used. Consider checking the first actual argument of the 'gmtime' function. elm_calendar.c 720
V621 (1 ошибка)
static void
_opcodes_unregister_all(Eina_Debug_Session *session)
{
Eina_List *l;
int i;
_opcode_reply_info *info = NULL;
if (!session) return;
session->cbs_length = 0;
for (i = 0; i < session->cbs_length; i++)
eina_list_free(session->cbs[i]);
....
}Предупреждение PVS-Studio: V621 Consider inspecting the 'for' operator. It's possible that the loop will be executed incorrectly or won't be executed at all. eina_debug.c 405
V630 (2 ошибки)
Есть обыкновенный класс btVector3 с конструктором. Впрочем, этот конструктор ничего не делает.
class btVector3
{
public:
....
btScalar m_floats[4];
inline btVector3() { }
....
};И есть вот такая структура Simulation_Msg:
typedef struct _Simulation_Msg Simulation_Msg;
struct _Simulation_Msg {
EPhysics_Body *body_0;
EPhysics_Body *body_1;
btVector3 pos_a;
btVector3 pos_b;
Eina_Bool tick:1;
};Обратите внимание, что некоторые члены класса имеют тип btVector3. Теперь посмотрим, как создаётся структура:
_ephysics_world_tick_dispatch(EPhysics_World *world)
{
Simulation_Msg *msg;
if (!world->ticked)
return;
world->ticked = EINA_FALSE;
world->pending_ticks++;
msg = (Simulation_Msg *) calloc(1, sizeof(Simulation_Msg));
msg->tick = EINA_TRUE;
ecore_thread_feedback(world->cur_th, msg);
}Предупреждение PVS-Studio: V630 The 'calloc' function is used to allocate memory for an array of objects which are classes containing constructors. ephysics_world.cpp 299
Структура, содержащая в себе non-POD члены, создаётся с помощью вызова функции calloc.
На практике этот код будет работать, но вообще так делать очень нехорошо. Формально использование такой структуры будет приводить к неопределённому поведению.
Ещё одна ошибка: V630 The 'calloc' function is used to allocate memory for an array of objects which are classes containing constructors. ephysics_world.cpp 471
Comment by Carsten Haitzler. Because the other end of the pipe is C code that is passing around a raw ptr as the result from thread A to thread B, it's a mixed c and c++ environment. In the end we'd be sending raw ptr's around no matter what...
V654 (2 ошибки)
int
evas_mem_free(int mem_required EINA_UNUSED)
{
return 0;
}
int
evas_mem_degrade(int mem_required EINA_UNUSED)
{
return 0;
}
void *
evas_mem_calloc(int size)
{
void *ptr;
ptr = calloc(1, size);
if (ptr) return ptr;
MERR_BAD();
while ((!ptr) && (evas_mem_free(size))) ptr = calloc(1, size);
if (ptr) return ptr;
while ((!ptr) && (evas_mem_degrade(size))) ptr = calloc(1, size);
if (ptr) return ptr;
MERR_FATAL();
return NULL;
}Предупреждения PVS-Studio:
- V654 The condition '(!ptr) && (evas_mem_free(size))' of loop is always false. main.c 44
- V654 The condition '(!ptr) && (evas_mem_degrade(size))' of loop is always false. main.c 46
Видимо это какой-то недописанный код.
Comment by Carsten Haitzler. Old old code because caching was implemented, so it was basically a lot of NOP's waiting to be filled in. since evas speculatively cached data (megabytes of it) the idea was that if allocs fail — free up some cache and try again… if that fails then actually try nuke some non-cached data that could be reloaded/rebuilt but with more cost… and only fail after that. But because of overcommit this didn't end up practical as allocs would succeed then just fall over often enough if you did hit a really low memory situation, so I gave up. it's not a bug. it's a piece of history :).
Следующий случай более интересный и непонятный.
EAPI void evas_common_font_query_size(....)
{
....
size_t cluster = 0;
size_t cur_cluster = 0;
....
do
{
cur_cluster = cluster + 1;
glyph--;
if (cur_w > ret_w)
{
ret_w = cur_w;
}
}
while ((glyph > first_glyph) && (cur_cluster == cluster));
....
}Предупреждение PVS-Studio: V654 The condition of loop is always false. evas_font_query.c 376
В цикле выполняется вот это присваивание:
cur_cluster = cluster + 1;Это означает, что сравнение (cur_cluster == cluster) всегда вычисляется как false.
Comment by Carsten Haitzler. Above… it seems you built without harfbuzz support… we highly don't recommend that. it's not tested. Building without basically nukes almost all of the interesting unicode/intl support for text layout. You do have to explicitly — disable it… because with harfbuzz support we have opentype enabled and a different bit of code is executed due to ifdefs… if you actually check history of the code before adding opentype support it didn't loop over clusters at all or even glyphs… so really the ifdef just ensures the loop only loops one and avoids more ifdefs later in the loop conditions making the code easier to maintain — beware the ifdefs!
V668 (21 ошибка)
Как мы узнали ранее, в коде имеются сотни мест, где отсутствует проверка указателя после выделения памяти с помощью функции malloc / calloc.
На фоне этого проверки после использования оператора new выглядят как какая-то шутка.
Есть безобидные ошибки:
static EPhysics_Body *
_ephysics_body_rigid_body_add(....)
{
....
motion_state = new btDefaultMotionState();
if (!motion_state)
{
ERR("Couldn't create a motion state.");
goto err_motion_state;
}
....
}Предупреждение PVS-Studio: V668 There is no sense in testing the 'motion_state' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. ephysics_body.cpp 837
Выполнять проверку не имеет смысла, так как в случае невозможности выделения памяти будет сгенерировано исключение std::bad_alloc.
Comment by Carsten Haitzler. Fair enough, but be aware some compiler DON'T throw exceptions… they return NULL on new… so not totally useless code depending on the compiler. I believe VSC6 didn't throw an exception — so before exceptions were a thing this actually was correct behavior, also I depends on the allocator func if it throws and exception or not, so all in all, very minor harmless code.
Комментарий Андрей Карпова. Не согласен. См. следующий мой комментарий.
Есть и более серьезные ошибки:
EAPI EPhysics_Constraint *
ephysics_constraint_linked_add(EPhysics_Body *body1,
EPhysics_Body *body2)
{
....
constraint->bt_constraint = new btGeneric6DofConstraint(
*ephysics_body_rigid_body_get(body1),
*ephysics_body_rigid_body_get(body2),
btTransform(), btTransform(), false);
if (!constraint->bt_constraint)
{
ephysics_world_lock_release(constraint->world);
free(constraint);
return NULL;
}
....
} Предупреждение PVS-Studio: V668 There is no sense in testing the 'constraint->bt_constraint' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. ephysics_constraints.cpp 382
Если возникнет исключение, то не только нарушится логика работы программы, так ещё и произойдёт утечка памяти, так как не будет вызвана функция free.
Comment by Carsten Haitzler. Same as previous new + NULL check.
Комментарий Андрей Карпова. Мне не понятно, причём здесь Visual C++ версии 6.0. Да, он и некоторые другие древние компиляторы не генерируют исключения при использовании оператора new. Да, если используется древний компилятор, то программа будет работать корректно. Но Tizen никогда не будет компилироваться с помощью Visual C++ 6.0! Нет смысла про это думать. Новый компилятор будет генерировать исключение, и это приведёт к ошибкам. Ещё раз подчеркну, это не просто лишняя проверка. Программа работает не так, как ожидает программист. Более того, во втором примере ещё и memory-leak. Если рассматривается вариант, что new не генерирует исключение, то надо использовать вариант new(nothrow). Тогда и анализатор ругаться не будет. Так что, как не посмотреть на этот код, он неправильный.
Другие ошибки: EFL_V668.txt.
V674 (2 ошибки)
Для начала посмотрим, как объявлена функция abs:
extern int abs (int __x) __attribute__ ((__nothrow__ , __leaf__))
__attribute__ ((__const__)) ;Как видите, она принимает и возвращает значения типа int.
Теперь посмотрим, как эта функция используется.
#define ELM_GESTURE_MINIMUM_MOMENTUM 0.001
typedef int Evas_Coord;
struct _Elm_Gesture_Momentum_Info
{
....
Evas_Coord mx;
Evas_Coord my;
....
};
static void
_momentum_test(....)
{
....
if ((abs(st->info.mx) > ELM_GESTURE_MINIMUM_MOMENTUM) ||
(abs(st->info.my) > ELM_GESTURE_MINIMUM_MOMENTUM))
state_to_report = ELM_GESTURE_STATE_END;
....
}Предупреждения PVS-Studio:
- V674 The '0.001' literal of the 'double' type is compared to a value of the 'int' type. Consider inspecting the 'abs(st->info.mx) > 0.001' expression. elm_gesture_layer.c 2533
- V674 The '0.001' literal of the 'double' type is compared to a value of the 'int' type. Consider inspecting the 'abs(st->info.my) > 0.001' expression. elm_gesture_layer.c 2534
Согласитесь, странно сравнивать значения типа int с константой 0.001. Здесь какая-то ошибка.
V686 (3 ошибки)
static Image_Entry *
_scaled_image_find(Image_Entry *im, ....)
{
size_t pathlen, keylen, size;
char *hkey;
Evas_Image_Load_Opts lo;
Image_Entry *ret;
if (((!im->file) || ((!im->file) && (!im->key))) || (!im->data1) ||
((src_w == dst_w) && (src_h == dst_h)) ||
((!im->flags.alpha) && (!smooth))) return NULL;
....
}Предупреждение PVS-Studio: V686 A pattern was detected: (!im->file) || ((!im->file) && ...). The expression is excessive or contains a logical error. evas_cache2.c 825
Скорее всего это не настоящая ошибка, а избыточный код. Поясню на простом примере.
if (A || (A && B) || C)
Это выражение можно упростить до:
if (A || C)
Хотя есть вероятность, что в выражении допущена какая-то опечатка и не проверятся что-то важное. В таком случае это именно ошибка. Мне сложно судить об этом незнакомом коде.
Аналогичные избыточные условия:
- V686 A pattern was detected: (!im->file) || ((!im->file) && ...). The expression is excessive or contains a logical error. evas_cache2.c 905
- V686 A pattern was detected: (nextc == '*') || ((nextc == '*') && ...). The expression is excessive or contains a logical error. cpplib.c 1022
V694 (2 ошибки)
#define CPP_PREV_BUFFER(BUFFER) ((BUFFER)+1)
static void
initialize_builtins(cpp_reader * pfile)
{
....
cpp_buffer *pbuffer = CPP_BUFFER(pfile);
while (CPP_PREV_BUFFER(pbuffer))
pbuffer = CPP_PREV_BUFFER(pbuffer);
....
}Предупреждение PVS-Studio: V694 The condition ((pbuffer) + 1) is only false if there is pointer overflow which is undefined behavior anyway. cpplib.c 2496
Чтобы было понятнее, раскрою макрос.
cpp_buffer *pbuffer = ....;
while (pbuffer + 1)
....Условие цикла всегда истинно. Теоретически оно может стать ложным в случае, если указатель равен своему предельному значению. При этом происходит переполнение. Это считается undefined behavior, а значит компилятор имеет право не учитывать такую ситуацию. Компилятор может удалить проверку условия, и тогда получится:
while (true)
pbuffer = CPP_PREV_BUFFER(pbuffer);Вот такая интересная ошибка.
Ещё одна такая неправильная проверка: V694 The condition ((ip) + 1) is only false if there is pointer overflow which is undefined behavior anyway. cpplib.c 2332
Comment by Carsten Haitzler. This old code indeed has issues. There should be checks against CPP_NULL_BUFFER(pfile) because if its a linked list this is a null heck, if its a static buffer array as a stack, it checks stack end position — interestingly in decades it's never been triggered that I know of.
V701 (69 ошибок)
static void
_efl_vg_gradient_efl_gfx_gradient_stop_set(
...., Efl_VG_Gradient_Data *pd, ....)
{
pd->colors = realloc(pd->colors,
length * sizeof(Efl_Gfx_Gradient_Stop));
if (!pd->colors)
{
pd->colors_count = 0;
return ;
}
memcpy(pd->colors, colors,
length * sizeof(Efl_Gfx_Gradient_Stop));
pd->colors_count = length;
_efl_vg_changed(obj);
}Предупреждение PVS-Studio: V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'pd->colors' is lost. Consider assigning realloc() to a temporary pointer. evas_vg_gradient.c 14
Ошибка заключается в этой строчке:
pd->colors = realloc(pd->colors, ....);Старое значение указателя pd->colors нигде не сохраняется. Если не удастся выделить память, то произойдёт утечка памяти. Адрес предыдущего буфера будет потерян, так как в pd->colors будет записано значение NULL.
В некоторых случаях утечка памяти дополняется ещё и разыменованием нулевого указателя. Если разыменование нулевого указателя никак не обрабатывается, то программа аварийно завершится. Если обрабатывается, и программа продолжает работу, то произойдёт утечка памяти. Вот пример такого кода:
EOLIAN void _evas_canvas_key_lock_add(
Eo *eo_e, Evas_Public_Data *e, const char *keyname)
{
if (!keyname) return;
if (e->locks.lock.count >= 64) return;
evas_key_lock_del(eo_e, keyname);
e->locks.lock.count++;
e->locks.lock.list =
realloc(e->locks.lock.list,
e->locks.lock.count * sizeof(char *));
e->locks.lock.list[e->locks.lock.count - 1] = strdup(keyname);
eina_hash_free_buckets(e->locks.masks);
}Предупреждение PVS-Studio: V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'e->locks.lock.list' is lost. Consider assigning realloc() to a temporary pointer. evas_key.c 142
Другие ошибки: EFL_701.txt.
V728 (4 ошибки)
static Eina_Bool
_evas_textblock_node_text_adjust_offsets_to_start(....)
{
Evas_Object_Textblock_Node_Format *last_node, *itr;
....
if (!itr || (itr && (itr->text_node != n)))
....
}Предупреждение PVS-Studio: V728 An excessive check can be simplified. The '||' operator is surrounded by opposite expressions '!itr' and 'itr'. evas_object_textblock.c 9505
Не ошибка, но избыточно сложное условие. Его можно упростить:
if (!itr || (itr->text_node != n))Другие избыточные условия:
- V728 An excessive check can be simplified. The '||' operator is surrounded by opposite expressions '!p' and 'p'. elm_theme.c 447
- V728 An excessive check can be simplified. The '||' operator is surrounded by opposite expressions '!ss' and 'ss'. config.c 3932
- V728 An excessive check can be simplified. The '||' operator is surrounded by opposite expressions '!icon_version' and 'icon_version'. efreet_icon_cache_create.c 917
V769 (11 ошибок)
Ранее мы рассматривали предупреждение V522, когда отсутствует проверка значения указателя после выделения памяти. Сейчас мы рассмотрим схожий вариант ошибки.
EAPI Eina_Bool
edje_edit_sound_sample_add(
Evas_Object *obj, const char *name, const char *snd_src)
{
....
ed->file->sound_dir->samples =
realloc(ed->file->sound_dir->samples,
sizeof(Edje_Sound_Sample) *
ed->file->sound_dir->samples_count);
sound_sample = ed->file->sound_dir->samples +
ed->file->sound_dir->samples_count - 1;
sound_sample->name = (char *)eina_stringshare_add(name);
....
}Предупреждение PVS-Studio: V769 The 'ed->file->sound_dir->samples' pointer in the expression could be nullptr. In such case, resulting value of arithmetic operations on this pointer will be senseless and it should not be used. edje_edit.c 1271
Вызывается функция выделения памяти. Полученный указатель не разыменовывается, а используется в выражении. К указателю прибавляется некое значение и говорить далее, что используется нулевой указатель, уже некорректно. Указатель в любом случае не нулевой, но может быть не валидным, если не удалось выделить память. Именно такой код и выявляет рассматриваемая диагностика.
Кстати, такие указатели более опасны, чем нулевые. Разыменование нулевого указателя гарантировано будет выявлено операционной системой. Используя указатель (NULL + N) можно попасть в область памяти, в которой хранятся какие-то данные и которая доступна для модификации.
Другие ошибки:
- V769 The 'new_txt' pointer in the 'new_txt + outlen' expression could be nullptr. In such case, resulting value will be senseless and it should not be used. eina_str.c 539
- V769 The 'new_txt' pointer in the 'new_txt + outlen' expression could be nullptr. In such case, resulting value will be senseless and it should not be used. eina_str.c 611
- V769 The 'tmp' pointer in the 'tmp ++' expression could be nullptr. In such case, resulting value will be senseless and it should not be used. evas_object_textblock.c 11131
- V769 The 'dst' pointer in the 'dst += sizeof (int)' expression could be nullptr. In such case, resulting value will be senseless and it should not be used. evas_font_compress.c 218
- V769 The 'content' pointer in the 'content + position' expression could be nullptr. In such case, resulting value will be senseless and it should not be used. elm_code_line.c 78
- V769 The 'newtext' pointer in the 'newtext + length1' expression could be nullptr. In such case, resulting value will be senseless and it should not be used. elm_code_line.c 102
- V769 The 'tmp' pointer in the 'tmp + dirlen' expression could be nullptr. In such case, resulting value will be senseless and it should not be used. elm_code_file.c 101
- V769 The 'ptr' pointer in the 'ptr += strlen(first) + newline_len' expression could be nullptr. In such case, resulting value will be senseless and it should not be used. elm_code_widget_text.c 72
- V769 The 'content' pointer in the 'content + 319' expression could be nullptr. In such case, resulting value will be senseless and it should not be used. test_store.c 198
- V769 The 'pos' pointer in the 'pos += sizeof (msg)' expression could be nullptr. In such case, resulting value will be senseless and it should not be used. evas_cserve2_cache.c 2534
V779 (19 ошибок)
Иногда диагностика V779 указывает на код, который плохо написан, но безвреден. Пример:
EAPI Eina_Bool
ecore_x_xinerama_screen_geometry_get(int screen,
int *x, int *y,
int *w, int *h)
{
LOGFN(__FILE__, __LINE__, __FUNCTION__);
#ifdef ECORE_XINERAMA
if (_xin_info)
{
int i;
for (i = 0; i < _xin_scr_num; i++)
{
if (_xin_info[i].screen_number == screen)
{
if (x) *x = _xin_info[i].x_org;
if (y) *y = _xin_info[i].y_org;
if (w) *w = _xin_info[i].width;
if (h) *h = _xin_info[i].height;
return EINA_TRUE;
}
}
}
#endif /* ifdef ECORE_XINERAMA */
if (x) *x = 0;
if (y) *y = 0;
if (w) *w = DisplayWidth(_ecore_x_disp, 0);
if (h) *h = DisplayHeight(_ecore_x_disp, 0);
return EINA_FALSE;
screen = 0; // <=
}Предупреждение PVS-Studio: V779 Unreachable code detected. It is possible that an error is present. ecore_x_xinerama.c 92
При определённых условиях компиляции, в теле функции нигде не используется аргумент screen. Видимо какой-то компилятор или анализатор предупреждал про неиспользуемый аргумент, и чтобы угодить этому инструменту, написали присваивание, которое на самом деле никогда не выполняется.
На мой взгляд было бы лучше пометить аргумент с помощью EINA_UNUSED.
Теперь рассмотрим более серьёзный случай:
extern void _exit (int __status) __attribute__ ((__noreturn__));
static void _timeout(int val)
{
_exit(-1);
if (val) return;
}Предупреждение PVS-Studio: V779 Unreachable code detected. It is possible that an error is present. timeout.c 30
Функция _exit не возвращает управления. Ооочень странный код. Мне кажется, функция должна была выглядеть так:
static void _timeout(int val)
{
if (val) return;
_exit(-1);
}Comment by Carsten Haitzler. Not a bug. it's also an unused param thing from before the macros. The timeout has the process self exit in case it takes too long (assuming the decoder lib is stuck if a timeout happens).
Комментарий Карпова Андрея. Я не согласен. Возможно, программа работает правильно, но с точки зрения профессионально программирования такой код недопустим. Я считаю ошибкой писать такой код для подавления предупреждения. Код запутывает человека, который будет сопровождать такой код.
Другие ошибки: EFL_V779.txt.
V1001 (6 ошибок)
static Elocation_Address *address = NULL;
EAPI Eina_Bool
elocation_address_get(Elocation_Address *address_shadow)
{
if (!address) return EINA_FALSE;
if (address == address_shadow) return EINA_TRUE;
address_shadow = address;
return EINA_TRUE;
}Предупреждение PVS-Studio: V1001 The 'address_shadow' variable is assigned but is not used until the end of the function. elocation.c 1122
Странный код. Мне кажется, что на самом деле здесь должно быть написано:
*address_shadow = *address;Другие ошибки:
- V1001 The 'screen' variable is assigned but is not used until the end of the function. ecore_x_xinerama.c 92
- V1001 The 'ret' variable is assigned but is not used until the end of the function. edje_edit.c 12774
- V1001 The 'ret' variable is assigned but is not used until the end of the function. edje_edit.c 15884
- V1001 The 'position_shadow' variable is assigned but is not used until the end of the function. elocation.c 1133
- V1001 The 'status_shadow' variable is assigned but is not used until the end of the function. elocation.c 1144
Комментарий Carsten Haitzler
PVS-Studio находит разные ошибки по сравнению с Coverity. Пока он кажется более шумным (больше ложных срабатываний, выданных за ошибки). НО некоторые из них действительно являются ошибками, поэтому если есть желание пройтись по всем ложным срабатываниям, там на самом деле есть некоторые важные находки (жемчужины). Текстовые отчеты намного менее полезные, чем у Coverity, но свою работу они делают. На самом деле, я бы рассмотрел PVS-Studio как второй добавочный инструмент защиты после Coverity так как он может уловить некоторые вещи, которые Coverity не может, и если есть желание посвятить этому время, это хорошая вещь. Я уже исправил некоторые предупреждения, исправление других займет время, а некоторые это просто не-ошибки, на которых могут жаловаться и PVS-Studio и Coverity, но не придать им значения — это лучшее решение.
Заключение
Ещё раз напомню, что желающие могут самостоятельно изучить отчёт анализатора и убедиться, что приведённые в статье характеристики анализатора соответствуют реальности.
Среди описанных в статье ошибок нет ничего эпичного, но это и неудивительно. Как я уже говорил, проект EFL регулярно проверяется с помощью Coverity. И несмотря на это, анализатору PVS-Studio всё равно удалось выявить много ошибок. Думаю, PVS-Studio показал бы себя намного лучше, если бы было возможно вернуться в прошлое и поменять анализаторы местами :). Я имею в виду, что, если бы изначально использовался PVS-Studio, а потом попробовали запустить Coverity, то PVS-Studio проявил бы себя ярче.
Предлагаю скачать и попробовать анализатор PVS-Studio для проверки своих проектов:
Спасибо за внимание и желаю поменьше ошибок в вашем коде.
