О PVS-Studio в преддверии открытой конференции ИСП РАН им. В.П. Иванникова17.11.2017 16:33

Научное сообщество практически незнакомо со статическим анализатором кода PVS-Studio. 30 ноября и 1 декабря состоится мероприятие «Технологии анализа, моделирования и трансформации программ» в рамках открытой конференции ИСП РАН им. В.П. Иванникова. Я уверен, что это одно из самых тематичных для нас мероприятий, где новая аудитория могла бы узнать о существовании и возможностях анализатора PVS-Studio. На мой взгляд, наиболее подходящим для этого мог стать доклад, касающийся поиска ошибок в операционной системе Tizen. К сожалению, доклад получил отрицательные отзывы рецензентов и не будет включен в программу конференции. Тем не менее, пользуясь случаем, подведу итоги наших исследований, касающихся кода Tizen.

Андрей Карпов и Евгений Рыжков


В прошлом (2016) году мы посетили открытую конференцию ИСП РАН им. В.П. Иванникова и подметили, что большое внимание уделяется операционной системе Tizen.

ISPRAS OPEN 2016 (ru)


→ Ссылка на видео

Tizen — открытая операционная система на базе ядра Linux, разрабатываемая и управляемая такими корпорациями, как Intel и Samsung.

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

До посещения конференции в 2016 году мы ничего не знали про Tizen кроме того, что она существует. Но после мы подумали, что можно использовать эту операционную систему для популяризации методологии статического анализа среди разработчиков открытых проектов. И заодно это хороший способ в очередной раз продемонстрировать возможности анализатора PVS-Studio.

В результате появился цикл статей, посвященных операционной системе Tizen, наибольший интерес из которых представляет статья »27000 ошибок в операционной системе Tizen». Другие статьи:

  1. Поговорим о микрооптимизациях на примере кода Tizen.
  2. Продолжаем изучать Tizen: C# компоненты оказались высокого качества.
  3. Tizen: подводим итоги.
  4. Характеристики анализатора PVS-Studio на примере EFL Core Libraries, 10–15% ложных срабатываний.


Статьи показали, что анализатор может существенно улучшить качество кода операционной системы Tizen.

Логическим завершением всей проделанной работы мог бы стать наш доклад на конференции.

Впрочем, читатель справедливо может спросить, а почему я считаю, что анализатор PVS-Studio может улучшить код Tizen?

Я основываю своё мнение на том, что описанные мною ошибки (или большая их часть) были исправлены разработчиками. Я случайным образом взял несколько ранее обнаруженных ошибок в разных проектах и посмотрел, что с ними произошло. Чтобы не быть голословным, остановлюсь на этом подробнее.

Первая исправленная ошибка

Итак, в статье про 27000 ошибок я выписал следующий фрагмент с ошибкой в проекте org.tizen.w-home-0.1.0.

static void __page_focus_changed_cb(void *data)
{
  int i = 0;
  int *focus_unit = (int *)data;
  if (focus_unit == NULL || focus_unit < 0) {    // <=
    _E("focus page is wrong");
    return ;
  }
  ....
}


Нет смысла сравнивать указатель с 0, используя оператор <. Ошибка найдена благодаря предупреждению PVS-Studio: V503 This is a nonsensical comparison: pointer < 0. apps_view_circle_indicator.c 193

Итак, код претерпел изменение, и в новой версии он выглядит так: 

if (focus_unit == NULL || (*focus_unit) < 0) {


Значит в статье я угадал, что забыли разыменовать указатель.

Вторая исправленная ошибка

Проект capi-media-codec-0.5.3.

void extract_input_aacdec_m4a_test(
  App * app, unsigned char **data, int *size, bool * have_frame)
{
  ....
  unsigned char buffer[100000];
  ....
DONE:
  *data = buffer;
  *have_frame = TRUE;
  if (read_size >= offset)
    *size = offset;
  else
    *size = read_size;
}


Здесь и далее я не буду разбирать суть ошибки, так как их описание можно посмотреть в предыдущей статье. Ошибка найдена благодаря предупреждению PVS-Studio: V507 Pointer to local array 'buffer' is stored outside the scope of this array. Such a pointer will become invalid. media_codec_test.c 793

В новой версии проекта функция extract_input_aacdec_m4a_test просто исчезла. Так что можно сказать, что и эта ошибка исправлена, хотя и косвенным образом. Возможно, весь этот тест был лишним или неправильным.

Третья исправленная ошибка

Проект bluetooth-frwk-0.2.157.

typedef int gint;
typedef gint gboolean;  

#define BT_REQUEST_ID_RANGE_MAX 245

static gboolean req_id_used[BT_REQUEST_ID_RANGE_MAX];

void _bt_init_request_id(void)
{
  assigned_id = 0;
  memset(req_id_used, 0x00, BT_REQUEST_ID_RANGE_MAX);
}


Предупреждение PVS-Studio: V512 A call of the 'memset' function will lead to underflow of the buffer 'req_id_used'. bt-service-util.c 38

Ошибка исправлена благодаря тому, что теперь размер буфера задан не константой, а вычисляется с помощью оператора sizeof: 

memset(req_id_used, 0x00, sizeof(req_id_used));


Выбрали второй вариант исправления из описанных мною в статье.

Четвёртая исправленная ошибка

Проект org.tizen.screen-reader-0.0.8.

static void _on_atspi_event_cb(const AtspiEvent * event)
{
  ....
  char buf[256] = "\0";
  ....
  snprintf(buf, sizeof(buf), "%s, %s, ",
           name, _("IDS_BR_BODY_IMAGE_T_TTS"));
  ....
  snprintf(buf + strlen(buf), sizeof(buf),
           "%s, ", _("IDS_ACCS_BODY_SELECTED_TTS"));
  ....
}


Предупреждение PVS-Studio: V512 A call of the 'snprintf' function will lead to overflow of the buffer 'buf + strlen (buf)'. app_tracker.c 450

Исправленный вариант соответствует предложенному мной в статье варианту: 

snprintf(buf + strlen(buf), sizeof(buf) - strlen(buf), "%s, ",
         _("IDS_ACCS_BODY_SELECTED_TTS"));


Пятая исправленная ошибка

Проект capi-network-http-0.0.23.

int _read_request_body(http_transaction_h http_transaction,
                       char **body)
{
  ....
  *body = realloc(*body, new_len + 1);
  ....
  memcpy(*body + curr_len, ptr, body_size);
  body[new_len] = '\0';                        // <=
  curr_len = new_len;
  ....
}


Предупреждение PVS-Studio: V527 It is odd that the '\0' value is assigned to 'char' type pointer. Probably meant: *body[new_len] = '\0'. http_request.c 370

Вновь код исправлен в точности так, как я и рекомендовал: 

(*body)[new_len] = '\0';


Шестая исправленная ошибка

Проект org.tizen.w-wifi-1.0.229.

static void SHA1Final(unsigned char digest[20],
                      SHA1_CTX* context)
{
  u32 i;
  unsigned char finalcount[8];
  ....
  memset(context->count, 0, 8);
  memset(finalcount, 0, 8);
}


Предупреждение PVS-Studio: V597 The compiler could delete the 'memset' function call, which is used to flush 'finalcount' buffer. The memset_s () function should be used to erase the private data. wifi_generate_pin.c 185

Здесь неверно затираются данные в буфере finalcount. Вернее, не затираются: компилятор вправе удалить вызов функции memset. На самом деле, это, видимо, была не настоящая ошибка, так как просто удалили вызов функции memset для затирания finalcount. Предполагаю, что этот буфер и не надо обнулять, так как в нём не содержится ничего важного.

Тем не менее код поправили, а значит, авторы прислушались к информации из статьи.

Седьмая исправленная ошибка

Проект org.tizen.setting-1.0.1.

static void __draw_remove_list(SettingRingtoneData *ad)
{
  char *full_path = NULL;
  ....
  full_path = (char *)alloca(PATH_MAX);                  // <=
  ....
  if (!select_all_item) {
    SETTING_TRACE_ERROR("select_all_item is NULL");
    free(full_path);                                     // <=
    return;
  }
  ....  
}


Предупреждение PVS-Studio: V611 The memory was allocated using 'alloca' function but was released using the 'free' function. Consider inspecting operation logics behind the 'full_path' variable. setting-ringtone-remove.c 88

Функция __draw_remove_list переместилась в другой файл setting-ringtone-remove.c, и её код был исправлен. Был удалён вызов функции free (full_path), так как он просто не нужен и даже вреден.

Дальше смотреть ошибки я не стал, так как это неинтересно: 7 из 7 просмотренных ошибок были исправлены. И так понятно, что статью прочитали, приняли к сведению и исправили описанные в ней ошибки (или какую-то их часть).

Мне приятно, что моя статья оказалась полезной и сделала проект Tizen хотя бы чуть-чуть лучше. Почему чуть-чуть? Потому что я осилил проанализировать только 3.3% кода этой ОС.

У читателя, возможно, возникнет вопрос:, а почему я уверен, что эти ошибки были исправлены именно благодаря мне? Возможно, все эти ошибки заметили сами разработчики или их обнаружили с помощью какого-то другого инструмента. Какой другой инструмент? Да любой! Взять хотя бы статический анализатор Svace.

Svace — это статический анализатор кода, разрабатываемый в ИСП РАН при финансировании компании Samsung и специализированный на поиске ошибок в операционной системе Tizen. Например, недавно этот анализатор упоминался в статье «Статические анализаторы кода на примере ClickHouse» (@o6CuFl2Q).

Да и портал разработчиков ОС Tizen рекомендует использовать Svace для обнаружения ошибок в исходных текстах программ. Так что версия вполне правдоподобная.

Какие Ваши доказательства?


Как говорится, «какие ваши доказательства»?

Почему я решил, что ошибки исправлены именно благодаря PVS-Studio, а не другому анализатору?

О! Я легко могу продемонстрировать, что эта заслуга именно PVS-Studio!

Как я уже писал, меня хватило только на предупреждения, относящиеся к 3.3% кода. Потом у меня кончились силы. Однако анализатором в те дни было проверено не 3.3% кода, а больше, около 5%. У меня остались эти неразобранные отчеты.

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

Итак, я произвольным образом открываю отчёты, которые не смотрел во времена написания статьи и ищу убедительные ошибки, которые заслуживают исправления. Начну с проекта amd-0.6.5. Хотя в названии написано AMD, этот код относится к Tizen и написан разработчиками из Samsung. Об этом говорит комментарий в начале:

// Copyright © 2000 — 2017 Samsung Electronics Co., Ltd. All rights reserved.

Первая неисправленная ошибка (так как я про неё не писал)

Итак, в старом коде я вижу: 

static int __get_instance_info(bundle *kb,
                               struct instance_info *info)
{
  ....
  gchar *query;
  ....
  if (query == NULL || query + 1 == NULL) {
  ....
}


Предупреждение PVS-Studio: V694 The condition (query + 1 == NULL) is only true if there is pointer overflow which is undefined behavior anyway. amd_request.c 1083

Выражение «query + 1 == NULL» может быть истинным, только в случае переполнения значения указателя. А так делать нельзя, это undefined behavior. С этим кодом явно что-то не так.

Заглядываю в свежевыкаченный код. Функция __get_instance_info перекочевала в файл amd_rua.c. Однако суть не изменилась, и ошибка осталась на своём месте: 

if (query == NULL || query + 1 == NULL) {


Вторая неисправленная ошибка (так как я про неё не писал)

Проект attach-panel-camera-0.1.0.

#define startfunc   LOGD("+-  START -------------------------");
#define endfunc     LOGD("+-  END  --------------------------");

static Eina_Bool
_main_view_send_result_after_transform(void *data)
{
  startfunc
  main_view *view = (main_view *)data;
  if (view->transformtype == CAM_TRANSFORM_CROP) {
    DBG("crop completed, Start resize");
    _main_view_ug_send_result(view, view->filename);
    _main_view_start_camera_preview(view->camera);
    view->transformtype = CAM_TRANSFORM_NONE;
  }
  return ECORE_CALLBACK_CANCEL;     // <=
  endfunc                           // <=
}


Предупреждение PVS-Studio: V779 Unreachable code detected. It is possible that an error is present. main-view.c 261

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

  return ECORE_CALLBACK_CANCEL;     // <=
  endfunc                           // <=
}


Третья неисправленная ошибка (так как я про неё не писал)

Проект aul-0.6.7.

static gboolean run_func(void *data)
{
  callfunc(cmd);
  if (strcmp(cmd, "launch_res") == 0 || strcmp(cmd, "all") == 0
      || strcmp(cmd, "dbuslaunch") == 0
      || strcmp(cmd, "listen_app_status") == 0    // <=
      || strcmp(cmd, "open_svc_res") == 0 ||
         strcmp(cmd, "listen_app_status") == 0)   // <=
    return 0;
  else
    g_main_loop_quit(mainloop);
  return 0;
}


Предупреждение PVS-Studio: V501 There are identical sub-expressions 'strcmp (cmd, «listen_app_status») == 0' to the left and to the right of the '||' operator. aul_test.c 898

Строку два раза сравнивают с «listen_app_status». Одно сравнение лишнее или надо было сравнить с чем-то другим.

В новой версии исходных кодов ничего не изменилось, ошибка на своём месте.

Доказательства!


Вот и доказательства.

Продолжать не вижу смысла. Ошибки, которые я не описывал в статье, продолжают жить в коде Tizen.

К сожалению, сообщество разработчиков Tizen познакомилось с PVS-Studio как-то однобоко. Да, они, наверное, исправили несколько сотен ошибок, основываясь на информации из статьи. Но не пошли дальше. Никто не переписывался со мной и не обсуждал возможности использования анализатора. Вернее, переписки были, но какие-то неубедительные и без заинтересованности.

В итоге исправлены сотни ошибок. А могли бы быть исправлены тысячи!

Именно поэтому так важно популяризировать статический анализ кода и учить правильно его использовать. Неэффективно править 3% кода и не заинтересоваться судьбой ошибок в оставшихся 97%. Да и вообще разовые проверки неэффективны: статический анализатор должен использоваться регулярно.

В общем грустно как-то всё это.

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

Странно оставаться в стороне от такого подходящего мероприятия, как «Технологии анализа, моделирования и трансформации программ». Тем более, раз были получены отличные практические результаты. В конце концов, не так уж много людей в России вообще занимаются этим направлением.

Меня подвело неумение подавать научные заявки. Одно дело — писать на Хабрахабр, проявлять стойкость на Linux.org.ru, и совсем другое — что-то излагать в виде научных тезисов :). Результат — отказы в рецензиях:

Отзыв первого рецензента.
Тема статьи «Практическое использование методологии и инструментов статического анализа для поиска ошибок в исходном коде операционной системы Tizen» полностью соответствует тематике секции конференции. Однако, в тексте статьи отсутствует описание или обзор методологии статического анализа, а также подхода применения инструментов статического анализа для поиска ошибок в исходном коде.

В статье утверждается, что при выборе некоторого подмножества кодовой базы операционной системы Tizen и отобрав около 900 предупреждений (точное количество не указывается, метод отбора предупреждений о реальных ошибках не приведен) делается вывод о плотности ошибок в 0,37 на 1000 строк исходного кода, который никак далее в статье не обосновывается.

Далее в статье приводится описание пяти ошибок, найденых некоторым статическим анализатором. При чём из текста статьи можно сделать вывод, что этот анализатор используется для проверки кода операционной системы Tizen уже проверенного статическим анализатором Svace. Не вполне очевидно почему ошибки типа «Выражение всегда выичслияется в ложь» (CWE-570) называется «Опечатка», ошибка типа «Присваивание переменной без дальнейшего использования» (CWE-563) называется «Логическая ошибка», а для ошибки «Непроверенное возвращаемое значение на NULL приводит к разыменованию нулевого указателя» (CWE-690) делается никак в статье необоснованный вывод, что сообщение об ошибке выведено для случая, в котором проверки на NULL возвращаемого значения функции malloc не производится, хотя в других местах подобная проверка есть (не приведена статистика проверки возвращаемого значения функции malloc на NULL).

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

В связи с вышесказанным можно сделать вывод, что утверждения и выводы статьи плохо обоснованы. С научной точки зрения статья не представляет интереса. В статье не указан список источников, на которые ссылаются авторы. Текст статьи оформлен небрежно и не соответствует требованиям, предъявляемым к оформлению текста статей, принимаемых к рассмотрению на конференцию.


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

К сожалению, статья не содержит никаких деталей, позволяющих судить об инструменте анализа:

— нет реализации алгоритмов статического анализа (по найденным ошибкам можно сделать вывод, что это простые проверки по синтаксическому дереву);

— нет результатов тестирования (уровня истинных срабатываний, масштабируемости и т.п.).

Вывод статьи о желательности использования нескольких инструментов не следует из сказанного и верен для инструментов класса Coverity Prevent. Для AST-проверок все их можно реализовать в одном инструменте.

Вывод статьи о плотности ошибок в Tizen также неверен, поскольку простая экстраполяция по числу проверенных ошибок в данном случае не работает.

Как следствие, статья не несет никакой ценной информации, имеет ошибки и, более того, вообще не является научной статьей. Более правильным было бы сосредоточиться на описании алгоритмов анализа, трудностях в применении к большому проекту типа Tizen, выполненных по этому поводу работах и т.д.


Я признаю, что статья написана неудачно. Единственное, меня немного смущает «статья не содержит никаких деталей, позволяющих судить об инструменте анализа». Так как же мне было написать, что это был PVS-Studio? Сразу понятно, что автор — Андрей Карпов, а это нарушение условия анонимности при подаче заявок.

А эксперт-то, говорят ненастоящий!


Впрочем, какая тут анонимность, всюду торчат уши PVS-Studio и статья про 27000 ошибок:). Странно, что рецензент не смог догадаться. Это значит, что эксперт по статическому анализу оказался не совсем эксперт. Или он догадался, но…? :)

Ну да ладно. Тяп-ляп сделал, признаю. В следующий раз постараюсь лучше, если будет повод.

Спасибо всем за внимание. Не поступайте как разработчики Tizen, делая правки по итогам разовой проверки (тем более неполной). Статический анализатор надо сразу внедрять в процесс разработки, и PVS-Studio обеспечивает возможность плавной интеграции. Желаю всем безбажного кода.

P.S. Если кто-то из ИСП РАН читает эту заметку, и хочется разнообразить конференцию неформатным докладом на тему статического анализа кода, то я всегда готов.

Неформат

© Habrahabr.ru