This article will demonstrate that during the development of large projects static analysis is not just a useful, but a completely necessary part of the development process. This article is the first one in a series of posts, devoted to the ability to use PVS-Studio static analyzer to improve the quality and reliability of the Tizen operating system. For a start, I checked a small part of the code of the operating system (3.3%) and noted down about 900 warnings pointing to real errors. If we extrapolate the results, we will see that our team is able to detect and fix about 27000 errors in Tizen. Using the results of the conducted study, I made a presentation for the demonstration to the Samsung representatives with the offers about possible cooperation. The meeting was postponed, that is why I decided not to waste time and transform the material of the presentation to an article. Get some coffee and cookies, as there is a long programmer thriller waiting for us.

We should probably start with the link to the presentation "PVS-Studio is ready to improve the code of Tizen operating system", that served as a basis for this article: pptx, slideshare. However, it is not necessary to watch the presentation, because all of its material will be covered here, in more detail. The topic of the presentation overlaps with an open letter, where we also speak about the offer to cooperate with Tizen.

Enough talking, let's get to the point. The first thing to do is to remind the reader of what is the Tizen operating system in general.

Tizen

Tizen is an operating system based on the Linux kernel and the GNU C Library implementing the Linux API. It works on a wide range of devices, including smartphones, tablets, in-vehicle infotainment (IVI) devices, smart TVs, PCs, smart cameras, wearable computing (such as smart watches), developed and managed by such corporations as Intel and Samsung. It supports hardware platforms on the processors of the ARM and x86 architectures. More detailed information can be found on the Wikipedia.

The Tizen platform shows a steady growth over the past several years, despite the abundance of operating systems for mobile devices and wearable devices on the market. According to the report by Samsung, the growth of mobile phones with Tizen operating system was 100% in 2017.

For our team, the Tizen operating system seems attractive in a sense that Samsung is interested in its reliability and makes an effort to improve the quality of its code. For example, Samsung invested into the development of a specialized analyzer Svace in ISP RAS. Svace is used as a main means of providing the security of the system and application software of the Tizen platform. Here are some quotes taken from the article "Samsung have Invested $10 Million in Svace, Security Solution to Analyze Tizen Apps":

As part of its security measures, Samsung are using the SVACE technology (Security Vulnerabilities and Critical Errors Detector) to detect potential vulnerabilities and errors that might exist in source code of applications created for the Tizen Operating System (OS). This technology was developed by ISP RAS (Institute for System Programming of the Russian Academy of Sciences), who are based in Moscow, Russia.

The solution is applied as part of the Tizen Static Analyzer tool that is included in the Tizen SDK and Studio. Using this tool, you can perform Static security analysis of the Tizen apps native C / C ++ source code and discover any issues that they might have. The tool helps discover a wide range of issues at compilation time, such as the dereference of Null Pointers, Memory Leaks, Division by Zero, and Double Free etc.

PVS-Studio team just could not miss a chance to check such an interesting open source project.

Analysis Of Tizen

The purpose of the presentation, which I mentioned earlier, was to demonstrate that PVS-Studio analyzer finds a lot of errors of various types. This is a kind of resume of our analyzer and our team that we want to show to Samsung.

Still, a reader may start doubting, if he should read such a "resume article" at all. Yes, it is really worth reading, as it has a lot of interesting and useful information. First of all, it is better to learn from other people's mistakes, rather than from your own. Secondly, the article perfectly shows that static analysis methodology is a must for large size projects. If some colleagues working on a large project, claim that they write high-quality code and almost without errors, just show them this article. I do not think the creators of Tizen wanted bugs to get into their project, but here they are - thousands of bugs.

As always, I would like to remind that static analysis should be used regularly. A single check of Tizen or any other project will be useful, of course, but ineffective. Most likely, there will be minor errors that do not affect the capacities of the project. All the obvious errors were already fixed by other means, for example, due to user complaints. Does it mean that static analysis isn't really useful? Of course, not. It is of great use, but as I have already said, one-time check is not an efficient way to use the analyzer. Analyzers should be used regularly: in this case, a lot of errors, including critical ones, will be detected at the earliest stage. The earlier an error is detected, the less expensive it is to correct it.

I believe that:

Now PVS-Studio detects more than 10% of errors that are present in the code of the Tizen project.

In the case of regular use of PVS-Studio on the new code, about 20% of errors can be prevented.

I predict that PVS-Studio team can detect and fix about 27000 errors in the Tizen project.

Of course, I could be wrong, but I am not manipulating the results here put the analyzer's best foot forward. It is just not necessary. PVS-Studio is a powerful tool that finds so many defects that there is just no sense to falsify the results. I will explain how I got all these figures.

Of course, I could not check the entire Tizen project. The whole Tizen project with the third-party libraries is 72 500 000 lines of C, C++ code (excluding the comments). That is why I decided to choose randomly several dozens of projects of Tizen: Unified (https://build.tizen.org/project/show/Tizen:Unified).

Choosing projects, I divided them into two groups. The first group are the projects written by the Samsung employers. Such comments in the beginning of the files were a sign of this:

/* * Copyright (c) 2015 Samsung Electronics Co., Ltd All Rights Reserved .... */

The second group are third-party projects, used in the Tizen project. However, many projects cannot be really called "third-party", as they have various patches. Here is an example of a patch made in efl-1.16.0 library:

//TIZEN_ONLY(20161121) // Pre-rotation should be enabled only when direct // rendering is set but client side rotation is not set if ((sfc->direct_fb_opt) && (!sfc->client_side_rotation) && (evgl_engine->funcs->native_win_prerotation_set)) { if (!evgl_engine->funcs->native_win_prerotation_set(eng_data)) ERR("Prerotation does not work"); } //

So, it is a somewhat relative division, however, precise accuracy is not really required for the general evaluation.

I randomly chose projects, started reviewing the analyzer logs and selecting those warnings that are worth taking a look at. Of course, some bugs are quite innocent and can show up extremely rarely. For example, the following code will fail very rarely:

m_ptr = (int *)realloc(m_ptr, newSize); if (!m_ptr) return ERROR;

A memory leak will occur, if it is not possible to allocate a new memory fragment. This type of an error will be considered later. Yes, the probability of a memory leak is extremely small, but to my mind, this is a real error that needs to be fixed.

It took me about a week to choose those warnings that, to my mind, point to real errors. With all that, I have also noted down a large amount of code fragments that I will use for preparation of presentations and articles.

Your attention, please. Further on in the article we will speak about the amount of errors, not about the number of the analyzer warnings. By saying "an error", I mean such code fragments that require fixing, in my view.

One of the developers, who has looked through our presentation and did not really think it through, commented something like "27 000 analyzer warnings isn't really an achievement, it's really not that much". So again, let me emphasize that we are talking about real errors. During the research, I was noting down and counting only errors, not just all the analyzer warnings.

Analysis of Projects Developed by Samsung Specialists

I have randomly chosen the following projects: : bluetooth-frwk-0.2.157, capi-appfw-application-0.5.5, capi-base-utils-3.0.0, capi-content-media-content-0.3.10, capi-maps-service-0.6.12, capi-media-audio-io-0.3.70, capi-media-codec-0.5.3, capi-media-image-util-0.1.15, capi-media-player-0.3.58, capi-media-screen-mirroring-0.1.78, capi-media-streamrecorder-0.0.10, capi-media-vision-0.3.24, capi-network-bluetooth-0.3.4, capi-network-http-0.0.23, cynara-0.14.10, e-mod-tizen-devicemgr-0.1.69, ise-engine-default-1.0.7, ise-engine-sunpinyin-1.0.10, ise-engine-tables-1.0.10, isf-3.0.186, org.tizen.app-selector-0.1.61, org.tizen.apps-0.3.1, org.tizen.bluetooth-0.1.2, org.tizen.browser-3.2.0, org.tizen.browser-profile_common-1.6.4, org.tizen.classic-watch-0.0.1, org.tizen.d2d-conv-setting-profile_mobile-1.0, org.tizen.d2d-conv-setting-profile_wearable-1.0, org.tizen.download-manager-0.3.21, org.tizen.download-manager-0.3.22, org.tizen.dpm-toolkit-0.1, org.tizen.elm-demo-tizen-common-0.1, org.tizen.indicator-0.2.53, org.tizen.inputdelegator-0.1.170518, org.tizen.menu-screen-1.2.5, org.tizen.myplace-1.0.1, org.tizen.privacy-setting-profile_mobile-1.0.0, org.tizen.privacy-setting-profile_wearable-1.0.0, org.tizen.quickpanel-0.8.0, org.tizen.screen-reader-0.0.8, org.tizen.service-plugin-sample-0.1.6, org.tizen.setting-1.0.1, org.tizen.settings-0.2, org.tizen.settings-adid-0.0.1, org.tizen.telephony-syspopup-0.1.6, org.tizen.voice-control-panel-0.1.1, org.tizen.voice-setting-0.0.1, org.tizen.volume-0.1.149, org.tizen.w-home-0.1.0, org.tizen.w-wifi-1.0.229, org.tizen.watch-setting-0.0.1, security-manager-1.2.17.

There is quite a big number of projects, but many of them have a very small size. Let's see which types of errors we managed to detect.

Note. In addition to the PVS-Studio warnings, I will try to classify the bugs found according to the CWE (Common Weakness Enumeration). However, I am not trying to find some vulnerabilities, I am providing CWE-ID solely for the convenience of those readers, who are used to this classification of defects. My aim is to find as many errors as possible, determining the extent, to which the error is dangerous from the security point of view, goes beyond my research.

This is going to be a long story, so I suggest making the first cup of tea or coffee. You will need another one later, as we are only in the beginning of the article.

A Typo in the Condition: the Same Code is Written to the Left and Right (2 errors)

Classic. The top-level classics, I should say.

Firstly, this error is detected by diagnostic V501. This diagnostic effectively detects typos and consequences of inattentive Copy-Paste. This is an extremely popular and widespread type of errors. You should definitely take a look at our great collection of bugs in open source projects that we gathered thanks to the V501 diagnostic.

Secondly, this error is in the "less than" operator. Incorrect comparison of two objects is also a classic error that occurs due to the fact that nobody checks these simple functions. Recently, I have written an interesting article on this topic: "Evil in the comparison functions". This is some kind of "At the Mountains of Madness" for programmers.

Here is the code I am talking about:

bool operator <(const TSegment& other) const { if (m_start < other.m_start) return true; if (m_start == other.m_start) return m_len < m_len; // <= return false; }

The error was found by the PVS-Studio warning: V501 There are identical sub-expressions to the left and to the right of the '<' operator: m_len < m_len segmentor.h 65

Software weaknesses type - CWE-570: Expression is Always False

Because of this error, objects that differ only in the value of the m_len member, will be compared incorrectly. Correct variant of the comparison:

return m_len < other.m_len;

A similar error: 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

A Typo in the Condition: Pointless Comparison (2 errors)

static void __page_focus_changed_cb(void *data) { int i = 0; int *focus_unit = (int *)data; if (focus_unit == NULL || focus_unit < 0) { // <= _E("focus page is wrong"); return ; } .... }

The error was found by the PVS-Studio warning: V503 This is a nonsensical comparison: pointer < 0. apps_view_circle_indicator.c 193

Software weaknesses type - CWE-697: Insufficient Comparison

The comparison "pointer < 0" is meaningless and indicates a typo in the code. As I understand, the indirection unary '*' operator is missing in the code, that was to dereference the pointer. Correct code:

if (focus_unit == NULL || *focus_unit < 0) {

This code was copied with a mistake, as a result of it, we can see the same bug in the function __page_count_changed_cb:

static void __page_count_changed_cb(void *data) { int i = 0; int *page_cnt = (int *)data; if (page_cnt == NULL || page_cnt < 0) { _E("page count is wrong"); return ; } .... }

Again this Copy-Paste method. The analyzer issued the following warning for this code: V503 This is a nonsensical comparison: pointer < 0. apps_view_circle_indicator.c 219

A Dangerous Way of Using alloca Function (1 error)

Let's take a look at a code fragment, which is bad, but will not lead to errors in practice. I didn't cover this case in the presentation, as it requires additional explanation. Now is a good time to do it and share my thoughts about it.

int audio_io_loopback_in_test() { .... while (1) { char *buffer = alloca(size); if ((ret = audio_in_read(input, (void *)buffer, size)) > AUDIO_IO_ERROR_NONE) { fwrite(buffer, size, sizeof(char), fp); printf("PASS, size=%d, ret=0x%x

", size, ret); } else { printf("FAIL, size=%d, ret=0x%x

", size, ret); } } .... }

PVS-Studio warning: V505 The 'alloca' function is used inside the loop. This can quickly overflow stack. audio_io_test.c 247

Software weaknesses type - CWE-770: Allocation of Resources Without Limits or Throttling

In the loop that runs until the audio-stream is over, we see the allocation of the stack memory by the function alloca. This code isn't great, as it can quickly run out of the stack memory.

However, I cannot say that I found a serious error. The thing is that this code is taken from tests. I am sure that the audio-stream is rather short in the tests and there should not be any errors in its processing.

Thus, it is not quite honest to say that this is an error, because the tests continue working.

However, I will not call this warning a false positive, because the code is really bad. In some time, there may be a need to run tests on the data of a larger size, which will cause a failure. At the same time the data flow does not have to be large. It is enough for the data to be the size of the free stack, and this is not much, as a rule.

Moreover, the code is easy to fix, which means that it should be done. It is enough to move the memory allocation outside the loop. This can be easily done, as the size of the allocated buffer does not change.

Here is an example of good code:

char *buffer = alloca(size); while (1) { if ((ret = audio_in_read(input, (void *)buffer, size)) > AUDIO_IO_ERROR_NONE) { fwrite(buffer, size, sizeof(char), fp); printf("PASS, size=%d, ret=0x%x

", size, ret); } else { printf("FAIL, size=%d, ret=0x%x

", size, ret); } }

A non-existing buffer being used (1 error)

The following code is also taken from tests, but it is much more serious. The error causes undefined behavior of a program, so this test cannot be trusted in any way. In other words, the test does not test anything.

void extract_input_aacdec_m4a_test( App * app, unsigned char **data, int *size, bool * have_frame) { .... unsigned char buffer[100000]; .... DONE: *data = buffer; *have_frame = TRUE; if (read_size >= offset) *size = offset; else *size = read_size; }

The error was found by the PVS-Studio warning: V507 Pointer to local array 'buffer' is stored outside the scope of this array. Such a pointer will become invalid. media_codec_test.c 793

Software weaknesses type - CWE-562: Return of Stack Variable Address

The function returns the address of the array, created on the stack. After the function exits, the array will be destroyed and the returned address from the function cannot be used.

More or less elements of the array are processed than it is needed (7 errors)

Firstly, let's consider a case when less elements get processed than it is needed.

typedef int gint; typedef gint gboolean; #define BT_REQUEST_ID_RANGE_MAX 245 static gboolean req_id_used[BT_REQUEST_ID_RANGE_MAX]; void _bt_init_request_id(void) { assigned_id = 0; memset(req_id_used, 0x00, BT_REQUEST_ID_RANGE_MAX); }

PVS-Studio warning: V512 A call of the 'memset' function will lead to underflow of the buffer 'req_id_used'. bt-service-util.c 38

Software weaknesses type - CWE-131: Incorrect Calculation of Buffer Size

Here the programmer forgot that the memset function takes the buffer size in bytes, but not the number of elements in the array. It was for a reason I called memset one of the most dangerous function in the world of programming in C/C++. This function continues to wreak havoc in various projects.

The gboolean type takes 4 bytes, not 1. As a result, only 1/4 of the array will be set to zero, other elements will remain uninitialized.

Correct variant of the code:

memset(req_id_used, 0x00, BT_REQUEST_ID_RANGE_MAX * sizeof(gboolean));

Or it is possible to write:

memset(req_id_used, 0x00, sizeof(req_id_used));

Now let's take a look at the case, when we may have array index out of bounds.

static void _on_atspi_event_cb(const AtspiEvent * event) { .... char buf[256] = "\0"; .... snprintf(buf, sizeof(buf), "%s, %s, ", name, _("IDS_BR_BODY_IMAGE_T_TTS")); .... snprintf(buf + strlen(buf), sizeof(buf), "%s, ", _("IDS_ACCS_BODY_SELECTED_TTS")); .... }

PVS-Studio warning: V512 A call of the 'snprintf' function will lead to overflow of the buffer 'buf + strlen(buf)'. app_tracker.c 450

Software weaknesses type - CWE-131: Incorrect Calculation of Buffer Size

A security operating system... Well...

Pay attention that the second call of snprintf should add something to the already existing string. That's why the buffer address is the expression buf + strlen(buf). And the function has a right to print less characters than the buffer size. We should subtract strlen(buf) from the size of the buffer. But it was forgotten and we can get a situation when the snprintf function writes data outside the array.

Correct code:

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

The third code fragment demonstrates a case when the array index out of bounds always occurs. First, let's take a look at some structures.

#define BT_ADDRESS_STRING_SIZE 18 typedef struct { unsigned char addr[6]; } bluetooth_device_address_t; typedef struct { int count; bluetooth_device_address_t addresses[20]; } bt_dpm_device_list_t;

Here it is important for us that the array addr consists of 6 elements. Remember this size, and that the macro BT_ADDRESS_STRING_SIZE expands to a constant 18.

Now here is incorrect code:

dpm_result_t _bt_dpm_get_bluetooth_devices_from_whitelist( GArray **out_param1) { dpm_result_t ret = DPM_RESULT_FAIL; bt_dpm_device_list_t device_list; .... for (; list; list = list->next, i++) { memset(device_list.addresses[i].addr, 0, BT_ADDRESS_STRING_SIZE); _bt_convert_addr_string_to_type(device_list.addresses[i].addr, list->data); } .... }

PVS-Studio warning: V512 A call of the 'memset' function will lead to overflow of the buffer 'device_list.addresses[i].addr'. bt-service-dpm.c 226

Software weaknesses type - CWE-805: Buffer Access with Incorrect Length Value

Here is the most important thing:

memset(device_list.addresses[i].addr, 0, BT_ADDRESS_STRING_SIZE);

So, as we saw earlier, the size of addr is just 6 bytes. At the same time the memset function sets to zero 18 bytes and as a result, we have array index out of bounds.

4 more bugs:

V512 A call of the 'memset' function will lead to overflow of the buffer 'device_list.addresses[i].addr'. bt-service-dpm.c 176

V512 A call of the 'memset' function will lead to underflow of the buffer 'formatted_number'. i18ninfo.c 544

V512 A call of the 'snprintf' function will lead to overflow of the buffer 'ret + strlen(ret)'. navigator.c 677

V512 A call of the 'snprintf' function will lead to overflow of the buffer 'trait + strlen(trait)'. navigator.c 514

A logic error in the sequences if .. else .. if (4 errors)

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

PVS-Studio warning: 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

Software weaknesses type - CWE-570 Expression is Always False

It's hard to say, where is an error here just by looking at the code. The thing is that the LANG_ES_MX and LANG_ES_US strings are identical. Here they are:

#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"

As I understand, they must be different. But since the strings are the same, the second condition will always be false and the function will never return the value "es_US".

Note. ES_MX - is Spanish (Mexico), ES_US - this is Spanish (United States).

This error was found in the project org.tizen.voice-setting-0.0.1. What's interesting, Copy-Paste fails again and exactly the same bug is in the project org.tizen.voice-control-panel-0.1.1.

Other errors:

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

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

Repeated assignment (11 errors)

Let's take a look at an error in the program logic. The developer wanted to exchange the values of two variables, but got confused and wrote the following code:

void isf_wsc_context_del (WSCContextISF *wsc_ctx) { .... WSCContextISF* old_focused = _focused_ic; _focused_ic = context_scim; _focused_ic = old_focused; .... }

PVS-Studio warning: V519 The '_focused_ic' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 1260, 1261. wayland_panel_agent_module.cpp 1261

Software weaknesses type - CWE-563 Assignment to Variable without Use ('Unused Variable')

The variable _focused_ic is assigned with different values twice. Correct code should be like this :

WSCContextISF* old_focused = _focused_ic; _focused_ic = context_scim; context_scim = old_focused;

However, it is better to use the function std::swap in such cases. Thus, there are less chances to make a mistake.

std::swap(_focused_ic, context_scim);

Let's consider another variant of an error that appeared upon writing similar code. Perhaps, Copy-Paste is here to blame again.

void create_privacy_package_list_view(app_data_s* ad) { .... Elm_Genlist_Item_Class *ttc = elm_genlist_item_class_new(); Elm_Genlist_Item_Class *ptc = elm_genlist_item_class_new(); Elm_Genlist_Item_Class *mtc = elm_genlist_item_class_new(); .... ttc->item_style = "title"; ttc->func.text_get = gl_title_text_get_cb; ttc->func.del = gl_del_cb; // <= ptc->item_style = "padding"; ptc->func.del = gl_del_cb; mtc->item_style = "multiline"; mtc->func.text_get = gl_multi_text_get_cb; ttc->func.del = gl_del_cb; // <= .... }

PVS-Studio warning: V519 The 'ttc->func.del' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 409, 416. privacy_package_list_view.c 416

Software weaknesses type - CWE-563 Assignment to Variable without Use ('Unused Variable')

In the latter case, the value should be assigned to the variable mtc->func.del.

Other errors:

V519 The 'item' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 1176, 1189. w-input-stt-voice.cpp 1189

V519 The 'ad->paired_item' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 1614, 1617. bt-main-view.c 1617

V519 The 'ret' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 191, 194. main.c 194

V519 The 'ret' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 372, 375. setting-about-status.c 375

V519 The 'ret' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 91, 100. setting-applications-defaultapp.c 100

V519 The 'ret' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 52, 58. smartmanager-battery.c 58

V519 The 'ret' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 853, 872. setting-time-main.c 872

V519 The 'ret_str' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 2299, 2300. setting-password-sim.c 2300

V519 The 'display_status' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 110, 117. view_clock.c 117

Viewing the analyzer log, I noticed only 11 fragments that need fixing. In fact, there were a lot more V519 warnings. Often they referred to the code when the result was stored in the variable many times in a row after the function call. We are talking about the following code:

status = Foo(0); status = Foo(1); status = Foo(2);

This code usually occurs in two cases:

The result of the function call isn't important, but some compiler or an analyzer issued a warning, that the programmer got rid of by writing a result to the variable. To my mind, it would be better to write (void)Foo(x);.

The result of the function call isn't important, but sometimes can be useful for debugging. It's much easier to debug the code, when the result is written to some variable.

I am writing about this moment, as this code isn't as safe as it may seem at the first glance. Perhaps, in some fragments the result that the functions returned lacks attention and is not checked. I looked through the code quite fast and did not go deep to see how it works. I think if we take a closer look at these warnings, there will be a chance to find way more defects.

Dereference of a (Potentially) Null Pointer (88 errors in total)

The use of null pointers is detected by V522 and V575 diagnostics. The warning V522 is issued when there is dereference of a pointer that may be null (*MyNullPtr = 2;). V575 - when a potentially null pointer is passed to a function inside of which it can be dereferenced (s = strlen(MyNullPtr);). Actually, V575 gets issued for some other cases, when a programmer uses incorrect arguments, but we are not interested in that at this point. From the point of view of this article, there is no difference between V522 and V575, that is why they will be considered in this chapter together.

Another story will be about such functions as malloc, realloc, strdup. We should check the pointers against NULL equality because of the possible situations when functions could not allocate the memory.

However, some programmers adhere to bad practices and intentionally never write checks. Their logic is that if there is no memory, then there is no need to worry, let the program crash. I believe that this approach is not great, but it is there and I heard arguments defending it.

Fortunately, the Tizen developers are not of that kind and usually check, if the memory was allocated or not. Sometimes they do this even where it is not necessary:

static FilterModule *__filter_modules = 0; static void __initialize_modules (const ConfigPointer &config) { .... __filter_modules = new FilterModule [__number_of_modules]; if (!__filter_modules) return; .... }

There is no sense in such a check, as in case when the program will fail to allocate the memory, the new operator will throw an exception std::bad_alloc. However, that is another story. I cited this code just to show that it is a usual practice for the Tizen developers to check if the memory was allocated.

Still, PVS-Studio detects that there are not enough checks in a lot of places. Here I will discuss only one case, because in general they are all the same.

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

PVS-Studio warning: V522 There might be dereferencing of a potential null pointer 'msg'. QuickAccess.cpp 743

Software weaknesses type - CWE-690: Unchecked Return Value to NULL Pointer Dereference

There is no guarantee that the malloc function will allocate the memory. Yes, the probability of such an event is extremely small, but if there are checks of the pointers for NULL in other fragments, it should be here too. That is why I think that the code contains a real error that needs to be fixed.

However, null pointers can return not only the functions that allocate memory. There are other situations when you should check a pointer before using it. Let's look at a couple of such examples. The first is related to the unsafe use of dynamic_cast operator.

int cpp_audio_in_peek(audio_in_h input, const void **buffer, unsigned int *length) { .... CAudioInput* inputHandle = dynamic_cast<CAudioInput*>(handle->audioIoHandle); assert(inputHandle); inputHandle->peek(buffer, &_length); .... }

PVS-Studio warning: V522 There might be dereferencing of a potential null pointer 'inputHandle'. cpp_audio_io.cpp 928

Software weaknesses type - CWE-690: Unchecked Return Value to NULL Pointer Dereference

Strange code. If you are sure that the handle->audioIoHandle stores a pointer to an object of CAudioInput type, you must use static_cast. If there is no such certainty, then the check is necessary, as the assert macro will not help in the release version.

I think it is reasonable to add this check:

CAudioInput* inputHandle = dynamic_cast<CAudioInput*>(handle->audioIoHandle); if (inputHandle == nullptr) { assert(false); THROW_ERROR_MSG_FORMAT( CAudioError::EError::ERROR_INVALID_HANDLE, "Handle is NULL"); }

By the way, similar code is written in other functions. So, the analyzer really found a flaw in the program.

The following code may not lead to a real error. Suppose, now the program always processes such strings, which have '-' and '.'. However, I hope you would agree that code is dangerous and it is better to play it safe. I chose it to demonstrate the diversity of situations, when the analyzer issues warnings.

int main(int argc, char *argv[]) { .... char *temp1 = strstr(dp->d_name, "-"); char *temp2 = strstr(dp->d_name, "."); strncpy(temp_filename, dp->d_name, strlen(dp->d_name) - strlen(temp1)); strncpy(file_format, temp2, strlen(temp2)); .... }

PVS-Studio warnings:

V575 The potential null pointer is passed into 'strlen' function. Inspect the first argument. image_util_decode_encode_testsuite.c 207

V575 The potential null pointer is passed into 'strlen' function. Inspect the first argument. image_util_decode_encode_testsuite.c 208

Software weaknesses type - CWE-690: Unchecked Return Value to NULL Pointer Dereference

The pointers temp1 and temp2 can become null, if the symbols '-' and '.' are not in the string. In this case later we'll have null pointer dereference.

There are 84 more code fragments, where the pointers that may be NULL get dereferenced. There is no sense to consider them in the article. There is even no point in providing a list of them, as it will still take a lot of space. That's why I've put these warning in a separate file: Tizen_V522_V575.txt.

Identical Actions (6 errors)

static void _content_resize(...., const char *signal) { .... if (strcmp(signal, "portrait") == 0) { evas_object_size_hint_min_set(s_info.layout, ELM_SCALE_SIZE(width), ELM_SCALE_SIZE(height)); } else { evas_object_size_hint_min_set(s_info.layout, ELM_SCALE_SIZE(width), ELM_SCALE_SIZE(height)); } .... }

PVS-Studio warning: V523 The 'then' statement is equivalent to the 'else' statement. page_setting_all.c 125

Software weaknesses type - I do not know how to classify it, I'll be really grateful for a hint.

Regardless of the conditions, two similar actions are carried out. As I understand, in one of the two calls of the evas_object_size_hint_min_set functions, we should change width and height.

Let's take a look at an error of this kind:

static Eina_Bool _move_cb(void *data, int type, void *event) { Ecore_Event_Mouse_Move *move = event; mouse_info.move_x = move->root.x; mouse_info.move_y = move->root.y; if (mouse_info.pressed == false) { return ECORE_CALLBACK_RENEW; // <= } return ECORE_CALLBACK_RENEW; // <= }

PVS-Studio warning: V523 The 'then' statement is equivalent to the subsequent code fragment. mouse.c 143

Software weaknesses type - CWE-393 Return of Wrong Status Code

It's very strange that the function does some check, but it still returns a value ECORE_CALLBACK_RENEW. I think the return values must be different.

Other errors of this type:

V523 The 'then' statement is equivalent to the 'else' statement. bt-httpproxy.c 383

V523 The 'then' statement is equivalent to the 'else' statement. streamrecorder_test.c 342

V523 The 'then' statement is equivalent to the 'else' statement. ise-stt-option.cpp 433

V523 The 'then' statement is equivalent to the subsequent code fragment. dpm-toolkit-cli.c 87

The Pointer was not Dereferenced (1 error)

A very beautiful error: the data is written out of place.

int _read_request_body(http_transaction_h http_transaction, char **body) { .... *body = realloc(*body, new_len + 1); .... memcpy(*body + curr_len, ptr, body_size); body[new_len] = '\0'; // <= curr_len = new_len; .... }

PVS-Studio warning: V527 It is odd that the '\0' value is assigned to 'char' type pointer. Probably meant: *body[new_len] = '\0'. http_request.c 370

Software weaknesses type - CWE-787: Out-of-bounds Write

The function takes a pointer to a pointer. This allows to reallocate the memory and return the address of a new string.

The error is in the line:

body[new_len] = '\0';

It turns out that a pointer to a pointer is interpreted as an array of pointers. There is no array of course. That's why NULL ('\0' in this case is interpreted as a null pointer) will be written out of place. Some unknown memory block gets damaged.

In addition, there is another error. The line won't end with a terminal null. So, the situation isn't really great.

Correct code:

(*body)[new_len] = '\0';

The Condition is Always True/False (9 errors)

There are many reasons causing an error, when a condition is always true or false, but in this article, I will consider only three variants of how the error can appear.

The first variant.

unsigned m_candiPageFirst; bool CIMIClassicView::onKeyEvent(const CKeyEvent& key) { .... if (m_candiPageFirst > 0) { m_candiPageFirst -= m_candiWindowSize; if (m_candiPageFirst < 0) m_candiPageFirst = 0; changeMasks |= CANDIDATE_MASK; } .... }

PVS-Studio warning: V547 Expression 'm_candiPageFirst < 0' is always false. Unsigned type value is never < 0. imi_view_classic.cpp 201

Software weaknesses type - CWE-570: Expression is Always False

The variable m_candiPageFirst has unsigned type. Therefore, the value of this variable cannot be less than zero. To protect the code against overflow, it should be rewritten like this:

if (m_candiPageFirst > 0) { if (m_candiPageFirst > m_candiWindowSize) m_candiPageFirst -= m_candiWindowSize; else m_candiPageFirst = 0; changeMasks |= CANDIDATE_MASK; }

The second variant:

void QuickAccess::_grid_mostVisited_del(void *data, Evas_Object *) { BROWSER_LOGD("[%s:%d]", __PRETTY_FUNCTION__, __LINE__); if (data) { auto itemData = static_cast<HistoryItemData*>(data); if (itemData) delete itemData; } }

PVS-Studio warning: V547 Expression 'itemData' is always true. QuickAccess.cpp 571

Software weaknesses type - CWE-571: Expression is Always True

This is a very suspicious code fragment. If the pointer data != nullptr, then the pointer is itemData != nullptr. Therefore, the second check is meaningless. Here we have one of two situations:

This is an error. Instead of the static_cast operator, we should use an operator dynamic_cast, which can return nullptr.

There is no real error here, it is a just messy code. The second check is just not necessary and it should be removed, so that it does not confuse code analyzers and other programmers.

It is hard for me to say, whether we should choose the 1 or the 2 point, but this code should be corrected.

The third variant.

typedef enum { BT_HID_MOUSE_BUTTON_NONE = 0x00, BT_HID_MOUSE_BUTTON_LEFT = 0x01, BT_HID_MOUSE_BUTTON_RIGHT = 0x02, BT_HID_MOUSE_BUTTON_MIDDLE = 0x04 } bt_hid_mouse_button_e; int bt_hid_device_send_mouse_event(const char *remote_address, const bt_hid_mouse_data_s *mouse_data) { .... if (mouse_data->buttons != BT_HID_MOUSE_BUTTON_LEFT || mouse_data->buttons != BT_HID_MOUSE_BUTTON_RIGHT || mouse_data->buttons != BT_HID_MOUSE_BUTTON_MIDDLE) { return BT_ERROR_INVALID_PARAMETER; } .... }

PVS-Studio warning: V547 Expression is always true. Probably the '&&' operator should be used here. bluetooth-hid.c 229

Software weaknesses type - CWE-571: Expression is Always True

To understand, where the mistake is here, I will put the values of the constants and shorten the code:

if (buttons != 1 || buttons != 2 || buttons != 4) {

Regardless of the value, stored in the variable, it will always be not 1, 2 or 4.

Other errors:

V547 Expression 'ad->transfer_info' is always true. bt-share-ui-popup.c 56

V547 Expression 'item_name' is always true. SettingsUI.cpp 222

V547 Expression 'item_name' is always true. SettingsUI.cpp 226

V547 Expression '!urlPair' is always false. GenlistManager.cpp 143

V547 Expression 'strlen(s_formatted) < 128' is always true. clock.c 503

V547 Expression 'doc != ((void *) 0)' is always true. setting-common-data-slp-setting.c 1450

Confusion with Enum (18 errors)

I have not provided this kind of errors in the presentation, as the examples are too long, but in the article, I think it makes sense to write about them.

There are two types of enum, where there are constants with similar names declared:

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;

It is not surprising that one can be lost in the names and write such code:

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

Variable level type is wifi_manager_rssi_level_e. Constants' type is wifi_rssi_level_e. It turns out that there are five wrong comparisons at once that is why the analyzer issues five warnings:

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

Software weaknesses type - CWE-697: Insufficient Comparison

What is funny is that this code works exactly as the programmer intended. Thanks to luck, the constant WIFI_MANAGER_RSSI_LEVEL_0 is equal to WIFI_RSSI_LEVEL_0, and so on.

Despite the fact that the code works at the moment, it is wrong and it should be corrected. There are two reasons for this:

This code looks weird to the analyzer and it means that it will look confusing to the programmers who will maintain the code.

If at least one of the enum values changes over time, the constants will no longer match and the program will perform incorrectly.

Other incorrect comparisons:

V556 The values of different enum types are compared: switch(ENUM_TYPE_A) { case ENUM_TYPE_B: ... }. e_devicemgr_video.c 885

V556 The values of different enum types are compared: switch(ENUM_TYPE_A) { case ENUM_TYPE_B: ... }. e_devicemgr_video.c 889

V556 The values of different enum types are compared: switch(ENUM_TYPE_A) { case ENUM_TYPE_B: ... }. e_devicemgr_video.c 892

V556 The values of different enum types are compared: switch(ENUM_TYPE_A) { case ENUM_TYPE_B: ... }. e_devicemgr_video.c 895

V556 The values of different enum types are compared: switch(ENUM_TYPE_A) { case ENUM_TYPE_B: ... }. e_devicemgr_video.c 898

V556 The values of different enum types are compared: switch(ENUM_TYPE_A) { case ENUM_TYPE_B: ... }. e_devicemgr_video.c 901

V556 The values of different enum types are compared: switch(ENUM_TYPE_A) { case ENUM_TYPE_B: ... }. e_devicemgr_video.c 904

V556 The values of different enum types are compared: switch(ENUM_TYPE_A) { case ENUM_TYPE_B: ... }. e_devicemgr_video.c 907

V556 The values of different enum types are compared: switch(ENUM_TYPE_A) { case ENUM_TYPE_B: ... }. myplace-placelist.c 239

V556 The values of different enum types are compared: switch(ENUM_TYPE_A) { case ENUM_TYPE_B: ... }. myplace-placelist.c 253

V556 The values of different enum types are compared: switch(ENUM_TYPE_A) { case ENUM_TYPE_B: ... }. myplace-placelist.c 264

V556 The values of different enum types are compared: switch(ENUM_TYPE_A) { case ENUM_TYPE_B: ... }. telephony_syspopup_noti.c 82

V556 The values of different enum types are compared: switch(ENUM_TYPE_A) { case ENUM_TYPE_B: ... }. telephony_syspopup_noti.c 87

A Part of the Condition is Always True/False (2 errors)

I noticed only two such errors, but they are both interesting, so let's take a look at them.

int bytestream2nalunit(FILE * fd, unsigned char *nal) { unsigned char val, zero_count, i; .... val = buffer[0]; while (!val) { // <= if ((zero_count == 2 || zero_count == 3) && val == 1) // <= break; zero_count++; result = fread(buffer, 1, read_size, fd); if (result != read_size) break; val = buffer[0]; } .... }

PVS-Studio warning: V560 A part of conditional expression is always false: val == 1. player_es_push_test.c 284

Software weaknesses type - CWE-570: Expression is Always False

The loop runs until the variable val is zero. At the beginning of the loop, the variable val is compared with the value 1. Certainly, the variable val could never be equal to 1, otherwise the loop would already stop. Here is the logical error.

Now let's take a look at another mistake.

const int GT_SEARCH_NO_LONGER = 0, GT_SEARCH_INCLUDE_LONGER = 1, GT_SEARCH_ONLY_LONGER = 2; bool GenericTableContent::search (const String &key, int search_type) const { .... else if (nkeys.size () > 1 && GT_SEARCH_ONLY_LONGER) { .... }

PVS-Studio warning: V560 A part of conditional expression is always true: GT_SEARCH_ONLY_LONGER. scim_generic_table.cpp 1884

Software weaknesses type - CWE-571: Expression is Always True

The constant GT_SEARCH_ONLY_LONGER is part of the condition. This is very odd, and I suspect that the condition should actually look like this:

if (nkeys.size () > 1 && search_type == GT_SEARCH_ONLY_LONGER)

Confusion with Types of Created and Destroyed Objects (4 Mistakes)

Three structures are declared and they are not related to each other at all:

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

The error lies in the fact that the objects are created as objects of one type and are destroyed as of another type:

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; // <= } };

Analyzer warning:

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

Software weaknesses type - CWE-762: Mismatched Memory Management Routines.

Structures of type sockaddr_un and sockaddr_in are created. However, they are stored and destroyed as sockaddr structures. All three types of the mentioned structures are not related among themselves. Three different structures have different sizes. Now the code may work well, because these structures are of POD types (do not contain destructors, etc.) and the call of the delete operator becomes a simple call of the free function. Formally, the code is incorrect. One has to destroy an object of the same type that was used while creating the object.

As I said, at this moment the program is working, although formally it is incorrect. One has to understand that the considered code is very dangerous as it is enough for one of the classes of constructor/destructor to appear or to add a member of a complex type (for example, std::string) to break down everything completely.

Other errors:

V572 It is odd that the object which was created using 'new' operator is immediately cast to another type. scim_socket.cpp 167

V572 It is odd that the object which was created using 'new' operator is immediately cast to another type. scim_socket.cpp 171

Incorrect Using of printf-like Functions (4 bugs)

static int _write_file(const char *file_name, void *data, unsigned long long data_size) { FILE *fp = NULL; if (!file_name || !data || data_size <= 0) { fprintf(stderr, "\tinvalid data %s %p size:%lld

", file_name, data, data_size); return FALSE; } .... }

PVS-Studio warning: V576 Incorrect format. Consider checking the third actual argument of the 'fprintf' function. Under certain conditions, the pointer can be null. image_util_decode_encode_testsuite.c 124

Software weaknesses type - CWE-476: NULL Pointer Dereference

It is possible that a pointer file_name will contain NULL. It is impossible to predict how function printf will work. In practice, its behavior depends on the used implementation of printf. See the discussion of "What is the behavior of NULL with printf's printing %s specifier?".

Let's look at one more mistake.

void subscribe_to_event() { .... int error = ....; .... PRINT_E("Failed to destroy engine configuration for event trigger.", error); .... }

PVS-Studio warning: V576 Incorrect format. A different number of actual arguments is expected while calling 'printf' function. Expected: 1. Present: 2. surveillance_test_suite.c 393

Software weaknesses type - I do not know exactly how to classify it, I would be grateful for a hint.

Macro PRINT_E expands into printf. As you can see, the error variable is not used at all. Apparently, one forgot to print the error number.

Other errors:

V576 Incorrect format. A different number of actual arguments is expected while calling 'printf' function. Expected: 1. Present: 2. surveillance_test_suite.c 410

V576 Incorrect format. A different number of actual arguments is expected while calling 'printf' function. Expected: 1. Present: 2. surveillance_test_suite.c 417

Checking of the Pointer is Executed after its Dereference (5 errors)

static void _show(void *data) { SETTING_TRACE_BEGIN; struct _priv *priv = (struct _priv *)data; Eina_List *children = elm_box_children_get(priv->box); // <= Evas_Object *first = eina_list_data_get(children); Evas_Object *selected = eina_list_nth(children, priv->item_selected_on_show); // <= if (!priv) { // <= _ERR("Invalid parameter."); return; } .... }

PVS-Studio warning: V595 The 'priv' pointer was utilized before it was verified against nullptr. Check lines: 110, 114. view_generic_popup.c 110

Software weaknesses type - CWE-476: NULL Pointer Dereference

The pointer priv is dereferenced twice in the expressions.

priv->box

priv->item_selected_on_show

Only after that, the pointer is verified against null. To fix the code, the check should be moved higher in the code:

static void _show(void *data) { SETTING_TRACE_BEGIN; struct _priv *priv = (struct _priv *)data; if (!priv) { _ERR("Invalid parameter."); return; } Eina_List *children = elm_box_children_get(priv->box); Evas_Object *first = eina_list_data_get(children); Evas_Object *selected = eina_list_nth(children, priv->item_selected_on_show); .... }

Now let's look at a more difficult case.

There is a function _ticker_window_create, in which the pointer, passed into the function as an argument, is dereferenced.

static Evas_Object *_ticker_window_create(struct appdata *ad) { .... // there is no check of the 'ad' pointer .... evas_object_resize(win, ad->win.w, indicator_height_get()); .... }

It is important to note that the pointer is dereferenced without checking for NULL. In other words, one can pass only non-null pointers into the function _ticker_window_create. Now let's see, how this function is actually used.

static int _ticker_view_create(void) { if (!ticker.win) ticker.win = _ticker_window_create(ticker.ad); // <= if (!ticker.layout) ticker.layout = _ticker_layout_create(ticker.win); if (!ticker.scroller) ticker.scroller = _ticker_scroller_create(ticker.layout); evas_object_show(ticker.layout); evas_object_show(ticker.scroller); evas_object_show(ticker.win); if (ticker.ad) // <= util_signal_emit_by_win(&ticker.ad->win, "message.show.noeffect", "indicator.prog"); .... }

PVS-Studio warning: V595 The 'ticker.ad' pointer was utilized before it was verified against nullptr. Check lines: 590, 600. ticker.c 590

Software weaknesses type - CWE-476: NULL Pointer Dereference

ticker.ad pointer is passed to the _ticker_window_create function. There is a check "if (ticket.ad)" below, which indicates that this pointer may be null.

Other errors:

V595 The 'core' pointer was utilized before it was verified against nullptr. Check lines: 2252, 2254. media_codec_port_gst.c 2252

V595 The 'eyeCondition' pointer was utilized before it was verified against nullptr. Check lines: 162, 168. FaceEyeCondition.cpp 162

V595 The 'dev->name' pointer was utilized before it was verified against nullptr. Check lines: 122, 127. e_devicemgr_device.c 122

Not Deleted Private Data (1 error)

static void SHA1Final(unsigned char digest[20], SHA1_CTX* context) { u32 i; unsigned char finalcount[8]; .... memset(context->count, 0, 8); memset(finalcount, 0, 8); }

PVS-Studio warning: V597 The compiler could delete the 'memset' function call, which is used to flush 'finalcount' buffer. The memset_s() function should be used to erase the private data. wifi_generate_pin.c 185

Software weaknesses type - CWE-14: Compiler Removal of Code to Clear Buffers

The compiler may remove the memset function that erases your private data in the buffer finalcount. In terms of C and C++ languages, a function call can be removed because the buffer is not used anywhere else. I would like to note that this is not only theoretically possible compiler way of working but a common thing. Compilers really remove such functions (see. V597, CWE-14).

Confusion with Memory Allocation and Release (2 errors)

The first error.

void GenericTableContent::set_max_key_length (size_t max_key_length) { .... std::vector<uint32> *offsets; std::vector<OffsetGroupAttr> *offsets_attrs; offsets = new(std::nothrow) // <= std::vector <uint32> [max_key_length]; if (!offsets) return; offsets_attrs = new(std::nothrow) std::vector <OffsetGroupAttr> [max_key_length]; if (!offsets_attrs) { delete offsets; // <= return; } .... }

PVS-Studio warning: V611 The memory was allocated using 'new T[]' operator but was released using the 'delete' operator. Consider inspecting this code. It's probably better to use 'delete [] offsets;'. scim_generic_table.cpp 998

Software weaknesses type - CWE-762: Mismatched Memory Management Routines

A pointer to an array of objects created using the new[] operator is stored in the variable offsets. This means these objects must be destroyed using operator delete[].

The second error.

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

PVS-Studio warning: 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

Software weaknesses type - CWE-762: Mismatched Memory Management Routines

The buffer memory is allocated on the stack. Further on, it is possible that the address of this buffer is passed as an actual argument into the function free, which is not allowed.

Potentially Uninitialized Variable (1 error)

The body of the function _app_create, which has an error is very long, so I will highlight only the very essence of it:

Eext_Circle_Surface *surface; .... if (_WEARABLE) surface = eext_circle_surface_conformant_add(conform); .... app_data->circle_surface = surface;

PVS-Studio warning: V614 Potentially uninitialized pointer 'surface' used. w-input-selector.cpp 896

Software weaknesses type - CWE-457: Use of Uninitialized Variable

Variable surface is initialized only if the condition "if (_WEARABLE)" is performed.

The Code Is Not Protected Against Incorrect Strings (6)

I did not pay much attention first to this kind of defect and did not note a number of warnings. That is why there can be not only 6 cases but much more. I was not interested in returning to the analyzer reports I have seen, so let there be only 6 defects.

void ise_show_stt_mode(Evas_Object *win) { .... snprintf(buf, BUF_LEN, gettext("IDS_ST_SK_CANCEL")); .... }

PVS-Studio warning: V618 It's dangerous to call the 'snprintf' function in such a manner, as the line being passed could contain format specification. The example of the safe code: printf("%s", str); ise-stt-mode.cpp 802

Software weaknesses type - CWE-134 Use of Externally-Controlled Format String

The code is working correctly, but it is very unreliable and dangerous for two reasons:

The program will be broken if someone uses the format specifier characters in IDS_ST_SK_CANCEL later. An abstract example: one needs to display the message "bla-bla %SystemDrive% bla-bla". Then %S will be perceived as a format specifier, which will print abracadabra or an access violation.

This weakness can be exploited. If an attacker replaced the string that you want to print, he could potentially use this to his benefit.

In any case, the operating system, claiming to be secure, should not have such code, especially when the situation is very easy to fix. It's enough to write:

snprintf(buf, BUF_LEN, "%s", gettext("IDS_ST_SK_CANCEL"));

Other weaknesses:

V618 It's dangerous to call the 'snprintf' function in such a manner, as the line being passed could contain format specification. The example of the safe code: printf("%s", str); app_tracker.c 459

V618 It's dangerous to call the 'snprintf' function in such a manner, as the line being passed could contain format specification. The example of the safe code: printf("%s", str); screen_reader_system.c 443

V618 It's dangerous to call the 'snprintf' function in such a manner, as the line being passed could contain format specification. The example of the safe code: printf("%s", str); screen_reader_system.c 447

V618 It's dangerous to call the 'snprintf' function in such a manner, as the line being passed could contain format specification. The example of the safe code: printf("%s", str); navigator.c 550

V618 It's dangerous to call the 'snprintf' function in such a manner, as the line being passed could contain format specification. The example of the safe code: printf("%s", str); navigator.c 561

Self-made Declaration of the Mathematical Constants (2 errors)

#define PI 3.141592 void __apps_view_circle_get_pos( int radius, double angle, int *x, int *y) { *x = radius * sin(angle * PI / 180); *y = radius * cos(angle * PI / 180); *x = *x + WINDOW_CENTER_X; *y = WINDOW_CENTER_Y - *y; }

PVS-Studio warnings:

V624 The constant 3.141592 is being utilized. The resulting value could be inaccurate. Consider using the M_PI constant from <math.h>. apps_view_circle.c 306

V624 The constant 3.141592 is being utilized. The resulting value could be inaccurate. Consider using the M_PI constant from <math.h>. apps_view_circle.c 307

Software weaknesses type - I do not know exactly how to classify it, I would be grateful for a hint.

I admit that this situation can be hardly called erroneous. The precision of the constant 3.141592 is more than enough for any practical calculations.

However, I believe that this code should be modified. Macro PI is odd and simply should not be written. In such cases, there is a standard macro M_PI, which expands into a more accurate value.

Dangerous Arithmetic (4 bugs)

__extension__ typedef long int __time_t; __extension__ typedef long int __suseconds_t; struct timeval { __time_t tv_sec; __suseconds_t tv_usec; }; static struct timeval _t0 = {0, 0}; static struct timeval _t1; void ISF_PROF_DEBUG_TIME (....) { float etime = 0.0; .... etime = ((_t1.tv_sec * 1000000 + _t1.tv_usec) - (_t0.tv_sec * 1000000 + _t0.tv_usec))/1000000.0; .... }

PVS-Studio warning: V636 The '_t1.tv_sec * 1000000' expression was implicitly cast from 'long' type to 'float' type. Consider utilizing an explicit type cast to avoid overflow. An example: double A = (double)(X) * Y;. scim_utility.cpp 1492

Software weaknesses type - CWE-681: Incorrect Conversion between Numeric Types

The number of seconds is calculated between two timestamps. Calculations are conducted in microseconds and for that the number of seconds is multiplied by a million. Calculations are conducted in the long type, which is 32-bit in 32-bit system Tizen. Here the overflow may occur very easily. To avoid this, you should use the type long long or double for calculations.

Other errors:

V636 The 'w / 2' expression was implicitly cast from 'int' type to 'float' type. Consider utilizing an explicit type cast to avoid the loss of a fractional part. An example: double A = (double)(X) / Y;. poly_shape_hit_test.cpp 97

V636 The 'w / 2' expression was implicitly cast from 'int' type to 'float' type. Consider utilizing an explicit type cast to avoid the loss of a fractional part. An example: double A = (double)(X) / Y;. poly_shape_hit_test.cpp 98

V636 The 'duration / 1000' expression was implicitly cast from 'int' type to 'double' type. Consider utilizing an explicit type cast to avoid the loss of a fractional part. An example: double A = (double)(X) / Y;. e_devicemgr_device.c 648

The code doesn't look the way it works (error 2)

In the first case, despite the error, the code works correctly. Yes, there are such lucky coincidences.

int bt_tds_provider_send_activation_resp( char *address, int result, bt_tds_provider_h provider) { .... if (error_code != BT_ERROR_NONE) BT_ERR("%s(0x%08x)", _bt_convert_error_to_string(error_code), error_code); return error_code; return error_code; }

PVS-Studio warning: V640 The code's operational logic does not correspond with its formatting. The statement is indented to the right, but it is always executed. It is possible that curly brackets are missing. bluetooth-tds.c 313

Software weaknesses type - CWE-483: Incorrect Block Delimitation

The programmer was lucky, because regardless of conditions, the program should return the same value. Here the programmer forgot curly brackets. Then the correct code should be as follows:

if (error_code != BT_ERROR_NONE) { BT_ERR("%s(0x%08x)", _bt_convert_error_to_string(error_code), error_code); return error_code; } return error_code;

Or you can remove a single return and make your code shorter:

if (error_code != BT_ERROR_NONE) BT_ERR("%s(0x%08x)", _bt_convert_error_to_string(error_code), error_code); return error_code;

Now let's look at a more interesting case. This error occurs because of this macro:

#define MC_FREEIF(x) \ if (x) \ g_free(x); \ x = NULL;

Now let's see how the macro is used:

static gboolean __mc_gst_init_gstreamer() { .... int i = 0; .... for (i = 0; i < arg_count; i++) MC_FREEIF(argv2[i]); .... }

PVS-Studio warning: V640 The code's operational logic does not correspond with its formatting. The second statement will always be executed. It is possible that curly brackets are missing. media_codec_port_gst.c 1800

Software weaknesses type - CWE-483: Incorrect Block Delimitation, CWE-787: Out-of-bounds Write

When you expand a macro, you get this code:

for (i = 0; i < arg_count; i++) if (argv2[i]) g_free(argv2[i]); argv2[i] = NULL;

The result:

The pointers will not be nullified.

NULL will be written outside the array bound.

The Loss of High Bits (1 error)

typedef unsigned char Eina_Bool; static Eina_Bool _state_get(....) { .... if (!strcmp(part, STATE_BROWSER)) return !strcmp(id, APP_ID_BROWSER); else if (!strcmp(part, STATE_NOT_BROWSER)) return strcmp(id, APP_ID_BROWSER); .... }

PVS-Studio warning: V642 Saving the 'strcmp' function result inside the 'unsigned char' type variable is inappropriate. The significant bits could be lost breaking the program's logic. grid.c 137

Software weaknesses type - CWE-197: Numeric Truncation Error

The strcmp function returns the following values of int type:

< 0 - buf1 less than buf2;

0 - buf1 identical to buf2;

> 0 - buf1 greater than buf2;

Please pay attention. "Greater than 0" means any number, but not only 1. These numbers can be: 2, 3, 100, 256, 1024, 5555 and so on. Similar is the case with "less than 0". Hence, the result cannot be placed in a variable of type unsigned char, since the significant bits can be lost. This would violate the logic of program execution, for instance, the number of 256 will turn into 0.

This risk may seem far-fetched. However, this error was caused by a serious vulnerability in MySQL/MariaDB to 5.1.61, 5.2.11, 5.3.5, 5.5.22. The thing is that when a user connects to MySQL/MariaDB, a token is evaluated (SHA from the password and hash) and then compared with the expected value of memcmp function. But on some platforms the return value can fall out from the range [-128..127]. As a result, in case 1 of 256 hash the comparison procedure with an expected value always returns true, regardless of the hash. Eventually, a simple command on bash gives an attacker the root access to vulnerable MySQL server, even if he does not know the password. The reason for this became the following code in the file 'sql/password.c':

typedef char my_bool; ... my_bool check(...) { return memcmp(...); }

A more detailed description of this issue can be found here: Security vulnerability in MySQL/MariaDB.

Let's get back to Tizen project. It seems to me, in this code fragment, the negation operator '!' is missing. Then the correct code should be as follows:

else if (!strcmp(part, STATE_NOT_BROWSER)) return !strcmp(id, APP_ID_BROWSER);

Off-by-one Error (2 errors)

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

PVS-Studio warning: 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

Software weaknesses type - CWE-193: Off-by-one Error

The programmer did not consider that the third argument of the strncat function sets how many more characters can be added to the string, not including the terminal null. I will explain this error on a simpler example:

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

There is no longer space for new characters in the buffer. It contains four characters and the terminal null. The expression 5-strlen (buf) is equal to 1. The strncpy function copies the character E to the last element of the array. Terminal 0 will be written outside the buffer.

The correct variant of the code:

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

Another similar error: V645 The 'strncat' function call could lead to the 'dd_info->name' buffer overflow. The bounds should not contain the size of the buffer, but a number of characters it can hold. oma-parser-dd1.c 433

Insidious C Language. An Undeclared Function is Used (1 error)

void person_recognized_cb( mv_surveillance_event_trigger_h handle, mv_source_h source, int video_stream_id, mv_surveillance_result_h event_result, void *user_data) { .... int *labels = malloc(sizeof(int) * number_of_persons); .... }

PVS-Studio warning: V647 The value of 'int' type is assigned to the pointer of 'int' type. surveillance_test_suite.c 928

Software weaknesses type - CWE-822: Untrusted Pointer Dereference

Here is the hidden trap. It will "spring" when the code turns into 64-bit Tizen operating system.

The thing that the malloc function has not been declared, so there is no #include <stdlib.h> anywhere. You can verify this by performing preprocessing and looking inside i-file. If the function is not declared, it shall be deemed that it returns an int type. The analyzer warns exactly about this, saying that an int value is converted to a pointer.

In the 32-bit system, everything is correct, because the pointer size matches the size of int. Error may reveal itself in the 64-bit program, where the significant bits of the pointer will be lost. More about this error is written in the article "A collection of examples of 64-bit errors in real programs" (see example 7. Undeclared functions in C.)

It is not taken into account that the new operator, unlike malloc, does not return NULL (54 errors)

If the malloc function cannot allocate memory, returns NULL. The new operator generates the std::bad_alloc exception in case of memory leaks.

If you want the new operator to return nullptr, the nothrow version of the operator should be used:

P = new (std::nothrow) T;

PVS-Studio analyzer knows about the differences between the two types of the new operator and warns you when an ordinary new operator, generating an exception, is used.

The idea of the PVS-Studio warning is that there is no point in checking, if the new operator returns a null pointer or not.

The detected errors can be divided into harmless and serious ones. Let's start with a harmless error.

template <class T> class vector { private: .... void push_back(const T &value) { T *clone = new T(value); if (clone) { g_array_append_val(parray, clone); current_size++; } } .... };

PVS-Studio warning: V668 There is no sense in testing the 'clone' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. maps_util.h 153

Software weaknesses type - CWE-697: Insufficient Comparison / CWE-571: Expression is Always True

The check here is not dangerous at all and it can be removed. In other words, the error is in the excessive check, which clutters the code and makes it more complicated.

Now let's consider a dangerous mistake.

bool CThreadSlm::load(const char* fname, bool MMap) { int fd = open(fname, O_RDONLY); .... if ((m_buf = new char[m_bufSize]) == NULL) { close(fd); return false; } .... }

PVS-Studio warning: V668 There is no sense in testing the 'm_buf' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. slm.cpp 97

Software weaknesses type - I do not even know how to classify it. In my opinion, three options fit here at once:

CWE-697: Insufficient Comparison

CWE-570: Expression is Always False

CWE-404: Improper Resource Shutdown or Release

It is assumed that if it is not possible to allocate memory for an array of characters, then the file descriptor will be closed and the function will return false status. In reality, if the memory is not allocated, the descriptor will not be closed and there will be a resource leak. In addition, instead of the function exit, an exception will be thrown, which will violate the expected workflow of the program.

Usually such errors appear during the refactoring, when the call of the malloc function is replaced with the new operator. The following code fragment demonstrates this case quite well:

void SettingsAFCreator::createNewAutoFillFormItem() { .... auto item_data = new AutoFillFormItemData; if (!item_data) { BROWSER_LOGE("Malloc failed to get item_data"); return; } .... }

PVS-Studio warning: 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 text of the message shows that a malloc function used to be here.

Recommendation. The replacement of the malloc with new, done in the sake of beauty, does not really affect anything and can only provoke more errors. That is why the old code with malloc is better to leave as it is, but if you decide to change it, do it carefully and attentively.

We reviewed three errors. There are 51 errors left. We will not review them all in this article; I will just give the analyzer warnings as one list in the file Tizen_V668.txt.

Confusion of Integer and Double (1 error)

The code is long, but I am not going to format it for the article, as I would like to show the fragment of the program in the way it is. That is why I will give you a picture (click on the image to enlarge).

PVS-Studio warning: V674 The '0.5' literal of the 'double' type is assigned to a variable of the 'int' type. Consider inspecting the '= 0.5' expression. add-viewer.c 824

Software weaknesses type - CWE-681: Incorrect Conversion between Numeric Types

There was some code, which evaluated the delay value, expressed in milliseconds. The default value was 500 milliseconds. One of the programmers commented out this code and decided that the value of 500 milliseconds will always be used here. At the same time, he was not very attentive and used the 0.5 value, which means a half-second in his opinion, i.e. 500 milliseconds. As a result, the variable of int type is initialized with the value 0.5 that turns into 0.

Correct variant:

int delay = 500;

Writing in the Readonly Memory (1 error)

int test_batch_operations() { .... char *condition = "MEDIA_PATH LIKE \'"; strncat(condition, tzplatform_mkpath(TZ_USER_CONTENT, "test/image%%jpg\'"), 17); .... }

PVS-Studio warning: V675 Calling the 'strncat' function will cause the writing into the read-only memory. Inspect the first argument. media-content_test.c 2952

Software weaknesses type - I do not know how to classify it, I will be grateful for a hint.

Luckily, this code is written in the tests and cannot cause a serious harm. Nevertheless, this is an error and it deserves attention.

A read-only memory address is stored in the condition variable. The change of this memory will result in undefined behavior. Most likely, this undefined behavior will be an access violation.

Incorrect do ... while Loops (2 errors)

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

PVS-Studio warning: V696 The 'continue' operator will terminate 'do { ... } while (FALSE)' loop because the condition is always false. Check lines: 73, 75. nss_securitymanager.cpp 73

Software weaknesses type - CWE-670: Always-Incorrect Control Flow Implementation

It's easy to forget that the operator continue in the do { ... } while(0) loop will stop the loop, but not resume it. The continue statement passes control to the condition of the check of the loop exit, but not to the beginning of the loop. As the condition is always false, the operator continue stops the loop.

The code should be rewritten in the following way to fix this error:

while (true) { ret = TEMP_FAILURE_RETRY(getpwnam_r(....)); if (ret != ERANGE || buffer.size() >= MEMORY_LIMIT) { break; buffer.resize(buffer.size() << 1); };

The second error is in the same file: V696 The 'continue' operator will terminate 'do { ... } while (FALSE)' loop because the condition is always false. Check lines: 120, 122. nss_securitymanager.cpp 120

A Potential Memory Leak During the Realloc Usage (11 errors)

The analyzer issues the V701 warnings, when it sees code of this kind:

P = (T *)realloc(P, n);

If it will not be possible to allocate the memory, there can be a memory leak, because NULL will be written to the P pointer. Whether a memory leak will occur or not, it depends on the previous value of the P pointer, if it is stored somewhere and used. The analyzer cannot sort out the intricacies of the program logic that is why some of the V701 warnings are false positives. There was a large number of warnings in total; I chose only 11 of them that seemed most credible to me. Perhaps, I am not right and there can be less or more errors of this type.

Let's consider one of the detected errors.

static int _preference_get_key_filesys( keynode_t *keynode, int* io_errno) { .... char *value = NULL; .... case PREFERENCE_TYPE_STRING: while (fgets(file_buf, sizeof(file_buf), fp)) { if (value) { value_size = value_size + strlen(file_buf); value = (char *) realloc(value, value_size); // <= if (value == NULL) { func_ret = PREFERENCE_ERROR_OUT_OF_MEMORY; break; } strncat(value, file_buf, strlen(file_buf)); } else { .... } } .... if (value) free(value); break; .... }

PVS-Studio warning: V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'value' is lost. Consider assigning realloc() to a temporary pointer. preference.c 951

Software weaknesses type - CWE-401: Improper Release of Memory Before Removing Last Reference ('Memory Leak')

In the loop, the data from the file are read and placed into the buffer. The buffer size increases using the realloc function call. In this example, it is clearly seen that if the realloc function returns NULL value at some point, there will be a memory leak.

Other errors:

V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'char_info->char_value' is lost. Consider assigning realloc() to a temporary pointer. bt-gatt-service.c 2430

V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'gif_data->frames' is lost. Consider assigning realloc() to a temporary pointer. image_util.c 1709

V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'gif_data->frames' is lost. Consider assigning realloc() to a temporary pointer. image_util.c 1861

V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer '_handle->src_buffer' is lost. Consider assigning realloc() to a temporary pointer. image_util.c 1911

V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'gif_data->frames' is lost. Consider assigning realloc() to a temporary pointer. image_util.c 1930

V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer '* body' is lost. Consider assigning realloc() to a temporary pointer. http_request.c 362

V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'header->rsp_header' is lost. Consider assigning realloc() to a temporary pointer. http_transaction.c 82

V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'name' is lost. Consider assigning realloc() to a temporary pointer. popup.c 179

V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'tmphstbuf' is lost. Consider assigning realloc() to a temporary pointer. scim_socket.cpp 95

V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'wsc_ctx->preedit_str' is lost. Consider assigning realloc() to a temporary pointer. wayland_panel_agent_module.cpp 1745

Memory Leaks (3 errors)

First, let's consider three used functions. It is important for us that they all return a pointer to the allocated memory.

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

Now let's consider the function body containing 3 errors.

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

}

PVS-Studio warnings:

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

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

If the function atspi_accessible_get_description fails, the generate_description_from_relation_object function should cease working. At the same time the memory, whose pointer is stored in the desc variable, gets freed. The author of the code forgot about the variables role_name, description_from_role and state_from_role, so we will have 3 memory leaks.

A Typo in Identical Code Blocks (1 error)

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

PVS-Studio warning: 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

Software weaknesses type - CWE-675: Duplicate Operations on Resource

The code of the destructor was written by Copy-Paste. Accidentally, in the one fragment the name m_modulesToolbar was not replaced with m_navigatorToolbar.

Dead Code (8 errors)

Sometimes, before throwing an exception, the information, which makes easier the debugging of applications, is written to the log. This is how the correct code looks like:

void Integrity::syncElement(....) { .... if (ret < 0) { int err = errno; LOGE("'close' function error [%d] : <%s>",err,strerror(err)); throw UnexpectedErrorException(err, strerror(err)); } }

Now let's take a look at the code, written with an error:

void Integrity::createHardLink(....) { int ret = link(oldName.c_str(), newName.c_str()); if (ret < 0) { int err = errno; throw UnexpectedErrorException(err, strerror(err)); LOGN("Trying to link to non-existent...", oldName.c_str()); } }

PVS-Studio warning: V779 Unreachable code detected. It is possible that an error is present. Integrity.cpp 233

Software weaknesses type - CWE-561: Dead Code

I think it is obvious that it is needed to swap the lines, so that the exception would be thrown after writing to the log.

Let's look at one more mistake.

#define LS_FUNC_ENTER LS_LOGD("(%s) ENTER", __FUNCTION__); #define LS_FUNC_EXIT LS_LOGD("(%s) EXIT", __FUNCTION__); static bool __check_myplace_automation(void) { LS_FUNC_ENTER bool myplace_automation_supported = false; bool myplace_automation_consent = false; .... return false; LS_FUNC_EXIT }

PVS-Studio warning: V779 Unreachable code detected. It is possible that an error is present. myplace-suggest.c 68

Software weaknesses type - CWE-561: Dead Code

Macro-epilogue is not used. The last two lines of the function should be changed places.

Other errors:

V779 Unreachable code detected. It is possible that an error is present. bt-hdp.c 295

V779 Unreachable code detected. It is possible that an error is present. media_codec_port_gst.c 2672

V779 Unreachable code detected. It is possible that an error is present. myplace.c 197

V779 Unreachable code detected. It is possible that an error is present. setting-common-view.c 124

V779 Unreachable code detected. It is possible that an error is present. layout_network.c 1666

V779 Unreachable code detected. It is possible that an error is present. ad-id.c 472

Incorrect Initialization of Objects (2 errors)

First, let's take a look at the way some data types are declared.

struct _VoiceData { int voicefw_state; .... std::vector<std::string> stt_results; .... is::ui::MicEffector *effector; }; typedef struct _VoiceData VoiceData;

Pay attention that one of the members of the VoiceData class is an array of strings. Now, let's see how the instance of the class is created and destroyed.

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

Software weaknesses type - CWE-762 Mismatched Memory Management Routines

An object is created by the malloc and memset functions and destroyed using the free function. As a result:

The constructor for the object of the std::vector<std::string> is not called. Such an object cannot be used.

The destructor is not called. At least there will be a memory leak.

In general, there is no point in thinking how this code may work. There will be definitely undefined behavior. Terrible.

It was ise-default-1.3.34 project. Exactly the same error is in the project org.tizen.inputdelegator-0.1.170518. The errors are multiplied by copying the code: V780 The object 'my_voicedata' of a non-passive (non-PDS) type cannot be initialized using the memset function. w-input-stt-ise.cpp 51

Other Issues (73 errors)

There are 73 more errors, whose description I will not provide here. These are not very interesting errors, or they will require a lot of code for demonstration. The article is already quite lengthy, and I wanted to speak about third-party libraries. That is why I will enumerate the types of the remaining errors as a list.

V524. It is odd that the body of 'Foo_1' function is fully equivalent to the body of 'Foo_2' function. (1 error)

V535. The variable 'X' is being used for this loop and for the outer loop. (4 errors)

V571. Recurring check. This condition was already verified in previous line. (1 error)

V622. Consider inspecting the 'switch' statement. It's possible that the first 'case' operator is missing. (1 error)

V646. Consider inspecting the application's logic. It's possible that 'else' keyword is missing. (2 errors)

V686. A pattern was detected: A || (A && ...). The expression is excessive or contains a logical error. (1 error)

V690. The class implements a copy constructor/operator=, but lacks the operator=/copy constructor. (7 errors)

V692. An inappropriate attempt to append a null character to a string. To determine the length of a string by 'strlen' function correctly, a string ending with a null terminator should be used in the first place. (2 errors)

V746. Type slicing. An exception should be caught by reference rather than by value. (32 errors)

V759. Violated order of exception handlers. Exception caught by handler for base class. (9 errors)

V762. Consider inspecting virtual function arguments. See NN argument of function 'Foo' in derived class and base class. (6 errors)

V769. The pointer in the expression equals nullptr. The resulting value is meaningless and should not be used. (2 errors)

V783. Dereferencing of invalid iterator 'X' might take place. (4 errors)

V786. Assigning the value C to the X variable looks suspicious. The value range of the variable: [A, B]. (1 error)

The warnings themselves can be found in the file Tizen_other_things.txt.

Intermediate Totals

I detected 344 errors. In the presentation, I stated the number 345. I decided to exclude one error, because when writing this article, I noticed that one warning is actually a false positive. It is not essential for statistics, but I decided to explain why the number in the article and in the presentation is different.

In general, 1036000 lines of code were analyzed, of which 19.9% are comments. Thus, there were "true 830000 lines of code" (without comments).

It turns out that the analyzer detects 0.41 errors on 1000 lines of code.

Is it a lot or not? Difficult question. To answer it, you have to know the average density of Tizen errors in code, created in the Samsung company. I do not have such data, so let's try to do an expert evaluation. Yes, there is a high chance to be mistaken, but still it is worth trying to count.

According to the information of the researchers from Carnegie-Mellon University, 1000 lines of code contain 5-15 errors. In turn, even in 2011 the Linux operating system was called by analysts as one of the "quality standards" of code. It is considered that Linux and its components have less than one error per 1000 lines of code. I cannot find where exactly I got such information, so I do not guarantee its accuracy, but it looks like the truth.

The operating system Tizen is based on Linux, so theoretically it should also be of high quality. So how many errors on the 1000 lines of code are there in Tizen? Let's take the average between 1 and 5. We assume that in average there are three errors in 1000 lines of code.

If so, the PVS-Studio analyzer helps you eliminate more than 10% of still undetected errors. This percent will be higher for the new code that will be written further on. We can safely say that PVS-Studio analyzer can prevent about 20% of errors.

We finished parsing bugs that I found in the code, written under copyright of Samsung company. Now we are moving to the analysis of the external libraries. I will pay them less attention, and as it is still a long way to the end of the article, it is time for a coffee/tea break.

Analysis of Third-Party Projects Used in Tizen

When I say third-party projects, I mean those, where it is directly not stated that they are made by Samsung company. Here is a list of these projects, also chosen randomly: alsa-lib-1.0.28, aspell-0.60.6.1, augeas-1.3.0, bind-9.11.0, efl-1.16.0, enlightenment-0.20.0, ise-engine-anthy-1.0.9.

There are much less projects by the quantity, but they are several times larger than the ones we previously inspected. The total size of the projects, listed here is larger than the total size of the projects, described in the previous part of the article.

I am sure, the reader understands that if I describe each error in detail, the article will just turn into a book. So, I will speak only about a small number of errors that seemed worth taking a look at.

A Typo in the Condition: the Same is Written to the Left and Right (4 errors)

static void _edje_generate_source_state_map(....) { for (i = 0; i < pd->map.colors_count; ++i) { if ((pd->map.colors[i]->r != 255) || (pd->map.colors[i]->g != 255) || (pd->map.colors[i]->b != 255) || (pd->map.colors[i]->b != 255)) ..... }

PVS-Studio warning: V501 There are identical sub-expressions '(pd->map.colors[i]->b != 255)' to the left and to the right of the '||' operator. edje_edit.c 14052

Software weaknesses type - CWE-570: Expression is Always False

A blue component was rechecked instead of an alpha channel. This example shows once more the great abilities of PVS-Studio analyzer to detect various typos.

Other errors:

V501 There are identical sub-expressions to the left and to the right of the '&&' operator: out_ && out_ != stdout && out_ != stdout checker_string.cpp 74

V501 There are identical sub-expressions '(revoked_zsk[i] != 0)' to the left and to the right of the '||' operator. dnssectool.c 1832

V501 There are identical sub-expressions to the left and to the right of the '>' operator: ob->priv.last > ob->priv.last evas_outbuf.c 684

Dereference of a (Potentially) Null Pointer (269 errors in total)

In the previous chapter we discussed null pointer dereference, but we spoke only about potentially null pointers, that returned such functions as malloc, strdup and so on. In other words, in case of luck, the program could work correctly.

Now let's take a look at the case when a "great" null pointer gets dereferenced.

static isc_result_t setup_style(dns_master_style_t **stylep) { isc_result_t result; dns_master_style_t *style = NULL; REQUIRE(stylep != NULL || *stylep == NULL); .... }

PVS-Studio warning: V522 Dereferencing of the null pointer 'stylep' might take place. Check the logical condition. delv.c 500

Software weaknesses type - CWE-476: NULL Pointer Dereference

The check is written incorrectly: if the pointer is null, it will be dereferenced. Apparently, a programmer planned to write such a check.

REQUIRE(stylep != NULL && *stylep != NULL);

Such a type of errors is rare, because the error shows itself very quickly. In general, V522 and V575 diagnostics detect pointers that will be null only under certain conditions. We have already considered these situations earlier.

The remaining warnings, pointing to 268 errors, I have put in the file Tizen_third_party_V522_V575.txt.

The Function Returns a Random Value (3 errors)

The following error is interesting because it is written in the patch, that the Tizen developers apply to the third-party libraries to get the required functionality.

static Eina_Bool _ipc_server_data(void *data, int type EINA_UNUSED, void *event) { .... //TIZEN_ONLY(170317): add skipping indicator buffer logic if (indicator_buffer_skip) return; //END .... }

PVS-Studio warning: V591 Non-void function should return a value. ecore_evas_extn.c 1526

Software weaknesses type - CWE-393: Return of Wrong Status Code

The function can return incorrect status (a random value) of Eina_Bool type.

Other errors:

V591 Non-void function should return a value. lsort.hpp 159

V591 Non-void function should return a value. ecore_evas_extn.c 1617

Using the Freed Memory (5 errors)

static char *readline_path_generator(const char *text, int state) { .... if (ctx != NULL) { char *c = realloc(child, strlen(child)-strlen(ctx)+1); // <= if (c == NULL) return NULL; int ctxidx = strlen(ctx); if (child[ctxidx] == SEP) // <= ctxidx++; strcpy(c, &child[ctxidx]); // <= child = c; } .... }

The analyzer warning:

V774 The 'child' pointer was used after the memory was reallocated. augtool.c 151

V774 The 'child' pointer was used after the memory was reallocated. augtool.c 153

Software weaknesses type - CWE-416: Use after free

This code is completely incorrect, but sometimes it may work.

After a successful call of the realloc function, the pointer child becomes invalid and it can no longer be used.

Why can we say that it works at times? The thing is that the memory manager can return the same buffer address as it used to be, i.e. the buffer size increases without a change in its address. This is the way the memory manager optimizes the speed, as there is no need to copy data from the old buffer to the new one.

Other errors:

V774 The 'res' pointer was used after the memory was released. sample-request.c 225

V774 The 'res' pointer was used after the memory was released. sample-update.c 193

V774 The 'res' pointer was used after the memory was released. sample-update.c 217

A Typo in Identical Code Blocks (1 error)

void Config::del() { while (first_) { Entry * tmp = first_->next; delete first_; first_ = tmp; } while (others_) { Entry * tmp = others_->next; delete first_; others_ = tmp; } ..... }

PVS-Studio warning: V778 Two similar code fragments were found. Perhaps, this is a typo and 'others_' variable should be used instead of 'first_'. config.cpp 185

Software weaknesses type - CWE-401: Improper Release of Memory Before Removing Last Reference ('Memory Leak')

A very beautiful Copy-Paste error was found. The author copied a text block, but forgot to change the variable name in one fragment.

After the first loop, the variable first_ has the nullptr value. Which means that during the execution of the second loop nothing will be deleted and multiple memory leaks will occur.

The Condition is Always True/False

StyleLineType StyleLine::get_type (void) { .... unsigned int spos, epos; .... for (epos = m_line.length () - 1; epos >= 0 && isspace (m_line[epos]); epos--); .... }

PVS-Studio warning: V547 Expression 'epos >= 0' is always true. Unsigned type value is always >= 0. scim_anthy_style_file.cpp 103

Software weaknesses type - CWE-571 Expression is Always True

It is hard to notice an error in this code, just quickly reviewing this fragment. The error is that epos is an unsigned variable. This means that the statement epos >= 0 is always true.

Due to this error, the code is not protected from a situation, when the string m_line becomes empty. If the string is empty, then the epos variable will be UINT_MAX, and as a result, the access to the array (m_line[epos]) will lead to unpleasant consequences.

Other errors:

V547 Expression is always false. pcm_extplug.c 769

V547 Expression is always false. pcm_extplug.c 791

V547 Expression is always false. pcm_extplug.c 817

V547 Expression is always false. pcm_extplug.c 839

V547 Expression is always false. pcm_ioplug.c 1022

V547 Expression is always false. pcm_ioplug.c 1046

V547 Expression 'pathPosEnd >= 0' is always true. Unsigned type value is always >= 0. new_fmode.cpp 566

V547 Expression 'epos >= 0' is always true. Unsigned type value is always >= 0. scim_anthy_style_file.cpp 137

Private Data is not Cleared (52 errors)

I made an interesting conclusion. In the reviewed code of Samsung I found only one error of clearing private data, while the third-party libraries are full of these errors. I think this is a serious omission, since is does not matter which part of the program will be erroneous, when private data will remain somewhere in memory and then someone will use it.

I will review only two fragments of code in the article, as all these bags are typical.

void isc_hmacsha1_sign(isc_hmacsha1_t *ctx, unsigned char *digest, size_t len) { unsigned char opad[ISC_SHA1_BLOCK_LENGTH]; unsigned char newdigest[ISC_SHA1_DIGESTLENGTH]; .... memset(newdigest, 0, sizeof(newdigest)); }

PVS-Studio warning: V597 The compiler could delete the 'memset' function call, which is used to flush 'newdigest' buffer. The memset_s() function should be used to erase the private data. hmacsha.c 1140

Software weaknesses type - CWE-14: Compiler Removal of Code to Clear Buffers

Private data stored in the buffer newdigest will not be erased.

Let's look at another function. Unlike the example described previously, the buffer is created not in the stack but in the heap memory.

static void _e_icon_smart_del(Evas_Object *obj) { E_Smart_Data *sd; if (!(sd = evas_object_smart_data_get(obj))) return; evas_object_del(sd->obj); evas_object_del(sd->eventarea); .... evas_object_smart_data_set(obj, NULL); memset(sd, 0, sizeof(*sd)); // <= free(sd); }

PVS-Studio warning: V597 The compiler could delete the 'memset' function call, which is used to flush 'sd' object. The memset_s() function should be used to erase the private data. e_icon.c 838

Software weaknesses type - CWE-14: Compiler Removal of Code to Clear Buffers

The pointer sd is still used after resetting the memory, as it is passed to the fee function. However, it does not mean anything, and the compiler may remove a function call to memset for optimization.

You can have a look at another 50 warnings, indicating the errors in the file Tizen_third_party_V597.txt.

Other issues (227 errors)

There are still many undescribed errors left in the code, but I am sure, the reader will agree that it is time to conclude. I have done a very diligent work and introduced its results in this article of elephant size. However, some interesting things have been left off screen.

This is the list of other types of errors:

V502. Perhaps the '?:' operator works in a different way than it was expected. The '?:' operator has a lower priority than the 'foo' operator. (1 error)

V505. The 'alloca' function is used inside the loop. This can quickly overflow stack. (25 errors)

V517. The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. (4 errors)

V519. The 'x' variable is assigned values twice successively. Perhaps this is a mistake. (3 errors)

V523. The 'then' statement is equivalent to the 'else' statement. (2 errors)

V528. It is odd that pointer is compared with the 'zero' value. Probably meant: *ptr != zero. (1 error)

V547. Expression is always true/false. (10 errors)

V556. The values of different enum types are compared. (6 errors)

V571. Recurring check. This condition was already verified in previous line. (1 erro