Our website uses cookies to enhance your browsing experience.
Your message has been sent. We will email you at
If you do not see the email in your inbox, please check if it is filtered to one of the following folders:
Webinar: Evaluation - 05.12
Static analysis is most useful when it is done on a regular basis. Especially when the project is rapidly developing, like the Blender project, for example. Now it's time to check it once more, and see what suspicious fragments we'll find this time.
Blender is a free, open source professional 3D creation suite. It supports the entirety of the 3D pipeline—modeling, rigging, animation, simulation, rendering, compositing, and motion tracking; even video editing and game creation.
We have already checked this project before. You may find the results of the previous check of v2.62 in the article "Analyzing the Blender project with PVS-Studio".
Since the last time we checked it, the size of the source code, including additional libraries, increased to 77 mb. Its codebase is now 2206 KLOC. At the time of the previous check the project was 68 mb (2105 KLOC).
The SourceMonitor utility was a great help to me in evaluating the codebase size. This utility is able to analyze the code in C++, C, C #, VB.NET, Java, and Delphi, and is able to evaluate various metrics. For example, it can determine the cyclomatic complexity of your projects, as well as generate detailed statistics for each of the project files, and show the results as a table or diagrams.
So this article is about errors and suspicious fragments that were found in Blender, v2.77a. To do the analysis, we used PVS-Studio 6.05
During the active usage of copying mechanism and automatic code completion, you may have errors in the names of various variables and constants. Such errors can result in incorrect evaluation results or unexpected program behavior. In the Blender project there were several such examples. Let's take a closer look.
CurvePoint::CurvePoint(CurvePoint *iA, CurvePoint *iB, float t3)
{
....
if ((iA->getPoint2D() - // <=
iA->getPoint2D()).norm() < 1.0e-6) { // <=
....
}
....
}V501 There are identical sub-expressions to the left and to the right of the '-' operator: iA->getPoint2D() - iA->getPoint2D() curve.cpp 136
Inside the CurvePoint function the program handles two objects with similar names - iA and iB. Different methods of these objects get intersected all the time in various operations in quite a long tree of conditions. There is a typo in one of these conditional blocks. As a result we have a subtraction operation between the properties of one and the same object. Without knowing the peculiarities of the code, it's quite hard to say, in which operand we have an error. I can suggest two variants of how to fix it:
if ((iA->getPoint2D()-iB->getPoint2D()).norm()<1.0e-6)....or
if ((iB->getPoint2D()-iA->getPoint2D()).norm()<1.0e-6)....The following error was also hiding inside a conditional statement.
template<typename MatrixType, int QRPreconditioner>
void JacobiSVD<MatrixType, QRPreconditioner>::allocate(....)
{
....
if(m_cols>m_rows)m_qr_precond_morecols.allocate(*this);
if(m_rows>m_cols)m_qr_precond_morerows.allocate(*this);
if(m_cols!=m_cols)m_scaledMatrix.resize(rows,cols); // <=
}V501 There are identical sub-expressions to the left and right of the '!=' operator: m_cols != m_cols jacobisvd.h 819
In the given fragment, you can see the equalization of the number of rows and columns inside some matrix. If the quantity isn't the same, the program allocates memory for new elements and creates them. Later, if new cells are added, there is an operation of altering the matrix size. Unfortunately, due to the error in the conditional statement the operation will never be executed, because the condition m_cols!=m_cols is always false. In this case it doesn't matter which part is changed, so I suggest the following variant:
if(m_cols!=m_rows) m_scaledMatrix.resize(rows,cols)Several more problem areas were detected by the V501 diagnostic:
The typo in the names had more serious consequences.
int QuantitativeInvisibilityF1D::operator()(....)
{
ViewEdge *ve = dynamic_cast<ViewEdge*>(&inter);
if (ve) {
result = ve->qi();
return 0;
}
FEdge *fe = dynamic_cast<FEdge*>(&inter);
if (fe) {
result = ve->qi(); // <=
return 0;
}
....
}V522 Dereferencing of the null pointer 've' might take place. functions1d.cpp 107
This function is fairly short, but typos can trap us, even in simple functions. We can see in the code that two objects get created and checked. However, after the check of the second object, an error occurred, and even if fe was successfully created, instead of it, the result of function work from the first object is written to the result; according to the previous conditions, this object wasn't created at all. This will most likely lead to a crash of the program if this exception isn't caught by a handler of a higher level.
Apparently, the second code fragment was written using Copy-Paste. By accident the programmer forgot to change the variable name ve. The correct code should probably be like this:
FEdge *fe = dynamic_cast<FEdge*>(&inter);
if (fe) {
result = fe->qi();
return 0;
}static ImBuf *accessor_get_ibuf(....)
{
ImBuf *ibuf, *orig_ibuf, *final_ibuf;
....
/* First try to get fully processed image from the cache. */
ibuf = accesscache_get(accessor,
clip_index,
frame,
input_mode,
downscale,
transform_key);
if (ibuf != NULL) {
return ibuf;
}
/* And now we do postprocessing of the original frame. */
orig_ibuf = accessor_get_preprocessed_ibuf(accessor,
clip_index,
frame);
if (orig_ibuf == NULL) {
return NULL;
}
....
if (downscale > 0) {
if (final_ibuf == orig_ibuf) {
final_ibuf = IMB_dupImBuf(orig_ibuf);
}
IMB_scaleImBuf(final_ibuf,
ibuf->x / (1 << downscale), // <=
ibuf->y / (1 << downscale)); // <=
}
....
if (input_mode == LIBMV_IMAGE_MODE_RGBA) {
BLI_assert(ibuf->channels == 3 || // <=
ibuf->channels == 4); // <=
}
....
return final_ibuf;
}Warnings:
In the fragment given above, you can see that the check of ibuf variable interrupts the function much sooner than this variable is used if the object was created. We could probably stop here and confirm the fact of a pointer dereferencing. At the same time, if we do a more painstaking examination of the code and the comments to it, we see a true reason for the error. This is a typo, again. In the fragments indicated by the analyzer, the programmer should have used the variable orig_ibuf instead of ibuf.
typedef enum eOutlinerIdOpTypes {
OUTLINER_IDOP_INVALID = 0,
OUTLINER_IDOP_UNLINK,
OUTLINER_IDOP_LOCAL,
....
} eOutlinerIdOpTypes;
typedef enum eOutlinerLibOpTypes {
OL_LIB_INVALID = 0,
OL_LIB_RENAME,
OL_LIB_DELETE,
} eOutlinerLibOpTypes;
static int outliner_lib_operation_exec(....)
{
....
eOutlinerIdOpTypes event; // <=
....
event = RNA_enum_get(op->ptr, "type");
switch (event) {
case OL_LIB_RENAME: // <=
{
....
}
case OL_LIB_DELETE: // <=
{
....
}
default:
/* invalid - unhandled */
break;
}
....
}Warnings:
In this example you can see two types that are enumerations. It's quite an expected fact that there was a typo in the names that are almost the same.
In fact, the code works correctly. At the same time it confuses us by the mismatch of the types. The variable gets a value of an enumeration, and is compared with the constants of a different one. To correct this error it's enough to change the type of the variable event to eOutlinerLibOpTypes.
static void blf_font_draw_buffer_ex(....)
{
....
cbuf[3] = (unsigned char)((alphatest = ((int)cbuf[3] +
(int)(a * 255)) < 255) ? alphatest : 255);
....
}V593 Consider reviewing the expression of the 'A = B < C' kind. The expression is calculated as following: 'A = (B < C)'. blf_font.c 414
Failure to comply with the operation precedence is one of the most common errors when working with complex expressions. In this case it's just a typo, but it led to a breach of the logic of the ternary operator. Due to an incorrectly put parentheses, there was an operation precedence error. On top of that, the value of alphatest variable also gets damaged. Instead of the value, which is evaluated by the ternary operator, the alphatest variable is assigned with a value of bool-type that was obtained in the result of a comparison operation. Only after that the ternary operator works with the value of the alphatest variable, and the result doesn't get saved. To fix this bug, we should change the expression as follows:
cbuf[3] = (unsigned char)(alphatest = (((int)cbuf[3] +
(int)(a * 255)) < 255) ? alphatest : 255);bool BKE_ffmpeg_alpha_channel_is_supported(RenderData *rd)
{
int codec = rd->ffcodecdata.codec;
if (codec == AV_CODEC_ID_QTRLE)
return true;
if (codec == AV_CODEC_ID_PNG)
return true;
if (codec == AV_CODEC_ID_PNG)
return true;
....
}V649 There are two 'if' statements with identical conditional expressions. The first 'if' statement contains function return. This means that the second 'if' statement is senseless. Check lines: 1672, 1675. writeffmpeg.c 1675
We see a successive check of the variable value to match the flag with the help of single-line conditions. Because of the typo one of the flags gets checked twice. Most likely, a different constant should have been checked instead of a repeated check. There are a lot of variants of these constants that's why it's hard to say how this code should be fixed.
bool BM_face_exists_overlap_subset(...., const int len)
{
int i;
....
for (i = 0; i < len; i++) {
BM_ITER_ELEM (f, &viter, varr[i], BM_FACES_OF_VERT) {
if ((f->len <= len) && (....)) {
BMLoop *l_iter, *l_first;
if (is_init == false) {
is_init = true;
for (i = 0; i < len; i++) { // <=
BM_ELEM_API_FLAG_ENABLE(varr[i], _FLAG_OVERLAP);
}
}
....
}
}
}
}V535 The variable 'i' is being used for this loop and for the outer loop. Check lines: 2204, 2212. bmesh_queries.c 2212
Using the same variable in the outer and inner loop may lead to incorrect execution of the outer loop. In this case this will less likely be an error, as the loop is probably looking for the necessary element and exits, and the second loop is triggered only in this case. But still, using a single variable is a dangerous trick and may lead to real errors if there is a necessity to optimize this code fragment.
Excessive code fragments can be found in any program. Sometimes this is old code that was forgotten after refactoring. But at times those extra fragments serve as a way to keep up the project style. Such fragments can be quite dangerous. In other words, duplicate code often indicates the presence of logical errors.
static void knife_add_single_cut(....)
{
....
if ((lh1->v && lh2->v) && // <=
(lh1->v->v && lh2->v && lh2->v->v) && // <=
(e_base = BM_edge_exists(lh1->v->v, lh2->v->v)))
{
....
return;
}
....
}V501 There are identical sub-expressions 'lh2->v' to the left and right of the '&&' operator. editmesh_knife.c 781
This is one of the variants of a condition that wasn't thought out well. It's certainly not a mistake, just an extra check, but that doesn't mean that the code does not need additional review. The condition consists of several expressions. At the same time a part of the second expression is the same as the check of one variable from the first expression, so it's not needed here. To fix this code we need to remove the excessive check lh2->v from the second expression. After that the code will become much easier to read.
Another example:
static int edbm_rip_invoke__vert(....)
{
....
if (do_fill) {
if (do_fill) {
....
}
}
....
}V571 Recurring check. The 'if (do_fill)' condition was already verified in line 751. editmesh_rip.c 752
One more variant of a logical error. Absolutely identical expressions are checked inside an outer and inner condition. The double check will always give the same result, which makes no sense. Of course, this code does not affect the program work in any way. But it's not clear how this code will change over time, and extra checks can mislead a person in the future.
Unnecessary checks can be found in several fragments of the project. Here are several more spots, detected by the analyzer:
And the third example is obviously redundant code:
static void writedata_do_write(....)
{
if ((wd == NULL) || wd->error ||
(mem == NULL) || memlen < 1) return;
if (wd->error) return;
....
}V649 There are two 'if' statements with identical conditional expressions. The first 'if' statement contains function return. This means that the second 'if' statement is senseless. Check lines: 331, 332. writefile.c 332
The string if (wd->error) return; is excessive, and the function will exit earlier than this condition will be handled. And so, it should just be removed.
static int select_less_exec(....)
{
....
if ((lastsel==0)&&(bp->hide==0)&&(bp->f1 & SELECT)){
if (lastsel != 0) sel = 1;
else sel = 0;
....
}
....
}V637 Two opposite conditions were encountered. The second condition is always false. Check lines: 938, 939. editcurve_select.c 938
In the fragment we can see that there is an extra condition inside an outer condition block. The inner condition is opposite the main one and always gives the same result; the sel variable will never be 1. Therefore, it is enough to simply write sel = 0 without additional check. Although this error may have been fixed by changing one of the expressions. Since I didn't take part in the creation of this project, it's hard for me to say for sure.
DerivedMesh *fluidsimModifier_do(....)
{
....
if (!fluidmd || (fluidmd && !fluidmd->fss))
return dm;
....
}V728 An excessive check can be simplified. The '||' operator is surrounded by opposite expressions '!fluidmd' and 'fluidmd'. mod_fluidsim_util.c 528
Opposite values of one and the same variable are checked under one condition. Such conditions are often found of different kinds and variations. They don't cause any harm to the software, but they can complicate the code. This expression can be simplified and written as follows:
if (!fluidmd || !fluidmd->fss)) ....Similar fragments:
One more such a condition:
void ED_transverts_create_from_obedit(....)
{
....
if ((tipsel && rootsel) || (rootsel)) {....}
....
}V686 A pattern was detected: (rootsel) || ((rootsel) && ...). The expression is excessive or contains a logical error. ed_transverts.c 325
As in the example given above, the same variable is checked twice inside one expression. This expression is not an erroneous one, but it definitely has an extra check. Let's simplify it to make it more compact and easy-to-read.
if ((tipsel || rootsel) {....}There were such errors in other places in the project.
static bool find_prev_next_keyframes(....)
{
....
do {
aknext = (ActKeyColumn *)BLI_dlrbTree_search_next(
&keys, compare_ak_cfraPtr, &cfranext);
if (aknext) {
if (CFRA == (int)aknext->cfra) {
cfranext = aknext->cfra; // <=
}
else {
if (++nextcount == U.view_frame_keyframes)
donenext = true;
}
cfranext = aknext->cfra; // <=
}
} while ((aknext != NULL) && (donenext == false));
....
}V519 The 'cfranext' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 447, 454. anim_draw.c 454
The assignment inside conditional blocks makes no sense, because its value is assigned again in the end of the loop without any condition. A loop, placed in the code right after the given fragment helps us draw the conclusion that the excessive string is placed above. It differs only in the prev variables, and the absence of this string in the condition. Moreover, assuming that the extra string is underneath, and the condition CFRA == (int)aknext->cfra turns out to be false, then this loop will become an infinite one. This fragment really needs some fixing, but how to do it exactly - only the developers of the project know.
There were many such fragments with initialized, but unused, variables in the project. Some of them can be considered as logical errors and excessive checks, but we have already spoken about them a lot. There are also constants that probably should have been changed inside the functions. But as a result they are just checks, always returning the same result. An example of such a fragment:
static int rule_avoid_collision(....)
{
....
int n, neighbors = 0, nearest = 0; // <=
....
if (ptn && nearest==0) // <=
MEM_freeN(ptn);
return ret;
}V560 A part of conditional expression is always true: nearest == 0. boids.c 361
I'll just provide the other fragments as a list. Perhaps some of them are debatable, but they are worth paying attention to.
int TileManager::gen_tiles(bool sliced)
{
....
state.tiles.clear(); // <=
....
int tile_index = 0;
state.tiles.clear();
state.tiles.resize(num);
....
}V586 The 'clear' function is called twice for deallocation of the same resource. Check lines: 149, 156. tile.cpp 156
In this case, it might just be the extra line. There probably used to be some code between the two list clearings, but in this case it's just another useless fragment that should be removed so that the code isn't cluttered up. This string can be a consequence of the fact that some other object should be cleared in it, which is not seen at first glance. In this case the fragment will be a real error that may lead to unexpected results for the program.
Very often such seemingly redundant code may lead to really serious errors, or my help avoiding them in the future during further modifications. This is why you should pay attention to these analyzer warnings, and not mark them as "unimportant".
PVS-Studio Team is now actively working on a new direction in development. And I am covering backs, filling the information field with articles about the rechecking of some open source projects. What is the direction that we are talking about? I cannot say. I'll just leave a picture here that you are free to interpret as you wish.
The analyzer detected quite a number of troublesome spots in the project. However, at times, the coding style in Blender is quite strange and we cannot say for sure that these are errors. In my opinion, dangerous errors often occur because of typos. PVS-Studio is especially good at catching such bugs. Those bugs, described in this article reflect the personal opinion of the author which is quite subjective. To see the full range of analyzer abilities, you should download it, and try it out yourself.
0
0
0
0