Команда PVS-Studio готова поработать над проектом Tizen (открытое письмо)

Андрей Карпов
Статей: 375



Открытое письмо разработчикам операционной системы 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.

Я вижу два варианта возможного сотрудничества:

  • Сценарий аналогичный сотрудничеству с компанией Epic Games: "How the PVS-Studio Team Improved Unreal Engine's Code". Компания Samsung приобретает расширенный вариант лицензии. В рамках этой лицензии, мы не только предоставляем анализатор PVS-Studio, но и самостоятельно выполняем разовую проверку и правку кода. В результате: a) исправляются ошибки б) разработчики Tizen получат код, чистый от предупреждений анализатора, что облегчит им в дальнейшем использование инструмента PVS-Studio. Также мы поможем с интеграцией инструмента в процесс разработки.
  • Долгосрочное сотрудничество, в рамках которого мы будем регулярно проводить аудит кода Tizen и править обнаруживаемые дефекты.

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

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

Примечание. Дополнительно см. презентацию: RU: pptx, slideshare; EN: pptx, slideshare.

Примеры дефектов будут оформлены следующим образом:

  • порядковый номер примера;
  • название проекта;
  • тип дефекта, согласно классификации CWE;
  • предупреждение PVS-Studio, с помощью которого был найден дефект;
  • фрагмент кода;
  • пояснение.

15 примеров ошибок в коде Tizen

Фрагмент 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

  • V556 The values of different enum types are compared: switch(ENUM_TYPE_A) { case ENUM_TYPE_B: ... }. wifi.c 163
  • V556 The values of different enum types are compared: switch(ENUM_TYPE_A) { case ENUM_TYPE_B: ... }. wifi.c 164
  • V556 The values of different enum types are compared: switch(ENUM_TYPE_A) { case ENUM_TYPE_B: ... }. wifi.c 166
  • V556 The values of different enum types are compared: switch(ENUM_TYPE_A) { case ENUM_TYPE_B: ... }. wifi.c 168
  • V556 The values of different enum types are compared: switch(ENUM_TYPE_A) { case ENUM_TYPE_B: ... }. wifi.c 170
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.

Благодаря везению:

  • константа WIFI_RSSI_LEVEL_0 равна константе WIFI_MANAGER_RSSI_LEVEL_0
  • константа WIFI_RSSI_LEVEL_1 равна константе WIFI_MANAGER_RSSI_LEVEL_1
  • и так далее.

Код работает, как и ожидал программист, но при этом некорректен.

Фрагмент N5

org.tizen.screen-reader-0.0.8

CWE-401 Improper Release of Memory Before Removing Last Reference ('Memory Leak')

  • V773 The function was exited without releasing the 'role_name' pointer. A memory leak is possible. navigator.c 991
  • V773 The function was exited without releasing the 'description_from_role' pointer. A memory leak is possible. navigator.c 991
  • V773 The function was exited without releasing the 'state_from_role' pointer. A memory leak is possible. navigator.c 991
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

  • V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 792, 800. setting-common-general-func.c 792
  • V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 801, 805. setting-common-general-func.c 801
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

  • V696 The 'continue' operator will terminate 'do { ... } while (FALSE)' loop because the condition is always false. Check lines: 73, 75. nss_securitymanager.cpp 73
  • V696 The 'continue' operator will terminate 'do { ... } while (FALSE)' loop because the condition is always false. Check lines: 120, 122. nss_securitymanager.cpp 120
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;
}

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

5 примеров ошибок из сторонних библиотек

Мы рассмотрели 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;                              // <=
  }
};
  • V572 It is odd that the object which was created using 'new' operator is immediately cast to another type. scim_socket.cpp 136
  • V572 It is odd that the object which was created using 'new' operator is immediately cast to another type. scim_socket.cpp 140

Создаются структуры типа 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 заинтересуется, мы готовы сделать такую работу.

Update

Дополнительно я подготовил презентацию, в которой приведены расчёты, согласно которым наша команда может найти и исправить около 27000 ошибок в проекте Tizen. Исследование я проводил по следующей схеме: случайным образом были выбраны проекты и изучено, сколько реальных ошибок в них находит PVS-Studio. Всего я нашел около 900 настоящих ошибок, проверив 3.3% кода. Соответственно, экстраполировав результат, я и получил озвученное ранее число 27000. Презентация: RU: pptx, slideshare; EN: pptx, slideshare.

С уважением, Андрей Карпов

E-Mail: karpov [@] viva64.com

Microsoft MVP, к.ф.-м.н.,

Технический директор



Найдите ошибки в своем C, C++, C# и Java коде

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

goto PVS-Studio;

Андрей Карпов
Статей: 375


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

Проверено проектов
361
Собрано ошибок
13 417

А ты совершаешь ошибки в коде?

Проверь с помощью
PVS-Studio

Статический анализ
кода для C, C++, C#
и Java

goto PVS-Studio;