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.
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:
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 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:
Note. For more details see the presentation: pptx, slideshare.
Fragment No.1
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
}
....
}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 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));
....
}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:
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:
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 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;
}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
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:
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')
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 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"));
....
}The programmers deceive the snprintf function by telling it that the buffer has more space than it actually does. Fixed code:
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 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_......."));
....
}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 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"))
....
}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
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:
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 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;
}
....
}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 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;
}Trace ending failure.
Fragment No.12
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
}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 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) 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 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;
}
....
}I have a strong suspicion that the line:
} if (was meant to look like this:
} 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
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..while. 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:
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.
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 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;
....
}One condition is checked twice because of a typo.
Fixed code:
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 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)
....
}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 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);
....
}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 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;
....
}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 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();
}The cherry on top: the address of the destroyed buffer is returned. Thank you, Intel:
// Copyright 2014 Intel Mobile Communications GmbH All Rights Reserved.
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:
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
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 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
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:
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:
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:
void on_destroy(VoiceData *r_voicedata)
{
....
VoiceData *voicedata = (VoiceData *)r_voicedata;
....
free(voicedata); // <=
}PVS-Studio warning: 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:
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; // <=
}
};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.
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 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.
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, slideshare.
Sincerely,
Andrey Karpov
E-Mail: karpov [@] viva64.com
CTO, Microsoft MVP, Cand.Sc. (Physics and Mathematics),