Эта статья продемонстрирует, что при разработке крупных проектов статический анализ кода является не просто полезным, а совершенно необходимым элементом процесса разработки. Я начинаю цикл статей, посвященных возможности использования статического анализатора кода PVS-Studio для повышения качества и надежности операционной системы Tizen. Для начала я проверил небольшую часть операционной системы (3.3%) и выписал около 900 предупреждений, указывающих на настоящие ошибки. Если экстраполировать результаты, то получается, что наша команда способна выявить и устранить в Tizen около 27000 ошибок. По итогам проведённого исследования я подготовил презентацию, которая предназначалась для демонстрации представителям Samsung и была посвящена возможному сотрудничеству. Встреча перенесена на неопределённый срок, поэтому я решил не тратить время и трансформировать материал презентации в статью. Запасайтесь вкусняшками и напитками, нас ждёт длинный программистский триллер.
Начать, пожалуй, следует со ссылки на презентацию "Как команда PVS-Studio может улучшить код операционной системы Tizen", из которой и родилась эта статья: RU - pptx; EN - pptx. Впрочем, смотреть презентацию вовсе не обязательно, так как весь содержащийся в ней материал будет рассмотрен здесь, причём более подробно. Тема презентации пересекается с открытым письмом, в котором мы также рассказываем, что готовы поработать с проектом Tizen.
С разными воспоминаниями и ссылками я закончил и перехожу к сути дела. Первое, что стоит сделать, это напомнить читателю, что вообще из себя представляет операционная система Tizen.
Tizen - открытая операционная система на базе ядра Linux, предназначенная для широкого круга устройств, включая смартфоны, интернет-планшеты, компьютеры, автомобильные информационно-развлекательные системы, "умные" телевизоры и цифровые камеры, разрабатываемая и управляемая такими корпорациями, как Intel и Samsung. Поддерживает аппаратные платформы на процессорах архитектур ARM и x86. Более подробная информация доступна на сайте Wikipedia.
Платформа Tizen показывает уверенный рост в течение нескольких последних лет, несмотря на насыщенность рынка операционных систем для мобильных и носимых устройств. Согласно отчёту Samsung, рост продаж мобильных телефонов с OC Tizen составил 100% в 2017 году.
Для нашей команды операционная система 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 просто не могла пройти мимо такого интересного открытого проекта, не попробовав его проверить.
Целью презентации, о которой я упоминал ранее, было продемонстрировать, что анализатор PVS-Studio находит очень много ошибок различного типа. Это своего рода резюме анализатора и нашей команды, которое мы хотим показать компании Samsung.
Читатель может засомневаться, стоит ли ему читать такую "статью-резюме". Да, стоит, здесь будет много интересного и полезного. Во-первых, лучше учиться на чужих ошибках, а не на своих. Во-вторых, статья отлично показывает, что методологию статического анализа просто необходимо применять в проектах большого размера. Если кто-то из коллег, занятых в большом проекте, уверяет вас, что они пишут код высокого качества и почти без ошибок, то покажите ему эту статью. Я не думаю, что создатели Tizen хотели, чтобы в проекте было много дефектов, но вот они - тысячи багов.
Как всегда, хочу напомнить, что статический анализ кода должен применяться регулярно. Разовая проверка 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 предупреждений анализатора, тоже мне достижение, это даже мало". Поэтому ещё раз подчеркну, что речь идет именно об ошибках. В процессе исследования я считал и выписывал именно ошибки, а не просто все предупреждения анализатора без разбора.
Для проверки я случайным образом выбрал следующие проекты: 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 исключительно для удобства тех читателей, которые привыкли именно к этой классификации дефектов. Моя основная цель - найти как можно больше ошибок, а определение того, насколько опасна ошибка с точки зрения безопасности, выходит за рамки моего исследования.
Это будет большой раздел, поэтому предлагаю заварить первую кружку чая или кофе. Вторая нам понадобится позже, так как мы только в самом начале статьи.
Классика. Даже так: классика самого высшего сорта!
Во-первых, эта ошибка выявляется с помощью диагностики 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
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
Рассмотрим код, который хотя и плох, но на практике не приведёт к проблемам. Я не рассматривал эту ситуацию в презентации, так как она требует специального пояснения. Теперь же, в статье, я могу остановиться на данном примере и поделиться своими мыслями.
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);
}
}Следующий код также относится к тестам, но ошибка в нем гораздо серьезнее. Из-за ошибки возникает неопределённое поведение программы, поэтому доверять такому тесту совершенно нельзя. Другими словами - тест ничего не тестирует.
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
Функция возвращает адрес массива, созданного на стеке. После завершения функции этот массив будет уничтожен, и возвращенный из функции адрес никак нельзя использовать.
Для начала рассмотрим случай, когда обрабатывается меньше элементов, чем требуется.
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 найденные ошибки:
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.
Другие ошибки:
Рассмотрим ошибку в логике работы программы. Разработчик хотел обменять значения двух переменных, но запутался и у него получился следующий код:
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.
Другие ошибки:
При просмотре отчета анализатора я выделил только 11 мест, которые, на мой взгляд, следует исправить. Но на самом деле сообщений V519 было намного больше. Очень часто они относились к коду, когда в переменную много раз подряд сохранялся результат после вызова функций. Речь идёт о коде вида:
status = Foo(0);
status = Foo(1);
status = Foo(2);Такой код возникает, как правило, в двух случаях:
Про этот момент я пишу, так как, возможно, не везде такой код безопасен, как кажется на первый взгляд. Вероятно, в некоторых местах результат, который вернули функции, зря обделён вниманием и не проверен. Я смотрел код быстро и не разбирался, как и что устроено. Думаю, если подойти к этим предупреждениям внимательнее, то можно будет обнаружить гораздо больше дефектов.
Использование нулевых указателей выявляется с помощью диагностик 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:
Software weaknesses type - CWE-690: Unchecked Return Value to NULL Pointer Dereference
Указатели temp1 и temp2 могут оказаться нулевыми, если в строке не обнаружатся символы '-' и '.'. В этом случае чуть позже произойдёт разыменование нулевого указателя.
Есть ещё 84 участка кода, где разыменовываются указатели, которые могут оказаться равны NULL. Рассматривать их в статье нет никакого смысла. Нет смысла даже приводить их просто списком, так как это все равно займёт много места. Поэтому я выписал эти предупреждения в отдельный файл: Tizen_V522_V575.txt.
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. Думаю, возвращаемые значения должны различаться.
Другие ошибки этого типа:
Очень красивая ошибка: пишем данные неизвестно куда.
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';Есть много причин, приводящих к ошибке, из-за которой условие всегда является истинным или ложным, но в статье для краткости я рассмотрю только 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 или 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.
Другие ошибки:
В презентации этот вид ошибки я пропустил, так как примеры слишком длинные. Но в статье, я думаю, есть смысл вспомнить про них.
Есть два 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 предупреждений:
Software weaknesses type - CWE-697: Insufficient Comparison
Что забавно - этот код работает именно так, как задумывал программист. Благодаря везению, константа WIFI_MANAGER_RSSI_LEVEL_0 равна WIFI_RSSI_LEVEL_0 и так далее.
Несмотря на то, что сейчас код работает, это ошибка и её следует исправить. На это две причины:
Другие неправильные сравнения:
Я заметил всего 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)Объявлены три структуры, никак не связанные между собой:
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; // <=
}
};Предупреждения анализатора:
Software weaknesses type - CWE-762: Mismatched Memory Management Routines.
Создаются структуры типа sockaddr_un и sockaddr_in. При этом хранятся и уничтожаются они как структуры типа sockaddr. Типы всех трёх названных структур никак не связаны между собой. Это три разные структуры, имеющие разный размер. Сейчас код вполне может работать, так как структуры являются POD типами (не содержат деструкторы и т.д.) и вызов оператора delete превращается в простой вызов функции free. Однако формально код неверен. Нужно уничтожать объект точно такого же типа, который использовался при создании объекта.
Как я уже сказал, сейчас программа может работать, хотя формально и является неправильной. Надо понимать, что рассмотренный код очень опасен и достаточно, чтобы в одном из классов появился конструктор/деструктор или был добавлен член сложного типа (например std::string), как всё сразу сломается окончательно.
Другие ошибки:
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 никак не используется. Видимо номер ошибки забыли распечатать.
Другие ошибки:
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 дважды разыменовывается в выражениях:
И только затем указатель проверяется на равенство нулю. Чтобы исправить код, проверку следует передвинуть выше:
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)", которая свидетельствует о том, что этот указатель может быть нулевым.
Другие ошибки:
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).
Первая ошибка.
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, что недопустимо.
Тело функции _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 дефектов.
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
Код работает корректно, но он очень ненадёжен и опасен по двум причинам:
В любом случае, в операционной системе, претендующей на звание надежной, такого кода быть не должно, тем более что ситуацию очень легко исправить. Достаточно написать так:
snprintf(buf, BUF_LEN, "%s", gettext("IDS_ST_SK_CANCEL"));Другие слабые места:
#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:
Software weaknesses type - не знаю как классифицировать, буду благодарен за подсказку.
Признаю, что данную ситуацию можно признать ошибочной только с очень большой натяжкой. Точности константы 3.141592 более чем достаточно для любых практических расчётов.
Тем не менее, я считаю, что этот код надо модифицировать. Макрос PI - это лишняя сущность, которой просто не должно быть. Для таких случаев есть стандартный макрос M_PI, который вдобавок раскрывается в более точное значение.
__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.
Другие ошибки:
В первом случае, несмотря на ошибку, код работает правильно. Да, бывают такие счастливые совпадения.
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;Результат:
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" означает любые числа, а вовсе не 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(...);
}Вернемся к проекту Tizen. Мне кажется, в рассмотренном фрагменте кода пропустили оператор отрицания '!'. Тогда корректный код должен быть таким:
else if (!strcmp(part, STATE_NOT_BROWSER))
return !strcmp(id, APP_ID_BROWSER);#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
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. Необъявленные функции в Си.)
Функция 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 - даже не знаю как классифицировать. На мой взгляд, здесь сразу подходят три варианта:
Предполагается, что если не удастся выделить память для массива символов, то дескриптор файла будет закрыт и функция вернёт статус 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.
Код длинный, но я не буду его форматировать для статьи, т.к. хочется показать фрагмент программы таким, каков он есть. Поэтому я приведу картинку (можно нажать на картинку для её увеличения).
Предупреждение 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;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.
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
Анализатор выдаёт предупреждения 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, то произойдёт утечка памяти.
Другие ошибки:
В начале рассмотрим три используемые функции. Для нас важно, что все они возвращают указатель на выделенную память.
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:
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 утечки памяти.
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.
Иногда, прежде чем сгенерировать исключение, в лог записывается информация, облегчающая отладку приложений. Вот как выглядит правильный код:
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
Макрос-эпилог не используется. Две последние строки в функции следует поменять местами.
Другие ошибки:
Для начала рассмотрим объявление некоторых типов данных.
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. В результате:
Вообще, рассуждать, как будет работать этот код, не имеет смысла. Тут сплошные 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 ошибки, описание которых я опущу. Эти ошибки, на мой взгляд, неинтересные, или придётся приводить слишком много кода для их демонстрации, а я уже устал. Тем более, что и без них статья получается угрожающего размера, а я ведь хотел ещё немного поговорить об ошибках в сторонних библиотеках. Поэтому перечислю типы оставшихся ошибок общим списком.
Сами предупреждения можно найти в файле 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. Теперь нас ждут внешние библиотеки. Им я уделю меньше внимания, но всё равно до конца статьи ещё далеко, поэтому пришло время второй чашечки кофе/чая.
Сторонними проектами я посчитал проекты, в которых не сказано явно, что они созданы компанией 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.
Проектов гораздо меньше по количеству, но зато они в несколько раз крупнее рассматриваемых ранее. Суммарный размер проектов, перечисленных здесь, по размеру больше, чем суммарный размер кода проектов, рассматриваемых в предыдущей главе.
Уверен, читатель понимает, что если я буду так же подробно рассматривать ошибки, то статья превратится в книгу. Поэтому я ограничусь рассмотрением совсем небольшого количества ошибок, которые мне показались наиболее заслуживающими внимания.
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 умеет выявлять различные опечатки.
Другие ошибки:
В предыдущей главе мы обсуждали разыменование нулевых указателей. Но речь шла только о потенциально нулевых указателях, которые возвращали такие функции, как 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.
Следующая ошибка интересна тем, что она находится в патче, который разработчики 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.
Другие ошибки:
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;
}
....
}Предупреждения анализатора:
Software weaknesses type - CWE-416: Use after free
Этот код совершенно неправильный, но иногда он может работать.
После успешного вызова функции realloc указатель child становится невалидным и его больше нельзя использовать.
Почему тогда я говорю, что временами это работает? Дело в том, что менеджер памяти может возвращать тот же адрес буфера, что и был раньше. То есть просто размер буфера увеличивается без изменения его адреса. Так менеджер памяти оптимизирует скорость, так как не требуется копировать данные из старого буфера в новый.
Другие ошибки:
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. Это значит, что во время выполнения второго цикла ничего удаляться не будет, и возникнут множественные утечки памяти.
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]) приведёт к неприятным последствиям.
Другие ошибки:
Сделал интересное наблюдение. Если в просмотренном коде, написанном в 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.
Осталось ещё множество неописанных ошибок в коде, однако, я уверен, читатель согласится, что пора заканчивать. Я проделал кропотливую работу и представил её результаты в этой статье слоновьего размера. Да, что-то интересное осталось за кадром, но что делать.
Перечислю типы оставшихся ошибок общим списком:
Сами предупреждения можно найти в файле Tizen_third_party_other_things.txt.
Найдено 570 ошибок. В презентации было указано 564, видимо раньше я что-то забыл посчитать. Проанализировано 1915000 строк кода. Из них комментариев - 17,6%.
PVS-Studio обнаруживает 0,36 ошибки на 1000 строк кода. Это означает, что предполагаемая плотность ошибок в сторонних библиотеках чуть-чуть ниже, чем плотность ошибок в коде самого Tizen (анализатор обнаружил 0.41 ошибки на 1000 строк кода).
Почему плотность ошибок в библиотеках немного ниже?
Поэтому не стоит придавать большое значение этой разнице. Можно сказать, что плотность ошибок для двух рассмотренных групп проектов приблизительно равна.
Программирование и статический анализ закончились! Пришло время статистики!
Для тех, кто пролистал статью не читая, повторю, что здесь речь идёт не о количестве предупреждений, выданных анализатором, а о настоящих ошибках. И когда я говорю, что в ходе исследования мной найдено 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 |
|
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. Типы и количество ошибок, найденных в случайно выбранных проектах.
Всего я выявил 913 ошибок. Округлим для простоты до 900 ошибок.
Я не оценивал количество ложных срабатываний. Дело в том, что не производилась даже минимальная настройка анализатора, поэтому нет смысла пытаться считать процент ложных срабатываний. Это будет просто нечестно по отношению к анализатору. Большинство ложных срабатываний возникает из-за нескольких неудачных макросов. Настроив анализатор, чтобы не ругаться на них, можно в несколько раз сократить количество ложных срабатываний.
По личному ощущению, ложных срабатываний немного. Если бы это было не так, я бы не смог в одиночку так быстро провести такое обширное исследование.
Плюс отмечу, что количество ложных срабатываний вообще не имеет значения. Если мы начнем сотрудничество, ложные срабатывания будут в первую очередь головной болью нашей команды, а не разработчиков, трудящихся над Tizen.
Настал момент, когда станет понятно, почему я заявил о 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.
Поддерживаемые языки и компиляторы:
Всем спасибо за внимание. Приглашаю почитать про анализ других открытых проектов, а также подписывайтесь на мой твиттер @Code_Analysis. С уважением, Андрей Карпов.