# PVS\-Studio Team Willing to Work on Improving Tizen Project \(open letter\)

This is an open letter from Andrey Karpov, representative of the PVS\-Studio team, to the developers of the Tizen OS\. Our team is willing to work on improving the quality of Tizen project\. The text contains remarks to the code fragments, but this is not criticism\. All projects have bugs\. The aim was to show by real examples that we aren't talking about abstract recommendations concerning the code improvement, but about real defects that we can find and fix\.

## Hello

First of all, let me introduce myself\. My name is Andrey Karpov, I am CTO and one of the developers of PVS\-Studio static code analyzer\.

Evgeny Ryzhkov \(CEO\) asked me to scan the source code of the Tizen operating system with our PVS\-Studio analyzer, choose ten errors to my liking and comment on them\. Based on those errors, I was then to write a letter to the Tizen developers to demonstrate that PVS\-Studio could help in their work\. In other words, those errors are the grounds for further communication and collaboration\.

PVS\-Studio finds lots of defects in Tizen's code, so picking a few was not a problem\. I decided to describe 20 faulty code fragments instead of 10 and tried to collect different error patterns, but some still had to be left out\.

I know that Samsung does care about the quality and reliability of the Tizen OS a lot\. That's why I'm sure PVS\-Studio analyzer could become a great complement to their development process\.

So, I suggest that the Tizen team consider hiring the PVS\-Studio team for working on Tizen project\. 

I see two possible ways we could do it:

1. The same approach that we used when collaborating with Epic Games company: "[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 purchases an extended license under which we do not only provide them with a copy of PVS\-Studio but also run the first check and do the fixing ourselves\. This results in: a\) bugs getting fixed; b\) Tizen developers getting warning\-free code, making further use of PVS\-Studio easier\. We could also help integrate the tool into the development process\.
1. Long\-term partnership under which we would regularly audit Tizen's code and fix issues found\.

As I already said, PVS\-Studio analyzer detects tons of defects in Tizen's code\. I will discuss some of them in a new article coming soon, as we [typically do](https://pvs-studio.com/en/blog/inspections/) when checking open\-source projects\. In the case of Tizen, though, it's going to be a series of articles rather than a single article\.

This one is just a warm\-up\. We have checked just a couple of dozens of small projects from Tizen, while their total number is a few hundreds\. Today we are going to discuss 20 errors\. 15 of these were found in the code created by the Tizen developer team itself, while the rest 5 issues were found in the third\-party libraries containing hundreds of patches for Tizen\. Personally, I believe that the quality of the libraries is just as important as that of the operating system's code itself\. After all, it makes no difference from a user's perspective if a memory leak occurs in the operating system's code or in one of the libraries\.

The error descriptions below have the following format:

* example's sequence number;
* project name;
* defect type under CWE classification;
* PVS\-Studio warning describing the defect;
* code fragment;
* comment\.

Note\. For more details see the presentation: [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)\.

## 15 errors in Tizen's code

**Fragment No\.1**

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

CWE\-675 Duplicate Operations on Resource

[V778](https://pvs-studio.com/en/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
  }
  ....
}
```

The destructor's code was written using the Copy\-Paste method: the programmer forgot to replace one of the instances of the name _m\_modulesToolbar_ with _m\_navigatorToolbar_\.

**Fragment No\.2**

org\.tizen\.download\-manager\-0\.3\.21

CWE\-193 Off\-by\-one Error

[V645](https://pvs-studio.com/en/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));
  ....
}
```

This is a typical mistake when using the _strncat_ function\. Programmers tend to forget that the third argument defines how many more characters can be added to the string, not counting the terminal null\. Here is a simpler example to illustrate this error:

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

The buffer has no more room for new characters\. It contains 4 characters and a terminal null already\. The "5 \- strlen\(buf\)" expression evaluates to 1\. The _strncpy_ function will copy the 'E' character to the array's last element, while the terminal null will go beyond its bounds\.

This is the fixed version:

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

**Fragment No\.3**

org\.tizen\.indicator\-0\.2\.53

CWE\-571 Expression is Always True

[V547](https://pvs-studio.com/en/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;
}
```

The _strlen\(s\_formatted\) < CLOCK\_STR\_LEN_ condition is always true, as the number of characters in a string is always less than the size of the buffer it is stored in\. If the condition turns out to be false for some reason, it will mean that data have been written outside the buffer's bounds and it's just too late to check anything, anyway\. This suggests that something is wrong with the execution logic here\.

**Fragment No\.4**

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

The _WIFI\_RSSI\_LEVEL\_\*_ constants refer to an enumeration of type _wifi\_rssi\_level\_e_, while the _level_ variable is of type _wifi\_manager\_rssi\_level\_e_\.

It is thanks to sheer luck:

* constant_ WIFI\_RSSI\_LEVEL\_0_ is equal to constant _WIFI\_MANAGER\_RSSI\_LEVEL\_0_
* constant _WIFI\_RSSI\_LEVEL\_1_ is equal to constant _WIFI\_MANAGER\_RSSI\_LEVEL\_1_
* and so forth\.

that the code works just as the programmer expected while still being faulty\.

**Fragment No\.5**

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

If the error occurs, the program fails to free 3 memory blocks referenced by the pointers _role\_name_, _description\_from\_role_, and _state\_from\_role_\.

**Fragment No\.6**

org\.tizen\.screen\-reader\-0\.0\.8

CWE\-131 Incorrect Calculation of Buffer Size

[V512](https://pvs-studio.com/en/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"));
  ....
}
```

The programmers deceive the _snprintf_ function by telling it that the buffer has more space than it actually does\. Fixed code:

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

**Fragment No\.7**

org\.tizen\.screen\-reader\-0\.0\.8

CWE\-131 Incorrect Calculation of Buffer Size

[V512](https://pvs-studio.com/en/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_......."));
  ....
}
```

This defect is just like the previous one, only that the buffer size is specified by the constant _HOVERSEL\_TRAIT\_SIZE_ rather than being evaluated using the _sizeof_ operator\.

**Fragment No\.8**

org\.tizen\.setting\-1\.0\.1

CWE\-570 Expression is Always False

[V501](https://pvs-studio.com/en/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"))
  ....
}
```

One part of the compound conditional expression is always false since the previous line contains the same check\. This typo might indicate that some other check is missing\.

**Fragment No\.9**

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

Something is wrong with this function's execution logic\. Here are the key lines:

```cpp
if (str[1] == '\0') {
} else if (str[1] == 0)

if (str[2] == 0) {
} else if (str[2] == '\0') {
```

What we have here are two similar cases of meaningless repeated checks\. One character is compared with 0 and '\\0'\.

**Fragment No\.10**

org\.tizen\.setting\-1\.0\.1

CWE\-762 Mismatched Memory Management Routines

[V611](https://pvs-studio.com/en/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;
  }
  ....  
}
```

The buffer allocated on the stack using the _alloca _function could be passed to the _free_ function\.

**Fragment No\.11**

org\.tizen\.setting\-1\.0\.1

[V779](https://pvs-studio.com/en/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;
}
```

Trace ending failure\.

**Fragment No\.12**

org\.tizen\.settings\-adid\-0\.0\.1

[V779](https://pvs-studio.com/en/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
}
```

Another trace ending error\. This example is only different from the previous one in that it uses a different macro\. 

Note\. This letter discusses only two cases of this kind, but there are much more throughout the project\.

**Fragment No\.13**

org\.tizen\.voice\-setting\-0\.0\.1

CWE\-570 Expression is Always False

[V517](https://pvs-studio.com/en/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\) users were left out\.

This happened because _LANG\_ES\_MX_ and _LANG\_ES\_US_ are identical strings, so es\_MX \- Spanish \(Mexico\) language is selected in any case\.

**Fragment No\.14**

security\-manager\-1\.2\.17

I don't know to which CWE defect this one corresponds\.

[V646](https://pvs-studio.com/en/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;
  }
  ....
}
```

I have a strong suspicion that the line:

```cpp
} if (
```

was meant to look like this:

```cpp
} else if (
```

Another possible variant is that the code simply needs to be properly formatted and the _if_ statement should move to the next line\.

Anyway, the code is implemented incorrectly and should be fixed so that it doesn't confuse those who will maintain it in the future\.

**Fragment No\.15**

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

The author intended to iterate through the buffer size to increment it until it reaches the needed size\.

Sadly, they forgot about the specifics of how the _continue_ statement works within the loop _do\.\.whil_e\. The point is that _continue_ jumps to the check rather than resuming the loop immediately\. Since the condition is always false, the loop will terminate in any case\.

This means that only one iteration will be executed increasing the buffer size and the loop will terminate right after that\. As a result, the code executes as if there were no loop at all\.

Fixed code:

```cpp
while(true) {
  ret = TEMP_FAILURE_RETRY(getpwnam_r(....));
  if (ret == ERANGE && buffer.size() < MEMORY_LIMIT) {
    buffer.resize(buffer.size() << 1);
    continue;
  }
  break;
}
```

This solution doesn't look neat, and there's probably a better way to write it, but this matter is beyond the scope of this article\.

## 5 defects in third\-party libraries

Those were the 15 errors from the code written by the developers of Samsung Electronics\. However, it doesn't make any difference to smartwatch/smartphone users if a bug results from a mistake made by Samsung Electronics programmers or anyone else\. That's why it does make sense to look for bugs in the code of the third\-party libraries the project depends on\. There are tons of bugs there\. We'll discuss only five examples since we do not want this letter to turn into a bug reference manual\.

**Fragment from libraries No\.1**

elementary\-1\.16\.0

CWE\-570 Expression is Always False

[V517](https://pvs-studio.com/en/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;
  ....
}
```

One condition is checked twice because of a typo\.

Fixed code:

```cpp
else if ((mode & mask) == (ELM_GLVIEW_STENCIL_2 & mask))
  sd->config->stencil_bits = EVAS_GL_STENCIL_BIT_2;
```

**Fragment from libraries No\.2**

elementary\-1\.16\.0

CWE\-467 Use of sizeof\(\) on a Pointer Type

[V568](https://pvs-studio.com/en/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)
  ....
}
```

This example evaluates the size of the pointer to the _Elm\_Config\_Derived_ structure instead of the size of the structure itself\. Luckily, the code works as expected, as there is currently only one pointer in the structure\.

**Fragment from libraries No\.3**

enlightenment\-0\.20\.0

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

[V773](https://pvs-studio.com/en/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);
  ....
}
```

The _EINA\_SAFETY\_ON\_NULL\_RETURN\_VAL_ macro can lead to the exit from the function\. If it does, a memory leak will occur\.

**Fragment from libraries No\.4**

enlightenment\-0\.20\.0

CWE\-131 Incorrect Calculation of Buffer Size

[V666](https://pvs-studio.com/en/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;
  ....
}
```

This is quite a strange snippet\. A string is compared with only the first three characters of the string "create"\. What does "add" have to do with this at all?\!

**Fragment from libraries No\.5**

iotivity\-1\.2\.1

CWE\-416 Use after free

[V723](https://pvs-studio.com/en/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();
}
```

The cherry on top: the address of the destroyed buffer is returned\. Thank you, Intel:

// Copyright 2014 Intel Mobile Communications GmbH All Rights Reserved\.

## Memory management refactoring needed

I'd like to specifically discuss the mess in the code that is related to memory allocation\. The reason behind it is clear: the project employs both C and C\+\+ code\. More than that, it was being written at different times and would change and turn from C to C\+\+\. Some of it is located in the third\-party libraries, which have their own ways of handling memory management\.

However, even though the reason is clear, it doesn't mean everything is fine\. My opinion is the whole such codebase has to be unified; otherwise, the memory management issues are going to become a permanent source of bugs\. Our team could deal with both memory management and code refactoring\.

I will show you a few examples to explain why I'm using the word "mess"\.

Some parts of the code performing memory management are written in a neat and proper way: after calling _malloc_ functions, the pointer is checked for _NULL,_ and this situation is then handled accordingly\.

At the same time, there are tons of fragments around where pointers are used without checks\. Here's an example from the project 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.com/en/docs/warnings/v522/) There might be dereferencing of a potential null pointer 'msg'\. QuickAccess\.cpp 743

The buffer is used immediately after allocation\. By the way, why do the authors use the _malloc _function to create a fixed\-size array in C\+\+ code? A better solution would be to use _std::array_ or at least the _alloca_ function\.

At the same time, I get the following warning for the same project:

[V668](https://pvs-studio.com/en/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.com/en/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

The check is pointless\. The _new_ operator throws the _std::bad\_alloc_ exception when memory can't be allocated\. I suspect this example used the _malloc_ function previously, which is suggested by the message "Malloc failed to get item\_data"\.

Here's another example, taken from the project ise\-default\-1\.3\.34\. Look at this structure:

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

One of its members is of type _std::vector<std::string\>_\. 

The horrible thing about it is that this structure is created and initialized using _malloc_ and _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));             // <=
  ....
}
```

while being destroyed using a call to the function _free_:

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

PVS\-Studio warning: [V780](https://pvs-studio.com/en/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

This issue can be classified as CWE\-762 Mismatched Memory Management Routines\. Put briefly, you shouldn't do that\.

In the project isf\-3\.0\.186, I came across the following interesting fragment:

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

Structures of types _sockaddr\_un_ and _sockaddr\_in_ are created, but they are stored and destroyed as if they were of type _sockaddr_\. The types of all the three structures have nothing in common; these are just three different structures of different sizes\.

As far as I understand, one shouldn't do anything like this\. When you create an object, you must destroy this particular object\. The C\+\+ standard reads: "The value of the operand of delete shall be the pointer value which resulted from a previous array new\-expression"\. I suspect this code may lead to undefined behavior, but a quick glance through the standard revealed nothing about that\.

The last example uses a function from the library efl\-1\.16\.0\. There are so many strange things in this library that it deserves a separate chapter\.

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

This function attracted my attention after I noticed this message by PVS\-Studio: [V773](https://pvs-studio.com/en/docs/warnings/v773/) The function was exited without releasing the 'colors' pointer\. A memory leak is possible\. edje\_cc\_handlers\.c 7335

However, when you start examining the function body closely, the entire tangle of oddities becomes apparent\.

There is a check in the project's code to see if memory has been allocated when calling the _malloc_ function\. On the other hand, there is no such check for the _colors_ pointer, and data are written to it without any hesitation\.

Dynamic memory is allocated to store the object of type _Edje\_Map\_Color_\. The address of this memory block is stored in the _c_ pointer, but for some reason the _color_ pointer is checked instead and then copied to the new memory block\. This must be a typo\.

It's not clear what the programmer actually meant to do \- terminate the program by calling _exit\(\-1\)_ or return _NULL_ from the function\.

If the former, then why use _return NULL_?

If the latter, then another error shows up: the objects that have been already created and written to the _colors_ array won't be destroyed\. This will result in a memory leak\.

Let's finish with memory management issues here\. There are lots of other defects and strange fragments, but those examples discussed above should suffice to prove that this entire code needs tidying up\. If Samsung finds our offer interesting, we can do that\.

## Update

Additionally, I made a presentation, which provides calculations, according to which our company can find and fix about 27 000 errors in the Tizen project\. The research was done according to the following scheme: random choice of projects for analysis, research of the amount of real errors detected by PVS\-Studio\. In sum total I found about 900 real errors, having checked 3\.3% of the code\. Thus, I got the previously stated number 27 000 by extrapolating the result\. Presentation: [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)\.

Sincerely, 

Andrey Karpov

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

CTO, Microsoft MVP, Cand\.Sc\. \(Physics and Mathematics\),