Открытое письмо разработчикам операционной системы Tizen от Андрея Карпова, представляющего команду анализатора PVS-Studio. Наша команда готова взяться за улучшение качества кода проекта Tizen. В тексте присутствуют замечания к фрагментам кода, но это не является критикой кода. В любом проекте есть ошибки. Цель замечаний - показать на примерах, что мы ведём речь не об абстрактных рекомендациях по улучшению кода, а о настоящих дефектах, которые мы можем находить и исправлять.
В начале кратко представлюсь: Андрей Карпов, CTO, один из разработчиков статического анализатора кода PVS-Studio.
Евгений Рыжков (CEO) обратился ко мне с просьбой найти 10 ошибок в исходном коде операционной системы Tizen с помощью анализатора PVS-Studio и описать их. На базе этих ошибок он попросил написать письмо, демонстрирующее разработчикам Tizen, что анализатор PVS-Studio может быть полезен в их работе. Другими словами, эти ошибки - повод для дальнейшего общения и сотрудничества.
PVS-Studio находит много дефектов в коде Tizen, поэтому выписывать их не составляет труда. И я решил выписать для начала не 10, а 20 фрагментов кода с ошибками. При этом постарался выбирать различные паттерны ошибок, но всё равно некоторые типы дефектов остались за кадром.
Я знаю, что компания Samsung уделяет большое внимание качеству и надёжности операционной системы Tizen. Поэтому я уверен, что анализатор PVS-Studio может стать отличным дополнением к процессу разработки этой системы.
Предлагаю команде Tizen рассмотреть возможность привлечения команды PVS-Studio к работе над проектом Tizen.
Я вижу два варианта возможного сотрудничества:
Как я уже сказал, анализатор PVS-Studio выявляет гору дефектов в коде Tizen. Некоторые из них, в скором времени, я опишу в новой статье, как мы это обычно делаем в рамках проверки различных открытых проектов. Хотя в случае с Tizen, это, скорее, будет не статья, а цикл статей.
Сейчас просто разминка. Мы проверили только пару десятков небольших проектов, входящих в состав Tizen, а их общее число составляет несколько сотен. И я предлагаю познакомиться с 20 ошибками. 15 относятся к коду, созданному непосредственно командой разработчиков Tizen. И ещё 5 примеров относятся к сторонним библиотекам, содержащим сотни патчей для Tizen. На мой взгляд, качество используемых библиотек не менее важно, чем код самой операционной системы. В конце концов, пользователям всё равно - происходит утечка памяти в коде ОС или в одной из используемых библиотек.
Примечание. Дополнительно см. презентацию: RU: pptx, slideshare; EN: pptx, slideshare.
Примеры дефектов будут оформлены следующим образом:
Фрагмент N1
org.tizen.browser-3.2.0
CWE-675 Duplicate Operations on Resource
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
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); // <= m_navigatorToolbar
}
....
}Код деструктора писался методом Copy-Paste. Случайно в одном месте забыли заменить m_modulesToolbar на m_navigatorToolbar.
Фрагмент N2
org.tizen.download-manager-0.3.21
CWE-193 Off-by-one Error
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
#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));
....
}Типовая ошибка использования функции 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);Фрагмент N3
org.tizen.indicator-0.2.53
CWE-571 Expression is Always True
V547 Expression 'strlen(s_formatted) < 128' is always true. clock.c 503
#define CLOCK_STR_LEN 128
void indicator_get_time_by_region(char* output,void *data)
{
....
char s_formatted[CLOCK_STR_LEN] = { 0, };
....
if (strlen(s_formatted) < CLOCK_STR_LEN) {
strncpy(output, s_formatted, strlen(s_formatted));
}
else {
strncpy(output, s_formatted, CLOCK_STR_LEN - 1);
}
return;
}Условие strlen(s_formatted) < CLOCK_STR_LEN всегда истинно, ведь количество символов в строке всегда меньше, чем размер буфера в котором она содержится. Если условие всё же не выполняется, то это означает, что произошла запись за границу буфера, и что-то проверять всё равно уже поздно. Можно сделать вывод, что с логикой работы здесь не всё в порядке.
Фрагмент N4
CWE-697 Insufficient Comparison
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;
}
}Константы WIFI_RSSI_LEVEL_* относятся к перечислению типа wifi_rssi_level_e. При этом переменная level имеет тип wifi_manager_rssi_level_e.
Благодаря везению:
Код работает, как и ожидал программист, но при этом некорректен.
Фрагмент N5
org.tizen.screen-reader-0.0.8
CWE-401 Improper Release of Memory Before Removing Last Reference ('Memory Leak')
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);
}
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);
}
....
}Если происходит ошибка, то не высвобождается 3 области памяти, на которую ссылаются указатели: role_name, description_from_role, state_from_role.
Фрагмент N6
org.tizen.screen-reader-0.0.8
CWE-131 Incorrect Calculation of Buffer Size
V512 A call of the 'snprintf' function will lead to overflow of the buffer 'buf + strlen(buf)'. app_tracker.c 450
static void _on_atspi_event_cb(const AtspiEvent * event)
{
....
char buf[256] = "\0";
....
snprintf(buf + strlen(buf), sizeof(buf),
"%s, ", _("IDS_ACCS_BODY_SELECTED_TTS"));
....
}Функцию snprintf обманывают, говоря ей что в буфере больше места, чем есть на самом деле. Правильно:
snprintf(buf + strlen(buf), sizeof(buf) - strlen(buf),
"%s, ", _("IDS_ACCS_BODY_SELECTED_TTS"));Фрагмент N7
org.tizen.screen-reader-0.0.8
CWE-131 Incorrect Calculation of Buffer Size
V512 A call of the 'snprintf' function will lead to overflow of the buffer 'trait + strlen(trait)'. navigator.c 514
#define HOVERSEL_TRAIT_SIZE 200
void add_slider_description(....)
{
....
char trait[HOVERSEL_TRAIT_SIZE] = "";
....
snprintf(trait + strlen(trait), HOVERSEL_TRAIT_SIZE,
", %s", _IGNORE_ON_TV("IDS_......."));
....
}Дефект аналогичен предыдущему. Разница только в том, что размер буфера вычисляется не с помощью оператора sizeof, а задаётся константой HOVERSEL_TRAIT_SIZE.
Фрагмент N8
org.tizen.setting-1.0.1
CWE-570 Expression is Always False
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
EXPORT_PUBLIC
int get_popup_btn_response_type(Evas_Object *obj)
{
....
if (0 == safeStrCmp(btn_str, _("IDS_CST_BUTTON_CLOSE"))
|| 0 == safeStrCmp(btn_str, _("IDS_SAPPS_SK_TRY_ABB"))
|| 0 == safeStrCmp(btn_str, _("IDS_ST_BUTTON_OK")) // <=
|| 0 == safeStrCmp(btn_str, _("IDS_ST_BUTTON_OK")) // <=
|| 0 == safeStrCmp(btn_str, _("IDS_ST_SK_YES"))
|| 0 == safeStrCmp(btn_str, _("IDS_ST_BUTTON_STOP"))
....
}Часть условия всегда ложное, так как этот случай уже проверялся раньше. Возможно, из-за этой опечатки забыли проверить что-то другое.
Фрагмент N9
org.tizen.setting-1.0.1
CWE-570 Expression is Always False
EXPORT_PUBLIC bool get_substring_int(....)
{
const char *str = *ipStr;
....
if (str[1] == '\0') { // <= 1
str++;
*ipStr = str;
return TRUE;
} else if (str[1] == delim) {
str += 2;
*ipStr = str;
return TRUE;
} else if (str[1] == 0) { // <= 1
if (str[2] == 0) { // <= 2
str += 3;
*ipStr = str;
return TRUE;
} else if (str[2] == '\0') { // <= 2
str += 2;
*ipStr = str;
return TRUE;
} else {
str += 2;
}
....
}В логике работы этой функции что-то не так. Выделю суть проблемы:
if (str[1] == '\0') {
} else if (str[1] == 0)
if (str[2] == 0) {
} else if (str[2] == '\0') {Два однотипных случая бессмысленных повторных проверок. Один и тот же символ сравниваем с 0 и с '\0'.
Фрагмент N10
org.tizen.setting-1.0.1
CWE-762 Mismatched Memory Management Routines
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
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;
}
....
}Буфер, выделенный на стеке с помощью функции alloca, может быть передан в функцию free.
Фрагмент N11
org.tizen.setting-1.0.1
V779 Unreachable code detected. It is possible that an error is present. setting-common-view.c 124
#define SETTING_TRACE_END do {\
SECURE_LOGD("\033[0;35mEXIT FUNCTION: %s. \033[0m\n", \
__FUNCTION__);\
} while (0)
EXPORT_PUBLIC
setting_view *setting_view_get_topview(setting_view *view)
{
SETTING_TRACE_BEGIN;
retv_if(NULL == view, NULL);
int idx = 0;
SettingViewNode *viewnode = NULL;
....
if (viewnode && viewnode->topview)
return viewnode->topview;
else
return NULL;
SETTING_TRACE_END;
}Не выполняется завершающее логирование.
Фрагмент N12
org.tizen.settings-adid-0.0.1
V779 Unreachable code detected. It is possible that an error is present. ad-id.c 472
#define AI_FUNC_EXIT AI_LOGD("(%s) EXIT", __FUNCTION__);
int main(int argc, char *argv[])
{
AI_FUNC_ENTER
int ret = APP_ERROR_NONE;
ad_id_app_data_s ad = {0,};
....
if (ret != APP_ERROR_NONE)
AI_LOGE("ui_app_main() is failed. err=%d", ret);
return 0;
AI_FUNC_EXIT
}Ещё одна ошибка завершающего логирования. Отличие от предыдущего случая только в том, что используется другой макрос.
Примечание. В этом письме представлены только 2 таких случая, а вообще, подобных ошибок встречается весьма много.
Фрагмент N13
org.tizen.voice-setting-0.0.1
CWE-570 Expression is Always False
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
#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"
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";
....
}Обидели тех, кто использует язык es_US - Spanish (United States).
Дело в том, что LANG_ES_MX и LANG_ES_US являются идентичными строками. Поэтому в любом случае всегда будет выбран язык es_MX - Spanish (Mexico).
Фрагмент N14
security-manager-1.2.17
Не знаю, какому CWE это может соответствовать.
V646 Consider inspecting the application's logic. It's possible that 'else' keyword is missing. client-offline.cpp 52
ClientOffline::ClientOffline(bool wakeUp)
: m_offlineMode(false)
, m_serviceLock(nullptr)
{
....
if (wakeUp && m_serviceLock->Locked()) {
....
if (ClientRequest(.....).send().failed()) {
LogInfo("Socket activation attempt failed.");
m_serviceLock->Lock();
m_offlineMode = m_serviceLock->Locked();
} else
LogInfo("Service seems to be running now.");
} if (m_serviceLock->Locked()) {
m_offlineMode = true;
}
....
}Есть большое подозрение, что вместо:
} if (должно быть написано:
} else if (Другой вариант: надо просто отформатировать код и перенести if на следующую строку.
В любом случае, код оформлен неправильно и его следует изменить, чтобы он не сбивал тех, кто будет его сопровождать.
Фрагмент N15
security-manager-1.2.17
CWE-670 Always-Incorrect Control Flow Implementation
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);
....
do {
ret = TEMP_FAILURE_RETRY(getgrnam_r((....));));
if (ret == ERANGE && buffer.size() < MEMORY_LIMIT) {
buffer.resize(buffer.size() << 1);
continue;
}
} while(0);
....
}Автор хотел итерационно увеличивать размер буфера, пока он не станет нужного ему размера.
К сожалению, автор забыл тонкости работы оператора continue в цикле do..while. Дело в том, что continue не возобновляет сразу цикл, а осуществляет переход к проверке. Так как условие всегда ложно, то цикл в любом случае завершается.
Таким образом, будет выполнена только одна операция по увеличению буфера, после чего цикл остановится. В итоге, код работает так, как будто тут вообще нет никакого цикла.
Правильный вариант кода:
while(true) {
ret = TEMP_FAILURE_RETRY(getpwnam_r(....));
if (ret == ERANGE && buffer.size() < MEMORY_LIMIT) {
buffer.resize(buffer.size() << 1);
continue;
}
break;
}Выглядит не очень красиво и, скорее всего, можно написать как-то лучше, но это уже другая история.
Мы рассмотрели 15 ошибок из кода, написанном непосредственно в компании Samsung Electronics. Однако, пользователю часов/телефона не важно, допущена ошибка программистами Samsung Electronics или кем-то ещё. Поэтому есть смысл искать ошибки и в коде используемых сторонних библиотек. А ошибок полно. Рассмотрим только пяток для примера, а то получится не письмо, а справочник багов.
Фрагмент из библиотек N1
elementary-1.16.0
CWE-570 Expression is Always False
V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 382, 384. elm_glview.c 382
EOLIAN static Eina_Bool
_elm_glview_mode_set(Eo *obj, Elm_Glview_Data *sd,
Elm_GLView_Mode mode)
{
....
const int mask = 7 << 9;
if ((mode & mask) == (ELM_GLVIEW_STENCIL_1 & mask))
sd->config->stencil_bits = EVAS_GL_STENCIL_BIT_1;
else if ((mode & mask) == (ELM_GLVIEW_STENCIL_1 & mask)) // <=
sd->config->stencil_bits = EVAS_GL_STENCIL_BIT_2;
else if ((mode & mask) == (ELM_GLVIEW_STENCIL_4 & mask))
sd->config->stencil_bits = EVAS_GL_STENCIL_BIT_4;
else if ((mode & mask) == (ELM_GLVIEW_STENCIL_8 & mask))
sd->config->stencil_bits = EVAS_GL_STENCIL_BIT_8;
else if ((mode & mask) == (ELM_GLVIEW_STENCIL_16 & mask))
sd->config->stencil_bits = EVAS_GL_STENCIL_BIT_16;
else
sd->config->stencil_bits = EVAS_GL_STENCIL_BIT_8;
....
}Из-за опечатки два раза повторяется одно и тоже условие.
Должно быть:
else if ((mode & mask) == (ELM_GLVIEW_STENCIL_2 & mask))
sd->config->stencil_bits = EVAS_GL_STENCIL_BIT_2;Фрагмент из библиотек N2
elementary-1.16.0
CWE-467 Use of sizeof() on a Pointer Type
V568 It's odd that 'sizeof()' operator evaluates the size of a pointer to a class, but not the size of the 'derived' class object. elm_config.c 936
struct _Elm_Config_Derived
{
Eina_List *profiles;
};
typedef struct _Elm_Config_Derived Elm_Config_Derived;
EAPI void
elm_config_profile_derived_add(const char *profile,
const char *derive_options)
{
Elm_Config_Derived *derived;
derived = _elm_config_derived_load(_elm_profile);
if (!derived) derived = calloc(1, sizeof(derived)); // <=
if (derived)
....
}Вычисляет размер указателя на структуру Elm_Config_Derived, а не размер самой структуры. При этом код, благодаря везению, работает правильно, так как структура пока содержит только один указатель.
Фрагмент из библиотек N3
enlightenment-0.20.0
CWE-401 Improper Release of Memory Before Removing Last Reference ('Memory Leak')
V773 The function was exited without releasing the 'dupname' pointer. A memory leak is possible. e_comp_wl_rsm.c 639
#define EINA_SAFETY_ON_NULL_RETURN_VAL(exp, val) \
do \
{ \
if (EINA_UNLIKELY((exp) == NULL)) \
{ \
EINA_LOG_ERR("%s", "safety ......: " # exp " == NULL"); \
return (val); \
} \
} \
while (0)
static const char *
_remote_source_image_data_save(Thread_Data *td, const char *path,
const char *name)
{
....
const char *dupname;
....
dupname = strdup(fname);
....
if (shm_buffer)
{
ptr = wl_shm_buffer_get_data(shm_buffer);
EINA_SAFETY_ON_NULL_RETURN_VAL(ptr, NULL);
....
}Макрос EINA_SAFETY_ON_NULL_RETURN_VAL может завершать работу функции. В этом случае произойдёт утечка памяти.
Фрагмент из библиотек N4
enlightenment-0.20.0
CWE-131 Incorrect Calculation of Buffer Size
V666 Consider inspecting third argument of the function 'strncmp'. It is possible that the value does not correspond with the length of a string which was passed with the second argument. e_info_client.c 1801
static void
_e_info_client_proc_slot_set(int argc, char **argv)
{
....
if (!strncmp(argv[2], "start", strlen("start")))
mode = E_INFO_CMD_MESSAGE_START;
if (!strncmp(argv[2], "list", strlen("list")))
mode = E_INFO_CMD_MESSAGE_LIST;
if (!strncmp(argv[2], "create", strlen("add")) // <=
mode = E_INFO_CMD_MESSAGE_CREATE;
if (!strncmp(argv[2], "modify", strlen("modify")))
mode = E_INFO_CMD_MESSAGE_MODIFY;
if (!strncmp(argv[2], "del", strlen("del")))
mode = E_INFO_CMD_MESSAGE_DEL;
....
}Очень странно. Сравнивает только с первыми тремя символами строки "create". Причём тут вообще "add"?!
Фрагмент из библиотек N5
iotivity-1.2.1
CWE-416 Use after free
V723 Function returns a pointer to the internal string buffer of a local object, which will be destroyed: return ret.c_str(); ResourceInitException.h 112
virtual const char* what() const BOOST_NOEXCEPT
{
std::string ret;
....
return ret.c_str();
}Вишенка на торте. Возвращаем адрес разрушенного буфера. Спасибо компании Intel:
// Copyright 2014 Intel Mobile Communications GmbH All Rights Reserved.
Отдельно хочется обратить внимание на бардак, связанный с выделением памяти. Причина понятна: в проекте используется код как на C, так и C++. При этом код писался в разное время, менялся, превращался из C в C++. Часть кода относится к сторонним библиотекам, где свои предпочтения по работе с памятью.
Однако, хоть причина и понятна, это не значит, что всё нормально. На мой взгляд, всё это надо приводить к каким-то единообразным подходам, иначе работа с памятью будет постоянным источником различных ошибок. Наша команда может поработать и в этом направлении, и прорефакторить код.
Поясню на примерах, почему я использовал слово "бардак".
Часть кода написана аккуратно и правильно: после вызова функций malloc указатель проверяется на NULL, и эта ситуация обрабатывается соответствующим образом.
При этом рядом масса мест, где указатели используются без проверки. Пример из проекта org.tizen.browser-3.2.0:
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);
}CWE-252 Unchecked Return Value.
PVS-Studio: V522 There might be dereferencing of a potential null pointer 'msg'. QuickAccess.cpp 743
Выделили память и сразу используют. Кстати, вообще не понятно, зачем было применять функция malloc в С++ для создания массива фиксированного размера. Здесь лучше использовать std::array или, по крайней мере, функцию alloca.
При этом, рядом в этом же проекте я вижу:
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
void SettingsAFCreator::createNewAutoFillFormItem()
{
....
auto item_data = new AutoFillFormItemData;
if (!item_data) {
BROWSER_LOGE("Malloc failed to get item_data");
return;
}
....
}CWE-697 Insufficient Comparison.
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
Проверка бессмысленна. Оператор new при ошибке выделения памяти генерирует исключение std::bad_alloc. Скорее всего, ранее здесь использовалась функция malloc, на что намекает сообщение "Malloc failed to get item_data".
Или, например, код из проекта ise-default-1.3.34. Взгляните вот на эту структуру:
typedef struct _VoiceData VoiceData;
struct _VoiceData
{
int voicefw_state;
stt_h voicefw_handle;
....
std::vector<std::string> stt_results;
....
is::ui::WInputSttMicEffect *ieffect;
is::ui::MicEffector *effector;
};Один из членов в ней имеет тип std::vector<std::string>.
Ужас в том, что создают и инициализируют эту структуру с помощью malloc и memset:
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)); // <=
....
}А уничтожают с помощью вызова функции free:
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
Это можно классифицировать как CWE-762 Mismatched Memory Management Routines. В общем, нельзя так делать.
В проекте isf-3.0.186 мне встретился вот такой интересный код:
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; // <=
}
};Создаются структуры типа sockaddr_un и sockaddr_in. При этом хранятся и уничтожаются они как структуры типа sockaddr. Типы всех трёх названных структур никак не связаны между собой. Это просто три разные структуры, имеющие разный размер.
Как я понимаю, так делать нельзя. Что создали, то и надо уничтожать. Стандарт говорит: "The value of the operand of delete shall be the pointer value which resulted from a previous array new-expression". Есть подозрение, что этот код приводит к undefined behavior, но с ходу не смог найти в стандарте текст на эту тему.
Напоследок рассмотрим функцию из библиотеки efl-1.16.0. Про эту функцию вообще можно писать отдельную главу, так много в ней странного.
static Edje_Map_Color **
_copied_map_colors_get(Edje_Part_Description_Common *parent)
{
Edje_Map_Color **colors;
Edje_Map_Color *color;
int i;
if (parent->map.colors_count == 0) return NULL;
colors = malloc(sizeof(Edje_Map_Color *) * // <= #1
parent->map.colors_count);
for (i = 0; i < (int)parent->map.colors_count; i++)
{
color = parent->map.colors[i];
Edje_Map_Color *c = mem_alloc(SZ(Edje_Map_Color));
if (!color) // <= #2
{
ERR("not enough memory");
exit(-1); // <= #3
return NULL; // <= #4
}
memcpy(c, color, sizeof(Edje_Map_Color));
colors[i] = c;
}
return colors;
}Я обратил внимание на эту функцию, заметив сообщение анализатора PVS-Studio: V773 The function was exited without releasing the 'colors' pointer. A memory leak is possible. edje_cc_handlers.c 7335
Однако, когда начинаешь изучать тело функции подробнее, то становится виден сразу клубок странностей.
В коде проекта выполняют проверку: выделилась ли память при вызове функции malloс. Но для указателя colors этого не сделали, и смело записывают в него данные.
Для хранения объекта типа Edje_Map_Color выделяют динамическую память. Адрес этой памяти хранится в указателе c, но проверяют почему-то указатель color, который потом копируют в новую область памяти. Видимо, это опечатка.
Непонятно, что действительно хотел сделать автор кода: завершить работу программы с помощью вызова функции exit(-1) или вернуть из функции NULL.
Если завершить программу, то зачем нужен return NULL?
Если хотелось вернуть NULL, то есть другая ошибка. Не уничтожаются объекты, которые уже были созданы и помещены в массив colors. Будет утечка памяти.
На этом я закончу тему работы с памятью. Есть масса других ошибок и странных мест, но приведённых примеров, думаю, достаточно, чтобы показать необходимость приведения всего этого кода в порядок. Если компании Samsung заинтересуется, мы готовы сделать такую работу.
Дополнительно я подготовил презентацию, в которой приведены расчёты, согласно которым наша команда может найти и исправить около 27000 ошибок в проекте Tizen. Исследование я проводил по следующей схеме: случайным образом были выбраны проекты и изучено, сколько реальных ошибок в них находит PVS-Studio. Всего я нашел около 900 настоящих ошибок, проверив 3.3% кода. Соответственно, экстраполировав результат, я и получил озвученное ранее число 27000. Презентация: RU: pptx, slideshare; EN: pptx, slideshare.
С уважением, Андрей Карпов
E-Mail: karpov [@] viva64.com
Microsoft MVP, к.ф.-м.н.,
Технический директор