PVS-Studio and Tizen

Эта статья продемонстрирует, что при разработке крупных проектов статический анализ кода является не просто полезным, а совершенно необходимым элементом процесса разработки. Я начинаю цикл статей, посвященных возможности использования статического анализатора кода PVS-Studio для повышения качества и надежности операционной системы Tizen. Для начала я проверил небольшую часть операционной системы (3.3%) и выписал около 900 предупреждений, указывающих на настоящие ошибки. Если экстраполировать результаты, то получается, что наша команда способна выявить и устранить в Tizen около 27000 ошибок. По итогам проведённого исследования я подготовил презентацию, которая предназначалась для демонстрации представителям Samsung и была посвящена возможному сотрудничеству. Встреча перенесена на неопределённый срок, поэтому я решил не тратить время и трансформировать материал презентации в статью. Запасайтесь вкусняшками и напитками, нас ждёт длинный программистский триллер.

Начать, пожалуй, следует со ссылки на презентацию «Как команда PVS-Studio может улучшить код операционной системы Tizen», из которой и родилась эта статья: RU — pptx, slideshare; EN — pptx, slideshare. Впрочем, смотреть презентацию вовсе не обязательно, так как весь содержащийся в ней материал будет рассмотрен здесь, причём более подробно. Тема презентации пересекается с открытым письмом, в котором мы также рассказываем, что готовы поработать с проектом Tizen.

С разными воспоминаниями и ссылками я закончил и перехожу к сути дела. Первое, что стоит сделать, это напомнить читателю, что вообще из себя представляет операционная система Tizen.

Tizen


Tizen — открытая операционная система на базе ядра Linux, предназначенная для широкого круга устройств, включая смартфоны, интернет-планшеты, компьютеры, автомобильные информационно-развлекательные системы, «умные» телевизоры и цифровые камеры, разрабатываемая и управляемая такими корпорациями, как Intel и Samsung. Поддерживает аппаратные платформы на процессорах архитектур ARM и x86. Более подробная информация доступна на сайте Wikipedia.

Платформа Tizen показывает уверенный рост в течение нескольких последних лет, несмотря на насыщенность рынка операционных систем для мобильных и носимых устройств. Согласно отчёту Samsung, рост продаж мобильных телефонов с OC Tizen составил 100% в 2017 году.

Tizen


Для нашей команды операционная система Tizen привлекательна тем, что компания Samsung заинтересована в её надёжности и прикладывает усилия для улучшения качества кода. Например, с целью улучшения качества кода, Samsung инвестировал разработку в ИСП РАН специализированного анализатора кода Svace. Инструмент Svace используется как основное средство для обеспечения безопасности системного и прикладного ПО платформы Tizen. Вот некоторые цитаты, взятые из заметки «Samsung have Invested $10 Million in Svace, Security Solution to Analyze Tizen Apps»:

As part of its security measures, Samsung are using the SVACE technology (Security Vulnerabilities and Critical Errors Detector) to detect potential vulnerabilities and errors that might exist in source code of applications created for the Tizen Operating System (OS). This technology was developed by ISP RAS (Institute for System Programming of the Russian Academy of Sciences), who are based in Moscow, Russia.

The solution is applied as part of the Tizen Static Analyzer tool that is included in the Tizen SDK and Studio. Using this tool you can perform Static security analysis of the Tizen apps native C / C ++ source code and discover any issues that they might have. The tool helps discover a wide range of issues at compilation time, such as the dereference of Null Pointers, Memory Leaks, Division by Zero, and Double Free etc.

Также стоит отметить новость «В России впервые создали защищенную операционную систему», опубликованную на сайте rbc.ru. Цитата:

О создании российской версии операционной системы (ОС) Tizen было объявлено 2 июня 2016. Tizen является независимой ОС, то есть она не привязана к облаку Apple или Google, и поэтому ее можно адаптировать под локальные рынки, рассказал президент ассоциации «Тайзен.ру» Андрей Тихонов. Основная особенность представленной ОС — в ее безопасности, уточнил Тихонов. Это первая и единственная в России сертифицированная в Федеральной службе по техническому и экспортному контролю (ФСТЭК) мобильная операционная система, уточнил представитель Samsung.

Команда PVS-Studio просто не могла пройти мимо такого интересного открытого проекта, не попробовав его проверить.

Анализ Tizen


Целью презентации, о которой я упоминал ранее, было продемонстрировать, что анализатор PVS-Studio находит очень много ошибок различного типа. Это своего рода резюме анализатора и нашей команды, которое мы хотим показать компании Samsung.

Читатель может засомневаться, стоит ли ему читать такую «статью-резюме». Да, стоит, здесь будет много интересного и полезного. Во-первых, лучше учиться на чужих ошибках, а не на своих. Во-вторых, статья отлично показывает, что методологию статического анализа просто необходимо применять в проектах большого размера. Если кто-то из коллег, занятых в большом проекте, уверяет вас, что они пишут код высокого качества и почти без ошибок, то покажите ему эту статью. Я не думаю, что создатели Tizen хотели, чтобы в проекте было много дефектов, но вот они — тысячи багов.

Как всегда, хочу напомнить, что статический анализ кода должен применяться регулярно. Разовая проверка Tizen или любого другого проекта, конечно, будет полезна, но малоэффективна. В основном будут найдены малозначительные ошибки, которые слабо оказывают влияние на работоспособность проекта. Все явные ошибки были выявлены другими методами, например, благодаря жалобам пользователей. Означает ли это, что толку от статического анализа мало? Конечно нет, просто, как я уже сказал, разовые проверки — это неэффективный способ использования анализаторов кода. Анализаторы должны использоваться регулярно и тогда многие, в том числе и критические, ошибки будут обнаруживаться на самом раннем этапе. Чем раньше ошибка выявлена, тем дешевле её исправить.

Я считаю, что:

  • Сейчас анализатор PVS-Studio выявляет более 10% ошибок, которые присутствуют в коде проекта Tizen.
  • В случае регулярного использования PVS-Studio в новом коде можно будет предотвратить около 20% ошибок.
  • Я прогнозирую, что команда PVS-Studio может на данный момент выявить и исправить около 27000 ошибок в проекте Tizen.

Конечно, я могу ошибаться, но я не подгонял результаты исследования, чтобы показать возможности анализатора PVS-Studio в лучшем свете. Это просто не нужно. Анализатор PVS-Studio — мощный инструмент, который находит так много дефектов, что просто нет смысла фальсифицировать результаты. Далее я продемонстрирую, как получились все приведённые здесь числа.

Конечно, я не мог проверить весь проект Tizen. Вместе со сторонними библиотеками он насчитывает 72 500 000 строк C и C++ кода (без учёта комментариев). Поэтому я решил случайным образом проверить несколько десятков проектов, входящих в состав Tizen:Unified.

Выбирая проекты, я делил их на две группы. Первая группа — это проекты, написанные непосредственно сотрудниками компании Samsung. Я определял такие проекты по наличию в начале файлов комментариев, выглядящих вот так:

/*
* Copyright (c) 2015 Samsung Electronics Co., Ltd All Rights Reserved
....
*/

Вторая группа — это сторонние проекты, используемые в проекте Tizen. Впрочем, многие проекты нельзя считать полностью сторонними, так как в них присутствуют различные патчи. Вот пример патча, сделанного в библиотеке efl-1.16.0:

//TIZEN_ONLY(20161121)
// Pre-rotation should be enabled only when direct
// rendering is set but client side rotation is not set
if ((sfc->direct_fb_opt) &&
    (!sfc->client_side_rotation) &&
    (evgl_engine->funcs->native_win_prerotation_set))
  {
    if (!evgl_engine->funcs->native_win_prerotation_set(eng_data))
      ERR("Prerotation does not work");
  }
//

Так что деление получилось несколько условным, однако, точность для общей оценки и не требуется.

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

m_ptr = (int *)realloc(m_ptr, newSize);
if (!m_ptr) return ERROR;

Если не удастся выделить новый фрагмент памяти, то произойдёт утечка памяти. Подробнее этот тип ошибки будет рассмотрен далее. Да, вероятность утечки памяти крайне мала, но, на мой взгляд, это именно ошибка, которую следует исправить.

У меня ушло около недели на то, чтобы отобрать предупреждения, которые, по моему мнению, указывают на настоящие ошибки. Заодно я выписал большое количество примеров кода, которые я буду использовать для подготовки презентаций и статей.

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

Один из разработчиков, посмотрев презентацию и не разобравшись что к чему, написал комментарий вида «27000 предупреждений анализатора, тоже мне достижение, это даже мало». Поэтому ещё раз подчеркну, что речь идет именно об ошибках. В процессе исследования я считал и выписывал именно ошибки, а не просто все предупреждения анализатора без разбора.

Анализ проектов, разработанных специалистами компании Samsung


Для проверки я случайным образом выбрал следующие проекты: bluetooth-frwk-0.2.157, capi-appfw-application-0.5.5, capi-base-utils-3.0.0, capi-content-media-content-0.3.10, capi-maps-service-0.6.12, capi-media-audio-io-0.3.70, capi-media-codec-0.5.3, capi-media-image-util-0.1.15, capi-media-player-0.3.58, capi-media-screen-mirroring-0.1.78, capi-media-streamrecorder-0.0.10, capi-media-vision-0.3.24, capi-network-bluetooth-0.3.4, capi-network-http-0.0.23, cynara-0.14.10, e-mod-tizen-devicemgr-0.1.69, ise-engine-default-1.0.7, ise-engine-sunpinyin-1.0.10, ise-engine-tables-1.0.10, isf-3.0.186, org.tizen.app-selector-0.1.61, org.tizen.apps-0.3.1, org.tizen.bluetooth-0.1.2, org.tizen.browser-3.2.0, org.tizen.browser-profile_common-1.6.4, org.tizen.classic-watch-0.0.1, org.tizen.d2d-conv-setting-profile_mobile-1.0, org.tizen.d2d-conv-setting-profile_wearable-1.0, org.tizen.download-manager-0.3.21, org.tizen.download-manager-0.3.22, org.tizen.dpm-toolkit-0.1, org.tizen.elm-demo-tizen-common-0.1, org.tizen.indicator-0.2.53, org.tizen.inputdelegator-0.1.170518, org.tizen.menu-screen-1.2.5, org.tizen.myplace-1.0.1, org.tizen.privacy-setting-profile_mobile-1.0.0, org.tizen.privacy-setting-profile_wearable-1.0.0, org.tizen.quickpanel-0.8.0, org.tizen.screen-reader-0.0.8, org.tizen.service-plugin-sample-0.1.6, org.tizen.setting-1.0.1, org.tizen.settings-0.2, org.tizen.settings-adid-0.0.1, org.tizen.telephony-syspopup-0.1.6, org.tizen.voice-control-panel-0.1.1, org.tizen.voice-setting-0.0.1, org.tizen.volume-0.1.149, org.tizen.w-home-0.1.0, org.tizen.w-wifi-1.0.229, org.tizen.watch-setting-0.0.1, security-manager-1.2.17.

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

Примечание. Помимо предупреждения PVS-Studio я буду стараться классифицировать найденные ошибки согласно CWE (Common Weakness Enumeration). Однако, я вовсе не делаю попытку найти какие-то уязвимости, а привожу CWE-ID исключительно для удобства тех читателей, которые привыкли именно к этой классификации дефектов. Моя основная цель — найти как можно больше ошибок, а определение того, насколько опасна ошибка с точки зрения безопасности, выходит за рамки моего исследования.

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

Чай, кофе


Опечатка в условии: слева и справа одно и то же (2 ошибки)


Классика. Даже так: классика самого высшего сорта!

Во-первых, эта ошибка выявляется с помощью диагностики V501. Диагностика выявляет опечатки и последствия неудачного Copy-Paste. Это невероятно популярный и распространенный тип ошибок. Посмотрите сами, какую замечательную коллекцию багов в открытых проектах мы собрали благодаря диагностике V501.

Во-вторых, эта ошибка находится в операторе меньше (<). Неправильное сравнение двух объектов — это тоже классическая ошибка, возникающая из-за того, что никто не проверяет такие простые функции. Недавно я написал интересную статью на эту тему "Зло живёт в функциях сравнения". Почитайте, это своего рода «Хребты безумия» для программистов.

Код, о котором идёт речь:

bool operator <(const TSegment& other) const {
  if (m_start < other.m_start)
    return true;
  if (m_start == other.m_start)
    return m_len < m_len;                // <=
  return false;
}

Ошибка найдена благодаря предупреждению PVS-Studio: V501 There are identical sub-expressions to the left and to the right of the '<' operator: m_len < m_len segmentor.h 65

Software weaknesses type — CWE-570: Expression is Always False

Из-за этой ошибки объекты, которые отличаются только значением члена m_len, будут сравниваться некорректно. Правильный вариант сравнения:

return m_len < other.m_len;

Аналогичная ошибка: V501 There are identical sub-expressions '0 == safeStrCmp(btn_str, setting_gettext(«IDS_ST_BUTTON_OK»))' to the left and to the right of the '||' operator. setting-common-general-func.c 919

Опечатка в условии: бессмысленное сравнение (2 ошибки)


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 ;
  }
  ....
}

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

Software weaknesses type — CWE-697: Insufficient Comparison

Сравнение вида «pointer < 0» не имеет смысла и свидетельствует об опечатке в коде. Как я понимаю, пропущен оператор звездочка (*), который должен был разыменовать указатель. Корректный код:

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

Этот код вместе с ошибкой скопировали, в результате чего точно такую же ошибку можно наблюдать в функции __page_count_changed_cb:

static void __page_count_changed_cb(void *data)
{
  int i = 0;
  int *page_cnt = (int *)data;
  if (page_cnt == NULL || page_cnt < 0) {
    _E("page count is wrong");
    return ;
  }
  ....
}

Ох уж эта Copy-Paste технология. Для этого кода анализатор выдал предупреждение: V503 This is a nonsensical comparison: pointer < 0. apps_view_circle_indicator.c 219

Опасный способ использования функции alloca (1 ошибка)


Рассмотрим код, который хотя и плох, но на практике не приведёт к проблемам. Я не рассматривал эту ситуацию в презентации, так как она требует специального пояснения. Теперь же, в статье, я могу остановиться на данном примере и поделиться своими мыслями.

int audio_io_loopback_in_test()
{
  ....
  while (1) {
    char *buffer = alloca(size);
    if ((ret = audio_in_read(input, (void *)buffer, size)) >
        AUDIO_IO_ERROR_NONE)
    {
      fwrite(buffer, size, sizeof(char), fp);
      printf("PASS, size=%d, ret=0x%x\n", size, ret);
    } else {
      printf("FAIL, size=%d, ret=0x%x\n", size, ret);
    }
  }
  ....
}

Предупреждение PVS-Studio: V505 The 'alloca' function is used inside the loop. This can quickly overflow stack. audio_io_test.c 247

Software weaknesses type — CWE-770: Allocation of Resources Without Limits or Throttling

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

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

Таким образом, не совсем честно говорить, что это именно ошибка, ведь тесты работают.

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

Более того, код можно легко исправить, а значит — это надо сделать. Достаточно вынести выделение памяти из цикла. Это можно сделать, так как размер выделяемого буфера не изменяется.

Хороший код:

char *buffer = alloca(size);
while (1) {
  if ((ret = audio_in_read(input, (void *)buffer, size)) >
      AUDIO_IO_ERROR_NONE)
  {
    fwrite(buffer, size, sizeof(char), fp);
    printf("PASS, size=%d, ret=0x%x\n", size, ret);
  } else {
    printf("FAIL, size=%d, ret=0x%x\n", size, ret);
  }
}

Используется уже несуществующий буфер (1 ошибка)


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

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

Software weaknesses type — CWE-562: Return of Stack Variable Address

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

Обрабатывается меньше или больше элементов массива, чем требуется (7 ошибок)


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

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

Software weaknesses type — CWE-131: Incorrect Calculation of Buffer Size

Здесь программист забыл, что в качестве третьего аргумента функция memset принимает размер буфера в байтах, а не количество элементов массива. Не зря я в своё время назвал memset самой опасной функцией в мире программирования на C/C++. Эта функция продолжает сеять хаос в различных проектах.

Тип gboolean занимает не 1 байт, а 4. В результате будет обнулена только 1/4 часть массива, а остальные элементы останутся неинициализированными.

Правильный вариант кода:

memset(req_id_used, 0x00, BT_REQUEST_ID_RANGE_MAX * sizeof(gboolean));

Или можно написать так:

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

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

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

Software weaknesses type — CWE-131: Incorrect Calculation of Buffer Size

Надежная операционная система… Эх…

Обратите внимание, что второй вызов snprintf должен добавить что-то к уже имеющейся строке. Поэтому адресом буфера является выражение buf + strlen(buf). При этом функция имеет право печатать символов меньше, чем размер буфера. Из размера буфера надо вычесть strlen(buf). Но про это забыли и может возникнуть ситуация, когда функция snprintf запишет данные за границу массива.

Корректный код:

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

Третий фрагмент кода демонстрирует ситуацию, когда выход за границу происходит всегда. Для начала рассмотрим некоторые структуры.

#define BT_ADDRESS_STRING_SIZE 18

typedef struct {
 unsigned char addr[6];
} bluetooth_device_address_t;

typedef struct {
 int count;
 bluetooth_device_address_t addresses[20];
} bt_dpm_device_list_t;

Здесь нам важно, что массив addr состоит из 6 элементов. Запомните этот размер, а также, что макрос BT_ADDRESS_STRING_SIZE раскрывается в константу 18.

Теперь собственно некорректный код:

dpm_result_t _bt_dpm_get_bluetooth_devices_from_whitelist(
  GArray **out_param1)
{
  dpm_result_t ret = DPM_RESULT_FAIL;
  bt_dpm_device_list_t device_list;
  ....
  for (; list; list = list->next, i++) {
    memset(device_list.addresses[i].addr, 0, BT_ADDRESS_STRING_SIZE);
    _bt_convert_addr_string_to_type(device_list.addresses[i].addr,
                                    list->data);
  }
  ....
}

Предупреждение PVS-Studio: V512 A call of the 'memset' function will lead to overflow of the buffer 'device_list.addresses[i].addr'. bt-service-dpm.c 226

Software weaknesses type — CWE-805: Buffer Access with Incorrect Length Value

Выделю самое главное:

memset(device_list.addresses[i].addr, 0, BT_ADDRESS_STRING_SIZE);

Итак, как мы видели раньше, размер addr всего 6 байт. При этом функция memset обнуляет 18 байт и, как следствие, происходит выход за границу массива.

Ещё 4 найденные ошибки:

  • V512 A call of the 'memset' function will lead to overflow of the buffer 'device_list.addresses[i].addr'. bt-service-dpm.c 176
  • V512 A call of the 'memset' function will lead to underflow of the buffer 'formatted_number'. i18ninfo.c 544
  • V512 A call of the 'snprintf' function will lead to overflow of the buffer 'ret + strlen(ret)'. navigator.c 677
  • V512 A call of the 'snprintf' function will lead to overflow of the buffer 'trait + strlen(trait)'. navigator.c 514

Логическая ошибка в последовательностях if… else… if (4 ошибки)


char *voice_setting_language_conv_lang_to_id(const char* lang)
{
  ....
  } else if (!strcmp(LANG_PT_PT, lang)) {
    return "pt_PT";
  } else if (!strcmp(LANG_ES_MX, lang)) {     // <=
    return "es_MX";
  } else if (!strcmp(LANG_ES_US, lang)) {     // <=
    return "es_US";
  } else if (!strcmp(LANG_EL_GR, lang)) {
    return "el_GR";
  ....
}

Предупреждение PVS-Studio: V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 144, 146. voice_setting_language.c 144

Software weaknesses type — CWE-570 Expression is Always False

Рассматривая этот код, нельзя понять, в чём же заключается ошибка. Всё дело в том, что LANG_ES_MX и LANG_ES_US — это идентичные строки. Вот они:

#define LANG_ES_MX "\x45\x73\x70\x61\xC3\xB1\x6f\x6c\x20\x28\"  "x45\x73\x74\x61\x64\x6f\x73\x20\x55\x6e\x69\x64\x6f\x73\x29"

#define LANG_ES_US "\x45\x73\x70\x61\xC3\xB1\x6f\x6c\x20\x28\"  "x45\x73\x74\x61\x64\x6f\x73\x20\x55\x6e\x69\x64\x6f\x73\x29"

Как я понимаю, эти строки на самом деле должны отличаться. Но поскольку строки одинаковые, то второе условие всегда ложно и функция никогда не вернёт значение «es_US».

Примечание. ES_MX — это Spanish (Mexico), ES_US — это Spanish (United States).

Эту ошибку я нашел в проекте org.tizen.voice-setting-0.0.1. Что интересно, вновь подводит технология Copy-Paste, и точно такую же ошибку я встретил в проекте org.tizen.voice-control-panel-0.1.1.

Другие ошибки:

  • V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 144, 146. voice_setting_language.c 144
  • V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 792, 800. setting-common-general-func.c 792
  • V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 801, 805. setting-common-general-func.c 801

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


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

void
isf_wsc_context_del (WSCContextISF *wsc_ctx)
{
  ....
  WSCContextISF* old_focused = _focused_ic;
  _focused_ic = context_scim;
  _focused_ic = old_focused;
  ....
}

Предупреждение PVS-Studio: V519 The '_focused_ic' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 1260, 1261. wayland_panel_agent_module.cpp 1261

Software weaknesses type — CWE-563 Assignment to Variable without Use ('Unused Variable')

Переменной _focused_ic два раза подряд присваиваются разные значения. Правильный код должен быть таким:

WSCContextISF* old_focused = _focused_ic;
_focused_ic = context_scim;
context_scim = old_focused;

Впрочем, в таких случаях лучше использовать функцию std::swap. Так намного меньше шансов ошибиться:

std::swap(_focused_ic, context_scim);

Рассмотрим другой вариант ошибки, которая возникла при написании однотипного кода. Возможно, в её появлении виноват метод Copy-Paste.

void create_privacy_package_list_view(app_data_s* ad)
{
  ....
  Elm_Genlist_Item_Class *ttc = elm_genlist_item_class_new();
  Elm_Genlist_Item_Class *ptc = elm_genlist_item_class_new();
  Elm_Genlist_Item_Class *mtc = elm_genlist_item_class_new();
  ....
  ttc->item_style = "title";
  ttc->func.text_get = gl_title_text_get_cb;
  ttc->func.del = gl_del_cb;                   // <=

  ptc->item_style = "padding";
  ptc->func.del = gl_del_cb;

  mtc->item_style = "multiline";
  mtc->func.text_get = gl_multi_text_get_cb;
  ttc->func.del = gl_del_cb;                   // <=
  ....
}

Предупреждение PVS-Studio: V519 The 'ttc->func.del' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 409, 416. privacy_package_list_view.c 416

Software weaknesses type — CWE-563 Assignment to Variable without Use ('Unused Variable')

Во втором случае значение должно было присваиваться переменной mtc->func.del.

Другие ошибки:

  • V519 The 'item' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 1176, 1189. w-input-stt-voice.cpp 1189
  • V519 The 'ad->paired_item' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 1614, 1617. bt-main-view.c 1617
  • V519 The 'ret' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 191, 194. main.c 194
  • V519 The 'ret' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 372, 375. setting-about-status.c 375
  • V519 The 'ret' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 91, 100. setting-applications-defaultapp.c 100
  • V519 The 'ret' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 52, 58. smartmanager-battery.c 58
  • V519 The 'ret' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 853, 872. setting-time-main.c 872
  • V519 The 'ret_str' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 2299, 2300. setting-password-sim.c 2300
  • V519 The 'display_status' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 110, 117. view_clock.c 117

При просмотре отчета анализатора я выделил только 11 мест, которые, на мой взгляд, следует исправить. Но на самом деле сообщений V519 было намного больше. Очень часто они относились к коду, когда в переменную много раз подряд сохранялся результат после вызова функций. Речь идёт о коде вида:

status = Foo(0); 
status = Foo(1);
status = Foo(2);

Такой код возникает, как правило, в двух случаях:

  1. Результат вызова функций не важен, но какой-то компилятор или анализатор выдал предупреждение, от которого избавились, записав результат в переменную. По-моему мнению, тогда было бы лучше написать (void)Foo(x);.
  2. Результат вызова функций не важен, но иногда может пригодиться для отладки. Намного проще отлаживать код, когда результат записывается в какую-то переменную.

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

Разыменование (потенциально) нулевого указателя (всего ошибок 88)


Использование нулевых указателей выявляется с помощью диагностик V522 и V575. Предупреждение V522 выдаётся, кода происходит разыменование указателя, который может быть нулевым (*MyNullPtr = 2;). V575 — когда потенциально нулевой указатель передаётся в функцию, внутри которой он может быть разыменован (s = strlen(MyNullPtr);). На самом деле V575 выдаётся и для некоторых других случаев, когда используются некорректные аргументы, но на данный момент это нам не интересно. Сейчас, с точки зрения статьи, разницы между предупреждениями V522 и V575 нет, поэтому они будут рассмотрены в этой главе совместно.

Отдельно стоит поговорить о таких функциях, как malloc, realloc, strdup. Следует проверять, не вернули ли они NULL, если невозможно выделить блок памяти запрошенного размера. Впрочем, некоторые программисты придерживаются плохой практики и никогда сознательно не делают проверок. Их логика такова, раз нет памяти, то и не стоит дальше мучиться, пусть программа упадёт. Я считаю такой подход плохим, но он есть, и я не раз слышал доводы в его защиту.

К счастью, разработчики Tizen к упомянутой категории не относятся и обычно проверяют, удалось ли выделить память или нет. Иногда они это делают даже там, где не надо:

static FilterModule *__filter_modules = 0;
static void
__initialize_modules (const ConfigPointer &config)
{
  ....
  __filter_modules = new FilterModule [__number_of_modules];
  if (!__filter_modules) return;
  ....
}

В такой проверке нет смысла, так как в случае неудачи выделения памяти оператор new сгенерирует исключение типа std::bad_alloc. Впрочем, это совсем другая история. Я привел этот код исключительно для того, чтобы показать, что в проекте Tizen принято проверять, удалось ли выделить память.

Так вот, анализатор PVS-Studio обнаруживает, что во многих местах проверок не хватает. Здесь я рассмотрю только один случай, так как в общем-то они все однотипны.

void QuickAccess::setButtonColor(Evas_Object* button,
                                 int r, int g, int b, int a)
{
  Edje_Message_Int_Set* msg =
  (Edje_Message_Int_Set *)malloc(sizeof(*msg) + 3 * sizeof(int));
  msg->count = 4;
  msg->val[0] = r;
  msg->val[1] = g;
  msg->val[2] = b;
  msg->val[3] = a;
  edje_object_message_send(elm_layout_edje_get(button),
                           EDJE_MESSAGE_INT_SET, 0, msg);
  free(msg);
}

Предупреждение PVS-Studio: V522 There might be dereferencing of a potential null pointer 'msg'. QuickAccess.cpp 743

Software weaknesses type — CWE-690: Unchecked Return Value to NULL Pointer Dereference

Нет гарантии, что функция malloc выделит память. Да, вероятность такого события крайне мала, но если проверки на равенство указателей NULL есть в других местах, то проверка должна быть и здесь. Поэтому я считаю, что этот код содержит настоящую ошибку, которую необходимо исправить.

Однако нулевые указатели могут возвращать не только функции, выделяющие память. Есть и другие ситуации, когда следует проверять указатель перед использованием. Рассмотрим пару таких примеров. Первый из них связан с небезопасным использованием оператора dynamic_cast.

int cpp_audio_in_peek(audio_in_h input, const void **buffer,
                      unsigned int *length) {
  ....
  CAudioInput* inputHandle = 
    dynamic_cast<CAudioInput*>(handle->audioIoHandle);
  assert(inputHandle);
  inputHandle->peek(buffer, &_length);
  ....
}

Предупреждение PVS-Studio: V522 There might be dereferencing of a potential null pointer 'inputHandle'. cpp_audio_io.cpp 928

Software weaknesses type — CWE-690: Unchecked Return Value to NULL Pointer Dereference

Странный код. Если есть уверенность, что в handle->audioIoHandle хранится указатель на объект типа CAudioInput, то надо использовать static_cast. Если уверенности нет, то нужна проверка, так как макрос assert не поможет в release-версии.

Думаю, правильнее будет добавить вот такую проверку:

CAudioInput* inputHandle = 
  dynamic_cast<CAudioInput*>(handle->audioIoHandle);
if (inputHandle == nullptr) {
  assert(false);
  THROW_ERROR_MSG_FORMAT(
     CAudioError::EError::ERROR_INVALID_HANDLE, "Handle is NULL");
}

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

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

int main(int argc, char *argv[])
{
  ....
  char *temp1 = strstr(dp->d_name, "-");
  char *temp2 = strstr(dp->d_name, ".");

  strncpy(temp_filename, dp->d_name,
          strlen(dp->d_name) - strlen(temp1));
  strncpy(file_format, temp2, strlen(temp2));
  ....
}

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

  • V575 The potential null pointer is passed into 'strlen' function. Inspect the first argument. image_util_decode_encode_testsuite.c 207
  • V575 The potential null pointer is passed into 'strlen' function. Inspect the first argument. image_util_decode_encode_testsuite.c 208

Software weaknesses type — CWE-690: Unchecked Return Value to NULL Pointer Dereference

Указатели temp1 и temp2 могут оказаться нулевыми, если в строке не обнаружатся символы '-' и '.'. В этом случае чуть позже произойдёт разыменование нулевого указателя.

Есть ещё 84 участка кода, где разыменовываются указатели, которые могут оказаться равны NULL. Рассматривать их в статье нет никакого смысла. Нет смысла даже приводить их просто списком, так как это все равно займёт много места. Поэтому я выписал эти предупреждения в отдельный файл: Tizen_V522_V575.txt.

Одинаковые действия (6 ошибок)


static void _content_resize(...., const char *signal)
{
  ....
  if (strcmp(signal, "portrait") == 0) {
    evas_object_size_hint_min_set(s_info.layout,
      ELM_SCALE_SIZE(width), ELM_SCALE_SIZE(height));
  } else {
    evas_object_size_hint_min_set(s_info.layout,
      ELM_SCALE_SIZE(width), ELM_SCALE_SIZE(height));
  }
  ....
}

Предупреждение PVS-Studio: V523 The 'then' statement is equivalent to the 'else' statement. page_setting_all.c 125

Software weaknesses type — не знаю как классифицировать, буду благодарен за подсказку.

Вне зависимости от условия, выполняются одни и те же действия. Как я понимаю, в одном из двух вызовов функции evas_object_size_hint_min_set следует поменять местами width и height.

Рассмотрим ещё одну ошибку этого типа:

static Eina_Bool _move_cb(void *data, int type, void *event)
{
  Ecore_Event_Mouse_Move *move = event;

  mouse_info.move_x = move->root.x;
  mouse_info.move_y = move->root.y;

  if (mouse_info.pressed == false) {
    return ECORE_CALLBACK_RENEW;                 // <=
  }

  return ECORE_CALLBACK_RENEW;                   // <=
}

Предупреждение PVS-Studio: V523 The 'then' statement is equivalent to the subsequent code fragment. mouse.c 143

Software weaknesses type — CWE-393 Return of Wrong Status Code

Очень странно, что функция выполняет какую-то проверку, но всё равно всегда возвращает значение ECORE_CALLBACK_RENEW. Думаю, возвращаемые значения должны различаться.

Другие ошибки этого типа:

  • V523 The 'then' statement is equivalent to the 'else' statement. bt-httpproxy.c 383
  • V523 The 'then' statement is equivalent to the 'else' statement. streamrecorder_test.c 342
  • V523 The 'then' statement is equivalent to the 'else' statement. ise-stt-option.cpp 433
  • V523 The 'then' statement is equivalent to the subsequent code fragment. dpm-toolkit-cli.c 87

Забыли разыменовать указатель (1 ошибка)


Очень красивая ошибка: пишем данные неизвестно куда.

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

Software weaknesses type — CWE-787: Out-of-bounds Write

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

Ошибка кроется в строке:

body[new_len] = '\0';

Получается, что указатель на указатель интерпретируется как массив указателей. Никакого массива, конечно, нет. Поэтому NULL ('\0' в данном случае воспринимается как нулевой указатель) будет записан совершенно непонятно куда. Происходит порча какого-то неизвестного блока памяти.

Ох..

Помимо этого, возникает ещё одна ошибка. Строка не будет завершена терминальным нулём. В общем, всё плохо.

Правильный код:

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

Условие всегда true/false (9 ошибок)


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

Первый вариант.

unsigned m_candiPageFirst;

bool
CIMIClassicView::onKeyEvent(const CKeyEvent& key)
{
  ....
  if (m_candiPageFirst > 0) {
    m_candiPageFirst -= m_candiWindowSize;
    if (m_candiPageFirst < 0) m_candiPageFirst = 0;
    changeMasks |= CANDIDATE_MASK;
  }
  ....
}

Предупреждение PVS-Studio: V547 Expression 'm_candiPageFirst < 0' is always false. Unsigned type value is never < 0. imi_view_classic.cpp 201

Software weaknesses type — CWE-570: Expression is Always False

Переменная m_candiPageFirst имеет тип unsigned. Следовательно, значение этой переменной не может быть меньше нуля. Чтобы защититься от переполнения, следует переписать код так:

if (m_candiPageFirst > 0) {
  if (m_candiPageFirst > m_candiWindowSize)
    m_candiPageFirst -= m_candiWindowSize;
  else 
    m_candiPageFirst = 0;
  changeMasks |= CANDIDATE_MASK;
}

Второй вариант.

void
QuickAccess::_grid_mostVisited_del(void *data, Evas_Object *)
{
  BROWSER_LOGD("[%s:%d]", __PRETTY_FUNCTION__, __LINE__);
  if (data) {
    auto itemData = static_cast<HistoryItemData*>(data);
    if (itemData)
      delete itemData;
  }
}

Предупреждение PVS-Studio: V547 Expression 'itemData' is always true. QuickAccess.cpp 571

Software weaknesses type — CWE-571: Expression is Always True

Это очень подозрительный код. Если указатель data != nullptr, то и указатель itemData != nullptr. Следовательно, вторая проверка не имеет смысла. Здесь мы столкнулись с одной из двух ситуаций:

  1. Это ошибка. На самом деле вместо оператора static_cast следовало использовать оператор dynamic_cast, который может вернуть nullptr.
  2. Настоящей ошибки нет, это неаккуратный код. Вторая проверка просто лишняя и её следует удалить, чтобы она не сбивала с толку анализаторы кода и других программистов.

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

Третий вариант.

typedef enum {
  BT_HID_MOUSE_BUTTON_NONE = 0x00,
  BT_HID_MOUSE_BUTTON_LEFT = 0x01,
  BT_HID_MOUSE_BUTTON_RIGHT = 0x02,
  BT_HID_MOUSE_BUTTON_MIDDLE = 0x04
} bt_hid_mouse_button_e;

int bt_hid_device_send_mouse_event(const char *remote_address,
    const bt_hid_mouse_data_s *mouse_data)
{
  ....
  if (mouse_data->buttons != BT_HID_MOUSE_BUTTON_LEFT ||
      mouse_data->buttons != BT_HID_MOUSE_BUTTON_RIGHT ||
      mouse_data->buttons != BT_HID_MOUSE_BUTTON_MIDDLE) {
    return BT_ERROR_INVALID_PARAMETER;
  }
  ....
}

Предупреждение PVS-Studio: V547 Expression is always true. Probably the '&&' operator should be used here. bluetooth-hid.c 229

Software weaknesses type — CWE-571: Expression is Always True

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

if (buttons != 1 ||
    buttons != 2 ||
    buttons != 4) {

Какое бы значение не хранилось в переменной, оно всегда будет не равно 1 или 2 или 4.

Другие ошибки:

  • V547 Expression 'ad->transfer_info' is always true. bt-share-ui-popup.c 56
  • V547 Expression 'item_name' is always true. SettingsUI.cpp 222
  • V547 Expression 'item_name' is always true. SettingsUI.cpp 226
  • V547 Expression '!urlPair' is always false. GenlistManager.cpp 143
  • V547 Expression 'strlen(s_formatted) < 128' is always true. clock.c 503
  • V547 Expression 'doc != ((void *) 0)' is always true. setting-common-data-slp-setting.c 1450

Путаница с enum (18 ошибок)


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

Есть два enum, в которых объявлены константы с похожими именами:

typedef enum {
 WIFI_MANAGER_RSSI_LEVEL_0 = 0,
 WIFI_MANAGER_RSSI_LEVEL_1 = 1,
 WIFI_MANAGER_RSSI_LEVEL_2 = 2,
 WIFI_MANAGER_RSSI_LEVEL_3 = 3,
 WIFI_MANAGER_RSSI_LEVEL_4 = 4,
} wifi_manager_rssi_level_e;

typedef enum {
 WIFI_RSSI_LEVEL_0 = 0,
 WIFI_RSSI_LEVEL_1 = 1,
 WIFI_RSSI_LEVEL_2 = 2,
 WIFI_RSSI_LEVEL_3 = 3,
 WIFI_RSSI_LEVEL_4 = 4,
} wifi_rssi_level_e;

Неудивительно, что в именах можно запутаться и написать вот такой код:

static int
_rssi_level_to_strength(wifi_manager_rssi_level_e level)
{
  switch (level) {
    case WIFI_RSSI_LEVEL_0:
    case WIFI_RSSI_LEVEL_1:
      return LEVEL_WIFI_01;
    case WIFI_RSSI_LEVEL_2:
      return LEVEL_WIFI_02;
    case WIFI_RSSI_LEVEL_3:
      return LEVEL_WIFI_03;
    case WIFI_RSSI_LEVEL_4:
      return LEVEL_WIFI_04;
    default:
      return WIFI_RSSI_LEVEL_0;
  }
}

Переменная level имеет тип wifi_manager_rssi_level_e. Константы же имеют тип wifi_rssi_level_e. Получается, что есть сразу 5 неправильных сравнений, и поэтому анализатор выдаёт 5 предупреждений:

  • V556 The values of different enum types are compared: switch(ENUM_TYPE_A) { case ENUM_TYPE_B:… }. wifi.c 163
  • V556 The values of different enum types are compared: switch(ENUM_TYPE_A) { case ENUM_TYPE_B:… }. wifi.c 164
  • V556 The values of different enum types are compared: switch(ENUM_TYPE_A) { case ENUM_TYPE_B:… }. wifi.c 166
  • V556 The values of different enum types are compared: switch(ENUM_TYPE_A) { case ENUM_TYPE_B:… }. wifi.c 168
  • V556 The values of different enum types are compared: switch(ENUM_TYPE_A) { case ENUM_TYPE_B:… }. wifi.c 170

Software weaknesses type — CWE-697: Insufficient Comparison

Что забавно — этот код работает именно так, как задумывал программист. Благодаря везению, константа WIFI_MANAGER_RSSI_LEVEL_0 равна WIFI_RSSI_LEVEL_0 и так далее.

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

  1. Этот код удивляет анализатор, а значит он будет удивлять и программистов, которые будут его сопровождать.
  2. Если хотя бы один из enum со временем изменится и значения констант перестанут совпадать, то программа начнет вести себя некорректно.

Другие неправильные сравнения:

  • V556 The values of different enum types are compared: switch(ENUM_TYPE_A) { case ENUM_TYPE_B:… }. e_devicemgr_video.c 885
  • V556 The values of different enum types are compared: switch(ENUM_TYPE_A) { case ENUM_TYPE_B:… }. e_devicemgr_video.c 889
  • V556 The values of different enum types are compared: switch(ENUM_TYPE_A) { case ENUM_TYPE_B:… }. e_devicemgr_video.c 892
  • V556 The values of different enum types are compared: switch(ENUM_TYPE_A) { case ENUM_TYPE_B:… }. e_devicemgr_video.c 895
  • V556 The values of different enum types are compared: switch(ENUM_TYPE_A) { case ENUM_TYPE_B:… }. e_devicemgr_video.c 898
  • V556 The values of different enum types are compared: switch(ENUM_TYPE_A) { case ENUM_TYPE_B:… }. e_devicemgr_video.c 901
  • V556 The values of different enum types are compared: switch(ENUM_TYPE_A) { case ENUM_TYPE_B:… }. e_devicemgr_video.c 904
  • V556 The values of different enum types are compared: switch(ENUM_TYPE_A) { case ENUM_TYPE_B:… }. e_devicemgr_video.c 907
  • V556 The values of different enum types are compared: switch(ENUM_TYPE_A) { case ENUM_TYPE_B:… }. myplace-placelist.c 239
  • V556 The values of different enum types are compared: switch(ENUM_TYPE_A) { case ENUM_TYPE_B:… }. myplace-placelist.c 253
  • V556 The values of different enum types are compared: switch(ENUM_TYPE_A) { case ENUM_TYPE_B:… }. myplace-placelist.c 264
  • V556 The values of different enum types are compared: switch(ENUM_TYPE_A) { case ENUM_TYPE_B:… }. telephony_syspopup_noti.c 82
  • V556 The values of different enum types are compared: switch(ENUM_TYPE_A) { case ENUM_TYPE_B:… }. telephony_syspopup_noti.c 87

Часть условия всегда true/false (2 ошибки)


Я заметил всего 2 таких ошибки, но они обе интересные, поэтому давайте рассмотрим их.

int bytestream2nalunit(FILE * fd, unsigned char *nal)
{
  unsigned char val, zero_count, i;
  ....
  val = buffer[0];
  while (!val) {                                            // <=
    if ((zero_count == 2 || zero_count == 3) && val == 1)   // <=
      break;
    zero_count++;
    result = fread(buffer, 1, read_size, fd);

    if (result != read_size)
      break;
    val = buffer[0];
  }
  ....
}

Предупреждение PVS-Studio: V560 A part of conditional expression is always false: val == 1. player_es_push_test.c 284

Software weaknesses type — CWE-570: Expression is Always False

Цикл выполняется до тех пор, пока переменная val равна нулю. В начале тела цикла переменная val сравнивается со значением 1. Естественно, переменная val не может быть равна 1, иначе бы цикл уже остановился. Здесь явно какая-то логическая ошибка.

Теперь рассмотрим другую ошибку.

const int GT_SEARCH_NO_LONGER = 0,
          GT_SEARCH_INCLUDE_LONGER = 1,
          GT_SEARCH_ONLY_LONGER = 2;

bool GenericTableContent::search (const String &key,
                                  int search_type) const
{
  ....
  else if (nkeys.size () > 1 && GT_SEARCH_ONLY_LONGER) {
  ....
}

Предупреждение PVS-Studio: V560 A part of conditional expression is always true: GT_SEARCH_ONLY_LONGER. scim_generic_table.cpp 1884

Software weaknesses type — CWE-571: Expression is Always True

Константа GT_SEARCH_ONLY_LONGER является частью условия. Это очень странно и у меня есть подозрение, что на самом деле условие должно выглядеть так:

if (nkeys.size () > 1 && search_type == GT_SEARCH_ONLY_LONGER)

Путаница с типами создаваемых и уничтожаемых объектов (4 ошибки)


Объявлены три структуры, никак не связанные между собой:

struct sockaddr_un
{
  sa_family_t sun_family;
  char sun_path[108];
};

struct sockaddr_in
{
  sa_family_t sin_family;
  in_port_t sin_port;
  struct in_addr sin_addr;
  unsigned char sin_zero[sizeof (struct sockaddr) -
    (sizeof (unsigned short int)) -
    sizeof (in_port_t) -
    sizeof (struct in_addr)];
};

struct sockaddr
{
  sa_family_t sa_family;
  char sa_data[14];
};

Ошибка заключается в том, что создаются объекты одного типа, а уничтожаются они как объекты другого типа:

class SocketAddress::SocketAddressImpl
{
  struct sockaddr *m_data;
  ....
  SocketAddressImpl (const SocketAddressImpl &other)
  {
    ....
    case SCIM_SOCKET_LOCAL:
        m_data = (struct sockaddr*) new struct sockaddr_un; // <=
        len = sizeof (sockaddr_un);
        break;
    case SCIM_SOCKET_INET:
        m_data = (struct sockaddr*) new struct sockaddr_in; // <=
        len = sizeof (sockaddr_in);
        break;
    ....
  }

  ~SocketAddressImpl () {
    if (m_data) delete m_data;                              // <=
  }
};

Предупреждения анализатора:

  • V572 It is odd that the object which was created using 'new' operator is immediately cast to another type. scim_socket.cpp 136
  • V572 It is odd that the object which was created using 'new' operator is immediately cast to another type. scim_socket.cpp 140

Software weaknesses type — CWE-762: Mismatched Memory Management Routines.

Создаются структуры типа sockaddr_un и sockaddr_in. При этом хранятся и уничтожаются они как структуры типа sockaddr. Типы всех трёх названных структур никак не связаны между собой. Это три разные структуры, имеющие разный размер. Сейчас код вполне может работать, так как структуры являются POD типами (не содержат деструкторы и т.д.) и вызов оператора delete превращается в простой вызов функции free. Однако формально код неверен. Нужно уничтожать объект точно такого же типа, который использовался при создании объекта.

Как я уже сказал, сейчас программа может работать, хотя формально и является неправильной. Надо понимать, что рассмотренный код очень опасен и достаточно, чтобы в одном из классов появился конструктор/деструктор или был добавлен член сложного типа (например std::string), как всё сразу сломается окончательно.

Другие ошибки:

  • V572 It is odd that the object which was created using 'new' operator is immediately cast to another type. scim_socket.cpp 167
  • V572 It is odd that the object which was created using 'new' operator is immediately cast to another type. scim_socket.cpp 171

Неправильная работа с printf-подобными функциями (4 ошибки)


static int _write_file(const char *file_name, void *data,
                       unsigned long long data_size)
{
  FILE *fp = NULL;

  if (!file_name || !data || data_size <= 0) {
    fprintf(stderr,
            "\tinvalid data %s %p size:%lld\n",
            file_name, data, data_size);
    return FALSE;
  }
  ....
}

Предупреждение PVS-Studio: V576 Incorrect format. Consider checking the third actual argument of the 'fprintf' function. Under certain conditions the pointer can be null. image_util_decode_encode_testsuite.c 124

Software weaknesses type — CWE-476: NULL Pointer Dereference

Возможна ситуация, когда указатель file_name будет содержать NULL. Как в таком случае поведёт себя функция printf, предсказать невозможно. На практике поведение будет зависеть от используемой реализации функции printf. См. дискуссию "What is the behavior of printing NULL with printf's %s specifier?".

Рассмотрим ещё одну ошибку.

void subscribe_to_event()
{
  ....
  int error = ....;
  ....
  PRINT_E("Failed to destroy engine configuration for event trigger.",
          error);
  ....
}

Предупреждение PVS-Studio: V576 Incorrect format. A different number of actual arguments is expected while calling 'printf' function. Expected: 1. Present: 2. surveillance_test_suite.c 393

Software weaknesses type — не знаю как классифицировать, буду благодарен за подсказку.

Макрос PRINT_E раскрывается в printf. Как видите, переменная error никак не используется. Видимо номер ошибки забыли распечатать.

Другие ошибки:

  • V576 Incorrect format. A different number of actual arguments is expected while calling 'printf' function. Expected: 1. Present: 2. surveillance_test_suite.c 410
  • V576 Incorrect format. A different number of actual arguments is expected while calling 'printf' function. Expected: 1. Present: 2. surveillance_test_suite.c 417

Проверка указателя выполняется уже после его разыменования (5 ошибок)


static void _show(void *data)
{
  SETTING_TRACE_BEGIN;
  struct _priv *priv = (struct _priv *)data;
  Eina_List *children = elm_box_children_get(priv->box);    // <=
  Evas_Object *first = eina_list_data_get(children);
  Evas_Object *selected =
    eina_list_nth(children, priv->item_selected_on_show);   // <=

  if (!priv) {                                              // <=
    _ERR("Invalid parameter.");
    return;
  }
  ....
}

Предупреждение PVS-Studio: V595 The 'priv' pointer was utilized before it was verified against nullptr. Check lines: 110, 114. view_generic_popup.c 110

Software weaknesses type — CWE-476: NULL Pointer Dereference

Указатель priv дважды разыменовывается в выражениях:

  • priv->box
  • priv->item_selected_on_show

И только затем указатель проверяется на равенство нулю. Чтобы исправить код, проверку следует передвинуть выше:

static void _show(void *data)
{
  SETTING_TRACE_BEGIN;
  struct _priv *priv = (struct _priv *)data;
  if (!priv) {
    _ERR("Invalid parameter.");
    return;
  }
  Eina_List *children = elm_box_children_get(priv->box);
  Evas_Object *first = eina_list_data_get(children);
  Evas_Object *selected =
    eina_list_nth(children, priv->item_selected_on_show);
  ....
}

Теперь рассмотрим более сложный случай.

Есть функция _ticker_window_create, в которой разыменовывается указатель, переданный функции в качестве аргумента.

static Evas_Object *_ticker_window_create(struct appdata *ad)
{
  ....
  // нет проверки указателя 'ad'
  ....
  evas_object_resize(win, ad->win.w, indicator_height_get());
  ....
}

Важно отметить, что указатель разыменовывается без предварительной проверки на равенство NULL. Другими словами, в функцию _ticker_window_create можно передавать только ненулевые указатели. Теперь посмотрим, как эта функция используется на самом деле.

static int _ticker_view_create(void)
{
  if (!ticker.win)
    ticker.win = _ticker_window_create(ticker.ad);       // <=
  if (!ticker.layout)
    ticker.layout = _ticker_layout_create(ticker.win);
  if (!ticker.scroller)
    ticker.scroller = _ticker_scroller_create(ticker.layout);

  evas_object_show(ticker.layout);
  evas_object_show(ticker.scroller);
  evas_object_show(ticker.win);

  if (ticker.ad)                                         // <=
    util_signal_emit_by_win(&ticker.ad->win,
      "message.show.noeffect", "indicator.prog");
  ....
}

Предупреждение PVS-Studio: V595 The 'ticker.ad' pointer was utilized before it was verified against nullptr. Check lines: 590, 600. ticker.c 590

Software weaknesses type — CWE-476: NULL Pointer Dereference

Указатель ticker.ad передаётся в функцию _ticker_window_create. Ниже есть проверка "if (ticket.ad)", которая свидетельствует о том, что этот указатель может быть нулевым.

Другие ошибки:

  • V595 The 'core' pointer was utilized before it was verified against nullptr. Check lines: 2252, 2254. media_codec_port_gst.c 2252
  • V595 The 'eyeCondition' pointer was utilized before it was verified against nullptr. Check lines: 162, 168. FaceEyeCondition.cpp 162
  • V595 The 'dev->name' pointer was utilized before it was verified against nullptr. Check lines: 122, 127. e_devicemgr_device.c 122

Не затираются приватные данные (1 ошибка)


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

Software weaknesses type — CWE-14: Compiler Removal of Code to Clear Buffers

Компилятор вправе удалить функцию memset, которая затирает приватные данные в буфере finalcount. С точки зрения языка C и C++, вызов функции можно удалить, так как далее этот буфер нигде не используется. Хочу обратить внимание, что это не теоретически возможное поведение компилятора, а самое что ни на есть обыденное. Компиляторы действительно удаляют такие функции (см. V597, CWE-14).

Путаница с выделением и освобождением памяти (2 ошибки)


Первая ошибка.

void
GenericTableContent::set_max_key_length (size_t max_key_length)
{
  ....
  std::vector<uint32> *offsets;
  std::vector<OffsetGroupAttr> *offsets_attrs;

  offsets = new(std::nothrow)                       // <=
            std::vector <uint32> [max_key_length];
  if (!offsets) return;

  offsets_attrs = new(std::nothrow)
                  std::vector <OffsetGroupAttr> [max_key_length];
  if (!offsets_attrs) {
    delete offsets;                                 // <=
    return;
  }
  ....
}

Предупреждение PVS-Studio: V611 The memory was allocated using 'new T[]' operator but was released using the 'delete' operator. Consider inspecting this code. It's probably better to use 'delete [] offsets;'. scim_generic_table.cpp 998

Software weaknesses type — CWE-762: Mismatched Memory Management Routines

В переменной offsets хранится указатель на массив объектов, созданных с помощью оператора new[]. Следовательно, разрушаться эти объекты должны с помощью оператора delete[].

Вторая ошибка.

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

Software weaknesses type — CWE-762: Mismatched Memory Management Routines

Память для буфера выделяется на стеке. Далее возможна ситуация, когда адрес этого буфера будет передан в качестве фактического аргумента в функцию free, что недопустимо.

Потенциально неинициализированная переменная (1 ошибка)


Тело функции _app_create, в которой находится ошибка, весьма длинное, поэтому я выделю только самую суть:

Eext_Circle_Surface *surface;
....
if (_WEARABLE)
  surface = eext_circle_surface_conformant_add(conform);
....
app_data->circle_surface = surface;

Предупреждение PVS-Studio: V614 Potentially uninitialized pointer 'surface' used. w-input-selector.cpp 896

Software weaknesses type — CWE-457: Use of Uninitialized Variable

Переменная surface инициализируется только в том случае, если выполняется условие "if (_WEARABLE)".

Код не защищен от некорректных строк (6 ошибок)


Я не сразу обратил внимание на этот вид дефекта и, кажется, не выписал ряд предупреждений. Поэтому таких мест может быть не 6, а гораздо больше. Мне было неинтересно возвращаться к уже просмотренным отчётам анализатора, так что пусть будет только 6 дефектов.

void ise_show_stt_mode(Evas_Object *win)
{
  ....
  snprintf(buf, BUF_LEN, gettext("IDS_ST_SK_CANCEL"));
  ....
}

Предупреждение PVS-Studio: V618 It's dangerous to call the 'snprintf' function in such a manner, as the line being passed could contain format specification. The example of the safe code: printf("%s", str); ise-stt-mode.cpp 802

Software weaknesses type — CWE-134 Use of Externally-Controlled Format String

Код работает корректно, но он очень ненадёжен и опасен по двум причинам:

  1. Работа программы нарушится, если кто-то в дальнейшем в IDS_ST_SK_CANCEL использует символы спецификатора формата. Абстрактный пример: понадобится выводить сообщение «bla-bla %SystemDrive% bla-bla». Тогда %S будет воспринято как спецификатор формата, в результате чего будет напечатана абракадабра или возникнет access violation.
  2. Это слабое место может быть использовано для атаки. Если злоумышленнику удастся подменить строку, которая выводится на печать, он потенциально может использовать это для своей пользы.

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

snprintf(buf, BUF_LEN, "%s", gettext("IDS_ST_SK_CANCEL"));

Другие слабые места:

  • V618 It's dangerous to call the 'snprintf' function in such a manner, as the line being passed could contain format specification. The example of the safe code: printf("%s", str); app_tracker.c 459
  • V618 It's dangerous to call the 'snprintf' function in such a manner, as the line being passed could contain format specification. The example of the safe code: printf("%s", str); screen_reader_system.c 443
  • V618 It's dangerous to call the 'snprintf' function in such a manner, as the line being passed could contain format specification. The example of the safe code: printf("%s", str); screen_reader_system.c 447
  • V618 It's dangerous to call the 'snprintf' function in such a manner, as the line being passed could contain format specification. The example of the safe code: printf("%s", str); navigator.c 550
  • V618 It's dangerous to call the 'snprintf' function in such a manner, as the line being passed could contain format specification. The example of the safe code: printf("%s", str); navigator.c 561

Самодельное объявление математических констант (2 ошибки)


#define PI 3.141592
void __apps_view_circle_get_pos(
             int radius, double angle, int *x, int *y)
{
  *x = radius * sin(angle * PI / 180);
  *y = radius * cos(angle * PI / 180);

  *x = *x + WINDOW_CENTER_X;
  *y = WINDOW_CENTER_Y - *y;
}

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

  • V624 The constant 3.141592 is being utilized. The resulting value could be inaccurate. Consider using the M_PI constant from <math.h>. apps_view_circle.c 306
  • V624 The constant 3.141592 is being utilized. The resulting value could be inaccurate. Consider using the M_PI constant from <math.h>. apps_view_circle.c 307

Software weaknesses type — не знаю как классифицировать, буду благодарен за подсказку.

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

Тем не менее, я считаю, что этот код надо модифицировать. Макрос PI - это лишняя сущность, которой просто не должно быть. Для таких случаев есть стандартный макрос M_PI, который вдобавок раскрывается в более точное значение.

Опасная арифметика (4 ошибки)


__extension__ typedef long int __time_t;
__extension__ typedef long int __suseconds_t;

struct timeval
{
  __time_t tv_sec;
  __suseconds_t tv_usec;
};

static struct timeval _t0 = {0, 0};
static struct timeval _t1;

void ISF_PROF_DEBUG_TIME (....)
{
  float etime = 0.0;
  ....
  etime = ((_t1.tv_sec * 1000000 + _t1.tv_usec) -
           (_t0.tv_sec * 1000000 + _t0.tv_usec))/1000000.0;
  ....
}

Предупреждение PVS-Studio: V636 The '_t1.tv_sec * 1000000' expression was implicitly cast from 'long' type to 'float' type. Consider utilizing an explicit type cast to avoid overflow. An example: double A = (double)(X) * Y;. scim_utility.cpp 1492.

Software weaknesses type — CWE-681: Incorrect Conversion between Numeric Types

Здесь вычисляют количество секунд между двумя метками времени. Вычисления ведутся в микросекундах и для этого количество секунд умножается на миллион. Вычисления ведутся в типе long, который в 32-битной системе Tizen является 32-битным. Здесь очень легко может возникнуть переполнение. Чтобы этого избежать, для расчётов следует использовать тип long long или double.

Другие ошибки:

  • V636 The 'w / 2' expression was implicitly cast from 'int' type to 'float' type. Consider utilizing an explicit type cast to avoid the loss of a fractional part. An example: double A = (double)(X) / Y;. poly_shape_hit_test.cpp 97
  • V636 The 'w / 2' expression was implicitly cast from 'int' type to 'float' type. Consider utilizing an explicit type cast to avoid the loss of a fractional part. An example: double A = (double)(X) / Y;. poly_shape_hit_test.cpp 98
  • V636 The 'duration / 1000' expression was implicitly cast from 'int' type to 'double' type. Consider utilizing an explicit type cast to avoid the loss of a fractional part. An example: double A = (double)(X) / Y;. e_devicemgr_device.c 648

Код выглядит не так, как работает (2 ошибки)


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

int bt_tds_provider_send_activation_resp(
  char *address, int result, bt_tds_provider_h provider)
{
  ....
  if (error_code != BT_ERROR_NONE)
    BT_ERR("%s(0x%08x)",
           _bt_convert_error_to_string(error_code), error_code);
    return error_code;

  return error_code;
}

Предупреждение PVS-Studio: V640 The code's operational logic does not correspond with its formatting. The statement is indented to the right, but it is always executed. It is possible that curly brackets are missing. bluetooth-tds.c 313

Software weaknesses type — CWE-483: Incorrect Block Delimitation

Программисту повезло, так как независимо от условия следует вернуть одно и то же значение. Здесь забыты фигурные скобки. Корректный код должен был выглядеть так:

if (error_code != BT_ERROR_NONE)
{
  BT_ERR("%s(0x%08x)",
         _bt_convert_error_to_string(error_code), error_code);
  return error_code;
}
return error_code;

Или можно удалить один return и сделать код короче:

if (error_code != BT_ERROR_NONE)
  BT_ERR("%s(0x%08x)",
         _bt_convert_error_to_string(error_code), error_code);
return error_code;

Теперь рассмотрим более интересный случай. Ошибка возникает из-за этого макроса:

#define MC_FREEIF(x)   if (x)     g_free(x);   x = NULL;

Беда...

Теперь посмотрим, как макрос используется:

static gboolean __mc_gst_init_gstreamer()
{
  ....
  int i = 0;
  ....
  for (i = 0; i < arg_count; i++)
    MC_FREEIF(argv2[i]);
  ....
}

Предупреждение PVS-Studio: V640 The code's operational logic does not correspond with its formatting. The second statement will always be executed. It is possible that curly brackets are missing. media_codec_port_gst.c 1800

Software weaknesses type — CWE-483: Incorrect Block Delimitation, CWE-787: Out-of-bounds Write

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

for (i = 0; i < arg_count; i++)
  if (argv2[i])
    g_free(argv2[i]);
argv2[i] = NULL;

Результат:

  1. Не будут обнулены указатели.
  2. NULL будет записан за границу массива.

Потеря старших бит (1 ошибка)


typedef unsigned char Eina_Bool;
static Eina_Bool _state_get(....)
{
  ....
  if (!strcmp(part, STATE_BROWSER))
    return !strcmp(id, APP_ID_BROWSER);
  else if (!strcmp(part, STATE_NOT_BROWSER))
    return strcmp(id, APP_ID_BROWSER);
  ....
}

Предупреждение PVS-Studio: V642 Saving the 'strcmp' function result inside the 'unsigned char' type variable is inappropriate. The significant bits could be lost breaking the program's logic. grid.c 137

Software weaknesses type — CWE-197: Numeric Truncation Error

Функция strcmp возвращает следующие значения типа int:

  • < 0 — buf1 less than buf2;
  • 0 — buf1 identical to buf2;
  • > 0 — buf1 greater than buf2;

Обратите внимание. «Больше 0» означает любые числа, а вовсе не 1. Этими числами могут быть: 2, 3, 100, 256, 1024, 5555 и так далее. Аналогично дело обстоит и с «меньше 0». Отсюда следует, что результат нельзя поместить в переменную типа unsigned char, так как могут быть отброшены значащие биты. Это приведет к нарушению логики выполнения программы, например, число 256 превратится в 0.

Данная опасность может показаться надуманной. Однако, такая ошибка послужила причиной серьезной уязвимости в MySQL/MariaDB до версий 5.1.61, 5.2.11, 5.3.5, 5.5.22. Суть в том, что при подключении пользователя MySQL/MariaDB вычисляется токен (SHA от пароля и хэша), который сравнивается с ожидаемым значением функцией memcmp. На некоторых платформах возвращаемое значение может выпадать из диапазона [-128..127]. В итоге в 1 случае из 256 процедура сравнения хэша с ожидаемым значением всегда возвращает значение true, независимо от хэша. В результате простая команда на bash даёт злоумышленнику рутовый доступ к уязвимому серверу MySQL, даже если он не знает пароль. Причиной этому стал такой код в файле 'sql/password.c':

typedef char my_bool;
...
my_bool check(...) {
  return memcmp(...);
}

Более подробное описание этой проблемы приведено здесь: Security vulnerability in MySQL/MariaDB.

Вернемся к проекту Tizen. Мне кажется, в рассмотренном фрагменте кода пропустили оператор отрицания '!'. Тогда корректный код должен быть таким:

else if (!strcmp(part, STATE_NOT_BROWSER))
  return !strcmp(id, APP_ID_BROWSER);

Off-by-one Error (2 ошибки)


#define OP_MAX_URI_LEN 2048
char object_uri[OP_MAX_URI_LEN];
void op_libxml_characters_dd1(....)
{
  ....
  strncat(dd_info->object_uri, ch_str,
          OP_MAX_URI_LEN - strlen(dd_info->object_uri));
  ....
}

Предупреждение PVS-Studio: V645 The 'strncat' function call could lead to the 'dd_info->object_uri' buffer overflow. The bounds should not contain the size of the buffer, but a number of characters it can hold. oma-parser-dd1.c 422

Software weaknesses type — CWE-193: Off-by-one Error

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

char buf[5] = "ABCD";
strncat(buf, "E", 5 - strlen(buf));

В буфере уже нет места для новых символов. В нём находятся 4 символа и терминальный ноль. Выражение 5 — strlen(buf) равно 1. Функция strncpy скопирует символ E в последний элемент массива. Терминальный 0 будет записан уже за пределами буфера.

Правильный вариант кода:

strncat(dd_info->object_uri, ch_str,
        OP_MAX_URI_LEN - strlen(dd_info->object_uri) - 1);

Ещё одна аналогичная ошибка: V645 The 'strncat' function call could lead to the 'dd_info->name' buffer overflow. The bounds should not contain the size of the buffer, but a number of characters it can hold. oma-parser-dd1.c 433

Коварный язык C. Используется необъявленная функция (1 ошибка)


void person_recognized_cb(
  mv_surveillance_event_trigger_h handle,
  mv_source_h source,
  int video_stream_id,
  mv_surveillance_result_h event_result,
  void *user_data)
{
  ....
  int *labels = malloc(sizeof(int) * number_of_persons);
  ....
}

Предупреждение PVS-Studio: V647 The value of 'int' type is assigned to the pointer of 'int' type. surveillance_test_suite.c 928

Software weaknesses type — CWE-822: Untrusted Pointer Dereference

Здесь спрятана ловушка. Она «включится», когда код Tizen превратится в 64-битную операционную систему.

Дело в том, что не объявлена функция malloc, т.е. нигде нет #include <stdlib.h>. В этом можно убедиться, выполнив препроцессирование и заглянув внутрь i-файла. Раз функция не объявлена, то считается, что она возвращает тип int. Именно об этом и предупреждает анализатор, говоря, что значение типа int превращается в указатель.

В 32-битной системе всё хорошо, так как размер указателя совпадает с размером int. Ошибка может проявить себя в 64-битной программе, где будут отброшены старшие биты указателя. Подробнее суть этой ошибки рассматривается в статье "Коллекция примеров 64-битных ошибок в реальных программах" (см. Пример 7. Необъявленные функции в Си.)

Не учтено, что оператор new, в отличии от функции malloc, не возвращает NULL (54 ошибки)


Функция malloc, если не может выделить память, возвращает NULL. Оператор new при нехватке памяти генерирует исключение std::bad_alloc.

Если требуется, чтобы оператор new возвращал nullptr, следует использовать nothrow версию оператора:

P = new (std::nothrow) T;

Анализатор PVS-Studio знает про различия между двумя видами оператора new и выдаёт предупреждение только когда используется обыкновенный оператор new, генерирующий исключение.

Суть предупреждения анализатора PVS-Studio заключается в том, что нет смысла проверять, вернул оператор new нулевой указатель или нет.

Найденные ошибки можно разделить на безобидные и серьёзные. Начнем с безобидной ошибки.

template <class T> class vector {
private:
  ....
  void push_back(const T &value)
  {
    T *clone = new T(value);
    if (clone) {
      g_array_append_val(parray, clone);
      current_size++;
    }
  }
  ....
};

Предупреждение PVS-Studio: V668 There is no sense in testing the 'clone' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. maps_util.h 153

Software weaknesses type — CWE-697: Insufficient Comparison / CWE-571: Expression is Always True

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

Теперь рассмотрим опасную ошибку.

bool CThreadSlm::load(const char* fname, bool MMap)
{
  int fd = open(fname, O_RDONLY);
  ....
  if ((m_buf = new char[m_bufSize]) == NULL) {
    close(fd);
    return false;
  }
  ....
}

Предупреждение PVS-Studio: V668 There is no sense in testing the 'm_buf' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. slm.cpp 97

Software weaknesses type — даже не знаю как классифицировать. На мой взгляд, здесь сразу подходят три варианта:

  1. CWE-697: Insufficient Comparison
  2. CWE-570: Expression is Always False
  3. CWE-404: Improper Resource Shutdown or Release

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

Обычно такие ошибки появляются в процессе рефакторинга кода, когда вызов функции malloc заменяется на оператор new. Следующий фрагмент кода это очень хорошо демонстрирует:

void SettingsAFCreator::createNewAutoFillFormItem()
{
  ....
  auto item_data = new AutoFillFormItemData;
  if (!item_data) {
    BROWSER_LOGE("Malloc failed to get item_data");
    return;
  }
  ....
}

Предупреждение PVS-Studio: V668 There is no sense in testing the 'item_data' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. SettingsAFCreator.cpp 112

Текст сообщения подсказывает нам, что когда-то давно здесь использовалась функция malloc.

Рекомендация. Замена malloc на new ради красоты ничего не даёт и может только спровоцировать появление ошибок. Поэтому старый код с malloc лучше оставить как он есть, а если вы решили изменить его, то отнеситесь к этому ответственно и внимательно.

Мы рассмотрели 3 ошибки. Осталась ещё 51 ошибка. Рассматривать их в статье нет никакого смысла, поэтому я просто приведу предупреждения анализатора в файле Tizen_V668.txt.

Путаница между integer и double (1 ошибка)


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

фрагмент



Предупреждение PVS-Studio: V674 The '0.5' literal of the 'double' type is assigned to a variable of the 'int' type. Consider inspecting the '= 0.5' expression. add-viewer.c 824

Software weaknesses type — CWE-681: Incorrect Conversion between Numeric Types

Был некий код, который вычислял значение delay, представленное в миллисекундах. Значение по умолчанию было 500 миллисекунд. Один из программистов по какой-то причине закомментировал этот код и решил, что всегда будет использоваться значение равное 500 миллисекунд. При этом он был невнимателен и использовал значение 0.5, которое по его замыслу означает пол секунды, т.е. как раз 500 миллисекунд. В результате переменная типа int инициализируется значением 0.5, которое превращается в 0.

Правильный вариант:

int delay = 500;

Запись в readonly память (1 ошибка)


int test_batch_operations()
{
  ....
  char *condition = "MEDIA_PATH LIKE \'";
  strncat(condition,
          tzplatform_mkpath(TZ_USER_CONTENT, "test/image%%jpg\'"),
          17);
  ....
}

Предупреждение PVS-Studio: V675 Calling the 'strncat' function will cause the writing into the read-only memory. Inspect the first argument. media-content_test.c 2952

Software weaknesses type — не знаю как классифицировать, буду благодарен за подсказку.

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

В переменной condition хранится адрес памяти, предназначенной только для чтения. Изменение этой памяти приведёт к неопределённому поведению программы. Скорее всего это неопределенное поведение программы будет представлять собой access violation.

Неправильные циклы do… while (2 ошибки)


enum nss_status _nss_securitymanager_initgroups_dyn(....)
{
  ....
  do {
    ret = TEMP_FAILURE_RETRY(getpwnam_r(....));
    if (ret == ERANGE && buffer.size() < MEMORY_LIMIT) {
      buffer.resize(buffer.size() << 1);
      continue;                                          // <=
    }
  } while (0);
  ....
}

Предупреждение PVS-Studio: V696 The 'continue' operator will terminate 'do {… } while (FALSE)' loop because the condition is always false. Check lines: 73, 75. nss_securitymanager.cpp 73

Software weaknesses type — CWE-670: Always-Incorrect Control Flow Implementation

Легко забыть, что оператор continue в цикле do {… } while(0) остановит цикл, а не возобновит его. Оператор continue передаёт управление на условие проверки остановки цикла, а вовсе не на начало цикла. Так как условие всегда ложно, то continue останавливает цикл.

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

while (true) {
  ret = TEMP_FAILURE_RETRY(getpwnam_r(....));
  if (ret != ERANGE || buffer.size() >= MEMORY_LIMIT) {
    break;
  buffer.resize(buffer.size() << 1);
};

Вторая ошибка находится в этом же файле чуть ниже: V696 The 'continue' operator will terminate 'do {… } while (FALSE)' loop because the condition is always false. Check lines: 120, 122. nss_securitymanager.cpp 120

Потенциальная утечка памяти при использовании realloc (11 ошибок)


Анализатор выдаёт предупреждения V701, когда встречает код вида:

P = (T *)realloc(P, n);

Если память не удастся выделить, то может произойти её утечка, так как в указатель P будет записано значение NULL. Произойдет утечка памяти или нет, зависит от того, хранится ли где-то ещё предыдущее значение указателя P и как оно используется. Анализатор не может разобраться в хитросплетениях логики работы программы, и поэтому часть предупреждений V701 являются ложными. Всего предупреждений было много, и я отобрал среди них только 11, которые показались мне наиболее достоверными. Возможно, я не прав, и ошибок этого типа может быть на самом деле как больше, так и меньше.

Рассмотрим одну из найденных ошибок.

static int _preference_get_key_filesys(
  keynode_t *keynode, int* io_errno)
{
  ....
  char *value = NULL;
  ....
  case PREFERENCE_TYPE_STRING:
    while (fgets(file_buf, sizeof(file_buf), fp)) {
      if (value) {
        value_size = value_size + strlen(file_buf);
        value = (char *) realloc(value, value_size);      // <=
        if (value == NULL) {
          func_ret = PREFERENCE_ERROR_OUT_OF_MEMORY;
          break;
        }
        strncat(value, file_buf, strlen(file_buf));
      } else {
        ....
      }
    }
                ....
    if (value)
      free(value);
    break;
  
  ....
}

Предупреждение PVS-Studio: V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'value' is lost. Consider assigning realloc() to a temporary pointer. preference.c 951

Software weaknesses type — CWE-401: Improper Release of Memory Before Removing Last Reference ('Memory Leak')

В цикле из файла читаются данные и помещаются в буфер. Размер буфера увеличивается с помощью вызова функции realloc. В этом примере хорошо видно, что если функция realloc вернёт в какой-то момент значение NULL, то произойдёт утечка памяти.

Другие ошибки:

  • V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'char_info->char_value' is lost. Consider assigning realloc() to a temporary pointer. bt-gatt-service.c 2430
  • V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'gif_data->frames' is lost. Consider assigning realloc() to a temporary pointer. image_util.c 1709
  • V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'gif_data->frames' is lost. Consider assigning realloc() to a temporary pointer. image_util.c 1861
  • V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer '_handle->src_buffer' is lost. Consider assigning realloc() to a temporary pointer. image_util.c 1911
  • V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'gif_data->frames' is lost. Consider assigning realloc() to a temporary pointer. image_util.c 1930
  • V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer '* body' is lost. Consider assigning realloc() to a temporary pointer. http_request.c 362
  • V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'header->rsp_header' is lost. Consider assigning realloc() to a temporary pointer. http_transaction.c 82
  • V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'name' is lost. Consider assigning realloc() to a temporary pointer. popup.c 179
  • V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'tmphstbuf' is lost. Consider assigning realloc() to a temporary pointer. scim_socket.cpp 95
  • V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'wsc_ctx->preedit_str' is lost. Consider assigning realloc() to a temporary pointer. wayland_panel_agent_module.cpp 1745

Использование небезопасного источника данных (1 ошибка)


#define BUFFER_SIZE 128
int main(int argc, char *argv[])
{
  ....
  char temp[BUFFER_SIZE] = {0,};
  ....
  snprintf(temp, BUFFER_SIZE, "%s%s-%d%s",
           argv[2], temp_filename, i, file_format);
  ....
}

Предупреждение PVS-Studio: V755 A copy from unsafe data source to a buffer of fixed size. Buffer overflow is possible. image_util_decode_encode_testsuite.c 237

Software weaknesses type — CWE-121: Stack-based Buffer Overflow

Указатель на строку argv[2] является небезопасным источником данных, так как это аргумент командной строки. При формировании строки в буфере temp может произойти выход за границу массива.

Утечки памяти (3 ошибки)


В начале рассмотрим три используемые функции. Для нас важно, что все они возвращают указатель на выделенную память.

char *generate_role_trait(AtspiAccessible * obj) {
  ....
  return strdup(ret);
}
char *generate_description_trait(AtspiAccessible * obj) {
  ....
  return strdup(ret);
}
char *generate_state_trait(AtspiAccessible * obj) {
  ....
  return strdup(ret);
}

Теперь рассмотрим тело функции, содержащее 3 ошибки.

static char *generate_description_from_relation_object(....)
{
  ....
  char *role_name = generate_role_trait(obj);
  char *description_from_role = generate_description_trait(obj);
  char *state_from_role = generate_state_trait(obj);
  ....
  char *desc = atspi_accessible_get_description(obj, &err);

  if (err)
  {
    g_error_free(err);
    g_free(desc);
    return strdup(trait);
  }
  ....  
}

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

  • V773 The function was exited without releasing the 'role_name' pointer. A memory leak is possible. navigator.c 991
  • V773 The function was exited without releasing the 'description_from_role' pointer. A memory leak is possible. navigator.c 991
  • V773 The function was exited without releasing the 'state_from_role' pointer. A memory leak is possible. navigator.c 991

Software weaknesses type — CWE-401 Improper Release of Memory Before Removing Last Reference ('Memory Leak')

Если функция atspi_accessible_get_description отработает неудачно, то функция generate_description_from_relation_object должна прекратить свою работу. При этом освобождается память, указатель на которую хранится в переменной desc. Про переменные role_name, description_from_role и state_from_role автор забыл, и возникнет 3 утечки памяти.

Опечатка в однотипных блоках кода (1 ошибка)


BookmarkManagerUI::~BookmarkManagerUI()
{
  BROWSER_LOGD("[%s:%d] ", __PRETTY_FUNCTION__, __LINE__);
  if (m_modulesToolbar) {
    evas_object_smart_callback_del(m_modulesToolbar,
      "language,changed", _modules_toolbar_language_changed);
    evas_object_del(m_modulesToolbar);
  }
  if (m_navigatorToolbar) {
    evas_object_smart_callback_del(m_navigatorToolbar,
      "language,changed", _navigation_toolbar_language_changed);
    evas_object_del(m_modulesToolbar);                    // <=
  }
  ....
}

Предупреждение PVS-Studio: V778 Two similar code fragments were found. Perhaps, this is a typo and 'm_navigatorToolbar' variable should be used instead of 'm_modulesToolbar'. BookmarkManagerUI.cpp 66

Software weaknesses type — CWE-675: Duplicate Operations on Resource

Код деструктора писался методом Copy-Paste. Случайно в одном месте забыли заменить имя m_modulesToolbar на m_navigatorToolbar.

Мёртвый код (8 ошибок)


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

void Integrity::syncElement(....) {
  ....
  if (ret < 0) {
    int err = errno;
    LOGE("'close' function error [%d] : <%s>",err,strerror(err));
    throw UnexpectedErrorException(err, strerror(err));
  }
}

Теперь посмотрим на код, написанный с ошибкой:

void Integrity::createHardLink(....) {
  int ret = link(oldName.c_str(), newName.c_str());

  if (ret < 0) {
    int err = errno;
    throw UnexpectedErrorException(err, strerror(err));
    LOGN("Trying to link to non-existent...", oldName.c_str());
  }
}

Предупреждение PVS-Studio: V779 Unreachable code detected. It is possible that an error is present. Integrity.cpp 233

Software weaknesses type — CWE-561: Dead Code

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

Рассмотрим ещё одну ошибку.

#define LS_FUNC_ENTER LS_LOGD("(%s) ENTER", __FUNCTION__);
#define LS_FUNC_EXIT LS_LOGD("(%s) EXIT", __FUNCTION__);
static bool __check_myplace_automation(void)
{
  LS_FUNC_ENTER
  bool myplace_automation_supported = false;
  bool myplace_automation_consent = false;
  ....
  return false;
  LS_FUNC_EXIT
}

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

Software weaknesses type — CWE-561: Dead Code

Макрос-эпилог не используется. Две последние строки в функции следует поменять местами.

Другие ошибки:

  • V779 Unreachable code detected. It is possible that an error is present. bt-hdp.c 295
  • V779 Unreachable code detected. It is possible that an error is present. media_codec_port_gst.c 2672
  • V779 Unreachable code detected. It is possible that an error is present. myplace.c 197
  • V779 Unreachable code detected. It is possible that an error is present. setting-common-view.c 124
  • V779 Unreachable code detected. It is possible that an error is present. layout_network.c 1666
  • V779 Unreachable code detected. It is possible that an error is present. ad-id.c 472

Неправильная инициализация объектов (2 ошибки)


Для начала рассмотрим объявление некоторых типов данных.
struct _VoiceData {
  int voicefw_state;
  ....
  std::vector<std::string> stt_results;
  ....
  is::ui::MicEffector *effector;
};
typedef struct _VoiceData VoiceData;

Обратите внимание, что один из членов класса VoiceData представляет собой массив строк. Теперь посмотрим, как экземпляр класса создаётся и уничтожается.

void show_voice_input(....)
{
  ....
  my_voicedata = (VoiceData*)malloc(sizeof(VoiceData));
  if (my_voicedata == NULL) {
    LOGD("%d::::Heap Overflow, ......!", __LINE__);
    return;
  }
  memset(my_voicedata, 0, sizeof(VoiceData));
  ....
}

void on_destroy(VoiceData *r_voicedata)
{
  ....
  VoiceData *voicedata = (VoiceData *)r_voicedata;
  ....
  free(voicedata);
}

Предупреждение PVS-Studio: V780 The object 'my_voicedata' of a non-passive (non-PDS) type cannot be initialized using the memset function. ise-stt-mode.cpp 773

Software weaknesses type — CWE-762 Mismatched Memory Management Routines

Итак, объект создаётся с помощью функций malloc и memset, а уничтожается с помощью free. В результате:

  • Не зовётся конструктор для объекта типа std::vector<std::string>. Использовать такой объект нельзя.
  • Не зовётся деструктор. Как минимум будет утечка памяти.

Вообще, рассуждать, как будет работать этот код, не имеет смысла. Тут сплошные undefined behavior. Ужас.

Это был проект ise-default-1.3.34. Точно такую же ошибку мы встретим в проекте org.tizen.inputdelegator-0.1.170518. Ошибки размножаются почкованием (копирование кода): V780 The object 'my_voicedata' of a non-passive (non-PDS) type cannot be initialized using the memset function. w-input-stt-ise.cpp 51

Прочее (73 ошибки)


Есть ещё 73 ошибки, описание которых я опущу. Эти ошибки, на мой взгляд, неинтересные, или придётся приводить слишком много кода для их демонстрации, а я уже устал. Тем более, что и без них статья получается угрожающего размера, а я ведь хотел ещё немного поговорить об ошибках в сторонних библиотеках. Поэтому перечислю типы оставшихся ошибок общим списком.

  • V524. It is odd that the body of 'Foo_1' function is fully equivalent to the body of 'Foo_2' function. (1 ошибка)
  • V535. The variable 'X' is being used for this loop and for the outer loop. (4 ошибки)
  • V571. Recurring check. This condition was already verified in previous line. (1 ошибка)
  • V622. Consider inspecting the 'switch' statement. It's possible that the first 'case' operator is missing. (1 ошибка)
  • V646. Consider inspecting the application's logic. It's possible that 'else' keyword is missing. (2 ошибки)
  • V686. A pattern was detected: A || (A && ...). The expression is excessive or contains a logical error. (1 ошибка)
  • V690. The class implements a copy constructor/operator=, but lacks the operator=/copy constructor. (7 ошибок)
  • V692. An inappropriate attempt to append a null character to a string. To determine the length of a string by 'strlen' function correctly, a string ending with a null terminator should be used in the first place. (2 ошибки)
  • V746. Type slicing. An exception should be caught by reference rather than by value. (32 ошибки)
  • V759. Violated order of exception handlers. Exception caught by handler for base class. (9 ошибок)
  • V762. Consider inspecting virtual function arguments. See NN argument of function 'Foo' in derived class and base class. (6 ошибок)
  • V769. The pointer in the expression equals nullptr. The resulting value is meaningless and should not be used. (2 ошибки)
  • V783. Dereferencing of invalid iterator 'X' might take place. (4 ошибки)
  • V786. Assigning the value C to the X variable looks suspicious. The value range of the variable: [A, B]. (1 ошибка)

Сами предупреждения можно найти в файле Tizen_other_things.txt.

Промежуточные итоги


Я выявил 344 ошибки. В презентации я указывал число 345. Одну ошибку я решил исключить, так как, занимаясь написанием статьи, заметил, что одно из предупреждений на самом деле является ложным срабатыванием. Для статистики это несущественно, но я решил пояснить, почему число здесь и в презентации отличается.

Всего было проанализировано 1036000 строк кода, из которых 19.9% являются комментариями. Таким образом, «настоящих строк кода» (без комментариев): 830000.

Получается, что анализатор обнаруживает 0.41 ошибки на 1000 строк кода.

Много это или мало? Сложный вопрос. Чтобы на него ответить, надо знать среднюю плотность ошибок в коде Tizen, создаваемого в компании Samsung. У меня таких данных нет, давайте попробуем заняться экспертной оценкой. Да, тут можно сильно ошибиться, но всё равно интересно попробовать посчитать.

По данным исследователей университета Carnegie-Mellon на 1000 строк кода приходится от 5 до 15 ошибок. В свою очередь, ещё в 2011 году операционную систему Linux аналитики назвали одним из «эталонов качества» программного кода. Считается, что в Linux и его компонентах на 1000 строк кода приходится около 1 ошибки. Не могу найти, где я читал такую информацию, так что не гарантирую её достоверность, но выглядит она похожей на правду.

Операционная система Tizen основана на базе Linux, поэтому по идее также должна иметь высокое качество. Так сколько же ошибок на 1000 строк кода в Tizen? Давайте возьмем среднее между 1 и 5. Будем считать, что в среднем на 1000 строк кода присутствует 3 ошибки.

Если это так, то анализатор PVS-Studio поможет устранить более 10% ещё необнаруженных ошибок. Для нового непроверенного кода, который будет появляться, этот процент больше. Вполне можно говорить, что анализатор PVS-Studio сможет предотвращать около 20% ошибок.

Мы закончили разбор ошибок, которые я обнаружил в коде, написанном под копирайтом компании Samsung. Теперь нас ждут внешние библиотеки. Им я уделю меньше внимания, но всё равно до конца статьи ещё далеко, поэтому пришло время второй чашечки кофе/чая.

Чай/кофе. Новая порция.


Анализ сторонних проектов, используемых в Tizen


Сторонними проектами я посчитал проекты, в которых не сказано явно, что они созданы компанией Samsung. Вот список этих проектов, также отобранных случайным образом: alsa-lib-1.0.28, aspell-0.60.6.1, augeas-1.3.0, bind-9.11.0, efl-1.16.0, enlightenment-0.20.0, ise-engine-anthy-1.0.9.

Проектов гораздо меньше по количеству, но зато они в несколько раз крупнее рассматриваемых ранее. Суммарный размер проектов, перечисленных здесь, по размеру больше, чем суммарный размер кода проектов, рассматриваемых в предыдущей главе.

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

Опечатка в условии: слева и справа одно и то же (4 ошибки)


static void _edje_generate_source_state_map(....)
{
  for (i = 0; i < pd->map.colors_count; ++i)
  {
    if ((pd->map.colors[i]->r != 255) ||
        (pd->map.colors[i]->g != 255) ||
        (pd->map.colors[i]->b != 255) ||
        (pd->map.colors[i]->b != 255))
    .....
}

Предупреждение PVS-Studio: V501 There are identical sub-expressions '(pd->map.colors[i]->b != 255)' to the left and to the right of the '||' operator. edje_edit.c 14052

Software weaknesses type — CWE-570: Expression is Always False

Повторно проверили голубой компонент вместо альфа-канала. Данный пример в очередной раз демонстрирует, как здорово анализатор PVS-Studio умеет выявлять различные опечатки.

Другие ошибки:

  • V501 There are identical sub-expressions to the left and to the right of the '&&' operator: out_ && out_ != stdout && out_ != stdout checker_string.cpp 74
  • V501 There are identical sub-expressions '(revoked_zsk[i] != 0)' to the left and to the right of the '||' operator. dnssectool.c 1832
  • V501 There are identical sub-expressions to the left and to the right of the '>' operator: ob->priv.last > ob->priv.last evas_outbuf.c 684

Разыменование (потенциально) нулевого указателя (всего ошибок 269)


В предыдущей главе мы обсуждали разыменование нулевых указателей. Но речь шла только о потенциально нулевых указателях, которые возвращали такие функции, как malloc, strdup и т.д. Другими словами, при везении программа могла работать корректно.

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

static isc_result_t
setup_style(dns_master_style_t **stylep) {
  isc_result_t result;
  dns_master_style_t *style = NULL;

  REQUIRE(stylep != NULL || *stylep == NULL);
  ....
}

Предупреждение PVS-Studio: V522 Dereferencing of the null pointer 'stylep' might take place. Check the logical condition. delv.c 500

Software weaknesses type — CWE-476: NULL Pointer Dereference

Проверка написана неправильно: если указатель нулевой, то он будет разыменован. Видимо программист планировал написать вот такую проверку:

REQUIRE(stylep != NULL && *stylep != NULL);

Такой тип ошибки редок, так как ошибка быстро проявляет себя. В основном диагностики V522 и V575 выявляют указатели, которые будут нулевыми только при определённых условиях. Эти ситуации мы рассматривали ранее.

Оставшиеся предупреждения, указывающие на 268 ошибок, я собрал в файл Tizen_third_party_V522_V575.txt.

Функция возвращает случайное значение (3 ошибки)


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

static Eina_Bool
_ipc_server_data(void *data, int type EINA_UNUSED, void *event)
{
  ....
  //TIZEN_ONLY(170317): add skipping indicator buffer logic
  if (indicator_buffer_skip)
    return;
  //END
  ....
}

Предупреждение PVS-Studio: V591 Non-void function should return a value. ecore_evas_extn.c 1526

Software weaknesses type — CWE-393: Return of Wrong Status Code

Функция может возвращать некорректный статус (случайное значение) типа Eina_Bool.

Другие ошибки:

  • V591 Non-void function should return a value. lsort.hpp 159
  • V591 Non-void function should return a value. ecore_evas_extn.c 1617

Использование освобождённой памяти (5 ошибок)


static char *readline_path_generator(const char *text, int state) {
  ....
  if (ctx != NULL) {
    char *c = realloc(child, strlen(child)-strlen(ctx)+1);  // <=
    if (c == NULL)
      return NULL;
    int ctxidx = strlen(ctx);
    if (child[ctxidx] == SEP)                               // <=
      ctxidx++;
    strcpy(c, &child[ctxidx]);                              // <=
    child = c;
  }
  ....
}

Предупреждения анализатора:

  • V774 The 'child' pointer was used after the memory was reallocated. augtool.c 151
  • V774 The 'child' pointer was used after the memory was reallocated. augtool.c 153

Software weaknesses type — CWE-416: Use after free

Этот код совершенно неправильный, но иногда он может работать.

После успешного вызова функции realloc указатель child становится невалидным и его больше нельзя использовать.

Почему тогда я говорю, что временами это работает? Дело в том, что менеджер памяти может возвращать тот же адрес буфера, что и был раньше. То есть просто размер буфера увеличивается без изменения его адреса. Так менеджер памяти оптимизирует скорость, так как не требуется копировать данные из старого буфера в новый.

Другие ошибки:

  • V774 The 'res' pointer was used after the memory was released. sample-request.c 225
  • V774 The 'res' pointer was used after the memory was released. sample-update.c 193
  • V774 The 'res' pointer was used after the memory was released. sample-update.c 217

Опечатка в однотипных блоках кода (1 ошибка)


void Config::del()
{
  while (first_) {
    Entry * tmp = first_->next;
    delete first_;
    first_ = tmp;
  }

  while (others_) {
    Entry * tmp = others_->next;
    delete first_;
    others_ = tmp;
  }
  .....
}

Предупреждение PVS-Studio: V778 Two similar code fragments were found. Perhaps, this is a typo and 'others_' variable should be used instead of 'first_'. config.cpp 185

Software weaknesses type — CWE-401: Improper Release of Memory Before Removing Last Reference ('Memory Leak')

Красивая ошибка при использовании метода Copy-Paste. Скопировали блок текста, но в одном месте забыли изменить имя переменной.

После первого цикла переменная first_ имеет значение nullptr. Это значит, что во время выполнения второго цикла ничего удаляться не будет, и возникнут множественные утечки памяти.

Условие всегда true/false (9 ошибок)


StyleLineType StyleLine::get_type (void)
{
  ....
  unsigned int spos, epos;
  ....
  for (epos = m_line.length () - 1;
       epos >= 0 && isspace (m_line[epos]);
       epos--);
  ....
}

Предупреждение PVS-Studio: V547 Expression 'epos >= 0' is always true. Unsigned type value is always >= 0. scim_anthy_style_file.cpp 103

Software weaknesses type — CWE-571 Expression is Always True

Бегло просматривая этот код, сложно заметить ошибку. Ошибка заключается в том, что epos является беззнаковой переменной. Это значит, что выражение epos >= 0 всегда истинно.

Из-за этой ошибки код не защищён от ситуации, когда строка m_line окажется пустой. Если строка пустая, то переменная epos будет равна UINT_MAX и, как следствие, доступ к массиву (m_line[epos]) приведёт к неприятным последствиям.

Другие ошибки:

  • V547 Expression is always false. pcm_extplug.c 769
  • V547 Expression is always false. pcm_extplug.c 791
  • V547 Expression is always false. pcm_extplug.c 817
  • V547 Expression is always false. pcm_extplug.c 839
  • V547 Expression is always false. pcm_ioplug.c 1022
  • V547 Expression is always false. pcm_ioplug.c 1046
  • V547 Expression 'pathPosEnd >= 0' is always true. Unsigned type value is always >= 0. new_fmode.cpp 566
  • V547 Expression 'epos >= 0' is always true. Unsigned type value is always >= 0. scim_anthy_style_file.cpp 137

Не затираются приватные данные (52 ошибки)


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

Рассмотрю в статье только два фрагмента кода, так как все эти баги выглядят однотипно.

void
isc_hmacsha1_sign(isc_hmacsha1_t *ctx,
                  unsigned char *digest, size_t len) {
  unsigned char opad[ISC_SHA1_BLOCK_LENGTH];
  unsigned char newdigest[ISC_SHA1_DIGESTLENGTH];
  ....
  memset(newdigest, 0, sizeof(newdigest));
}

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

Software weaknesses type — CWE-14: Compiler Removal of Code to Clear Buffers

Приватные данные, хранящиеся в буфере newdigest стёрты не будут.

Рассмотрим ещё одну функцию. Отличие от рассмотренного ранее случая заключается в том, что буфер создаётся не в стековой, а в динамической памяти.

static void
_e_icon_smart_del(Evas_Object *obj)
{
  E_Smart_Data *sd;

  if (!(sd = evas_object_smart_data_get(obj))) return;
  evas_object_del(sd->obj);
  evas_object_del(sd->eventarea);
  ....
  evas_object_smart_data_set(obj, NULL);
  memset(sd, 0, sizeof(*sd));                          // <=
  free(sd);
}

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

Software weaknesses type — CWE-14: Compiler Removal of Code to Clear Buffers

Указатель sd после обнуления памяти ещё используется, так как он передаётся в функцию free. Однако это ничего не значит, и компилятор так же вправе удалить для оптимизации вызов функции memset.

С оставшимися 50 предупреждениями, указывающими на ошибки, вы можете ознакомиться в файле Tizen_third_party_V597.txt.

Прочее (227 ошибок)


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

Перечислю типы оставшихся ошибок общим списком:

  • V502. Perhaps the '?:' operator works in a different way than it was expected. The '?:' operator has a lower priority than the 'foo' operator. (1 ошибка)
  • V505. The 'alloca' function is used inside the loop. This can quickly overflow stack. (25 ошибок)
  • V517. The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. (4 ошибки)
  • V519. The 'x' variable is assigned values twice successively. Perhaps this is a mistake. (3 ошибки)
  • V523. The 'then' statement is equivalent to the 'else' statement. (2 ошибки)
  • V528. It is odd that pointer is compared with the 'zero' value. Probably meant: *ptr != zero. (1 ошибка)
  • V547. Expression is always true/false. (10 ошибок)
  • V556. The values of different enum types are compared. (6 ошибок)
  • V571. Recurring check. This condition was already verified in previous line. (1 ошибка)
  • V576. Incorrect format. Consider checking the N actual argument of the 'Foo' function. (1 ошибка)
  • V590. Consider inspecting this expression. The expression is excessive or contains a misprint. (3 ошибки)
  • V593. Consider reviewing the expression of the 'A = B == C' kind. The expression is calculated as following: 'A = (B == C)'. (1 ошибка)
  • V595. The pointer was utilized before it was verified against nullptr. Check lines: N1, N2. (23 ошибки)
  • V601. An odd implicit type casting. (1 ошибка)
  • V609. Divide or mod by zero. (1 ошибка)
  • V610. Undefined behavior. Check the shift operator. (2 ошибки)
  • V636. The expression was implicitly cast from integer type to real type. Consider utilizing an explicit type cast to avoid overflow or loss of a fractional part. (8 ошибок)
  • V640. The code's operational logic does not correspond with its formatting. (1 ошибка)
  • V645. The function call could lead to the buffer overflow. The bounds should not contain the size of the buffer, but a number of characters it can hold. (4 ошибки)
  • V646. Consider inspecting the application's logic. It's possible that 'else' keyword is missing. (2 ошибки)
  • V649. There are two 'if' statements with identical conditional expressions. The first 'if' statement contains function return. This means that the second 'if' statement is senseless. (1 ошибка)
  • V666. Consider inspecting NN argument of the function 'Foo'. It is possible that the value does not correspond with the length of a string which was passed with the YY argument. (6 ошибок)
  • V668. There is no sense in testing the pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. (1 ошибка)
  • V686. There is no sense in testing the pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. (1 ошибка)
  • V690. The class implements a copy constructor/operator=, but lacks the operator=/copy constructor. (1 ошибка)
  • V694. The condition (ptr — const_value) is only false if the value of a pointer equals a magic constant. (2 ошибки)
  • V701. realloc() possible leak: when realloc() fails in allocating memory, original pointer is lost. Consider assigning realloc() to a temporary pointer. (100 ошибок)
  • V760. Two identical text blocks detected. The second block starts with NN string. (1 ошибка)
  • V769. The pointer in the expression equals nullptr. The resulting value is meaningless and should not be used. (5 ошибок)
  • V773. The function was exited without releasing the pointer/handle. A memory/resource leak is possible. (3 ошибки)
  • V779. Unreachable code detected. It is possible that an error is present. (9 ошибок)

Сами предупреждения можно найти в файле Tizen_third_party_other_things.txt.

Промежуточные итоги


Найдено 570 ошибок. В презентации было указано 564, видимо раньше я что-то забыл посчитать. Проанализировано 1915000 строк кода. Из них комментариев — 17,6%.

PVS-Studio обнаруживает 0,36 ошибки на 1000 строк кода. Это означает, что предполагаемая плотность ошибок в сторонних библиотеках чуть-чуть ниже, чем плотность ошибок в коде самого Tizen (анализатор обнаружил 0.41 ошибки на 1000 строк кода).

Почему плотность ошибок в библиотеках немного ниже?

  • Я мог изучать код библиотек менее внимательно и заметил не все ошибки.
  • Некоторые библиотеки уже регулярно проверяются с помощью анализатора Coverity.
  • Это погрешность, вызванная тем, что изучена только небольшая часть проектов.

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

Итоги


Программирование и статический анализ закончились! Пришло время статистики!

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

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

Сводная таблица


Ещё раз перечислю все типы найденных ошибок и их количество:
Диагностика Сообщение Ошибок
V501 There are identical sub-expressions to the left and to the right of the 'foo' operator. 6
V502 Perhaps the '?:' operator works in a different way than it was expected. The '?:' operator has a lower priority than the 'foo' operator. 1
V503 This is a nonsensical comparison: pointer < 0. 2
V505 The 'alloca' function is used inside the loop. This can quickly overflow stack. 26
V507 Pointer to local array 'X' is stored outside the scope of this array. Such a pointer will become invalid. 1
V512 A call of the 'Foo' function will lead to a buffer overflow or underflow. 7
V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. 8
V519 The 'x' variable is assigned values twice successively. Perhaps this is a mistake. 14
V522 Dereferencing of the null pointer might take place. 276
V523 The 'then' statement is equivalent to the 'else' statement. 8
V524 It is odd that the body of 'Foo_1' function is fully equivalent to the body of 'Foo_2' function. 1
V527 It is odd that the 'zero' value is assigned to pointer. Probably meant: *ptr = zero. 1
V528 It is odd that pointer is compared with the 'zero' value. Probably meant: *ptr != zero. 1
V535 The variable 'X' is being used for this loop and for the outer loop. 4
V547 Expression is always true/false. 18
V556 The values of different enum types are compared. 24
V560 A part of conditional expression is always true/false. 2
V571 Recurring check. This condition was already verified in previous line. 2
V572 It is odd that the object which was created using 'new' operator is immediately cast to another type. 4
V575 Function receives an odd argument. 83
V576 Incorrect format. Consider checking the N actual argument of the 'Foo' function. 5
V590 Consider inspecting this expression. The expression is excessive or contains a misprint. 3
V591 Non-void function should return a value. 3
V593 Consider reviewing the expression of the 'A = B == C' kind. The expression is calculated as following: 'A = (B == C)'. 1
V595 The pointer was utilized before it was verified against nullptr. Check lines: N1, N2. 28
V597 The compiler could delete the 'memset' function call, which is used to flush 'Foo' buffer. The RtlSecureZeroMemory() function should be used to erase the private data. 53
V601 An odd implicit type casting. 1
V609 Divide or mod by zero. 1
V610 Undefined behavior. Check the shift operator. 2
V611 The memory allocation and deallocation methods are incompatible. 2
V614 Uninitialized variable 'Foo' used. 1
V618 It's dangerous to call the 'Foo' function in such a manner, as the line being passed could contain format specification. The example of the safe code: printf("%s", str); 6
V622 Consider inspecting the 'switch' statement. It's possible that the first 'case' operator is missing. 1
V624 The constant NN is being utilized. The resulting value could be inaccurate. Consider using the M_NN constant from <math.h>. 2
V636 The expression was implicitly cast from integer type to real type. Consider utilizing an explicit type cast to avoid overflow or loss of a fractional part. 12
V640 The code's operational logic does not correspond with its formatting. 3
V642 Saving the function result inside the 'byte' type variable is inappropriate. The significant bits could be lost breaking the program's logic. 1
V645 The function call could lead to the buffer overflow. The bounds should not contain the size of the buffer, but a number of characters it can hold. 6
V646 Consider inspecting the application's logic. It's possible that 'else' keyword is missing. 4
V647 The value of 'A' type is assigned to the pointer of 'B' type. 1
V649 There are two 'if' statements with identical conditional expressions. The first 'if' statement contains function return. This means that the second 'if' statement is senseless. 1
V666 Consider inspecting NN argument of the function 'Foo'. It is possible that the value does not correspond with the length of a string which was passed with the YY argument. 6
V668 There is no sense in testing the pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. 63
V674 The expression contains a suspicious mix of integer and real types. 1
V675 Writing into the read-only memory. 1
V686 A pattern was detected: A || (A && ...). The expression is excessive or contains a logical error. 2
V690 The class implements a copy constructor/operator=, but lacks the operator=/copy constructor. 8
V692 An inappropriate attempt to append a null character to a string. To determine the length of a string by 'strlen' function correctly, a string ending with a null terminator should be used in the first place. 2
V694 The condition (ptr — const_value) is only false if the value of a pointer equals a magic constant. 2
V696 The 'continue' operator will terminate 'do {… } while (FALSE)' loop because the condition is always false. 2
V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer is lost. Consider assigning realloc() to a temporary pointer. 113
V746 Type slicing. An exception should be caught by reference rather than by value. 32
V755 Copying from unsafe data source. Buffer overflow is possible. 1
V759 Violated order of exception handlers. Exception caught by handler for base class. 9
V760 Two identical text blocks detected. The second block starts with NN string. 1
V762 Consider inspecting virtual function arguments. See NN argument of function 'Foo' in derived class and base class. 6
V769 The pointer in the expression equals nullptr. The resulting value is meaningless and should not be used. 8
V773 The function was exited without releasing the pointer/handle. A memory/resource leak is possible. 6
V774 The pointer was used after the memory was released. 5
V778 Two similar code fragments were found. Perhaps, this is a typo and 'X' variable should be used instead of 'Y'. 2
V779 Unreachable code detected. It is possible that an error is present. 16
V780 The object of non-passive (non-PDS) type cannot be used with the function. 2
V783 Dereferencing of invalid iterator 'X' might take place. 4
V786 Assigning the value C to the X variable looks suspicious. The value range of the variable: [A, B]. 1
Таблица 1. Типы и количество ошибок, найденных в случайно выбранных проектах.

Всего я выявил 914 ошибок. Округлим для простоты до 900 ошибок.

Ложные срабатывания


Я не оценивал количество ложных срабатываний. Дело в том, что не производилась даже минимальная настройка анализатора, поэтому нет смысла пытаться считать процент ложных срабатываний. Это будет просто нечестно по отношению к анализатору. Большинство ложных срабатываний возникает из-за нескольких неудачных макросов. Настроив анализатор, чтобы не ругаться на них, можно в несколько раз сократить количество ложных срабатываний.

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

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

27 000 ошибок


Настал момент, когда станет понятно, почему я заявил о 27000 ошибках.

Всего я проанализировал около 2 400 000 строк кода (без учёта комментариев).

Я обнаружил 900 ошибок.

Весь проект Tizen вместе со сторонними библиотеками насчитывает 72 500 000 строк C и C++ кода (без учёта комментариев).

Это значит, что было проверено только около 3.3% кода.

Прогноз:
(72500000*900/2400000=27187)

Используя анализатор PVS-Studio, мы можем обнаружить и исправить 27 000 ошибок.

Как видите, расчёты совершенно честные и прозрачные.

Заключение


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

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

Предлагаю скачать и попробовать анализатор PVS-Studio.

Поддерживаемые языки и компиляторы:

  • Windows. Visual Studio 2017 C, C++, C++/CLI, C++/CX (WinRT), C#
  • Windows. Visual Studio 2015 C, C++, C++/CLI, C++/CX (WinRT), C#
  • Windows. Visual Studio 2013 C, C++, C++/CLI, C++/CX (WinRT), C#
  • Windows. Visual Studio 2012 C, C++, C++/CLI, C++/CX (WinRT), C#
  • Windows. Visual Studio 2010 C, C++, C++/CLI, C#
  • Windows. MinGW C, C++
  • Windows/Linux. Clang C, C++
  • Linux. GCC C, C++

Всем спасибо за внимание. Приглашаю почитать про анализ других открытых проектов, а также подписывайтесь на мой твиттер @Code_Analysis. С уважением, Андрей Карпов.



Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. 27 000 errors in the Tizen operating system

Прочитали статью и есть вопрос?
Часто к нашим статьям задают одни и те же вопросы. Ответы на них мы собрали здесь: Ответы на вопросы читателей статей про PVS-Studio, версия 2015. Пожалуйста, ознакомьтесь со списком.
Поделиться с друзьями
-->

Комментарии (79)


  1. sborisov
    12.07.2017 11:54
    -17

    Для начала я проверил небольшую часть операционной системы (3.3%) и выписал около 900 предупреждений, указывающих на настоящие ошибки. Если экстраполировать результаты, то получается, что наша команда способна выявить и устранить в Tizen около 27000 ошибок.


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

    Почему то вспомнился анекдот, про жену председателя и доярку, которые в среднем обе б@$ди…


    1. redmanmale
      12.07.2017 12:02
      -6

      1. Andrey2008
        12.07.2017 12:06
        +4

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


        1. redmanmale
          13.07.2017 16:52
          +1

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


          P.S. Мне нравится ваш продукт и ваш подход к его продвижению. И статью я прочитал, как и почти все остальные.


          1. myxo
            14.07.2017 10:20

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

            Вы тоже работаете в этой области, или просто теоретизируете?

            ps. И экстраполяция по определению не может быть верной. Она может быть допустимой.


            1. redmanmale
              14.07.2017 13:08

              Я работаю в области написания этих самых продуктов (в том числе крупных).


              И бывает, что какую-то часть проекта пишет один человек/команда, а другую часть — другой человек/команда. И качество кода может отличаться если не на порядки, то в разы.


              А потому экстраполировать результаты проверки 3.3% проекта на весь проект нельзя. Вполне возможна ситуация, когда в выборку (в статье не указана методика выборки, мы не знаем была ли она репрезентативной) попала только часть кода, написанная условно хорошо, тогда как другая часть написана условно хуже. Или наоброт.


              1. Andrey2008
                14.07.2017 13:56
                +1

                Я использовал метод выборки «пальцем в небо». Очень хороший, честный способ для решаемой задачи.

                В статье перечислены проекты, которые были взяты
                alsa-lib-1.0.28, aspell-0.60.6.1, augeas-1.3.0, bind-9.11.0, efl-1.16.0, enlightenment-0.20.0, ise-engine-anthy-1.0.9, bluetooth-frwk-0.2.157, capi-appfw-application-0.5.5, capi-base-utils-3.0.0, capi-content-media-content-0.3.10, capi-maps-service-0.6.12, capi-media-audio-io-0.3.70, capi-media-codec-0.5.3, capi-media-image-util-0.1.15, capi-media-player-0.3.58, capi-media-screen-mirroring-0.1.78, capi-media-streamrecorder-0.0.10, capi-media-vision-0.3.24, capi-network-bluetooth-0.3.4, capi-network-http-0.0.23, cynara-0.14.10, e-mod-tizen-devicemgr-0.1.69, ise-engine-default-1.0.7, ise-engine-sunpinyin-1.0.10, ise-engine-tables-1.0.10, isf-3.0.186, org.tizen.app-selector-0.1.61, org.tizen.apps-0.3.1, org.tizen.bluetooth-0.1.2, org.tizen.browser-3.2.0, org.tizen.browser-profile_common-1.6.4, org.tizen.classic-watch-0.0.1, org.tizen.d2d-conv-setting-profile_mobile-1.0, org.tizen.d2d-conv-setting-profile_wearable-1.0, org.tizen.download-manager-0.3.21, org.tizen.download-manager-0.3.22, org.tizen.dpm-toolkit-0.1, org.tizen.elm-demo-tizen-common-0.1, org.tizen.indicator-0.2.53, org.tizen.inputdelegator-0.1.170518, org.tizen.menu-screen-1.2.5, org.tizen.myplace-1.0.1, org.tizen.privacy-setting-profile_mobile-1.0.0, org.tizen.privacy-setting-profile_wearable-1.0.0, org.tizen.quickpanel-0.8.0, org.tizen.screen-reader-0.0.8, org.tizen.service-plugin-sample-0.1.6, org.tizen.setting-1.0.1, org.tizen.settings-0.2, org.tizen.settings-adid-0.0.1, org.tizen.telephony-syspopup-0.1.6, org.tizen.voice-control-panel-0.1.1, org.tizen.voice-setting-0.0.1, org.tizen.volume-0.1.149, org.tizen.w-home-0.1.0, org.tizen.w-wifi-1.0.229, org.tizen.watch-setting-0.0.1, security-manager-1.2.17.


      1. sevikl
        12.07.2017 14:25
        +2

        давайте я для вас пересчитаю: в коде tizen из 72,5 миллионов строк есть {900..24468300} ошибок.


    1. Andrey2008
      12.07.2017 12:04
      +18

      Прошу прочитать статью и тогда станет понятно, что этот подход достоверен. В данном случае 3.3% кода это 2 400 000 строк кода (без учёта комментариев). Это много, это размер некоторых проектов. Проверив такой размер кода вполне модно проводить расчеты и получить результат близкий к реальности.


      1. Deosis
        12.07.2017 12:35
        -4

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


        1. Andrey2008
          12.07.2017 12:42
          +7

          Конечно непостоянная. Но переживать об том есть смысл, если мы проверили 1000, 3000 или 10000 строк кода. Или стоит переживать, если проверялся только один проект написанный одной командой. Когда проверено 2400000 строк кода из множества разных проектов, вполне смело можно считать, что полученное значение плотности является средним и для оставшейся части проекта.


      1. vesper-bot
        12.07.2017 12:44
        -4

        Так навскидку, какой доверительный интервал у этой оценки? Тысяча наберется?


        Вообще, молодцы и те и другие — вы, что проверяете, они, что занимаются рефакторингом (пускай и создавая новые баги :D)


    1. RomanArzumanyan
      12.07.2017 13:09
      +11

      ИМХО, выборка была сделана достаточно большая для экстраполяции.


  1. Whatson
    12.07.2017 12:33
    +2

    sborisov наверное хотел услышать общие рассуждения о выборе и количестве репрезентативных строк кода…


    1. SvyatoslavMC
      12.07.2017 12:36
      -2

      В таком случае пошлый анекдот был не очень доходчивым вопросом)


  1. gurux13
    12.07.2017 13:28
    +3

    Интересные ошибки.

    Поясните, пожалуйста, одну:

    Указатель на строку argv[2] является небезопасным источником данных, так как это аргумент командной строки. При формировании строки в буфере temp может произойти выход за границу массива.
    Что argv[2] может быть любым, понятно. Но там же snprintf с правильным размером буфера. Как можно выйти за границу temp?


    1. myxo
      12.07.2017 15:07

      у temp фиксированная длина, и длина argv[2] может оказаться больше

      upd. Ой, невнимательно прочитал, там snprinf. Тогда присоединяюсь к вопросу.


      1. Andrey2008
        12.07.2017 15:48

        Да, какая-то глупость написана в статье. Видимо, выписал какой-то не тот фрагмента кода. Разбираться в причинах уже сил нет, поэтому просто удалил этот фрагмент из статьи и изменил, что находим не 914, а 913 ошибок :).

        P.S. Возможно и ещё что-то окажется не ошибкой при тщательном изучении. Но с другой стороны, при тщательном анализе и наоборот выяснится, что я пропускал некоторые ошибки. Например, я поленился изучать V730 (очень трудоёмко). А там наверняка несколько ошибок скрывается.


  1. technic93
    12.07.2017 13:47
    +2

    Зачем этот кактус с ручным выделением памяти и работой с указателями и си-строками на протяжении в 72 миллиона строчек кода? Может лучше сделайте им презентацию про std::unique_ptr, std::srting и std::vector. Из названий переменных видно что даже какой то шттп на голом си у них работает.


    1. knstqq
      12.07.2017 13:53

      сразу всё на плюсы не перепишут, даже если решат использовать. А так хоть проблем будет меньше


    1. myxo
      12.07.2017 15:06
      +2

      Ок, мы делаем презентацию, а вы выделяете им пару миллионов долларов на переписывание и тестирование кодовой базы.


  1. hermit931
    12.07.2017 14:39
    -1

    Может это не ошибки а фичи :). 27000 фич, хм, звучит круто. Маркетологи будут очень довольны, очень очень.


  1. kafeman
    12.07.2017 15:18

    Функция malloc, если не может выделить память, возвращает NULL. Оператор new при нехватке памяти генерирует исключение std::bad_alloc.
    У меня на паре личных проектов new возвращает NULL, чтобы избежать исключений. По стандарту это, конечно, неправильно, но я и не собираюсь писать свою стандартную библиотеку. Просто я выделяю память по-своему, и оператор new выглядит красивее, чем какой-нибудь my_memory_alloc(sizeof(*foo));

    Учитывая, что вы проверяете ОС — там такая перегрузка операторов очень даже возможна.


    1. Dim0v
      12.07.2017 17:29
      -2

      PVS Studio не пользовался, но мне кажется, она умеет отличать перегруженные операторы от встроенных и по-разному анализировать их использование.


      1. kafeman
        12.07.2017 17:51

        «Встроенного» глобального operator new, на сколько я знаю, ни в одном популярном компиляторе не существует. Как вы предлагаете отличать его от стандартного?


    1. yarric
      12.07.2017 19:41

      А почему не используете RAII?


      1. kafeman
        12.07.2017 20:16

        А как это мне поможет? Допустим, мы «отжали» у системы пару ГБ памяти, чтобы более быстро и эффективно поделить ее между своими объектами. RAII происходит в конструкторе, а объекта еще нет. Можно, наверное, как-то извратиться, но это будет ужасное решение. Например, если this не NULL, то объект находится в стеке, а иначе ему нужно выделить место в куче… Если бы это не был личный проект, коллеги бы меня придушили.

        Возможно, пример не очень удачный (тут, в лучших традициях ООП, «отжатая» память сама должна являться объектом), но глобальная перегрузка оператора new запросто может встретиться в коде ОС.


    1. dpantele
      15.07.2017 09:52

      Кстати, по стандарту есть new(std::nothrow), который как раз возвращает nullptr. Никогда сам его не использовал, правда, но работает: https://ideone.com/Z43jcF


      1. kafeman
        15.07.2017 13:39
        +2

        Об этом написано в посте:

        Если требуется, чтобы оператор new возвращал nullptr, следует использовать nothrow версию оператора:
        Но мне это требуется вообще везде, во всем коде. И с моим выделением памяти это работать не будет.


        1. dpantele
          15.07.2017 17:50

          А зачем так надо? Потому что кажется что если код не рассчитан на то, что вернётся nullptr, то там плохо возвращать nullptr, и уж лучше сразу std::terminate() если исключения нельзя. А если код пишется самостоятельно, то что мешает использовать nothrow-версию?


          1. kafeman
            15.07.2017 18:56
            +1

            Во-первых, что значит «что мешает использовать nothrow-версию»? У меня она всего одна, и она не бросает исключения. Конечно, можно дописать это, но зачем, если у меня вообще во всем коде no-throw guarantee.

            Во-вторых, я использую new, потому что он проще (размер считается автоматически) и короче. Если я начну писать везде (std::nothrow), то в чем будет заключаться выигрыш?

            Кстати, практически любую стандартную библиотеку можно собрать без исключений. Например, вот реализация из LLVM libcxx: (обратите внимание на _LIBCPP_NO_EXCEPTIONS)

            Заголовок спойлера
            __attribute__((__weak__, __visibility__("default")))
            void *
            operator new(std::size_t size)
            #if !__has_feature(cxx_noexcept)
                throw(std::bad_alloc)
            #endif
            {
                if (size == 0)
                    size = 1;
                void* p;
                while ((p = ::malloc(size)) == 0)
                {
                    // If malloc fails and there is a new_handler,
                    // call it to try free up memory.
                    std::new_handler nh = std::get_new_handler();
                    if (nh)
                        nh();
                    else
            #ifndef _LIBCPP_NO_EXCEPTIONS
                        throw std::bad_alloc();
            #else
                        break;
            #endif
                }
                return p;
            }
            
            __attribute__((__weak__, __visibility__("default")))
            void*
            operator new(size_t size, const std::nothrow_t&) _NOEXCEPT
            {
                void* p = 0;
            #ifndef _LIBCPP_NO_EXCEPTIONS
                try
                {
            #endif  // _LIBCPP_NO_EXCEPTIONS
                    p = ::operator new(size);
            #ifndef _LIBCPP_NO_EXCEPTIONS
                }
                catch (...)
                {
                }
            #endif  // _LIBCPP_NO_EXCEPTIONS
                return p;
            }


            1. dpantele
              16.07.2017 10:04

              Получается что libcxx без исключений не соблюдает стандарты? В отличие от libstdc++


              new_handler handler = std::get_new_handler ();
              if (! handler)
                  _GLIBCXX_THROW_OR_ABORT(bad_alloc());
              handler ();

              Ровно такое поведение я ожидал бы увидеть если exceptions запрещены.


              Видимо отклонение от стандарта позволяет писать код быстрее, но потом не надо ожидать что его будет правильно понимать PVS-Studio, когда для семантики return nullptr on fail есть специальный синтаксис, который можно использовать везде вместо обычного new. Так что мне кажется что они в своём праве всегда на это ругаться.


              По сравнению с malloc выигрыш в вызове конструктора и (судя по С++ 17) учёту extra type alignment.


              И я правильно понимаю что с вашим определением operator new стандартные контейнеры совсем не используются? Потому что вроде они никак нулевой указатель обрабатывать не станут, и действительно лучше было сразу std::terminate делать.


              1. kafeman
                16.07.2017 17:17

                Видимо отклонение от стандарта позволяет писать код быстрее, но потом не надо ожидать что его будет правильно понимать PVS-Studio
                Еще раз напоминаю, что проверяется код ОС, где можно встретить все, что угодно. Даже чтение-запись по NULL-указателю не должны вас удивлять.

                И я правильно понимаю что с вашим определением operator new стандартные контейнеры совсем не используются?
                С каких это пор стандартные контейнеры стали прибивать гвоздями к operator new? Там же сплошное мета-программирование, вся работа с памятью идет через отдельные абстракции, которые в итоге могут раскрыться во все, что угодно.

                действительно лучше было сразу std::terminate делать
                Как пользователь, прошу вас, никогда так не делайте! Уже достало, когда нехватка памяти рушит всю программу, вместо отмены запрошенной операции. Например, Tor Browser падает постоянно, хотя было бы достаточно убить пару вкладок. Встречаются графические редакторы, которые также завершаются при попытке импорта больших файлов, хотя достаточно отменить операцию импорта и вывести сообщение об ошибке.


                1. dpantele
                  17.07.2017 05:37
                  +1

                  Еще раз напоминаю, что проверяется код ОС, где можно встретить все, что угодно. Даже чтение-запись по NULL-указателю не должны вас удивлять.

                  Я очень удивлюсь если увижу любую реализацию libstdc++ в которой operator new() возвращает валидный адрес 0x0.


                  А в остальном: во-первых, спасибо за то что было не лень мне отвечать! Интересно подумать что же на самом деле вроде может работать, а что должно быть очень опасно.


                  TLDR: либо в коде должны быть написано new (std::nothrow), либо он не никогда не должен вернуться к исполнению после неуспеха выделения памяти в operator new.


                  Полная версия

                  Безусловно можно в случае нехватки памяти вместо terminate() попытаться сходить осободить кеши и ресурсы, или отменить текущую операцию. Но первое странно выносить за пределы работы с памятью, то есть в приципе это всё можно сделать внутри нашего operator new(). А для второго надо либо реализовывать какой-то вариант корутин (чтобы можно было выкинуть кусок стека который про это ничего не знает), либо код, который это использует, должен быть готов к этому.


                  Cтандартные контейнеры не готовы в целом к тому что allocate() вернёт невалидный указатель. Тот же самый std::list обязательно попытается слинковать ноды, записав что-то в память.


                  Наверное, можно попытаться возвращать в allocate() одинковый для всех, но хотя бы writable и хорошо выравненный кусочек памяти. А для std::vector можно внутри MyAllocator::construct() проверять была ли проблема с выделением памяти и если была то ничего не иницаильнизировать. Но тем самым нарушается всё больше и больше контрактов, и на каком-то этапе это может взорваться.


                  Лучший способ продемнострировать то, что код готов к невалидному указателю — написать лишние 15 символов на каждый new. И всем сразу всё ясно.


                  А если часть кода этого не ожидает, то надо прервать выполнение текущего стека. Это можно сделать разными способами, но я не могу придумать примеров когда это нужно делать за пределами operator new(), сначала вернув оттуда невалидный указатель.


                  1. kafeman
                    17.07.2017 12:05

                    Я очень удивлюсь если увижу любую реализацию libstdc++ в которой operator new() возвращает валидный адрес 0x0.
                    Я очень удивлюсь, если ОС будет использовать обычную user-space библиотеку. Для меня operator new — это, в первую очередь, конструкция языка, и лишь потом одна из реализаций.

                    Безусловно можно в случае нехватки памяти вместо terminate() попытаться сходить осободить кеши и ресурсы <...> Но первое странно выносить за пределы работы с памятью <...>
                    Как раз для этого есть std::new_handler. Он вызывается, когда памяти перестает хватать.

                    А для второго надо либо реализовывать какой-то вариант корутин
                    Обычный паттерн «команда» + какая-нибудь арена, чтобы разом прибить память, затраченную на операцию.

                    Cтандартные контейнеры не готовы в целом к тому что allocate() вернёт невалидный указатель.
                    Получается, Google уже давно должен был бы упасть в segmentation fault? Вот как раз кусок из их реализации арены:

                    pointer allocate(size_type n,
                                     std::allocator<void>::const_pointer /*hint*/ = 0) {
                        assert(arena_ && "No arena to allocate from!");
                        return reinterpret_cast<T*>(arena_->AllocAligned(n * sizeof(T),
                                                                         kAlignment));
                    }

                    AllocAligned вернет NULL, если что-то пошло не так.


                    1. mayorovp
                      17.07.2017 14:29

                      protobuf не выглядит как библиотека которая использует стандартные контейнеры.


                      1. kafeman
                        17.07.2017 14:35

                        Конкретно этот код скопирован из библиотеки, в которой используется STL. В шапке даже есть пример использования их аллокатора с std::vector. Я бы дал ссылку, но, похоже, проект похоронили вместе с Google Code. Ищите гугловский HTML-шаблонизатор для C++.


                    1. dpantele
                      18.07.2017 01:56
                      +1

                      Как раз для этого есть std::new_handler. Он вызывается, когда памяти перестает хватать.

                      Ну да, но это происходит в рамках аллокации, пока что управление находится внутри operator new(), то есть невалидный указатель нам никто не возвращает


                      Обычный паттерн «команда» + какая-нибудь арена, чтобы разом прибить память, затраченную на операцию.

                      То есть вы сами пишете все строки где вызывается operator new() и можно там везде вызвать именно nothrow-версию?


                      AllocAligned вернет NULL, если что-то пошло не так.

                      Это вот этот код?


                      Любопытный пример.


                      AllocAligned() вроде бы является по семантике (см комментарии в arena.h) просто malloc, то есть в конце концов он там ссылается на malloc. Но:


                      • В AllocNewBlock (цепочка AllocAligned()->GetMemory()->GetMemoryFallback()->AllocNewBlock()) есть вызов new std::vector и скорее всего это обычный глобальный new, из которого может прийти bad_alloc
                      • Потом там же есть вызов aligned_malloc и проверка на то что вернулось ненулевое значение.
                      • И там же есть вызов обычного malloc, уже безо всяких проверок.

                      То есть такое впечатление что про это просто не думали c таким ранообразием сценариев обработки этого события. Свидетельством этого же является попытка записать что-то без проверки получилось ли выделить память в классе Gladiator.


                      Вы уверены что оно там не падает в segmentation fault когда память заканчивается?


                      1. kafeman
                        18.07.2017 03:49

                        https://github.com/kafeman/mne_ne_len

                        Сейчас нет времени разбираться, но падать не должно. Этот класс, на сколько я знаю, используется также в других проектах Google.


                        1. dpantele
                          19.07.2017 07:27
                          +1

                          Ну вот стек gdb в момент SIGSEGV


                          Стек
                          Program received signal SIGSEGV, Segmentation fault.
                          0x00000000004044ae in ctemplate::ArenaAllocator<int, ctemplate::UnsafeArena>::construct (this=0x7fffffffda30, p=0x4) at arena-inl.h:109
                          109     new(reinterpret_cast<void*>(p)) T();
                          
                          #0  0x00000000004044ae in ctemplate::ArenaAllocator<int, ctemplate::UnsafeArena>::construct (this=0x7fffffffda30, p=0x4) at arena-inl.h:109
                          #1  0x0000000000404340 in std::allocator_traits<ctemplate::ArenaAllocator<int, ctemplate::UnsafeArena> >::_S_construct<int> (__a=..., __p=0x4)
                              at /usr/include/c++/5/bits/alloc_traits.h:256
                          #2  0x000000000040417c in std::allocator_traits<ctemplate::ArenaAllocator<int, ctemplate::UnsafeArena> >::construct<int> (__a=..., __p=0x4)
                              at /usr/include/c++/5/bits/alloc_traits.h:402
                          #3  0x0000000000403f41 in std::__uninitialized_default_n_a<int*, unsigned long, ctemplate::ArenaAllocator<int, ctemplate::UnsafeArena> > (__first=0x0, 
                              __n=9999999998, __alloc=...)
                              at /usr/include/c++/5/bits/stl_uninitialized.h:623
                          #4  0x0000000000403d13 in std::vector<int, ctemplate::ArenaAllocator<int, ctemplate::UnsafeArena> >::_M_default_append (this=0x7fffffffda30, __n=9999999999)
                              at /usr/include/c++/5/bits/vector.tcc:565
                          #5  0x0000000000403ab3 in std::vector<int, ctemplate::ArenaAllocator<int, ctemplate::UnsafeArena> >::resize (this=0x7fffffffda30, __new_size=9999999999)
                              at /usr/include/c++/5/bits/stl_vector.h:676
                          #6  0x00000000004035bf in test_vector () at main.cc:27
                          #7  0x000000000040374c in main () at main.cc:55


                          1. kafeman
                            19.07.2017 10:13

                            OK, буду смотреть. Части этого кода, с различными изменениями, используются и в других местах. Например, в Chromium, где также есть STL-контейнеры.

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


  1. old_bear
    12.07.2017 15:40

    КДПВ сделала мой день


  1. viru0
    12.07.2017 17:03
    +2

    Как бывший сотрудник Samsung. Дам вам ребят совет, если хотите продать ваш продукт компании(улучшив качество Tizen таким образом)
    Переведите ВСЕ материалы на качественный Корейский. И съездите на какую нибудь конференцию в Сеул(даже самую маленькую) по Tizen с докладом на корейском(желательно чтобы Samsung ее хостил). Вас тогда с руками и ногами оторвут. Все попытки что то сделать на английском и тем более на русском сразу летят в корзину у корейско говорящего менеджера, ибо у него нет времени переводить.


    1. midday
      12.07.2017 17:07
      +1

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


    1. viru0
      12.07.2017 17:18
      +3

      Отправил вашу презентацию бывшему коллеге из Samsung HQ. Очень адекватный парень, может чего и вырастит из этого.


      1. EvgeniyRyzhkov
        12.07.2017 17:23

        Спасибо!


      1. Andrey2008
        12.07.2017 17:23

        Спасибо.


  1. VMichael
    12.07.2017 17:04

    Оффтоп, извините.
    Но все пытаюсь понять вашу картинку.
    Какой то ослик с рогом блюет радугой, которая наверное содержит некий неправильный код?
    Такой посыл?
    А то «запаситесь вкусняшками», а картинка как то не располагает.


    1. SvyatoslavMC
      12.07.2017 17:21
      +1

      Не ослик с рогом, а благородный единорог :D


    1. jaiprakash
      12.07.2017 19:08
      -6

      Именно из-за этого блюющего маскота многие серьёзные фирмы наотрез отказываются работать с pvs.


      1. nikitasius
        12.07.2017 19:31
        +1

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


        Человеческий фактор — он может решать. Года назад были свои терки на хабре (=агрессивные владельцы, отсюда и ниже, про "закрытие статей и школу").


        Если сейчас они изменились к лучшему, то проблем быть не должно с тем, чтобы продать продукт, ибо имейдж не страдает.


  1. nikitasius
    12.07.2017 19:14
    -2

    то второе условие всегда ложно и функция никогда не вернёт значение «es_US»

    Так "рыжий" же деньги на строительство стены выделяет. Скоро и по стране es_US будет не найти.


  1. MCoder
    12.07.2017 20:55

    А можно вот такой вопрос:

    Вы анализируете разные проекты… А отклик-то есть вообще или это чисто для юзеров хабра?

    Интересует например как отреагировали в Qt…


    1. Andrey2008
      12.07.2017 20:58
      +3

      Написание подобных статей, пожалуй, основной способ продвижения анализатора PVS-Studio. Люди читают, пробуют на своих проектах, покупают.
      По поводу Хабра. Этот канал привёл к нам некоторое количество клиентов.
      По поводу Qt не помню. Кажется, поблагодарили и поправили то, что мы нашли.


  1. Areso
    13.07.2017 07:37

    У меня общий вопрос, связанный со статич. анализом кода, но не связанный с PVS-Studio.
    Как-то прочитав очередную вашу статью (я их почти всегда читаю), я решил, что пора. Поскольку пишу для себя на скриптовых ЯПах, нашел стат. анализатор для JS, загрузил туда свой код… и получил просто тонны ошибок и предупреждений. Буквально на каждую строчку (а то и не по одному разу).
    В общем, я так и не смог заставить уйти хотя бы часть этих вещей, потому что при исправлении кода согласно предупреждениям (не все получалось исправить, да), у меня ломалось поведение в браузере.
    Вопрос простой: что делать?


    1. EvgeniyRyzhkov
      13.07.2017 09:06
      +4

      Очевидно, что это не очень хороший анализатор кода. «Шумный».

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


    1. knstqq
      13.07.2017 09:19
      +1

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


      1. Andrey2008
        13.07.2017 09:27

        Да. Как раз недавно писал рассуждения на эту тему: "Философия статического анализатора PVS-Studio".


  1. sieur
    13.07.2017 08:02

    Только хотел купить телевизор Samsung на Tizen, и тут как тут ваша статья.
    Теперь очень крепко подумаю, и скорее всего куплю… хм, LG тормозят, Panasonic российской плохой сборки, ломаются… остаётся Sony. Но их диспели и дизайн что-то не впечатляют. Эх, дилема.

    извиняюсь за оффтоп, для меня выбор ТВ это боль.

    Анализ неплох, подумаю над тем чтобы прогнать свои проекты через анализатор.


    1. SvyatoslavMC
      13.07.2017 08:23

      Где же LG так тормозят, чтобы их не рассматривать? По-моему отличный выбор.


    1. EvilFox
      13.07.2017 15:54

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


  1. gasizdat
    13.07.2017 08:48

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

    и
    У меня ушло около недели на то, чтобы отобрать предупреждения, которые, по моему мнению, указывают на настоящие ошибки
    (и это жалкие 3% от общей кодовой базы).

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


    1. EvgeniyRyzhkov
      13.07.2017 08:58
      +3

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


      1. gasizdat
        14.07.2017 11:17

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


        1. EvgeniyRyzhkov
          14.07.2017 11:21
          +1

          1. Если вы НЕ внедрите статический анализ, то долг будет намного больше.
          2. НИКТО не мешает исправить эти ошибки позже, как будет время. Инструменты позволяют вернутся к «отложенным» ошибкам.


    1. andrew911
      13.07.2017 09:00
      +1

      Один человек за неделю проработал 1/33 огромного проекта, а сколько понадобилось человекочасов написать эти жалкие 3%?
      Я думаю для проекта можно проанализировать все, а потом анализировать только изменения (новый/измененный код).


    1. EvgeniyRyzhkov
      13.07.2017 09:05

      Не мешает. В инструментах типа PVS-Studio есть возможность легко подавить все старые сообщения об ошибках и получать срабатывания только на новый или модифицированный код. Это позволяет получать пользу от анализатора СРАЗУ на проекте любого размера.


      1. gasizdat
        14.07.2017 11:26

        Собственно попытка использовать cppcheck на проекте со сравнительно большим легаси наткнулась на подобную проблему (ну и ложно положительные ошибки еще). Не обнаружив с ходу аналогичной описанной вами фишки, пришлось его пока исключить из сборки. Так что, увы, проект медленно накапливает техдолги.


        1. EvgeniyRyzhkov
          14.07.2017 11:28

          А это ответ на вопрос некоторых комментаторов, которые говорят, что говно этот ваш PVS-Studio — лучше взять cppcheck или вообще clang.


    1. Am0ralist
      13.07.2017 11:47

      Действительно, поддерживать же такие проекты и править баги по неточным репортам пользователей (да еще прошедших через не всегда адекватную ТП) сильно легче, видимо. Особенно новым разработчикам, который его не писали — ведь для ни порог вхождения будет не в ту же гору высотой.
      Хотя тут уже было несколько статей по поводу настройки данного анализатора для того, чтоб начать с ним работать.


  1. tempico
    13.07.2017 12:22

    27к ошибок — выглядит как пасхалка, как бы намекает на соответствие ISO 27000 :)

    Было интересно почитать, спасибо!


  1. netch80
    15.07.2017 08:42

    > Создаются структуры типа sockaddr_un и sockaddr_in. При этом хранятся и уничтожаются они как структуры типа sockaddr.
    > Надо понимать, что рассмотренный код очень опасен и достаточно, чтобы в одном из классов появился конструктор/деструктор или был добавлен член сложного типа (например std::string), как всё сразу сломается окончательно.

    Появление конструктора в sockaddr_* это из области невероятной фантастики — это гарантированно плоские структуры данных.
    Код может быть тут сколько угодно формально некорректным, исходя из формальности типа «если new, то вероятен конструктор», но сама эта формальность неуместна.
    Более того, если бы это случилось, то удаление объекта через указатель на базовый класс с виртуальным деструктором — нормальная ситуация: если B и C — потомки A, а A имеет виртуальный деструктор, то код вида

    A* a1 = new B();
    A* a2 = new C();
    ...
    delete a1;
    delete a2;
    

    законен.

    Так что тут ваша проверка некорректна, рекомендую исправить. Для семейства sockaddr лучше вообще добавить исключение (работа через sockaddr* — общее правило BSD sockets API), а в общем случае ловить удаление через базовый класс без виртуального конструктора.

    А вот что я бы рекомендовал добавить — именно для sockaddr_* (всех адресных семейств) — это или использование calloc(), или заполнение структуры нулевыми байтами перед использованием. Отсутствие такого заполнения может приводить к странным эффектам при переносе между ОС: например, для BSD семейства (включая MacOS) sin_len (отсутствующее в Linux) должно быть или 0, или актуального размера.
    Также это касается ряда других интерфейсных структур — с ходу вспоминается sigaction. Можно вообще поставить общим правилом для ядерных структур.

    > Для таких случаев есть стандартный макрос M_PI, который вдобавок раскрывается в более точное значение.

    M_PI не определён ни в стандарте C, ни в базовом Posix; есть в Posix расширении XSI и в Microsoft SDK. Утверждение, что он стандартен, распространено, но неверно. Эта диагностика также требует исправления.

    > snprintf(buf + strlen(buf), sizeof(buf),

    Против такого лучший идиоматический приём выглядит примерно так:

    // удобно, потому что blimit уже сдвигаться не будет
    const char *blimit = buf + sizeof buf;
    // а вот этот едет каждый раз
    const char *bpos = buf;
    ...
    r = snprintf(bpos, blimit - bpos, порция1);
    // привет Шлемиэлю, но его подвиг мы не повторяем
    // также компилятор может соптимизировать blimit-bpos в одно вычисление
    bpos += min(r, blimit - bpos);
    r = snprintf(bpos, blimit - bpos, порция2);
    bpos += min(r, blimit - bpos);
    ...
    


    Хотя для короткого буфера и двух записей хватит и такого strlen.

    > Здесь забыты фигурные скобки.

    Вообще я бы рекомендовал настаивать на включении тел if, else, while в фигурные скобки в принудительном порядке везде и всегда. Новые языки (Swift, Go) это уже делают в основах синтаксиса.

    > argv2[i] = NULL;

    > Не будут обнулены указатели.
    > NULL будет записан за границу массива.

    Обнуление не обязательно. NULL, скорее всего, не будет записан за границу, потому что массив argv в Unix-стиле передачи аргументов другим процессам всегда заканчивается ещё одним NULL, не указанным в arg_count (argc — для main).
    Тут жалоба, я уверен, ложна, хотя определить это по самому коду может быть слишком сложно для анализатора.

    > The 'strncat' function call could lead to the 'dd_info->object_uri' buffer overflow.

    И опять strlen (см. выше), но strncat это вообще диверсия почти всегда.
    В случае Unix мира лучше рекомендовать переходить на strlcat, а в общем случае — на strncat_s.

    > Предупреждение PVS-Studio: V591 Non-void function should return a value.

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


    1. Andrey2008
      15.07.2017 10:53

      Примечание. Я постоянно сталкиваюсь с желанием программистов заявить, что это возможно не баг, а фича :). Пример с reddit. Мне это непонятно, зачем пытаться оправдать явно плохой, опасный и ошибочный код? В чем профит? :)

      Теперь про разные моменты.

      1) sockaddr_
      Не важно, появится там конструктор или нет. Код неправильный. Нельзя создавать объект одного размера, а уничтожать как объект другого размера. Да везёт, что пока всё это работает и я не могу продемонстрировать обратное (быть может кто-то поможет). Сбой может произойти по разным причинам. Например, менеджер памяти реализован так, что хранит информацию об объектах разного размера в разных таблицах и использовать разные пулы памяти. И тогда, он может не найти запись об выделенной памяти в соответствующей таблице.
      Или менеджер памяти особо-секьюрный и обнуляет освобождаемую память. А тут ему подсовывают объект не того размера.
      Так что анализатор ведёт себя правильно и ничего в нём исправлять не надо. Надо исправлять код программы.
      Вы приводите пример и говорите об наследовании с виртуальными деструкторами. Это совсем другая ситуация и в подобном случае естественно анализатор промолчит. Я ведь специально отметил в статье, что эти 3 класса никак не связаны между собой.

      2) M_PI
      Согласен, местами его нет. Но раз в других местах используется M_PI, то не вижу смысла городить велосипед. Первый попавшийся M_PI в C-файле (проект org.tizen.myplace-1.0.1, файл myplace-suggestedview.c):

      static inline double __rad_2_deg(double radians)
      {
      	return radians * (180.0 / M_PI);
      }


      3) argv2[i] = NULL;
      Это не тот argv, который приходит в main. Это нечто самобытное:
                    ....
      	argc = malloc(sizeof(int));
      	argv = malloc(sizeof(gchar *) *max_argc);
      	argv2 = malloc(sizeof(gchar *) *max_argc);
      
      	if (!argc || !argv || !argv2)
      		goto ERROR;
      
      	memset(argv, 0, sizeof(gchar *) *max_argc);
      	memset(argv2, 0, sizeof(gchar *) *max_argc);
                    ....
      


      4) Non-void function should return a value
      Я неприятно удивлю, но компиляторы до сих пор проглатывают такой код. Примеры.


      1. netch80
        15.07.2017 11:02

        > Например, менеджер памяти реализован так, что хранит информацию об объектах разного размера в разных таблицах и использовать разные пулы памяти.

        Да, это аргумент (хотя такой реализации для C/C++ ни разу вживую не видел).
        Тогда это надо переделать на malloc/free. Использование же через общий sockaddr* остаётся основным методом.

        > Так что анализатор ведёт себя правильно и ничего в нём исправлять не надо.

        Но про контроль обнуления структур рекомендую таки подумать.

        > Это не тот argv, который приходит в main. Это нечто самобытное:

        Я и не говорил, что это входной argv данного процесса. Это, скорее всего, выходной, для передачи в какой-то из exec*(). И он обязан завершаться NULL?ом, иначе дочерний процесс не запустится правильно.

        > Я неприятно удивлю, но компиляторы до сих пор проглатывают такой код.

        MSVC, да?


        1. mayorovp
          15.07.2017 11:51
          +1

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


  1. gurev_alexey
    16.07.2017 15:53
    -4

    Поддерживает ли C# анализатор у PVS-Studio data-flow-analysis?


    1. Andrey2008
      16.07.2017 15:53
      +4

      Да.


      1. gurev_alexey
        16.07.2017 17:23
        -6

        Интересно.
        Либо вы не понимаете, что стоит за словами data-flow-analysis, либо просто нагло врете.
        Просто ради интереса загрузил с вашего сайта анализатор и декомпилировал первой попавшийся под руку тулзой.
        Никакого анализа потока данных у вас не вижу абсолютно.
        Ожидал, что хоть хваленный механизм виртуальных значений будет честно строится, но нет…
        Все работает крайне наивным способом, разбито на какие-то частные случаи, проходах по AST, и с кучей дублирования кода.
        Я не буду тут приводить участки кода, просто скажу, что после увиденного, рекомендовать и уж тем более покупать ваш продукт отпало сразу.