After I wrote quite a big article about the analysis of the Tizen OS code, I received a large number of questions concerning the percentage of false positives and the density of errors (how many errors PVS-Studio detects per 1000 lines of code). Apparently, my reasoning that it strongly depends on the project to be analyzed and the settings of the analyzer didn't seem sufficient enough. Therefore, I decided to provide specific figures by doing a more thorough investigation of one of the project of the Tizen OS. I decided that it would be quite interesting to take EFL Core Libraries, because one of the developers, Carsten Haitzler, took an active part in the discussion of my articles. I hope this article would prove to Carsten that PVS-Studio is a worthy tool.
Prehistory
If there were people who missed the news, then I just inform that recently I have written an open letter to the Tizen developers, and then a monumental article "27000 Errors in the Tizen Operating System".
After that, there were several news post on various resources and quite vivid discussions. Here are some of them:
- Reddit. PVS-Studio Team Willing to Work on Improving Tizen Project (open letter).
- Reddit. Good news: Samsung's Tizen no longer worst code ever. Bad news: It's still pretty awful.
- Ycombinator. 27000 errors in the Tizen operating system.
- The Register. Good news: Samsung's Tizen no longer worst code ever. Bad news: It's still pretty awful
- The Hacker News. Researcher Claims Samsung's Tizen OS is Poorly Programmed; Contains 27,000 Bugs!
- Lists.tizen.org: 1, 2, 3, 4, 5, 6.
I would express special gratitude to Carsten Haitzler once more, for his attention to my post and active discussion of it.
There were various topics raised, some of them were covered in more details in the post "Tizen: summing up".
However, there are two eternal questions which continue haunting me.
- What is the percentage of false positives?
- How many errors does PVS-Studio find per 1000 lines of code?
Those programmers, who are well aware of what is the methodology of static analysis would agree with me that such generalized questions don't have any sense at all. It all depends of the project we are working with. Asking such questions is like trying to measure an average temperature of all patients in a hospital.
So I'll give the answer on the example of a specific project. I chose EFL Core Libraries. Firstly, this project is part of Tizen. Secondly, as I have already said, one of the developers is Carsten Haitzler, who would probably find these results interesting.
I could also check Enlightenment, but I didn't have enough energy for it. I feel that this article will be already rather long.
The Enlightenment Foundation Libraries (EFL) are a set of graphics libraries that grew out of the development of Enlightenment, a window manager and Wayland compositor.
To check the EFL Core Libraries I used the recent code, taken from the repository https://git.enlightenment.org/.
It's worth mentioning that this project is checked by Coverity static code analyzer. Here is a comment on this topic:
I will say that we take checking seriously. Coverity reports a bug rate of 0 for Enlightenment upstream (we've fixed all issues Coverity points out or dismissed them as false after taking a good look) and the bug rate for EFL is 0.04 issues per 1k lines of code which is pretty small (finding issues is easy enough i the codebase is large). They are mostly not so big impacting things. Every release we do has our bug rates go down and we tend to go through a bout of "fixing the issues" in the weeks prior to a release.
So, let's see what PVS-Studio can show us.
Characteristics
After proper configuration, PVS-Studio will issue 10-15% of false positives during the analysis of EFL Core Libraries.
The density of the detectable errors in EFL Core Libraries is 0.71 errors per 1000 lines of code at this point.
The Way I did the calculations
The project EFL Core Libraries at the moment of analysis has about 1 616 000 lines of code written in C and C++. 17.7% of them are comments. Thus, the number of code lines without comments - 1 330 000.
After the first run I saw the following number of general analysis warnings (GA):
- High level of certainty: 605
- Medium level of certainty: 3924
- Low level of certainty: 1186
Of course, this is a bad result. That's why I don't like to write abstract results of measurements. The work requires proper analyzer settings, this time I decided to spend some time on it.
Almost the whole project is written in C, and as a result, macros are widely used in it. They are the cause of most of the false positives. I spent about 40 minutes on a quick review of the report and came up with the file efl_settings.txt.
The file contains the necessary settings. To use them during the project analysis, it's necessary to specify in the configuration file of the analyzer (for example, in PVS-Studio.cfg) the following:
rules-config=/path/to/efl_settings.txt
The analyzer can be run in the following way:
pvs-studio-analyzer analyze ... --cfg /path/to/PVS-Studio.cfg ...
or like this:
pvs-studio ... --cfg /patn/to/PVS-Studio.cfg ...
depending on the way of integration.
With the help of these settings I specified in the analyzer, so that it doesn't issue some warnings for those code lines, in which there are names of certain macros or expressions. I have also disabled several diagnostics at all. For example, I disabled V505. It's not great to use the alloca function in the loops, but it's not a crucial error. I don't want to debate a lot, whether a certain warning is a false positive, so I thought it would be easier to disable something.
Yes, it should be noted that I reviewed and set up only the warnings of the first two certainty levels. Further on, I will review only them. We aren't going to consider warnings of low certainty level. At least, it would be irrational to start using the analyzer and review warnings of this level. Only after sorting out the warnings of the first two levels, you can take a look at the third and choose the useful warnings at your glance.
The second run had the following results:
- High level of certainty: 189
- Medium level of certainty: 1186
- Low level of certainty: 1186
The number 1186 is repeated twice. This is not a typo. These numbers have really turned up to be the same.
So, having spent 40 minutes to set up the analyzer, I significantly reduced the number of false positives. Of course, I have a lot of experience in it, it would probably take more time it were a programmer who is new to it, but there is nothing terrible and difficult in the configuring of the analyzer.
In total I got 189 +1186 = 1375 messages (High + Medium) with which I started working.
After I reviewed these warnings, I suppose that the analyzer detected 950 fragments of code, containing errors. In other words, I found 950 fragments that require fixing. I will give more details about these errors in the next chapter.
Let's evaluate the density of the detected errors.
950*1000/1330000 = about 0,71 errors per 1000 lines of code.
Now, let's evaluate the percentage of false positives:
((1375-950) / 1375) * 100% = 30%
Well, wait! In the beginning of the article there was a number 10-15% of false positives. Here it's 30%.
Let me explain. So, reviewing the report of 1375 warnings, I came to the conclusion that 950 of them indicate errors. There were 425 warnings left.
But not all these 425 warnings are false positives. There are a lot of messages, reviewing which it is impossible to say if there is an error or not.
Let's consider one example of a message that I decided to skip.
....
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 value with a subsequent expansion to the 64-bit type. evas_callbacks.c 709
Let's take a closer look at this line:
obj->callback_mask |= (1 << type);
It is used to write 1 to the necessary bit of the variable callback_mask. Pay attention that the variable callback_mask is of 64-bit type.
The statement (1 << type) has an int type, that's why you can change only the bits in the lower part of the variable. Bits [32-63] cannot be changed.
To understand, if there is a bug or not, we need to understand what range of values may the function _legacy_evas_callback_type return. Can it return a value greater than 31? I don't know, so I skip this warning.
Try to understand this case. I see the code for the first time and have no idea what it's doing. In addition, hundreds of the analyzer messages are still waiting for me. I just cannot deal with every case like this.
Comment by Carsten Haitzler. Above - actually is a bug that's a result of an optimization that setting bits to decide if it should bother trying to map new event types to old ones (we're refactoring huge chunks of our internals around a new object system and so we have to do this to retain compatibility, but like with any refactoring... stuff happens). Yes - it wraps the bitshift and does the extra work of a whole bunch of if's because the same bits in the mask are re-used for now 2 events due to wrap around. As such this doesn't lead to a bug, just slightly less micro optimizations when set as now that bit means "it has an event callback for type A OR B" not just "type A" ... the following code actually does the complete check/mapping. It surely was not intended to wrap so this was a catch but the way it's used means it was actually pretty harmless.
Among those 425 ones left, there will be warnings, pointing to errors. For now I just skipped them.
If it comes to the regular use of PVS-Studio, it will be possible to continue setting it up. As I have already said, I spent just 40 minutes on the settings. But it doesn't mean that I did everything I could. The number of false positives can be reduced even more by disabling the diagnostics for certain programming constructs.
After careful review of the remaining warnings and additional settings, there will be 10-15% of false positives. This is a good result.
Bugs found
Now let's take a look at the bugs I found. I cannot describe all the 950 errors, so I will limit myself to describing a pair of warnings of each type. The remaining warnings I will provide a list or a separate file.
The reader can also look at all the warnings by opening the report file: zip-archive with the report. Note that I have left only the General warnings of high and medium level of certainty.
I reviewed this report in Windows using PVS-Studio Standalone utility.
In Linux you can use a utility Plog Converter that converts the report into one of the following formats:
- xml - a convenient format for further processing of the results of the analysis, which is supported by the plugin for SonarQube;
- csv - a text format for providing data as a table;
- errorfile is the output format of the gcc and clang;
- tasklist - an error format that can be opened in QtCreator.
Further on, to view the reports, you can use QtCreator, Vim/gVim, GNU Emacs, LibreOffice Calc. The documentation "How to run PVS-Studio on Linux" gives a detailed description of this process. (see "Filtering and viewing the analyzer report").
V501 (1 error)
The V501 diagnostic detected just one error, but a very nice one. The error is in the comparison function, which echoes the topic of a recent article "Evil in the 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' to the left and to the right of the '>' operator. ephysics_body.cpp 450
A Typo. The last comparison should be as follows:
if (stacking1->stacking > stacking2->stacking) return 1;V512 (8 errors)
Firstly, let's 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;
};
There is no need to take a very close look at it. It's just a structure with some fields.
Now let's look at the definition of the structure Eina_Accessor_Array:
typedef struct _Eina_Accessor_Array Eina_Accessor_Array;
struct _Eina_Accessor_Array
{
Eina_Accessor accessor;
const Eina_Array *array;
Eina_Magic __magic;
};
Pay attention that the pointer to the structure Eina_Array is stored in the in the structure Eina_Accessor_Array. Apart from this, these structures are in no way connected with each other and have different sizes.
Now, here is the code fragment that was detected by the analyzer, and which I cannot 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 lead to the 'array' buffer becoming out of range. eina_array.c 186
Let me remove all unnecessary 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));
}
The memory is allocated for the object of the Eina_Accessor_Array type. Further on there is a strange thing.
An object of the Eina_Array type is copied to the allocated memory buffer.
I don't know what this function is supposed to do, but it does something strange.
Firstly, there is an index out of source bounds (of the structure Eina_Array).
Secondly, this copying has no sense at all. Structures have the set of members of completely different types.
Comment by Carsten Haitzler. Function content correct - Type in param is wrong. It didn't actually matter because the function is assigned to a func ptr that does have the correct type and since it's a generic "parent class" the assignment casts to a generic accessor type thus the compiler didn't complain and this seemed to work.
Let's 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 lead to overflow of the buffer 'bgra + k * 16'. draw_convert.c 318
It's all very simple. A usual array index out of bounds.
The array bgra consists of 16 elements of the unsigned int type.
The variable k takes values from 0 to 3 in the loop.
Take a look at the expression: bgra + k * 16.
When the variable k takes the value greater than 0, we'll have the evaluation of a pointer that points outside the array.
However, some messages V512 indicate some code fragments that do not have a real error. Still, I don't think that these are false positives of the analyzer. This code is pretty bad and should be fixed. Let's consider such a 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 overflow of the buffer '& (m)->xx'. eina_matrix.c 1003
The programmer could just copy the array to the structure. Instead of it, the address of the first xx member is used. Probably, it is assumed that further on there will be another fields in the beginning of the structure. This method is used to avoid the crash of the program behavior.
Comment by Carsten Haitzler. Above and related memcpy's - not a bug: taking advantage of guaranteed mem layout in structs.
I don't like it, actually. I recommend to write 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);
}
This is a little longer, but ideologically more correct. If there is no wish to fix the code, the warning can be suppressed using one of the following methods.
The first method. Add a comment to the code:
memcpy(&MATRIX_XX(m), v, sizeof(double) * 16); //-V512
The second method. Add a line to the settings file:
//-V:MATRIX_:512
The third method. Use a markup base.
Other errors:
- V512 A call of the 'memcpy' function will lead to overflow of the buffer '& (m)->xx'. eina_matrix.c 1098
- V512 A call of the 'memcpy' function will lead to overflow of the buffer '& (m)->xx'. eina_matrix.c 1265
- V512 A call of the 'memcpy' function will lead to the '& pd->projection.xx' buffer becoming out of range. evas_canvas3d_camera.c 120
- V512 A call of the 'memcpy' function will lead to the '& pd->projection.xx' buffer becoming out of range. evas_canvas3d_light.c 270
- V512 A call of the 'memcpy' function will lead to overflow of the buffer 'bgra + k * 16'. draw_convert.c 350
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 probability of logical error presence. Check lines: 433, 439. evas_image_load_bmp.c 433
The variable header.comp is compared with the constant 3 twice.
Other errors:
- V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 1248, 1408. evas_image_load_bmp.c 1248
- V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 426, 432. parser.c 426
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
Repeated assignment. One assignment is extra here, or something else was just not copied.
Comment by Carsten Haitzler. Not a bug. Just an overzealous copy & paste of the line.
One more 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 cases are so obvious. Nevertheless, I think that the warnings listed below most likely point to errors:
- V519 The 'pdata->seat->object.in' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 1519, 1521. evas_events.c 1521
- V519 The 'pdata->seat->object.in' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 2597, 2599. evas_events.c 2599
- V519 The 'b->buffer[r]' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 348, 353. evas_image_load_pmaps.c 353
- V519 The 'attr_amount' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 13891, 13959. edje_edit.c 13959
- V519 The 'async_loader_running' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 152, 165. evas_gl_preload.c 165
- V519 The 'textlen' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 86, 87. elm_code_widget_undo.c 87
- V519 The 'content' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 313, 315. elm_dayselector.c 315
- V519 The 'wd->resize_obj' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 3099, 3105. elm_entry.c 3105
- V519 The 'wd->resize_obj' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 3125, 3131. elm_entry.c 3131
- V519 The 'idata->values' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 128, 129. elm_view_list.c 129
- V519 The 'wd->resize_obj' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 2602, 2608. efl_ui_text.c 2608
- V519 The 'wd->resize_obj' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 2628, 2634. efl_ui_text.c 2634
- V519 The 'finfo' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 706, 743. evas_image_load_gif.c 743
- V519 The 'current_program_lookups' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 15819, 15820. edje_cc_handlers.c 15820
Note. Carsten Haitzler, commenting on the article, wrote that V519 warnings, given in the list, are false positives. I do not agree with such approach. Perhaps, code works correctly, but it is still worth paying attention and fixing. I decided to leave the list in the article, so that readers could estimate themselves, if repeats of variables assignment are false positives or not. But if Carsten says, that they are not errors, I will not take them into account.
V522 (563 errors)
The EFL project has a big problem - the checks if the memory was allocated or not. In general, there are such checks. Example:
if (!(el = malloc(sizeof(Evas_Stringshare_El) + slen + 1)))
return NULL;
Moreover, sometimes they are in those places, where they aren't really needed (see about the warning V668 below).
But in a huge number of cases there are no checks at all. Let's take a look at a couple of the analyzer warnings.
Comment by Carsten Haitzler. OK so this is a general acceptance that at least on Linux which was always our primary focus and for a long time was our only target, returns from malloc/calloc/realloc can't be trusted especially for small amounts. Linux overcommits memory by default. That means you get new memory but the kernel has not actually assigned real physical memory pages to it yet. Only virtual space. Not until you touch it. If the kernel cannot service this request your program crashes anyway trying to access memory in what looks like a valid pointer. So all in all the value of checking returns of allocs that are small at least on Linux is low. Sometimes we do it... sometimes not. But the returns cannot be trusted in general UNLESS its for very large amounts of memory and your alloc is never going to be serviced - e.g. your alloc cannot fit in virtual address space at all (happens sometimes on 32bit). Yes overcommit can be tuned but it comes at a cost that most people never want to pay or no one even knows they can tune. Secondly, fi an alloc fails for a small chunk of memory - e.g. a linked list node... realistically if NULL is returned... crashing is about as good as anything you can do. Your memory is so low that you can crash, call abort() like glib does with g_malloc because if you can't allocate 20-40 bytes ... your system is going to fall over anyway as you have no working memory left anyway. I'm not talking about tiny embedded systems here, but large machines with virtual memory and a few megabytes of memory etc. which has been our target. I can see why PVS-Studio doesn't like this. Strictly it is actually correct, but in reality code spent on handling this stuff is kind of a waste of code given the reality of the situation. 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. This is brand new code that arrived 2 months ago and still is being built out and tested and not ready for prime time. It's part of our live debugging infra where any app using EFL can be controlled by a debugger daemon (if it is run) and controlled (inspect all objects in memory and the object tree and their state with introspection live as it runs), collect execution timeline logs (how much time is spent in what function call tree where while rendering in which thread - what threads are using what cpu time at which slots down to the ms and below level, correlated with function calls, state of animation system and when wakeup events happen and the device timestamp that triggered the wakeup, and so on ... so given that scenario ... if you can't calloc a tiny session struct while debugging a crash accessing the first page of memory is pretty much about as good as anything... as above on memory and aborts etc.
Comment by Andrey Karpov. It is not very clear why it is mentioned here, that this is new and non-tested code. In the first place, static analyzers are intended to detect bugs in new code :).
PVS-Studio warning: V522 There might be dereferencing of a potential null pointer 'session'. eina_debug.c 440
The programmer allocated the memory with the calloc function and used it right away.
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
The same situation repeated 563 times. I cannot provide them all in the article. Here is 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
Correct variant of the check should be as follows:
if ((status < 500) || (status > 599))
A fragment of code with this error was copied in another two fragments:
- V547 Expression '(status < 500) && (status > 599)' is always false. ecore_con_url.c 658
- V547 Expression '(status < 500) && (status > 599)' is always false. ecore_con_url.c 1340
Another erroneous 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
After the pointer was added 1, there is no point in checking it against NULL.
The pointer match can become equal to null, only if there is an overflow upon the addition. However, the pointer overflow is thought to be undefined behavior, so this variant shouldn't be considered.
And another 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 there is an exit from the function. The repeated check has no sense.
Other errors: EFL_V547.txt.
There are warnings V547 that I skipped and didn't note them down, as it wasn't much interesting to sort them out. There can be several more errors among them.
V556 (8 errors)
All of them are issued for one code fragment. First let's take a look at the declaration of two enumerations.
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;
As you see, the names of these constants in the enumerations are similar. This is what was failing for a programmer.
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 warnings:
- V556 The values of different enum types are compared: switch(ENUM_TYPE_A) { case ENUM_TYPE_B: ... }. efl_ui_image.c 2141
- V556 The values of different enum types are compared: switch(ENUM_TYPE_A) { case ENUM_TYPE_B: ... }. efl_ui_image.c 2145
- V556 The values of different enum types are compared: switch(ENUM_TYPE_A) { case ENUM_TYPE_B: ... }. efl_ui_image.c 2149
- V556 The values of different enum types are compared: switch(ENUM_TYPE_A) { case ENUM_TYPE_B: ... }. efl_ui_image.c 2153
- V556 The values of different enum types are compared: switch(ENUM_TYPE_A) { case ENUM_TYPE_B: ... }. efl_ui_image.c 2157
- V556 The values of different enum types are compared: switch(ENUM_TYPE_A) { case ENUM_TYPE_B: ... }. efl_ui_image.c 2161
- V556 The values of different enum types are compared: switch(ENUM_TYPE_A) { case ENUM_TYPE_B: ... }. efl_ui_image.c 2165
- V556 The values of different enum types are compared: switch(ENUM_TYPE_A) { case ENUM_TYPE_B: ... }. efl_ui_image.c 2169
Instances from different enumerations are compared eight times.
At the same time, thanks to luck, these comparisons work correctly. The constants are the same:
- ELM_IMAGE_ORIENT_NONE = 0 ; EVAS_IMAGE_ORIENT_NONE = 0,
- ELM_IMAGE_ORIENT_0 = 0 ; EVAS_IMAGE_ORIENT_0 = 0
- ELM_IMAGE_ROTATE_90 = 1 ; EVAS_IMAGE_ORIENT_90 = 1
- and so on.
The function will work correctly, but still, these are errors.
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.
Comment by Andrey Karpov. I do not agree that these are false positives in this case and in some other ones. Following such logic, it turns out that a warning for incorrect but, for some reason, working code is false positive.
V558 (4 errors)
accessor_iterator<T>& operator++(int)
{
accessor_iterator<T> tmp(*this);
++*this;
return tmp;
}
PVS-Studio warning: V558 Function returns the reference to temporary local object: tmp. eina_accessor.hh 519
To fix the code, you should remove & from the function declaration:
accessor_iterator<T> operator++(int)
Other errors:
- V558 Function returns the reference to temporary local object: tmp. eina_accessor.hh 535
- V558 Function returns the reference to temporary local object: tmp. eina_accessor.hh 678
- V558 Function returns the reference to temporary local object: tmp. eina_accessor.hh 694
V560 (32 errors)
static unsigned int read_compressed_channel(....)
{
....
signed char headbyte;
....
if (headbyte >= 0)
{
....
}
else if (headbyte >= -127 && headbyte <= -1) // <=
....
}
PVS-Studio warning: V560 A part of conditional expression is always true: headbyte <= - 1. evas_image_load_psd.c 221
If the variable headbyte was >= 0, then there is no sense in the check <= -1.
Let's have a look at a different case.
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 warning: V560 A part of conditional expression is always true: (!test). eeze_disk.c 55
The second condition is redundant. If the test pointer were non-null pointer, then the function would have exited.
Other errors: EFL_V560.txt.
V568 (3 errors)
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 warning: 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
I have a suspicion that here the size of the structure should be evaluated, not the pointer size. Then the correct code should be as follows:
addrlen = sizeof(*addr);
Other errors:
- 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_udp.c 228
- 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_unix.c 198
V571 (6 errors)
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 warning: V571 Recurring check. The 'if (!disk->cache.vendor)' condition was already verified in line 298. eeze_disk.c 299
A redundant or incorrect check.
Other errors:
- V571 Recurring check. The 'if (!disk->cache.model)' condition was already verified in line 302. eeze_disk.c 303
- V571 Recurring check. The 'if (priv->last_buffer)' condition was already verified in line 150. emotion_sink.c 152
- V571 Recurring check. The 'if (pd->editable)' condition was already verified in line 892. elm_code_widget.c 894
- V571 Recurring check. The 'if (mnh >= 0)' condition was already verified in line 279. els_box.c 281
- V571 Recurring check. The 'if (mnw >= 0)' condition was already verified in line 285. els_box.c 287
Note. Carsten Haitzler does not consider them as erroneous. He thinks that such warnings are recommendations on micro optimizations. But I think that this code is incorrect and it needs to be fixed. In my opinion, these are errors. We have disagreement about this issue, how to consider these analyzer warnings.
V575 (126 errors)
The diagnostic is triggered when strange factual arguments are passed to the function. Let's consider several variants of how this diagnostic is triggered.
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 warning: V575 The 'munmap' function processes '0' elements. Inspect the second argument. eina_evlog.c 117
First, 0 was written to the variable b->size, then it was passed to the function munmap.
It seems to me that it should be written differently:
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;
}
Let's continue.
EAPI Eina_Bool
eina_simple_xml_parse(....)
{
....
else if ((itr + sizeof("<!>") - 1 < itr_end) &&
(!memcmp(itr + 2, "", sizeof("") - 1)))
....
}
PVS-Studio warning: V575 The 'memcmp' function processes '0' elements. Inspect the third argument. eina_simple_xml_parser.c 355
It's unclear why the programmer compares 0 bytes of memory.
Let's 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 warning: V575 The null pointer is passed into 'free' function. Inspect the first argument. edje_entry.c 2306
If the compress pointer is null, then there is no need to free the memory. The line
else free(compres);
can be removed.
Comment by Carsten Haitzler. Not a bug but indeed some extra if paranoia like code that isn't needed. Micro optimizations again?
Comment by Andrey Karpov. In this case, we also have different opinions. I consider this warning as useful, which points at the error. Probably, another pointer should have been freed and it is absolutely right that the analyzer points at this code. Even though there is no error, code should be fixed so that it would not confuse the analyzer and programmers.
Similarly:
- V575 The null pointer is passed into 'free' function. Inspect the first argument. efl_ui_internal_text_interactive.c 1022
- V575 The null pointer is passed into 'free' function. Inspect the first argument. edje_cc_handlers.c 15962
But most of the V575 diagnostic warnings are related to the use of potentially null pointers. These errors are quite similar to the ones we had a look at when we spoke about the V522 diagnostic.
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 warning: V575 The potential null pointer is passed into 'memcpy' function. Inspect the first argument. main.c 112
We use a pointer without checking if the memory was allocated.
Other errors: EFL_V575.txt.
V587 (2 errors)
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 warning: 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.
Comment by Andrey Karpov. Apparently, we cannot agree about some issues. Some of the cases are erroneous in my view, and inaccurate in Carsten's. As I said, I do not agree with it and, for this reason, I do not include some similar comments in the article.
Another error: V587 An odd sequence of assignments of this kind: A = B; B = A;. Check lines: 1050, 1051. ecore_x_events.c 1051
V590 (3 errors)
static int command(void)
{
....
while (*lptr == ' ' && *lptr != '\0')
lptr++; /* skip whitespace */
....
}
PVS-Studio warning: V590 Consider inspecting the '* lptr == ' ' && * lptr != '\0'' expression. The expression is excessive or contains a misprint. embryo_cc_sc2.c 944
Redundant check. It can be simplified:
while (*lptr == ' ')
Two more similar warnings:
- V590 Consider inspecting the 'sym->ident == 9 || sym->ident != 10' expression. The expression is excessive or contains a misprint. embryo_cc_sc3.c 1782
- V590 Consider inspecting the '* p == '\n' || * p != '\"'' expression. The expression is excessive or contains a misprint. cpplib.c 4012
V591 (1 error)
_self_type& operator=(_self_type const& other)
{
_base_type::operator=(other);
}
PVS-Studio warning: V591 Non-void function should return a value. eina_accessor.hh 330
V595 (4 errors)
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 warnings:
- V595 The 'w' pointer was utilized before it was verified against nullptr. Check lines: 575, 585. evas_engine.c 575
- V595 The 'h' pointer was utilized before it was verified against nullptr. Check lines: 576, 586. evas_engine.c 576
The if (w) and if (h) checks give a hint to the analyzer, that input arguments w and hmay be equal to NULL. It is dangerous that in the beginning of the function they are used without verification.
If you call the eng_image_size_get function, as follows:
eng_image_size_get(NULL, NULL, NULL, NULL);
it won't be ready to it and the null pointer dereference will occur.
The warnings, which, in my opinion, also indicate errors:
- V595 The 'cur->node' pointer was utilized before it was verified against nullptr. Check lines: 9889, 9894. evas_object_textblock.c 9889
- V595 The 'subtype' pointer was utilized before it was verified against nullptr. Check lines: 2200, 2203. eet_data.c 2200
V597 (6 errors)
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 warnings:
- V597 The compiler could delete the 'memset' function call, which is used to flush 'iv' buffer. The memset_s() function should be used to erase the private data. emile_cipher_openssl.c 293
- V597 The compiler could delete the 'memset' function call, which is used to flush 'ik' buffer. The memset_s() function should be used to erase the private data. emile_cipher_openssl.c 294
I have already written in articles many times, why the compiler can delete the calls of memset functions in such code. That's why I don't want to repeat myself. If someone is not familiar with this issue, I suggest reading the description of the V597 diagnostics.
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.
Other errors:
- V597 The compiler could delete the 'memset' function call, which is used to flush 'key_material' buffer. The memset_s() function should be used to erase the private data. emile_cipher_openssl.c 144
- V597 The compiler could delete the 'memset' function call, which is used to flush 'iv' buffer. The memset_s() function should be used to erase the private data. emile_cipher_openssl.c 193
- V597 The compiler could delete the 'memset' function call, which is used to flush 'ik' buffer. The memset_s() function should be used to erase the private data. emile_cipher_openssl.c 194
- V597 The compiler could delete the 'memset' function call, which is used to flush 'key_material' buffer. The memset_s() function should be used to erase the private data. emile_cipher_openssl.c 249
V609 (1 error)
First of all, let's take a look at eina_value_util_type_size function:
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;
}
Pay attention that the function may return 0. Now let's see, how this function is used.
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 warning: V609 Mod by zero. Denominator range [0..24]. eina_inline_value_util.x 60
Potential division by zero. I am not sure if there can be a real situation when eina_value_util_type_size function returns 0. In any case, the code is dangerous.
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 error)
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 warning: V610 Unspecified behavior. Check the shift operator '>>'. The left operand '(- 0x7fffffff - 1)' is negative. ector_software_gradient.c 412
V614 (1 error)
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 warning: V614 Uninitialized variable 'temp' used. Consider checking the first actual argument of the 'gmtime' function. elm_calendar.c 720
V621 (1 error)
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 warning: 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 errors)
There is an ordinary btVector3 class with a constructor. However, this constructor does nothing.
class btVector3
{
public:
....
btScalar m_floats[4];
inline btVector3() { }
....
};
There is also such a Simulation_Msg structure:
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;
};
Pay attention that some class members are of btVector3 type. Now let's see how the structure is created:
_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 warning: V630 The 'calloc' function is used to allocate memory for an array of objects which are classes containing constructors. ephysics_world.cpp 299
A structure, which contains non-POD members, is created using a call of callocfunction.
In practice, this code will work, but it is generally incorrect. Technically, the usage of this structure will end up with undefined behavior.
Another error: 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 errors)
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 warnings:
- V654 The condition '(!ptr) && (evas_mem_free(size))' of loop is always false. main.c 44
- V654 The condition '(!ptr) && (evas_mem_degrade(size))' of loop is always false. main.c 46
Obviously, this is incomplete code.
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 :).
The next case is more interesting and odd.
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 warning: V654 The condition of loop is always false. evas_font_query.c 376
This assignment is executed in the loop:
cur_cluster = cluster + 1;
This means that the comparison (cur_cluster == cluster) is always evaluated as 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 errors)
As we found out earlier, there are hundreds fragments of code where there is no checking of the pointer after the memory is allocated using malloc / calloc function.
Against this background the checks after the usage of the new operator seem like a joke.
There are some harmless errors:
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 warning: 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
There is no point in checking, as in case of impossibility of the memory allocation, the std::bad_alloc exception will be generated.
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.
Comment by Andrey Karpov. I do not agree. See the next comment.
There are more serious errors:
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 warning: 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
If the exception is thrown, not only the logic in the work will get broken. What is more, the memory leak will occur, as the free function will not be called.
Comment by Carsten Haitzler. Same as previous new + NULL check.
Comment by Andrey Karpov. It is not clear for me why it is said here about the Visual C++ 6.0. Yes, it does not through an exception while using a new operator, as well as other old compilers. Yes, if the old compiler is used, the program will work correctly. But Tizen will never be compiled using Visual C++ 6.0! There is no point in thinking about it. A new compiler will through an exception and this will lead to errors. I will emphasize one more time that this is not an extra check. The program works not the way the programmer expects. Moreover, in the second example there is a memory-leak. If we consider a case when new does not through an exception, new(nothrow) should be used. Then the analyzer will not complain. In any way, this code is incorrect.
Other errors: EFL_V668.txt.
V674 (2 errors)
First, let's see how the abs function is declared:
extern int abs (int __x) __attribute__ ((__nothrow__ , __leaf__))
__attribute__ ((__const__)) ;
As you can see, it possesses and returners the int values.
Now let's see, how this function is used.
#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 warnings:
- V674 The '0.001' literal of the 'double' type is compared to a value of the 'int' type. Consider inspecting the 'abs(st->info.mx) > 0.001' expression. elm_gesture_layer.c 2533
- V674 The '0.001' literal of the 'double' type is compared to a value of the 'int' type. Consider inspecting the 'abs(st->info.my) > 0.001' expression. elm_gesture_layer.c 2534
It is weird to compare the int values with constant 0.001. There is definitely a bug here.
V686 (3 errors)
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 warning: V686 A pattern was detected: (!im->file) || ((!im->file) && ...). The expression is excessive or contains a logical error. evas_cache2.c 825
Most likely this is not a real error, but redundant code. Let me explain this using a simple example.
if (A || (A && B) || C)
The expression can be simplified to:
if (A || C)
Although, it is possible that in the expression there is a typo and something important is not checked. In this very case, it is an error. It is hard for me to judge about this unfamiliar code.
Similar redundant conditions:
- V686 A pattern was detected: (!im->file) || ((!im->file) && ...). The expression is excessive or contains a logical error. evas_cache2.c 905
- V686 A pattern was detected: (nextc == '*') || ((nextc == '*') && ...). The expression is excessive or contains a logical error. cpplib.c 1022
V694 (2 errors)
#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 warning: V694 The condition ((pbuffer) + 1) is only false if there is pointer overflow which is undefined behavior anyway. cpplib.c 2496
I will expand the macro to make it clearer.
cpp_buffer *pbuffer = ....;
while (pbuffer + 1)
....
The loop condition is always true. Formally, it can become false in case, if the pointer is equal to its limit. In addition, an overflow occurs. This is considered as undefined behavior, it means that a compiler has the right not to take into account such situation. The compiler can delete the conditional check, and then we will get:
while (true)
pbuffer = CPP_PREV_BUFFER(pbuffer);
It's a very interesting bug.
Another incorrect check: 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 errors)
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 warning: 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
This line contains the error:
pd->colors = realloc(pd->colors, ....);
The old value of the pointer pd->colors does not retain anywhere. A memory leak will occur, if it isn't possible to allocate a new memory block. The address of the previous buffer will be lost, because in pd->colors value NULL will be written.
In some cases, the memory leak is complemented also by a null pointer dereference. If a null pointer dereference isn't handled in any way, the program will abort. If it is handled, the program will continue working, but the memory leak will occur. Here is an example of such code:
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 warning: 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
Other errors: EFL_701.txt.
V728 (4 errors)
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 warning: V728 An excessive check can be simplified. The '||' operator is surrounded by opposite expressions '!itr' and 'itr'. evas_object_textblock.c 9505
This is not a bug, but an unnecessarily complicated condition. It expression can be simplified:
if (!itr || (itr->text_node != n))
Other redundant conditions:
- V728 An excessive check can be simplified. The '||' operator is surrounded by opposite expressions '!p' and 'p'. elm_theme.c 447
- V728 An excessive check can be simplified. The '||' operator is surrounded by opposite expressions '!ss' and 'ss'. config.c 3932
- V728 An excessive check can be simplified. The '||' operator is surrounded by opposite expressions '!icon_version' and 'icon_version'. efreet_icon_cache_create.c 917
V769 (11 errors)
We have already considered the V522 warning in case when there is no checking of the pointer value after the memory allocation. We will now consider a similar error.
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 warning: 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
The allocation memory function is called. The resulting pointer is not dereferenced, but is used in an expression. A value is added to the pointer, so it becomes incorrect to say that the null pointer is used. The pointer is non-null in any case, but it can be invalid if the memory was not allocated. Exactly this code is shown by the considered diagnostic.
By the way, such pointers are more dangerous than null ones. The null pointer dereference will be definitely revealed by the operating system. Using the (NULL + N) pointer one can get to the memory location, which can be modified and stores some data.
Other errors:
- V769 The 'new_txt' pointer in the 'new_txt + outlen' expression could be nullptr. In such case, resulting value will be senseless and it should not be used. eina_str.c 539
- V769 The 'new_txt' pointer in the 'new_txt + outlen' expression could be nullptr. In such case, resulting value will be senseless and it should not be used. eina_str.c 611
- V769 The 'tmp' pointer in the 'tmp ++' expression could be nullptr. In such case, resulting value will be senseless and it should not be used. evas_object_textblock.c 11131
- V769 The 'dst' pointer in the 'dst += sizeof (int)' expression could be nullptr. In such case, resulting value will be senseless and it should not be used. evas_font_compress.c 218
- V769 The 'content' pointer in the 'content + position' expression could be nullptr. In such case, resulting value will be senseless and it should not be used. elm_code_line.c 78
- V769 The 'newtext' pointer in the 'newtext + length1' expression could be nullptr. In such case, resulting value will be senseless and it should not be used. elm_code_line.c 102
- V769 The 'tmp' pointer in the 'tmp + dirlen' expression could be nullptr. In such case, resulting value will be senseless and it should not be used. elm_code_file.c 101
- V769 The 'ptr' pointer in the 'ptr += strlen(first) + newline_len' expression could be nullptr. In such case, resulting value will be senseless and it should not be used. elm_code_widget_text.c 72
- V769 The 'content' pointer in the 'content + 319' expression could be nullptr. In such case, resulting value will be senseless and it should not be used. test_store.c 198
- V769 The 'pos' pointer in the 'pos += sizeof (msg)' expression could be nullptr. In such case, resulting value will be senseless and it should not be used. evas_cserve2_cache.c 2534
V779 (19 errors)
Sometimes the V779 diagnostic indicates the code which is harmless but written incorrectly. For 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 warning: V779 Unreachable code detected. It is possible that an error is present. ecore_x_xinerama.c 92
Under certain conditions, in the function body the screen argument is not used anywhere. Apparently a compiler or an analyzer warned about the argument which is not used. That's why the programmer wrote the assignment which is actually never performed to please that tool.
In my opinion it would be better to mark the argument using EINA_UNUSED.
Now let's look at a more difficult case.
extern void _exit (int __status) __attribute__ ((__noreturn__));
static void _timeout(int val)
{
_exit(-1);
if (val) return;
}
PVS-Studio warning: V779 Unreachable code detected. It is possible that an error is present. timeout.c 30
_exit function does not return the control. This code is incredibly strange. It seems to me, the function must be written as follows:
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).
Comment by Andrey Karpov. I do not agree. Perhaps, the program works correctly, but from professional programmer's point of view, such code is unacceptable. I think is it a mistake to write such code for suppressing false positives. It will confuse a person who will maintain such code.
Other errors: EFL_V779.txt.
V1001 (6 errors)
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 warning: V1001 The 'address_shadow' variable is assigned but is not used until the end of the function. elocation.c 1122
This is very strange code. It seems to me that in fact, something should be written here:
*address_shadow = *address;
Other errors:
- V1001 The 'screen' variable is assigned but is not used until the end of the function. ecore_x_xinerama.c 92
- V1001 The 'ret' variable is assigned but is not used until the end of the function. edje_edit.c 12774
- V1001 The 'ret' variable is assigned but is not used until the end of the function. edje_edit.c 15884
- V1001 The 'position_shadow' variable is assigned but is not used until the end of the function. elocation.c 1133
- V1001 The 'status_shadow' variable is assigned but is not used until the end of the function. elocation.c 1144
Comment By Carsten Haitzler
PVS-Studio seems to find different bugs to Coverity. So far it seems to be noisier than Coverity (more non-bugs pointed out as issues) BUT some of these are bugs so if you're willing to go through all the false positivies there are indeed some gems there. The text reports are far less useful than Coverity but they get the job done. I'd actually consider adding PVS-Studio as a second line of defense after Coverity as it can catch some things Coverity cannot and if you are willing to invest the time, that's a good thing. I've already addressed some of the above issues, others will take time, and some are just non-issues that a tool like PVS-Studio or Coverity will complain about, but being able to ignore it is the best solution.
Conclusion
I would like to remind one more time, that visitors can explore the analyzer reportto make sure that the analyzer characteristics given in the article, correspond to reality.
Among the errors described in the article there is nothing epic, but this is not surprising. As I have already said, the EFL project is regularly checked using Coverity. Despite this, PVS-Studio Analyzer still managed to identify many errors. I think that PVS-Studio would have shown itself better if it had been possible to return to past and swap the analyzers :). I mean, if PVS-Studio had been used first, and then Coverity, PVS-Studio would have shown itself brighter.
No comments:
Post a Comment