Our website uses cookies to enhance your browsing experience.
Your message has been sent. We will email you at
If you do not see the email in your inbox, please check if it is filtered to one of the following folders:
Webinar: Evaluation - 05.12
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. 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 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.
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:
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.
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.
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.
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
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
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\n", size, ret);
} else {
printf("FAIL, size=%d, ret=0x%x\n", 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\n", size, ret);
} else {
printf("FAIL, size=%d, ret=0x%x\n", size, ret);
}
}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.
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:
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:
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:
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:
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.
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:
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.
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:
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';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:
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:
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:
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:
Other incorrect comparisons:
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)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:
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:
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\n",
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:
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.
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:
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).
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.
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.
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:
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:
#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:
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.
__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:
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:
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:
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(...);
}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);#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
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.)
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:
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.
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;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.
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
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:
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:
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.
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.
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:
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:
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
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.
The warnings themselves can be found in the file Tizen_other_things.txt.
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.
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.
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:
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 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:
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:
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:
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.
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:
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.
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:
The warnings can be found in the following file Tizen_third_party_other_things.txt.
570 errors were found. In the presentation, 564 errors were stated. Apparently, I forgot to count some of them. 1915000 lines of code were analyzed, of which 17.6% were comments.
PVS-Studio detects 0.36 errors on 1000 lines of code. This means that the alleged error density in third-party libraries is slightly lower than the error density in code of Tizen (the analyzer has detected 0.41 errors on 1000 lines of code).
Why is the error density lower in the libraries?
For this reason, there is no point to pay attention to such difference. We can say that the density of errors for these two groups of projects is approximately equal.
Programming and static analysis are finished! It is time for statistics!
For those who have looked through the article without reading, I will repeat that here we are not talking about the number of warnings displayed by the analyzer, but about actual errors and when I say that during my analysis I found 900 errors, this means I found exactly 900 bugs, but not the number of the warnings I saw. Any doubts? If so, I suggest to read the article from the very beginning. :)
I apologize for repeating some things, but it is extremely important. Unfortunately, many people perceive incorrectly the information in our articles and presentations, confusing number of warnings and the number of errors.
The list all the types of errors and their quantity:
|
Rule |
Description |
Quantity |
|---|---|---|
|
V501 |
There are identical sub-expressions to the left and to the right of the 'foo' operator. |
6 |
|
V502 |
Perhaps the '?:' operator works in a different way than it was expected. The '?:' operator has a lower priority than the 'foo' operator. |
1 |
|
V503 |
This is a nonsensical comparison: pointer < 0. |
2 |
|
V505 |
The 'alloca' function is used inside the loop. This can quickly overflow stack. |
26 |
|
V507 |
Pointer to local array 'X' is stored outside the scope of this array. Such a pointer will become invalid. |
1 |
|
V512 |
A call of the 'Foo' function will lead to a buffer overflow or underflow. |
7 |
|
V517 |
The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. |
8 |
|
V519 |
The 'x' variable is assigned values twice successively. Perhaps this is a mistake. |
14 |
|
V522 |
Dereferencing of the null pointer might take place. |
276 |
|
V523 |
The 'then' statement is equivalent to the 'else' statement. |
8 |
|
V524 |
It is odd that the body of 'Foo_1' function is fully equivalent to the body of 'Foo_2' function. |
1 |
|
V527 |
It is odd that the 'zero' value is assigned to pointer. Probably meant: *ptr = zero. |
1 |
|
V528 |
It is odd that pointer is compared with the 'zero' value. Probably meant: *ptr != zero. |
1 |
|
V535 |
The variable 'X' is being used for this loop and for the outer loop. |
4 |
|
V547 |
Expression is always true/false. |
18 |
|
V556 |
The values of different enum types are compared. |
24 |
|
V560 |
A part of conditional expression is always true/false. |
2 |
|
V571 |
Recurring check. This condition was already verified in previous line. |
2 |
|
V572 |
It is odd that the object which was created using 'new' operator is immediately cast to another type. |
4 |
|
V575 |
Function receives an odd argument. |
83 |
|
V576 |
Incorrect format. Consider checking the N actual argument of the 'Foo' function. |
5 |
|
V590 |
Consider inspecting this expression. The expression is excessive or contains a misprint. |
3 |
|
V591 |
Non-void function should return a value. |
3 |
|
V593 |
Consider reviewing the expression of the 'A = B == C' kind. The expression is calculated as following: 'A = (B == C)'. |
1 |
|
V595 |
The pointer was utilized before it was verified against nullptr. Check lines: N1, N2. |
28 |
|
V597 |
The compiler could delete the 'memset' function call, which is used to flush 'Foo' buffer. The RtlSecureZeroMemory() function should be used to erase the private data. |
53 |
|
V601 |
An odd implicit type casting. |
1 |
|
V609 |
Divide or mod by zero. |
1 |
|
V610 |
Undefined behavior. Check the shift operator. |
2 |
|
V611 |
The memory allocation and deallocation methods are incompatible. |
2 |
|
V614 |
Uninitialized variable 'Foo' used. |
1 |
|
V618 |
It's dangerous to call the 'Foo' function in such a manner, as the line being passed could contain format specification. The example of the safe code: printf("%s", str); |
6 |
|
V622 |
Consider inspecting the 'switch' statement. It's possible that the first 'case' operator is missing. |
1 |
|
V624 |
The constant NN is being utilized. The resulting value could be inaccurate. Consider using the M_NN constant from <math.h>. |
2 |
|
V636 |
The expression was implicitly cast from integer type to real type. Consider utilizing an explicit type cast to avoid overflow or loss of a fractional part. |
12 |
|
V640 |
The code's operational logic does not correspond with its formatting. |
3 |
|
V642 |
Saving the function result inside the 'byte' type variable is inappropriate. The significant bits could be lost breaking the program's logic. |
1 |
|
V645 |
The function call could lead to the buffer overflow. The bounds should not contain the size of the buffer, but a number of characters it can hold. |
6 |
|
V646 |
Consider inspecting the application's logic. It's possible that 'else' keyword is missing. |
4 |
|
V647 |
The value of 'A' type is assigned to the pointer of 'B' type. |
1 |
|
V649 |
There are two 'if' statements with identical conditional expressions. The first 'if' statement contains function return. This means that the second 'if' statement is senseless. |
1 |
|
V666 |
Consider inspecting NN argument of the function 'Foo'. It is possible that the value does not correspond with the length of a string which was passed with the YY argument. |
6 |
|
V668 |
There is no sense in testing the pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. |
63 |
|
V674 |
The expression contains a suspicious mix of integer and real types. |
1 |
|
V675 |
Writing into the read-only memory. |
1 |
|
V686 |
A pattern was detected: A || (A && ...). The expression is excessive or contains a logical error. |
2 |
|
V690 |
The class implements a copy constructor/operator=, but lacks the operator=/copy constructor. |
8 |
|
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 |
|
V694 |
The condition (ptr - const_value) is only false if the value of a pointer equals a magic constant. |
2 |
|
V696 |
The 'continue' operator will terminate 'do { ... } while (FALSE)' loop because the condition is always false. |
2 |
|
V701 |
realloc() possible leak: when realloc() fails in allocating memory, original pointer is lost. Consider assigning realloc() to a temporary pointer. |
113 |
|
V746 |
Type slicing. An exception should be caught by reference rather than by value. |
32 |
|
V759 |
Violated order of exception handlers. Exception caught by handler for base class. |
9 |
|
V760 |
Two identical text blocks detected. The second block starts with NN string. |
1 |
|
V762 |
Consider inspecting virtual function arguments. See NN argument of function 'Foo' in derived class and base class. |
6 |
|
V769 |
The pointer in the expression equals nullptr. The resulting value is meaningless and should not be used. |
8 |
|
V773 |
The function was exited without releasing the pointer/handle. A memory/resource leak is possible. |
6 |
|
V774 |
The pointer was used after the memory was released. |
5 |
|
V778 |
Two similar code fragments were found. Perhaps, this is a typo and 'X' variable should be used instead of 'Y'. |
2 |
|
V779 |
Unreachable code detected. It is possible that an error is present. |
16 |
|
V780 |
The object of non-passive (non-PDS) type cannot be used with the function. |
2 |
|
V783 |
Dereferencing of invalid iterator 'X' might take place. |
4 |
|
V786 |
Assigning the value C to the X variable looks suspicious. The value range of the variable: [A, B]. |
1 |
Table 1. Types and number of errors found in the randomly selected projects.
I detected 913 errors in total. Let's round off to 900 errors for simplicity.
I did not evaluate the number of false positives. The thing that there was not performed even minimal analyzer configuration, so there is no point to count the percentage of false positives. It will be simply dishonest towards the analyzer. The majority of false positives appears because of several failed macros. Setting the analyzer, one can reduce in several times the number of false positives.
Judging by my personal feeling, there are not many false positives. If it was not so, I would not be able to undertake alone such an extensive study so quickly.
In addition, I will note that the number of false positives does not matter at all. If we start the cooperation, the false positives will be a headache of our team, not of the Tizen developers.
The moment has come when it will become clear why I stated 27000 errors.
In total, I analyzed more than 2 400 000 lines of code (excluding comments).
I detected 900 errors.
The whole Tizen project with the third-party libraries has 72 500 000 lines of C, C++ code (excluding the comments).
That means that I checked only 3.3% of the code.
Estimation:
(72500000 * 900/2400000 = 27187)
Using PVS-Studio, we can detect and fix 27 000 errors.
As you can see, the calculations are absolutely fair and transparent.
I think I managed to demonstrate again the abilities of PVS-Studio to detect various types of errors. Though, the article was long, but now no one will say that I embellish the PVS-Studio and fantasize about 27000 errors. The article presents all data and evaluations that anyone can verify himself.
Static analysis is a must when it comes to large projects such as Tizen. It also makes sense to use a few tools, since different analyzers complement each other.
I suggest to download and try PVS-Studio.
Supported languages and compilers:
Thank you for your attention. I invite you to read about the analysis of other open source projects, and to subscribe to my Twitter @Code_Analysis. Yours respectfully, Andrey Karpov.
0
0
0
0