rss rss hh new rss rss hh new

-Поиск по дневнику

-Подписка по e-mail

-Постоянные читатели

BlooDandMilK Elenka2000 Python3 Stock_Market_Trading _ghbhjif balloonic dmitriy1800 ВаШуня Виталий_Хлусов Главный_синоптик

-Статистика

Создан: 17.03.2011
Записей:
Комментариев:
Написано: 51

Отчеты:
Посетители
Поисковые фразы

Характеристики анализатора PVS-Studio на примере EFL Core Libraries, 10-15% ложных срабатываний

Понедельник, 31 Июля 2017 г. 17:12 + в цитатник

После большой статьи про проверку операционной системы 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& operator++(int)
{
  accessor_iterator tmp(*this);
  ++*this;
  return tmp;
}

Предупреждение PVS-Studio: V558 Function returns the reference to temporary local object: tmp. eina_accessor.hh 519

Чтобы исправить код, надо убрать & из объявления функции:

accessor_iterator 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] = '\0';
    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 != '\0')
    lptr++; /* skip whitespace */
  ....
}

Предупреждение PVS-Studio: V590 Consider inspecting the '* lptr == ' ' && * lptr != '\0'' 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 alloc

<a href="https://www.liveinternet.ru/users/rss_rss_hh_new/post418787033/">РҐР°СЂР°РєС‚РµСЂРёСЃС‚РёРєРё Р°РЅР°Р»РёР·Р°С‚РѕСЂР° PVS-Studio РЅР° РїСЂРёРјРµСЂРµ EFL Core Libraries, 10-15% Р»РѕР¶РЅС‹С… СЃСЂР°Р±Р°С‚С‹РІР°РЅРёР№</a><br/>
РџРѕСЃР»Рµ Р±РѕР»СЊС€РѕР№ СЃС‚Р°С‚СЊРё РїСЂРѕ РїСЂРѕРІРµСЂРєСѓ РѕРїРµСЂР°С†РёРѕРЅРЅРѕР№ СЃРёСЃС‚РµРјС‹ Tizen РјРЅРµ Р±С‹Р»Рѕ Р·Р°РґР°РЅРѕ РјРЅРѕРіРѕ РІРѕРїСЂРѕСЃРѕРІ Рѕ РїСЂРѕС†РµРЅС‚Рµ Р»РѕР¶РЅС‹С… СЃСЂР°Р±Р°С‚С‹РІР°РЅРёР№ Рё Рѕ РїР»РѕС‚РЅРѕСЃС‚Рё РѕС€РёР±РѕРє (СЃРєРѕР»СЊРєРѕ РѕС€РёР±РѕРє PVS-Studio РІС‹СЏРІР»СЏРµС‚ РЅР° 1000 СЃС‚СЂРѕРє РєРѕРґР°). РњРѕРё СЂР°СЃСЃСѓР¶РґРµРЅРёСЏ Рѕ С‚РѕРј, С‡С‚Рѕ СЌС‚Рѕ СЃРёР»СЊРЅРѕ Р·Р°РІРёСЃРёС‚ РѕС‚ Р°РЅР°Р»РёР·РёСЂСѓРµРјРѕРіРѕ РїСЂРѕРµРєС‚Р° Рё РЅР°СЃС‚СЂРѕРµРє Р°РЅР°Р»РёР·Р°С‚РѕСЂР° РЅРµ РІС‹РіР»СЏРґСЏС‚ РєР°Рє РЅР°СЃС‚РѕСЏС‰РёР№ РѕС‚РІРµС‚. РЇ СЂРµС€РёР» РїСЂРёРІРµСЃС‚Рё РєРѕРЅРєСЂРµС‚РЅС‹Рµ С‡РёСЃР»Р°, РїСЂРѕРІРµРґСЏ Р±РѕР»РµРµ С‚С‰Р°С‚РµР»СЊРЅРѕРµ РёСЃСЃР»РµРґРѕРІР°РЅРёРµ РѕРґРЅРѕРіРѕ РёР· РїСЂРѕРµРєС‚РѕРІ, РІС…РѕРґСЏС‰РёС… РІ СЃРѕСЃС‚Р°РІ Tizen. РџРѕСЃРєРѕР»СЊРєСѓ РІ РѕР±СЃСѓР¶РґРµРЅРёРё СЃС‚Р°С‚СЊРё Р°РєС‚РёРІРЅРѕРµ СѓС‡Р°СЃС‚РёРµ РїСЂРёРЅРёРјР°Р» Carsten Haitzler, СЏ СЂРµС€РёР», С‡С‚Рѕ Р±СѓРґРµС‚ РёРЅС‚РµСЂРµСЃРЅРѕ РІР·СЏС‚СЊ РґР»СЏ СЌРєСЃРїРµСЂРёРјРµРЅС‚Р° EFL Core Libraries, РІ СЂР°Р·СЂ... <a href="https://www.liveinternet.ru/users/rss_rss_hh_new/post418787033/">Р§РёС‚Р°С‚СЊ РґР°Р»РµРµ...</a>

Комментировать

« Пред. запись — К дневнику — След. запись »

Страницы: [1] [Новые]

LiveInternetLiveInternet

-Поиск по дневнику

-Подписка по e-mail

-Постоянные читатели

-Статистика

Характеристики анализатора PVS-Studio на примере EFL Core Libraries, 10-15% ложных срабатываний

Предыстория

Характеристики

Как проводились расчёты

Найденные ошибки

V501 (1 ошибка)

V512 (8 ошибок)

V517 (3 ошибки)

V519 (1 ошибка)

V522 (563 ошибки)

V547 (39 ошибок)

V556 (8 ошибок)

V558 (4 ошибки)

V560 (32 ошибок)

V568 (3 ошибки)

V571 (6 ошибок)

V575 (126 ошибок)

V587 (2 ошибки)

V590 (3 ошибки)

V591 (1 ошибка)

V595 (4 ошибок)

V597 (6 ошибок)

V609 (1 ошибка)

V610 (1 ошибка)

V614 (1 ошибка)

V621 (1 ошибка)

V630 (2 ошибки)

V654 (2 ошибки)

V668 (21 ошибка)