📜 ⬆️ ⬇️

About PVS-Studio on the eve of the open conference of the ISP RAS. V.P. Ivannikova

The scientific community is almost unfamiliar with the PVS-Studio static code analyzer. On November 30 and December 1, an event entitled “Technologies for Analysis, Modeling, and Transformation of Programs” will be held within the framework of the open conference of the Institute of Applied Physics RAS. V.P. Ivannikov. I am sure that this is one of the most relevant events for us, where a new audience could learn about the existence and capabilities of the PVS-Studio analyzer. In my opinion, the most suitable for this could be a report on finding errors in the Tizen operating system. Unfortunately, the report received negative reviews from reviewers and will not be included in the conference program. Nevertheless, taking this opportunity, I will summarize our research on the Tizen code.

Andrey Karpov and Evgeny Ryzhkov

In the past (2016) year, we attended the open conference of the ISP RAS. V.P. Ivannikov and noticed that much attention is paid to the operating system Tizen.

ISPRAS OPEN 2016 (ru)

Link to video
')
Tizen is an open source Linux-based operating system developed and managed by corporations like Intel and Samsung.

We support open projects and help them become more reliable thanks to the PVS-Studio static analyzer and popularization of the static analysis methodology in general. To this end, we are writing articles about the analysis of open source projects and thereby popularize static analysis tools. We hold contests . We provide a free license subject to certain conditions, and so on. In particular, we conduct separate work with some open projects and provide free licenses and support for people involved in them.

Before attending the conference in 2016, we did not know anything about Tizen except that it exists. But afterwards, we thought that this operating system could be used to popularize the static analysis methodology among open-source developers. And at the same time it is a good way to once again demonstrate the capabilities of the PVS-Studio analyzer.

As a result, a series of articles appeared on the Tizen operating system, the most interesting of which is the article " 27000 errors in the Tizen operating system ". Other articles:

  1. Let's talk about micro-optimizations on the example of Tizen code .
  2. We continue to study Tizen: C # components were of high quality .
  3. Tizen: sum up .
  4. Characteristics of the PVS-Studio analyzer on the example of EFL Core Libraries, 10-15% of false positives .

The articles showed that the analyzer can significantly improve the quality of the Tizen operating system code.

The logical conclusion of all the work done could be our report at the conference.

However, the reader may rightly ask, why do I think that the PVS-Studio analyzer can improve the Tizen code?

I base my opinion on the fact that the errors I described (or most of them) were corrected by the developers. I randomly picked up several previously detected errors in different projects and looked at what happened to them. Not to be unfounded, I’ll stop on this in more detail.

First fixed bug

So, in the article about 27000 errors, I wrote out the following fragment with an error in the org.tizen.w-home-0.1.0 project.

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

It makes no sense to compare the pointer with 0 using the <operator. Error found due to PVS-Studio warning: V503 This is a nonsensical comparison: pointer <0. apps_view_circle_indicator.c 193

So, the code has undergone a change, and in the new version it looks like this:

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

So in the article I guessed that they forgot to dereference the pointer.

Second fixed bug

The capi-media-codec-0.5.3 project.

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

Hereinafter I will not understand the essence of the error, as their description can be viewed in the previous article . Error found due to PVS-Studio warning: V507 Pointer to local array 'buffer' Such a pointer will become invalid. media_codec_test.c 793

In the new version of the project, the extract_input_aacdec_m4a_test function simply disappeared. So we can say that this error has been corrected, albeit indirectly. Perhaps this whole test was odd or wrong.

Third bug fix

Project bluetooth-frwk-0.2.157.

 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 req_id_used. bt-service-util.c 38

The error is corrected due to the fact that now the buffer size is not set to a constant, but is calculated using the sizeof operator:

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

We chose the second version of the correction described by me in the article.

Fourth bug fixed

Project org.tizen.screen-reader-0.0.8.

 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 make the buf + strlen (buf) '. app_tracker.c 450

The corrected version corresponds to the variant proposed by me in the article:

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

Fifth bug fix

The capi-network-http-0.0.23 project.

 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

Again, the code is corrected exactly as I recommended:

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

Sixth Corrected Error

Project org.tizen.w-wifi-1.0.229.

 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 the finalcount buffer. The memset_s () function should be used to erase the private data. wifi_generate_pin.c 185

Here, the data in the finalcount buffer is incorrectly overwritten . Or rather, do not overwrite: the compiler has the right to remove the memset function call. In fact, this was probably not a real mistake, as they simply removed the memset function call for mashing the finalcount . I assume that this buffer should not be reset, since it does not contain anything important.

Nevertheless, the code was corrected, which means that the authors listened to the information from the article.

Seventh bug fix

Project org.tizen.setting-1.0.1.

 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, it was allocated using the 'free' function. Consider inspecting operation logics behind the 'full_path' variable. setting-ringtone-remove.c 88

The __draw_remove_list function has been moved to another setting-ringtone-remove.c file , and its code has been fixed. The free (full_path) function call was deleted, since it is simply not needed and even harmful.

I didn’t look further at the mistakes, as this is not interesting: 7 out of 7 viewed errors were corrected. And so it is clear that the article was read, noted and corrected the errors described in it (or some part of them).

I am pleased that my article was useful and made the Tizen project at least a little better. Why a little bit? Because I was able to analyze only 3.3% of the code of this OS.

The reader may have a question: why am I sure that these errors were corrected thanks to me? Perhaps all these errors were noticed by the developers themselves or they were discovered with the help of some other tool. What other tool? Yes, anyone! Take at least a static analyzer Svace.

Svace is a static code analyzer developed at ISP RAS with funding from Samsung and specialized in finding errors in the Tizen operating system. For example, recently this analyzer was mentioned in the article " Static code analyzers on the example of ClickHouse " ( o6CuFl2Q ).

Yes, and the developer portal OS Tizen recommends using Svace to detect errors in the source code of programs. So the version is quite plausible.

What is your evidence?

As the saying goes, "what's your evidence?"

Why did I decide that the errors were corrected thanks to PVS-Studio, and not to another analyzer?

ABOUT! I can easily demonstrate that this merit is PVS-Studio!

As I already wrote, I was only enough for warnings related to 3.3% of the code. Then I ran out of power. However, the analyzer in those days did not check 3.3% of the code, but more, about 5%. I have these unparsed reports.

Let's take a look at them and study the history of life errors, which I did not describe in the article.

So, I randomly open reports that I didn’t look at at the time of writing this article and look for convincing errors that deserve correction. I'll start with the project amd-0.6.5. Although the name says AMD, this code refers to Tizen and is written by developers from Samsung. This is indicated by a comment at the beginning:

// Copyright © 2000 - 2017 Samsung Electronics Co., Ltd. All rights reserved.

The first uncorrected error (since I did not write about it)

So, in the old code, I see:

 static int __get_instance_info(bundle *kb, struct instance_info *info) { .... gchar *query; .... if (query == NULL || query + 1 == NULL) { .... } 

PVS-Studio warning: V694 The condition (query + 1 == NULL) is only true if there is a pointer overflow which is undefined behavior anyway. amd_request.c 1083

The expression "query + 1 == NULL" can be true only if the pointer value overflows. And so it is impossible to do, it is undefined behavior. There is clearly something wrong with this code.

I look into the new code. The function __get_instance_info migrated to the file amd_rua.c. However, the essence did not change, and the error remained in its place:

 if (query == NULL || query + 1 == NULL) { 

The second uncorrected error (since I did not write about it)

Project attach-panel-camera-0.1.0.

 #define startfunc LOGD("+- START -------------------------"); #define endfunc LOGD("+- END --------------------------"); static Eina_Bool _main_view_send_result_after_transform(void *data) { startfunc main_view *view = (main_view *)data; if (view->transformtype == CAM_TRANSFORM_CROP) { DBG("crop completed, Start resize"); _main_view_ug_send_result(view, view->filename); _main_view_start_camera_preview(view->camera); view->transformtype = CAM_TRANSFORM_NONE; } return ECORE_CALLBACK_CANCEL; // <= endfunc // <= } 

PVS-Studio warning: V779 Unreachable code detected. It is possible that an error is present. main-view.c 261

Unreachable code. The call to the logging function must be moved above the return statement . We look into the fresh code. Nothing has changed, no one found the error:

  return ECORE_CALLBACK_CANCEL; // <= endfunc // <= } 

The third uncorrected error (since I did not write about it)

Project aul-0.6.7.

 static gboolean run_func(void *data) { callfunc(cmd); if (strcmp(cmd, "launch_res") == 0 || strcmp(cmd, "all") == 0 || strcmp(cmd, "dbuslaunch") == 0 || strcmp(cmd, "listen_app_status") == 0 // <= || strcmp(cmd, "open_svc_res") == 0 || strcmp(cmd, "listen_app_status") == 0) // <= return 0; else g_main_loop_quit(mainloop); return 0; } 

PVS-Studio warning: V501 There are identical sub-expressions 'strcmp (cmd, "listen_app_status") == 0' to the left | operator. aul_test.c 898

The string is compared twice with listen_app_status. One comparison is superfluous, or it was necessary to compare it with something else.

In the new version of the source code, nothing has changed, the error is in its place.

Proof of!

Here is the evidence.

I see no point in continuing. Errors that I did not describe in the article continue to live in the Tizen code.

Unfortunately, the Tizen developer community got acquainted with PVS-Studio somehow one-sidedly. Yes, they probably corrected several hundred errors, based on information from the article. But did not go further. Nobody corresponded with me and did not discuss the possibility of using the analyzer. Rather, the correspondence was, but some unconvincing and without interest.

As a result, corrected hundreds of errors. And thousands could be fixed!

That is why it is so important to popularize static code analysis and learn how to use it correctly. It is ineffective to edit 3% of the code and not be interested in the fate of errors in the remaining 97%. In general, one-time checks are ineffective: a static analyzer should be used regularly.

In general, sadly somehow all this.

I wanted to sum up, get sad about the errors and once again draw the attention of the audience to our reviews of errors in the code. I would like to touch on all these topics in a speech at the conference.

It is strange to stay away from such a suitable event as "Technology analysis, modeling and transformation of programs." Especially since excellent practical results were obtained. In the end, not many people in Russia in general are engaged in this area.

I was let down by the inability to submit scientific applications. It’s one thing to write on Habrahabr, to be steadfast on Linux.org.ru, and quite another to state something in the form of scientific theses :). Result - failures in reviews:

Reviewed by the first reviewer.
The topic of the article “Practical use of the methodology and static analysis tools for finding errors in the source code of the Tizen operating system” fully corresponds to the subject of the conference section. However, in the text of the article there is no description or review of the methodology of static analysis, as well as the approach of using static analysis tools to search for errors in the source code.

The article argues that when choosing a subset of the code base of the Tizen operating system and selecting about 900 warnings (the exact number is not specified, the method for selecting warnings about real errors is not given) concludes that the error density is 0.37 per 1000 lines of source code, which No further explanation is given in the article.

The following article describes the five errors found by some static analyzer. Moreover, from the text of the article it can be concluded that this analyzer is used to check the code of the Tizen operating system already verified by the static analyzer Svace. It is not quite obvious why errors like “The expression is always degraded to false” (CWE-570) are called “Typo”, the error like “Assigning a variable without further use” (CWE-563) is called “Logical error”, and for the error “Unchecked return value on NULL leads to null pointer dereferencing ”(CWE-690). In no way does the article make an unreasonable conclusion that an error message is displayed for the case in which the malloc function is not checked for NULL, although there is a similar check in other places Dana statistics checking the return value of the malloc function to NULL).

In conclusion, the article makes no reasonable conclusion about the discovery of a large number of diverse errors without reducing the statistics on the results of the static analysis of the Tizen source code. The conclusion of the article about the need to use several code analysis tools is in no way justified, as is the statement that using several tools will ensure the highest code reliability.

In connection with the foregoing, it can be concluded that the statements and conclusions of the article are poorly substantiated. From a scientific point of view, the article is not of interest. The article does not list the sources referenced by the authors. The text of the article is decorated carelessly and does not meet the requirements for the design of the text of articles accepted for consideration at the conference.

Reviewed by a second reviewer.
The article describes a series of errors found in the source code of the Tizen operating system with a static analyzer. Since static analysis is already being used to test Tizen, it is concluded that it is necessary to use several tools.

Unfortunately, the article does not contain any details to judge the analysis tool:

- there is no implementation of static analysis algorithms (based on the errors found, it can be concluded that these are simple checks on the syntax tree);

- there are no test results (the level of true operations, scalability, etc.).

The conclusion of the article on the desirability of using multiple tools does not follow from the above and is true for tools of the Coverity Prevent class. For AST checks, all of them can be implemented in one tool.

The conclusion of the article on the density of errors in Tizen is also incorrect, since simple extrapolation by the number of errors verified does not work in this case.

As a result, the article does not carry any valuable information, has errors and, moreover, is not a scientific article at all. It would be more appropriate to focus on the description of analysis algorithms, difficulties in applying to a large project like Tizen, work done on this subject, etc.

I admit that the article was written poorly. The only thing that confuses me a little is “the article does not contain any details that allow to judge the analysis tool”. So how could I write that it was PVS-Studio? It is immediately clear that the author is Andrei Karpov, and this is a violation of the condition of anonymity when submitting applications.

And the expert, they say not real!

However, what anonymity is there, PVS-Studio's ears stick out everywhere and an article about 27000 errors :). It is strange that the reviewer could not guess. This means that the static analysis expert was not exactly an expert. Or did he guess, but ...? :)

Anyway. Tyap-bluff did, I admit. Next time I will try better if there is a reason.

Thank you all for your attention. Do not act like the developers of Tizen, making edits to the results of a one-time check (especially incomplete). The static analyzer should be immediately introduced into the development process, and PVS-Studio provides the possibility of smooth integration . I wish you all a hopeless code.

PS If someone from ISP RAS reads this note, and I want to diversify the conference with an unformatted report on static code analysis, I am always ready.

Unformat

Source: https://habr.com/ru/post/342624/


All Articles