Geekly Articles each Day
📜 ⬆️ ⬇️

Characteristics of the PVS-Studio analyzer on the example of EFL Core Libraries, 10-15% of false positives

EFL Core Libraries and PVS-Studio

After a big article about checking the Tizen operating system, I was asked a lot of questions about the percentage of false positives and the density of errors (how many errors does PVS-Studio detect per 1000 code lines). My reasoning that it strongly depends on the project being analyzed and the settings of the analyzer do not look like the real answer. I decided to give specific numbers, conducting a more thorough study of one of the projects that make up Tizen. Since Carsten Haitzler was actively involved in the discussion of the article, I decided that it would be interesting to take for the experiment the EFL Core Libraries, in whose development he participates. I hope this article will help Carsten become a fan of our analyzer :).


Prehistory


If any of my readers missed, then I inform them that I recently wrote an open letter to the developers of Tizen , and then the monumental article " 27000 errors in the operating system Tizen ".

After that, on some sites relevant news notes appeared, and several discussions unfolded. Here are some of them:
I want to express my appreciation to Carsten Haitzler , who paid attention to my publication and took an active part in its discussion.
')
Various topics were raised, to some of which I gave detailed comments in the note “ Tizen: Summing Up ”.

However, two eternal questions haunt me:
Programmers who understand well what a static analysis methodology is would agree with me that such generalized questions do not make sense. It all depends on the project with which we work. To ask such questions is the same as asking the doctor to answer the question “so what is the average temperature of patients in your hospital?”

Therefore, I will give an answer on the example of a specific project. I chose EFL Core Libraries. First, this project is part of Tizen. And secondly, Carsten Haitzler is involved in its development, and I think it will be interesting for him to look at my results.

You could still check Enlightenment, but I didn’t have the strength for it. I feel that even without this article will be very long.

The Enlightenment Foundation Libraries (EFL) is a collection of graphics libraries that emerged from the development of Enlightenment, the window manager and the Wayland protocol.

When checking EFL Core Libraries, we used the latest code taken from the https://git.enlightenment.org/ repository.

I will also note that the project under investigation is tested using the static Coverity analyzer. Here is a comment on this topic:

I will say that we take checking seriously. If you’re looking at what to look for at this time, it’s not a problem. finding issues is easy enough i the codebase is large). They are mostly not so big impacting things. We’ve been fixing the bug.

Well, let's see how the PVS-Studio analyzer demonstrates itself.

Specifications


After its configuration, the PVS-Studio analyzer will generate about 10-15% of false positives when checking the EFL Core Libraries project.

The density of currently detected errors in EFL Core Libraries is more than 0.71 errors per 1000 lines of code .

How the calculations were carried out


The EFL Core Libraries project at the time of analysis contains about 1,616,000 lines of code in C and C ++. Of these, 17.7% are comments. Thus, the number of lines of code without comments is 1,330,000.

After the first launch, I saw the following number of general-purpose messages (GA):
This is, of course, a bad result. That is why I do not like to write abstract measurement results. I need to set up the analyzer, and this time I decided to devote time to it.

Almost the entire project is written in C and, as a result, macros are widely used in it. It is macros that cause the majority of false positives. I spent about 40 minutes on a quick review of the report and compiled the efl_settings.txt file.

The file contains the necessary settings. To use them when checking a project, you need to specify in the analyzer's configuration file (for example, in PVS-Studio.cfg):
rules-config=/path/to/efl_settings.txt

The analyzer can be run like this:
 pvs-studio-analyzer analyze ... --cfg /path/to/PVS-Studio.cfg ...

or so
 pvs-studio ... --cfg /patn/to/PVS-Studio.cfg ...

depending on the integration method used.

Using the settings, I told the analyzer not to give some warnings for those lines of code that contain the names of certain macros or expressions. I also completely disabled several diagnostics. For example, I turned off the V505 . Using the alloca function in cycles is not good, but this is not an obvious error. I do not want to discuss a lot about whether this or that warning is a false positive, and I thought it would be easier to turn off something.

Yes, it should be noted that I was viewing and setting up warnings for only the first two levels. In the future, I will consider only them. Low confidence level warnings are not recommended. At least starting to use the analyzer, it is unreasonable to work with these warnings. Only having understood with the first two levels, it is possible to glance on the third and choose types of warnings useful in your opinion.

The restart resulted in the following results:
The number 1186 is repeated twice. This is not a typo. Indeed the numbers coincided so casually.

So, having spent 40 minutes on setup, I significantly reduced the number of false positives. Of course, I have a lot of experience, this process would take more time for a third-party developer, but there is nothing terrible and difficult to set up.

In total, I received 189 + 1186 = 1375 messages (High + Medium), with which I began to work.

After analyzing these messages, I believe that the analyzer detected 950 code fragments containing errors. In other words, I found 950 code snippets to fix. I will tell you more about these errors in the next chapter.

Let us now calculate the density of detected errors:

950 * 1000/1330000 = about 0.71 errors per 1000 lines of code.

Now let's calculate the percentage of false positives:

((1375-950) / 1375) * 100% = 30%

Stop, stop, stop! But after all, at the beginning of the article it was said about 10-15% of false positives. And here 30%.

I'll explain now. So, looking through the report from 1375 messages, I came to the conclusion that 950 indicate errors. There are 425 messages left.

Not all of these remaining 425 messages are false positives. There are a lot of messages here, about which I just can’t say whether an error was detected with their help or not.

Consider an example of one of the messages I missed.
 .... uint64_t callback_mask; .... static void _check_event_catcher_add(void *data, const Efl_Event *event) { .... Evas_Callback_Type type = EVAS_CALLBACK_LAST; .... else if ((type = _legacy_evas_callback_type(array[i].desc)) != EVAS_CALLBACK_LAST) { obj->callback_mask |= (1 << type); } .... }

PVS-Studio warning : V629 Consider inspecting the '1 << type' expression. Bit shifting of the 32-bit type. evas_callbacks.c 709

Consider the line more closely:
 obj->callback_mask |= (1 << type);

It is used to write 1 to the desired bit of the callback_mask variable. Please note that the callback_mask variable is 64-bit.

The expression (1 << type) is of type int , so with its help you can change bits only in the lower part of the variable callback_mask . Bits [32-63] cannot be changed.

To understand whether there is an error here or not, you need to figure out what range of values ​​the _legacy_evas_callback_type function can return. Can she return a value greater than 31? I do not know and miss this message.

Please understand me. I see this code for the first time and have no idea what it does. In addition, hundreds more analyzer messages are waiting for me. I just can not begin to carefully deal with each such case.

Comment by Carsten Haitzler. If you’re really a little bit of a bug, you’ll have to do it. do this to retain compatibility, but what happens with any refactoring ... stuff happens). This is a series of events. It’s not just “type A” ... but it doesn’t have a complete check. / mapping It was actually pretty harmless.

Among the remaining 425 messages there are indicative of errors. I just missed them.

If it comes to using PVS-Studio regularly, you can continue to configure it. As I said, I already spent only 40 minutes on the setup. But this does not mean that I did everything I could. You can also reduce the number of false positives by disabling diagnostics for certain software designs.

After a more careful study of the remaining messages and additional settings, just 10-15% of false positives will remain. Good result.

Bugs found


Now consider the errors I found. I cannot describe all 950, therefore I will limit myself to the analysis of a pair of warnings of each type. The rest of the warnings I give a list or a separate file.

Also, the reader himself can get acquainted with all the warnings by opening the report file: zip-archive with the report . Note that I only left General warnings of high and medium confidence in the file.

I was viewing this report in Windows, opening it with the help of the PVS-Studio Standalone utility.

In Linux, you can use the Plog Converter utility, which will convert the report to one of the following formats:
Then you can use QtCreator, Vim / gVim, GNU Emacs, LibreOffice Calc to view reports. This is described in detail in the documentation section " How to run PVS-Studio in Linux " (see "Viewing and filtering the analyzer report").

V501 (1 error)


Diagnostics V501 revealed only one error, but beautiful. The error is in the comparison function, which echoes the topic of the recent article " Evil lives in comparison functions ".
 static int _ephysics_body_evas_stacking_sort_cb(const void *d1, const void *d2) { const EPhysics_Body_Evas_Stacking *stacking1, *stacking2; stacking1 = (const EPhysics_Body_Evas_Stacking *)d1; stacking2 = (const EPhysics_Body_Evas_Stacking *)d2; if (!stacking1) return 1; if (!stacking2) return -1; if (stacking1->stacking < stacking2->stacking) return -1; if (stacking2->stacking > stacking2->stacking) return 1; return 0; }

PVS-Studio warning: V501 There are identical sub-expressions "stacking2-> stacking". ephysics_body.cpp 450

A typo. The last comparison should be:
 if (stacking1->stacking > stacking2->stacking) return 1;


V512 (8 errors)


First, take a look at the definition of the Eina_Array structure.
 typedef struct _Eina_Array Eina_Array; struct _Eina_Array { int version; void **data; unsigned int total; unsigned int count; unsigned int step; Eina_Magic __magic; };

Carefully consider it is not necessary. Just a structure with some kind of fields.

Now let's look at the definition of the Eina_Accessor_Array structure:
 typedef struct _Eina_Accessor_Array Eina_Accessor_Array; struct _Eina_Accessor_Array { Eina_Accessor accessor; const Eina_Array *array; Eina_Magic __magic; };

Note that the Eina_Accessor_Array structure stores a pointer to the Eina_Array structure. Otherwise, these structures are in no way interconnected and have a different size.

Now the code that detected the analyzer and which I do not understand:
 static Eina_Accessor * eina_array_accessor_clone(const Eina_Array *array) { Eina_Accessor_Array *ac; EINA_SAFETY_ON_NULL_RETURN_VAL(array, NULL); EINA_MAGIC_CHECK_ARRAY(array); ac = calloc(1, sizeof (Eina_Accessor_Array)); if (!ac) return NULL; memcpy(ac, array, sizeof(Eina_Accessor_Array)); return &ac->accessor; }

PVS-Studio warning : V512 A call of the 'memcpy' function will get to the array. eina_array.c 186

Let me remove all the extra details to make it easier:
 .... eina_array_accessor_clone(const Eina_Array *array) { Eina_Accessor_Array *ac = calloc(1, sizeof (Eina_Accessor_Array)); memcpy(ac, array, sizeof(Eina_Accessor_Array)); }

Memory is allocated for an object of type Eina_Accessor_Array . A strange thing happens next.

An object of type Eina_Array is copied to the allocated memory buffer.

Shta


I do not know what the considered function should do, but it is doing something wrong.

First, when copying occurs, the source goes beyond the boundary ( Eina_Array structures).

Secondly, in general, such a copy does not make sense. Structures have a set of fields of completely different types.

Comment by Carsten Haitzler. Function content correct - Type in param is wrong. It is not a funeral of the. .

Consider the following error.
 static Eina_Bool _convert_etc2_rgb8_to_argb8888(....) { const uint8_t *in = src; uint32_t *out = dst; int out_step, x, y, k; unsigned int bgra[16]; .... for (k = 0; k < 4; k++) memcpy(out + x + k * out_step, bgra + k * 16, 16); .... }

PVS-Studio warning: V512 A call of the 'memcpy' function will bgra + k * 16 '. draw_convert.c 318

Everything is simple here. Ordinary overflow buffer.

The bgra array consists of 16 unsigned int elements.

The variable k takes values ​​in a loop from 0 to 3.

Pay attention to the expression: bgra + k * 16 .

When the variable k takes a value greater than 0, a pointer will be calculated that points out of the array.

However, some V512 messages point to code that does not contain a real error. However, I do not consider such analyzer triggers to be false. The code is bad and, in my opinion, should be changed. Consider this case.
 #define MATRIX_XX(m) (m)->xx typedef struct _Eina_Matrix4 Eina_Matrix4; struct _Eina_Matrix4 { double xx; double xy; double xz; double xw; double yx; double yy; double yz; double yw; double zx; double zy; double zz; double zw; double wx; double wy; double wz; double ww; }; EAPI void eina_matrix4_array_set(Eina_Matrix4 *m, const double *v) { memcpy(&MATRIX_XX(m), v, sizeof(double) * 16); }

PVS-Studio warning: V512 A call of the 'memcpy' function will lead to the overflow of the buffer '& (m) -> xx'. eina_matrix.c 1003

You could just copy the array into the structure. But instead, the address of the first member xx is taken. Probably, it is assumed that in the future at the beginning of the structure other fields will appear. And so that the behavior of the program does not break, this technique is used.

Comment by Carsten Haitzler. Memcpy's not a bug: taking advantage of the guaranteed layout of structs.

I do not like him. I recommend writing something like this:
 struct _Eina_Matrix4 { union { struct { double xx; double xy; double xz; double xw; double yx; double yy; double yz; double yw; double zx; double zy; double zz; double zw; double wx; double wy; double wz; double ww; }; double RawArray[16]; }; }; EAPI void void eina_matrix4_array_set(Eina_Matrix4 *m, const double *v) { memcpy(m->RawArray, v, sizeof(double) * 16); }

It is a little longer, but more ideologically true. However, if you do not want to edit the code, you can suppress the warning in one of the following ways.

The first way. Add a comment to the code:
 memcpy(&MATRIX_XX(m), v, sizeof(double) * 16); //-V512

The second way. Add a line to the configuration file:
 //-V:MATRIX_:512

The third way. Use markup base .

Other errors:

V517 (3 errors)


 static Eina_Bool evas_image_load_file_head_bmp(void *loader_data, Evas_Image_Property *prop, int *error) { .... if (header.comp == 0) // no compression { // handled } else if (header.comp == 3) // bit field { // handled } else if (header.comp == 4) // jpeg - only printer drivers goto close_file; else if (header.comp == 3) // png - only printer drivers goto close_file; else goto close_file; .... }

PVS-Studio warning : V517 The use of if (A) {...} else if (A) {...} 'pattern was detected. There is a possibility of logical error presence. Check lines: 433, 439. evas_image_load_bmp.c 433

Two times the variable header.comp is compared to a constant of 3 .

Other errors:

V519 (1 error)


 EOLIAN static Efl_Object * _efl_net_ssl_context_efl_object_finalize(....) { Efl_Net_Ssl_Ctx_Config cfg; .... cfg.load_defaults = pd->load_defaults; // <= cfg.certificates = &pd->certificates; cfg.private_keys = &pd->private_keys; cfg.certificate_revocation_lists = &pd->certificate_revocation_lists; cfg.certificate_authorities = &pd->certificate_authorities; cfg.load_defaults = pd->load_defaults; // <= .... }

PVS-Studio warning : V519 The 'cfg.load_defaults' variable is assigned values ​​twice successively. Perhaps this is a mistake. Check lines: 304, 309. efl_net_ssl_context.c 309

Reassignment is repeated. One assignment is superfluous or you forgot to copy something else.

Comment by Carsten Haitzler. Not a bug. Just an overzealous copy & paste of the line.

Another simple case:
 EAPI Eina_Bool edje_edit_size_class_add(Evas_Object *obj, const char *name) { Eina_List *l; Edje_Size_Class *sc, *s; .... /* set default values for max */ s->maxh = -1; s->maxh = -1; .... }

PVS-Studio warning: V519 The 's-> maxh' variable is assigned values ​​twice successively. Perhaps this is a mistake. Check lines: 8132, 8133. edje_edit.c 8133

Of course, not all cases are so obvious. However, I believe that the warnings listed below most likely indicate errors:
Note. Carsten Haitzler, commenting on the article, wrote that the V519 warnings listed in the list are false positives. I do not agree with this approach. Perhaps the code works correctly, but still it deserves attention and editing. I decided to leave the list in the article so that the readers themselves could evaluate whether, from their point of view, repetitions in the assignment of variables are false positives or not. But since Carsten says that these are not errors, I will not take them into account when calculating.

V522 (563 errors)




In the EFL project there is a problem with the presence of checks: memory has been allocated or not. In general, there are such checks in the project. Example:
 if (!(el = malloc(sizeof(Evas_Stringshare_El) + slen + 1))) return NULL;

Moreover, they are even where they are not needed (see below for warning V668 ).

But in a huge number of places there is no verification. Consider for example a couple of analyzer messages.

Comment by Carsten Haitzler. It’s not always a good idea. Linux overcommits memory by default. This is not what you have been assigned. Only virtual space. Not until you touch it. It seems like a valid pointer. It is low. Sometimes we do it ... sometimes not. It can be used for a small number of people. It can be tune. Secondly, it is a list of chunk of memory - eg a linked list node ... realistically if NULL is returned ... you can do. If you can not allocate 20-40 bytes ... you can’t allocate 20–40 bytes ... But there are few megabytes of memory etc. which has been our target. I can see why PVS-Studio doesn't like this. It wasn’t been a reality that it wasn’t spent on it. I'll get more into that later.
 static Eina_Debug_Session * _session_create(int fd) { Eina_Debug_Session *session = calloc(1, sizeof(*session)); session->dispatch_cb = eina_debug_dispatch; session->fd = fd; // start the monitor thread _thread_start(session); return session; }

Comment by Carsten Haitzler. 2 months ago and it’s still being built out. If you’re running, you’ll be able to control it. What is the timeline of the logs? it’s so that it’s triggered the wakeup, it’s so you can’t get it. above on memory and aborts etc.

Comment by Andrei Karpov. Not very clear, and here is a new and untested code. Static analyzers are designed primarily to look for errors in the new code.

PVS-Studio warning : V522 There might be dereferencing of a potential null pointer 'session'. eina_debug.c 440

Allocated memory using the calloc function and immediately using it.

Another example:
 static Reference * _entry_reference_add(Entry *entry, Client *client, unsigned int client_entry_id) { Reference *ref; // increase reference for this file ref = malloc(sizeof(*ref)); ref->client = client; ref->entry = entry; ref->client_entry_id = client_entry_id; ref->count = 1; entry->references = eina_list_append(entry->references, ref); return ref; }

PVS-Studio warning: V522 There might be dereferencing of a potential null pointer 'ref'. evas_cserve2_cache.c 1404

And so it is 563 times. In the article, I can not bring them. I give a link to the file: EFL_V522.txt .

V547 (39 errors)


 static void _ecore_con_url_dialer_error(void *data, const Efl_Event *event) { Ecore_Con_Url *url_con = data; Eina_Error *perr = event->info; int status; status = efl_net_dialer_http_response_status_get(url_con->dialer); if ((status < 500) && (status > 599)) { DBG("HTTP error %d reset to 1", status); status = 1; /* not a real HTTP error */ } WRN("HTTP dialer error url='%s': %s", efl_net_dialer_address_dial_get(url_con->dialer), eina_error_msg_get(*perr)); _ecore_con_event_url_complete_add(url_con, status); }

PVS-Studio warning : V547 Expression '(status <500) && (status> 599)' is always false. ecore_con_url.c 351

The correct verification option should be:
 if ((status < 500) || (status > 599))

The code fragment with this error was copied to 2 more places:
The following error situation:
 EAPI void eina_rectangle_pool_release(Eina_Rectangle *rect) { Eina_Rectangle *match; Eina_Rectangle_Alloc *new; .... match = (Eina_Rectangle *) (new + 1); if (match) era->pool->empty = _eina_rectangle_skyline_list_update( era->pool->empty, match); .... }

PVS-Studio warning: V547 Expression 'match' is always true. eina_rectangle.c 798

Once a unit has been added to the pointer, it makes no sense to check it for NULL equality.

The match pointer can become zero only if an overflow occurs during the addition. However, pointer overflow is considered undefined behavior, so this option should not be considered.

And one more case.
 EAPI const void * evas_object_smart_interface_get(const Evas_Object *eo_obj, const char *name) { Evas_Smart *s; .... s = evas_object_smart_smart_get(eo_obj); if (!s) return NULL; if (s) .... }

PVS-Studio warning: V547 Expression 's' is always true. evas_object_smart.c 160

If the pointer is NULL , then the function exits. Rechecking does not make sense.

Other errors: EFL_V547.txt .

There are warnings V547, which I missed and did not write out, since I was not interested in understanding them. Among them may be a few more errors.

V556 (8 errors)


All 8 errors were issued for one code fragment. To start, look at the announcement of the two listings.
 typedef enum _Elm_Image_Orient_Type { ELM_IMAGE_ORIENT_NONE = 0, ELM_IMAGE_ORIENT_0 = 0, ELM_IMAGE_ROTATE_90 = 1, ELM_IMAGE_ORIENT_90 = 1, ELM_IMAGE_ROTATE_180 = 2, ELM_IMAGE_ORIENT_180 = 2, ELM_IMAGE_ROTATE_270 = 3, ELM_IMAGE_ORIENT_270 = 3, ELM_IMAGE_FLIP_HORIZONTAL = 4, ELM_IMAGE_FLIP_VERTICAL = 5, ELM_IMAGE_FLIP_TRANSPOSE = 6, ELM_IMAGE_FLIP_TRANSVERSE = 7 } Elm_Image_Orient; typedef enum { EVAS_IMAGE_ORIENT_NONE = 0, EVAS_IMAGE_ORIENT_0 = 0, EVAS_IMAGE_ORIENT_90 = 1, EVAS_IMAGE_ORIENT_180 = 2, EVAS_IMAGE_ORIENT_270 = 3, EVAS_IMAGE_FLIP_HORIZONTAL = 4, EVAS_IMAGE_FLIP_VERTICAL = 5, EVAS_IMAGE_FLIP_TRANSPOSE = 6, EVAS_IMAGE_FLIP_TRANSVERSE = 7 } Evas_Image_Orient;

, . .
 EAPI void elm_image_orient_set(Evas_Object *obj, Elm_Image_Orient orient) { Efl_Orient dir; Efl_Flip flip; EFL_UI_IMAGE_DATA_GET(obj, sd); sd->image_orient = orient; switch (orient) { case EVAS_IMAGE_ORIENT_0: .... case EVAS_IMAGE_ORIENT_90: .... case EVAS_IMAGE_FLIP_HORIZONTAL: .... case EVAS_IMAGE_FLIP_VERTICAL: .... }

PVS-Studio:
, .

, , . :
, .

Comment by Carsten Haitzler. All of the above orient/rotate enum stuff is intentional. We had to cleanup duplication of enums and we ensured they had the same values so they were interchangeable — we moved from rotate to orient and kept the compatibility. It's part of our move over to the new object system and a lot of code auto-generation etc. that is still underway and beta. It's not an error but intended to do this as part of transitioning, so it's a false positive.

. , false positives. , , - , .

V558 (4 )


 accessor_iterator<T>& operator++(int) { accessor_iterator<T> tmp(*this); ++*this; return tmp; }

PVS-Studio: V558 Function returns the reference to temporary local object: tmp. eina_accessor.hh 519

, & :
 accessor_iterator<T> operator++(int)

:

V560 (32 )


 static unsigned int read_compressed_channel(....) { .... signed char headbyte; .... if (headbyte >= 0) { .... } else if (headbyte >= -127 && headbyte <= -1) // <= .... }

PVS-Studio: V560 A part of conditional expression is always true: headbyte <= — 1. evas_image_load_psd.c 221

headbyte >= 0, <= -1 .

.
 static Eeze_Disk_Type _eeze_disk_type_find(Eeze_Disk *disk) { const char *test; .... test = udev_device_get_property_value(disk->device, "ID_BUS"); if (test) { if (!strcmp(test, "ata")) return EEZE_DISK_TYPE_INTERNAL; if (!strcmp(test, "usb")) return EEZE_DISK_TYPE_USB; return EEZE_DISK_TYPE_UNKNOWN; } if ((!test) && (!filesystem)) // <= .... }

PVS-Studio: V560 A part of conditional expression is always true: (!test). eeze_disk.c 55

. test , .

: EFL_V560.txt .

V568 (3 )


 EOLIAN static Eina_Error _efl_net_server_tcp_efl_net_server_fd_socket_activate(....) { .... struct sockaddr_storage *addr; socklen_t addrlen; .... addrlen = sizeof(addr); if (getsockname(fd, (struct sockaddr *)&addr, &addrlen) != 0) .... }

PVS-Studio: V568 It's odd that 'sizeof()' operator evaluates the size of a pointer to a class, but not the size of the 'addr' class object. efl_net_server_tcp.c 192

, , . :
 addrlen = sizeof(*addr);

:

V571 (6 )


 EAPI void eeze_disk_scan(Eeze_Disk *disk) { .... if (!disk->cache.vendor) if (!disk->cache.vendor) disk->cache.vendor = udev_device_get_sysattr_value(....); .... }

PVS-Studio: V571 Recurring check. The 'if (!disk->cache.vendor)' condition was already verified in line 298. eeze_disk.c 299

.

:
Note. Carsten Haitzler . . , . , . , .

V575 (126 )


, . .
 static void free_buf(Eina_Evlog_Buf *b) { if (!b->buf) return; b->size = 0; b->top = 0; # ifdef HAVE_MMAP munmap(b->buf, b->size); # else free(b->buf); # endif b->buf = NULL; }

PVS-Studio: V575 The 'munmap' function processes '0' elements. Inspect the second argument. eina_evlog.c 117

b->size 0, munmap .

, :
 static void free_buf(Eina_Evlog_Buf *b) { if (!b->buf) return; b->top = 0; # ifdef HAVE_MMAP munmap(b->buf, b->size); # else free(b->buf); # endif b->buf = NULL; b->size = 0; }

We continue.
 EAPI Eina_Bool eina_simple_xml_parse(....) { .... else if ((itr + sizeof("<!>") - 1 < itr_end) && (!memcmp(itr + 2, "", sizeof("") - 1))) .... }

PVS-Studio: V575 The 'memcmp' function processes '0' elements. Inspect the third argument. eina_simple_xml_parser.c 355

, 0 .

We continue.
 static void _edje_key_down_cb(....) { .... char *compres = NULL, *string = (char *)ev->string; .... if (compres) { string = compres; free_string = EINA_TRUE; } else free(compres); .... }

PVS-Studio: V575 The null pointer is passed into 'free' function. Inspect the first argument. edje_entry.c 2306

compress , .
 else free(compres);

.

Comment by Carsten Haitzler. Not a bug but indeed some extra if paranoia like code that isn't needed. Micro optimizations again?

. . , . , , . , , .

Similarly:
V575 . - , , V522.
 static void _fill_all_outs(char **outs, const char *val) { size_t vlen = strlen(val); for (size_t i = 0; i < (sizeof(_dexts) / sizeof(char *)); ++i) { if (outs[i]) continue; size_t dlen = strlen(_dexts[i]); char *str = malloc(vlen + dlen + 1); memcpy(str, val, vlen); memcpy(str + vlen, _dexts[i], dlen); str[vlen + dlen] = '\0'; outs[i] = str; } }

PVS-Studio: V575 The potential null pointer is passed into 'memcpy' function. Inspect the first argument. main.c 112

, , .

: EFL_V575.txt .

V587 (2 )


 void _ecore_x_event_handle_focus_in(XEvent *xevent) { .... e->time = _ecore_x_event_last_time; _ecore_x_event_last_time = e->time; .... }

PVS-Studio: V587 An odd sequence of assignments of this kind: A = B; B = A;. Check lines: 1006, 1007. ecore_x_events.c 1007

Comment by Carsten Haitzler. Not bugs as such — looks like just overzealous storing of last timestamp. This is adding a timestamp to an event when no original timestamp exists so we can keep a consistent structure for events with timestamps, but it is code clutter and a micro optimization.

. , . , Carsten , . .

: V587 An odd sequence of assignments of this kind: A = B; B = A;. Check lines: 1050, 1051. ecore_x_events.c 1051

V590 (3 )


 static int command(void) { .... while (*lptr == ' ' && *lptr != '\0') lptr++; /* skip whitespace */ .... }

PVS-Studio: V590 Consider inspecting the '* lptr == ' ' && * lptr != '\0'' expression. The expression is excessive or contains a misprint. embryo_cc_sc2.c 944

. :
 while (*lptr == ' ')

:

V591 (1 )


 _self_type& operator=(_self_type const& other) { _base_type::operator=(other); }

PVS-Studio: V591 Non-void function should return a value. eina_accessor.hh 330

V595 (4 )


 static void eng_image_size_get(void *engine EINA_UNUSED, void *image, int *w, int *h) { Evas_GL_Image *im; if (!image) { *w = 0; // <= *h = 0; // <= return; } im = image; if (im->orient == EVAS_IMAGE_ORIENT_90 || im->orient == EVAS_IMAGE_ORIENT_270 || im->orient == EVAS_IMAGE_FLIP_TRANSPOSE || im->orient == EVAS_IMAGE_FLIP_TRANSVERSE) { if (w) *w = im->h; if (h) *h = im->w; } else { if (w) *w = im->w; if (h) *h = im->h; } }

PVS-Studio:
if (w) if (h) , w h NULL . , .

eng_image_size_get :
 eng_image_size_get(NULL, NULL, NULL, NULL);

.

, , , :

V597 (6 )


 EAPI Eina_Binbuf * emile_binbuf_decipher(Emile_Cipher_Algorithm algo, const Eina_Binbuf *data, const char *key, unsigned int length) { .... Eina_Binbuf *result = NULL; unsigned int *over; EVP_CIPHER_CTX *ctx = NULL; unsigned char ik[MAX_KEY_LEN]; unsigned char iv[MAX_IV_LEN]; .... on_error: memset(iv, 0, sizeof (iv)); memset(ik, 0, sizeof (ik)); if (ctx) EVP_CIPHER_CTX_free(ctx); eina_binbuf_free(result); return NULL; }

PVS-Studio:
, memset . . - , V597 .

Comment by Carsten Haitzler. Above 2 — totally familiar with the issue. The big problem is memset_s is not portable or easily available, thus why we don't use it yet. You have to do special checks for it to see if it exists as it does not exist everywhere. Just as a simple example add AC_CHECK_FUNCS([memset_s]) to your configure.ac and memset_s is not found you have to jump through some more hoops like define __STDC_WANT_LIB_EXT1__ 1 before including system headers… and it's still not declared. On my pretty up to date Arch system memset_s is not defined by any system headers, same on debian testing… warning: implicit declaration of function 'memset_s'; did you mean memset'? [-Wimplicit-function-declaration], and then compile failure… no matter what I do. A grep -r of all my system includes shows no memset_s declared… so I think advising people to use memset_s is only a viable advice if its widely available and usable. Be aware of this.

:

V609 (1 )


eina_value_util_type_size .
 static inline size_t eina_value_util_type_size(const Eina_Value_Type *type) { if (type == EINA_VALUE_TYPE_INT) return sizeof(int32_t); if (type == EINA_VALUE_TYPE_UCHAR) return sizeof(unsigned char); if ((type == EINA_VALUE_TYPE_STRING) || (type == EINA_VALUE_TYPE_STRINGSHARE)) return sizeof(char*); if (type == EINA_VALUE_TYPE_TIMESTAMP) return sizeof(time_t); if (type == EINA_VALUE_TYPE_ARRAY) return sizeof(Eina_Value_Array); if (type == EINA_VALUE_TYPE_DOUBLE) return sizeof(double); if (type == EINA_VALUE_TYPE_STRUCT) return sizeof(Eina_Value_Struct); return 0; }

, 0. , :
 static inline unsigned int eina_value_util_type_offset(const Eina_Value_Type *type, unsigned int base) { unsigned size, padding; size = eina_value_util_type_size(type); if (!(base % size)) return base; padding = ( (base > size) ? (base - size) : (size - base)); return base + padding; }

PVS-Studio: V609 Mod by zero. Denominator range [0..24]. eina_inline_value_util.x 60

. , eina_value_util_type_size 0. .

Comment by Carsten Haitzler. The 0 return would only happen if you have provided totally invalid input, like again strdup(NULL)… So I call this a false positive as you cant have an eina_value generic value that is not valid without bad stuff happening — validate you passes a proper value in first. eina_value is performance sensitive btw so every check here costs something. it's like adding if() checks to the add opcode.

V610 (1 )


 void fetch_linear_gradient(....) { .... if (t + inc*length < (float)(INT_MAX >> (FIXPT_BITS + 1)) && t+inc*length > (float)(INT_MIN >> (FIXPT_BITS + 1))) .... }

PVS-Studio: V610 Unspecified behavior. Check the shift operator '>>'. The left operand '(- 0x7fffffff — 1)' is negative. ector_software_gradient.c 412

V614 (1 )


 extern struct tm *gmtime (const time_t *__timer) __attribute__ ((__nothrow__ , __leaf__)); static void _set_headers(Evas_Object *obj) { static char part[] = "ch_0.text"; int i; struct tm *t; time_t temp; ELM_CALENDAR_DATA_GET(obj, sd); elm_layout_freeze(obj); sd->filling = EINA_TRUE; t = gmtime(&temp); // <= .... }

PVS-Studio: V614 Uninitialized variable 'temp' used. Consider checking the first actual argument of the 'gmtime' function. elm_calendar.c 720

V621 (1 )


 static void _opcodes_unregister_all(Eina_Debug_Session *session) { Eina_List *l; int i; _opcode_reply_info *info = NULL; if (!session) return; session->cbs_length = 0; for (i = 0; i < session->cbs_length; i++) eina_list_free(session->cbs[i]); .... }

PVS-Studio: V621 Consider inspecting the 'for' operator. It's possible that the loop will be executed incorrectly or won't be executed at all. eina_debug.c 405

V630 (2 )


btVector3 . , .
 class btVector3 { public: .... btScalar m_floats[4]; inline btVector3() { } .... };

Simulation_Msg :
 typedef struct _Simulation_Msg Simulation_Msg; struct _Simulation_Msg { EPhysics_Body *body_0; EPhysics_Body *body_1; btVector3 pos_a; btVector3 pos_b; Eina_Bool tick:1; };

, btVector3 . , :
 _ephysics_world_tick_dispatch(EPhysics_World *world) { Simulation_Msg *msg; if (!world->ticked) return; world->ticked = EINA_FALSE; world->pending_ticks++; msg = (Simulation_Msg *) calloc(1, sizeof(Simulation_Msg)); msg->tick = EINA_TRUE; ecore_thread_feedback(world->cur_th, msg); }

PVS-Studio: V630 The 'calloc' function is used to allocate memory for an array of objects which are classes containing constructors. ephysics_world.cpp 299

, non-POD , calloc .

, . .

: V630 The 'calloc' function is used to allocate memory for an array of objects which are classes containing constructors. ephysics_world.cpp 471

Comment by Carsten Haitzler. Because the other end of the pipe is C code that is passing around a raw ptr as the result from thread A to thread B, it's a mixed c and c++ environment. In the end we'd be sending raw ptr's around no matter what...

V654 (2 )


 int evas_mem_free(int mem_required EINA_UNUSED) { return 0; } int evas_mem_degrade(int mem_required EINA_UNUSED) { return 0; } void * evas_mem_calloc(int size) { void *ptr; ptr = calloc(1, size); if (ptr) return ptr; MERR_BAD(); while ((!ptr) && (evas_mem_free(size))) ptr = calloc(1, size); if (ptr) return ptr; while ((!ptr) && (evas_mem_degrade(size))) ptr = calloc(1, size); if (ptr) return ptr; MERR_FATAL(); return NULL; }

PVS-Studio:
- .

Comment by Carsten Haitzler. Old old code because caching was implemented, so it was basically a lot of NOP's waiting to be filled in. since evas speculatively cached data (megabytes of it) the idea was that if allocs fail — free up some cache and try again… if that fails then actually try nuke some non-cached data that could be reloaded/rebuilt but with more cost… and only fail after that. But because of overcommit this didn't end up practical as allocs would succeed then just fall over often enough if you did hit a really low memory situation, so I gave up. it's not a bug. it's a piece of history :).

.

WTF

 EAPI void evas_common_font_query_size(....) { .... size_t cluster = 0; size_t cur_cluster = 0; .... do { cur_cluster = cluster + 1; glyph--; if (cur_w > ret_w) { ret_w = cur_w; } } while ((glyph > first_glyph) && (cur_cluster == cluster)); .... }

PVS-Studio: V654 The condition of loop is always false. evas_font_query.c 376

:
 cur_cluster = cluster + 1;

, (cur_cluster == cluster) false .

Comment by Carsten Haitzler. Above… it seems you built without harfbuzz support… we highly don't recommend that. it's not tested. Building without basically nukes almost all of the interesting unicode/intl support for text layout. You do have to explicitly — disable it… because with harfbuzz support we have opentype enabled and a different bit of code is executed due to ifdefs… if you actually check history of the code before adding opentype support it didn't loop over clusters at all or even glyphs… so really the ifdef just ensures the loop only loops one and avoids more ifdefs later in the loop conditions making the code easier to maintain — beware the ifdefs!

V668 (21 )


, , malloc / calloc .

new - .



:
 static EPhysics_Body * _ephysics_body_rigid_body_add(....) { .... motion_state = new btDefaultMotionState(); if (!motion_state) { ERR("Couldn't create a motion state."); goto err_motion_state; } .... }

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

, std::bad_alloc .

Comment by Carsten Haitzler. Fair enough, but be aware some compiler DON'T throw exceptions… they return NULL on new… so not totally useless code depending on the compiler. I believe VSC6 didn't throw an exception — so before exceptions were a thing this actually was correct behavior, also I depends on the allocator func if it throws and exception or not, so all in all, very minor harmless code.

. . . .

:
 EAPI EPhysics_Constraint * ephysics_constraint_linked_add(EPhysics_Body *body1, EPhysics_Body *body2) { .... constraint->bt_constraint = new btGeneric6DofConstraint( *ephysics_body_rigid_body_get(body1), *ephysics_body_rigid_body_get(body2), btTransform(), btTransform(), false); if (!constraint->bt_constraint) { ephysics_world_lock_release(constraint->world); free(constraint); return NULL; } .... }

PVS-Studio: V668 There is no sense in testing the 'constraint->bt_constraint' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. ephysics_constraints.cpp 382

, , , free .

Comment by Carsten Haitzler. Same as previous new + NULL check.

. , Visual C++ 6.0. , new. , , . Tizen Visual C++ 6.0! . , . , . , . , memory-leak. , new , new(nothrow). . , , .

: EFL_V668.txt .

V674 (2 )


, abs :
 extern int abs (int __x) __attribute__ ((__nothrow__ , __leaf__)) __attribute__ ((__const__)) ;

, int .

, .
 #define ELM_GESTURE_MINIMUM_MOMENTUM 0.001 typedef int Evas_Coord; struct _Elm_Gesture_Momentum_Info { .... Evas_Coord mx; Evas_Coord my; .... }; static void _momentum_test(....) { .... if ((abs(st->info.mx) > ELM_GESTURE_MINIMUM_MOMENTUM) || (abs(st->info.my) > ELM_GESTURE_MINIMUM_MOMENTUM)) state_to_report = ELM_GESTURE_STATE_END; .... }

PVS-Studio:
, int 0.001. - .

V686 (3 )


 static Image_Entry * _scaled_image_find(Image_Entry *im, ....) { size_t pathlen, keylen, size; char *hkey; Evas_Image_Load_Opts lo; Image_Entry *ret; if (((!im->file) || ((!im->file) && (!im->key))) || (!im->data1) || ((src_w == dst_w) && (src_h == dst_h)) || ((!im->flags.alpha) && (!smooth))) return NULL; .... }

PVS-Studio: V686 A pattern was detected: (!im->file) || ((!im->file) && ...). The expression is excessive or contains a logical error. evas_cache2.c 825

, . Let me explain with a simple example.

if (A || (A && B) || C)

:

if (A || C)

, - - . . .

:

V694 (2 )


 #define CPP_PREV_BUFFER(BUFFER) ((BUFFER)+1) static void initialize_builtins(cpp_reader * pfile) { .... cpp_buffer *pbuffer = CPP_BUFFER(pfile); while (CPP_PREV_BUFFER(pbuffer)) pbuffer = CPP_PREV_BUFFER(pbuffer); .... }

PVS-Studio: V694 The condition ((pbuffer) + 1) is only false if there is pointer overflow which is undefined behavior anyway. cpplib.c 2496

, .
 cpp_buffer *pbuffer = ....; while (pbuffer + 1) ....

. , . . undefined behavior, . , :
 while (true) pbuffer = CPP_PREV_BUFFER(pbuffer);

.

: V694 The condition ((ip) + 1) is only false if there is pointer overflow which is undefined behavior anyway. cpplib.c 2332

Comment by Carsten Haitzler. This old code indeed has issues. There should be checks against CPP_NULL_BUFFER(pfile) because if its a linked list this is a null heck, if its a static buffer array as a stack, it checks stack end position — interestingly in decades it's never been triggered that I know of.

V701 (69 )


 static void _efl_vg_gradient_efl_gfx_gradient_stop_set( ...., Efl_VG_Gradient_Data *pd, ....) { pd->colors = realloc(pd->colors, length * sizeof(Efl_Gfx_Gradient_Stop)); if (!pd->colors) { pd->colors_count = 0; return ; } memcpy(pd->colors, colors, length * sizeof(Efl_Gfx_Gradient_Stop)); pd->colors_count = length; _efl_vg_changed(obj); }

PVS-Studio: V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'pd->colors' is lost. Consider assigning realloc () to a temporary pointer. evas_vg_gradient.c 14

:
 pd->colors = realloc(pd->colors, ....);

pd->colors . , . , pd->colors NULL .

. , . , , . :
 EOLIAN void _evas_canvas_key_lock_add( Eo *eo_e, Evas_Public_Data *e, const char *keyname) { if (!keyname) return; if (e->locks.lock.count >= 64) return; evas_key_lock_del(eo_e, keyname); e->locks.lock.count++; e->locks.lock.list = realloc(e->locks.lock.list, e->locks.lock.count * sizeof(char *)); e->locks.lock.list[e->locks.lock.count - 1] = strdup(keyname); eina_hash_free_buckets(e->locks.masks); }

PVS-Studio: V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'e->locks.lock.list' is lost. Consider assigning realloc () to a temporary pointer. evas_key.c 142

: EFL_701.txt .

V728 (4 )


 static Eina_Bool _evas_textblock_node_text_adjust_offsets_to_start(....) { Evas_Object_Textblock_Node_Format *last_node, *itr; .... if (!itr || (itr && (itr->text_node != n))) .... }

PVS-Studio: V728 An excessive check can be simplified. The '||' operator is surrounded by opposite expressions '!itr' and 'itr'. evas_object_textblock.c 9505

, . :
 if (!itr || (itr->text_node != n))

:

V769 (11 )


V522, . .
 EAPI Eina_Bool edje_edit_sound_sample_add( Evas_Object *obj, const char *name, const char *snd_src) { .... ed->file->sound_dir->samples = realloc(ed->file->sound_dir->samples, sizeof(Edje_Sound_Sample) * ed->file->sound_dir->samples_count); sound_sample = ed->file->sound_dir->samples + ed->file->sound_dir->samples_count - 1; sound_sample->name = (char *)eina_stringshare_add(name); .... }

PVS-Studio: V769 The 'ed->file->sound_dir->samples' pointer in the expression could be nullptr. In such case, resulting value of arithmetic operations on this pointer will be senseless and it should not be used. edje_edit.c 1271

. , . , , . , , . .

, , . . (NULL + N) , - .

:

V779 (19 )


V779 , , . Example:
 EAPI Eina_Bool ecore_x_xinerama_screen_geometry_get(int screen, int *x, int *y, int *w, int *h) { LOGFN(__FILE__, __LINE__, __FUNCTION__); #ifdef ECORE_XINERAMA if (_xin_info) { int i; for (i = 0; i < _xin_scr_num; i++) { if (_xin_info[i].screen_number == screen) { if (x) *x = _xin_info[i].x_org; if (y) *y = _xin_info[i].y_org; if (w) *w = _xin_info[i].width; if (h) *h = _xin_info[i].height; return EINA_TRUE; } } } #endif /* ifdef ECORE_XINERAMA */ if (x) *x = 0; if (y) *y = 0; if (w) *w = DisplayWidth(_ecore_x_disp, 0); if (h) *h = DisplayHeight(_ecore_x_disp, 0); return EINA_FALSE; screen = 0; // <= }

PVS-Studio: V779 Unreachable code detected. It is possible that an error is present. ecore_x_xinerama.c 92

, screen . - , , , .

EINA_UNUSED .

:
 extern void _exit (int __status) __attribute__ ((__noreturn__)); static void _timeout(int val) { _exit(-1); if (val) return; }

PVS-Studio: V779 Unreachable code detected. It is possible that an error is present. timeout.c 30

_exit . . , :
 static void _timeout(int val) { if (val) return; _exit(-1); }

Comment by Carsten Haitzler. Not a bug. it's also an unused param thing from before the macros. The timeout has the process self exit in case it takes too long (assuming the decoder lib is stuck if a timeout happens).

. . , , . . , .

: EFL_V779.txt .

V1001 (6 )


 static Elocation_Address *address = NULL; EAPI Eina_Bool elocation_address_get(Elocation_Address *address_shadow) { if (!address) return EINA_FALSE; if (address == address_shadow) return EINA_TRUE; address_shadow = address; return EINA_TRUE; }

PVS-Studio: V1001 The 'address_shadow' variable is assigned but is not used until the end of the function. elocation.c 1122

. , :
 *address_shadow = *address;

:

Carsten Haitzler


PVS-Studio Coverity. ( , ). , , (). , Coverity, . , PVS-Studio Coverity , Coverity , , . , , -, PVS-Studio Coverity, — .

Conclusion


, , .

, . , EFL Coverity. , PVS-Studio . , PVS-Studio , :). , , PVS-Studio, Coverity, PVS-Studio .

PVS-Studio


PVS-Studio :
.



, : Andrey Karpov. Characteristics of PVS-Studio Analyzer by the Example of EFL Core Libraries, 10-15% of False Positives

?
. : PVS-Studio, 2015 . , .

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

More articles:


All Articles