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

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

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

1. Сценарий аналогичный сотрудничеству с компанией Epic Games: "[How the PVS\-Studio Team Improved Unreal Engine's Code](https://www.unrealengine.com/en-us/blog/how-pvs-studio-team-improved-unreal-engines-code)"\. Компания Samsung приобретает расширенный вариант лицензии\. В рамках этой лицензии, мы не только предоставляем анализатор PVS\-Studio, но и самостоятельно выполняем разовую проверку и правку кода\. В результате: a\) исправляются ошибки б\) разработчики Tizen получат код, чистый от предупреждений анализатора, что облегчит им в дальнейшем использование инструмента PVS\-Studio\. Также мы поможем с интеграцией инструмента в процесс разработки\.
1. Долгосрочное сотрудничество, в рамках которого мы будем регулярно проводить аудит кода Tizen и править обнаруживаемые дефекты\.

Как я уже сказал, анализатор PVS\-Studio выявляет гору дефектов в коде Tizen\. Некоторые из них, в скором времени, я опишу в новой статье, как мы это [обычно делаем](https://pvs-studio.ru/ru/blog/inspections/) в рамках проверки различных открытых проектов\. Хотя в случае с Tizen, это, скорее, будет не статья, а цикл статей\.

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

Примечание\. Дополнительно см\. презентацию: RU: [pptx](http://cppfiles.com/Tizen_2017_ru.pptx), [slideshare](https://www.slideshare.net/Andrey_Karpov/pvsstudio-tizen); EN: [pptx](http://cppfiles.com/Tizen_2017.pptx), [slideshare](https://www.slideshare.net/Andrey_Karpov/pvsstudio-is-ready-to-improve-the-code-of-tizen-operating-system)\.

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

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

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

**Фрагмент N1**

org\.tizen\.browser\-3\.2\.0

CWE\-675 Duplicate Operations on Resource

[V778](https://pvs-studio.ru/ru/docs/warnings/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

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/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

```cpp
#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_\. Забывают, что третий аргумент задаёт сколько ещё символов можно добавить в строку, не учитывая терминальный ноль\. Поясню эту ошибку на более простом примере:

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

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

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

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/v547/) Expression 'strlen\(s\_formatted\) < 128' is always true\. clock\.c 503

```cpp
#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

```cpp
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

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/v512/) A call of the 'snprintf' function will lead to overflow of the buffer 'buf \+ strlen\(buf\)'\. app\_tracker\.c 450

```cpp
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_ обманывают, говоря ей что в буфере больше места, чем есть на самом деле\. Правильно:

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/v512/) A call of the 'snprintf' function will lead to overflow of the buffer 'trait \+ strlen\(trait\)'\. navigator\.c 514

```cpp
#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](https://pvs-studio.ru/ru/docs/warnings/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

```cpp
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

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

В логике работы этой функции что\-то не так\. Выделю суть проблемы:

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/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

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/v779/) Unreachable code detected\. It is possible that an error is present\. setting\-common\-view\.c 124

```cpp
#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](https://pvs-studio.ru/ru/docs/warnings/v779/) Unreachable code detected\. It is possible that an error is present\. ad\-id\.c 472

```cpp
#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](https://pvs-studio.ru/ru/docs/warnings/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

```cpp
#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](https://pvs-studio.ru/ru/docs/warnings/v646/) Consider inspecting the application's logic\. It's possible that 'else' keyword is missing\. client\-offline\.cpp 52

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

Есть большое подозрение, что вместо:

```cpp
} if (
```

должно быть написано:

```cpp
} 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

```cpp
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\.\.whil_e\. Дело в том, что _continue_ не возобновляет сразу цикл, а осуществляет переход к проверке\. Так как условие всегда ложно, то цикл в любом случае завершается\.

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

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

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/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

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

Из\-за опечатки два раза повторяется одно и тоже условие\.

Должно быть:

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/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

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/v773/) The function was exited without releasing the 'dupname' pointer\. A memory leak is possible\. e\_comp\_wl\_rsm\.c 639

```cpp
#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](https://pvs-studio.ru/ru/docs/warnings/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

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/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

```cpp
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:

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/v522/) There might be dereferencing of a potential null pointer 'msg'\. QuickAccess\.cpp 743

Выделили память и сразу используют\. Кстати, вообще не понятно, зачем было применять функция _malloc_ в С\+\+ для создания массива фиксированного размера\. Здесь лучше использовать _std::array_ или, по крайней мере, функцию _alloca_\.

При этом, рядом в этом же проекте я вижу:

[V668](https://pvs-studio.ru/ru/docs/warnings/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

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/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\. Взгляните вот на эту структуру:

```cpp
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_:

```cpp
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:

```cpp
void on_destroy(VoiceData *r_voicedata)
{
  ....
  VoiceData *voicedata = (VoiceData *)r_voicedata;
  ....
  free(voicedata);                                        // <=
}
```

PVS\-Studio предупреждает: [V780](https://pvs-studio.ru/ru/docs/warnings/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 мне встретился вот такой интересный код:

```cpp
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\. Про эту функцию вообще можно писать отдельную главу, так много в ней странного\.

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/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](http://cppfiles.com/Tizen_2017_ru.pptx), [slideshare](https://www.slideshare.net/Andrey_Karpov/pvsstudio-tizen); EN: [pptx](http://cppfiles.com/Tizen_2017.pptx), [slideshare](https://www.slideshare.net/Andrey_Karpov/pvsstudio-is-ready-to-improve-the-code-of-tizen-operating-system)\.

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

E\-Mail: karpov \[@\] viva64\.com

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

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