Geekly Articles each Day
⬆️ ⬇️

PVS-Studio team is preparing a technical breakthrough, but for now we will double-check Blender

PVS-Studio, Blender, C / C ++ Static analysis is most useful with regular checks. Especially for such actively developing projects as Blender. It's time to check it again, and find out what suspicious places you can find this time.



Introduction



Blender is a professional package for creating three-dimensional computer graphics, which includes tools for modeling, animation, rendering, post-processing and editing video with sound, also used to create interactive games.



The project has already been tested previously. The results of testing version 2.62 are presented in the article " Checking a Blender Project with PVS-Studio ".

')

Since the last inspection, the size of the source code, along with additional libraries, has increased to 77 megabytes. And its volume increased to 2206 KLOC. At the time of the previous calibration, the project size was 68 megabytes (2105 KLOC).



The utility SourceMonitor helped me calculate the amount of code. This utility can analyze code in C ++, C, C #, VB.NET, Java, Delphi languages ​​and calculates 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 in the form of a table or diagrams.



So, this article will tell about the errors and suspicious places found in the Blender version 2.77a. The PVS-Studio analyzer version 6.05 was used for verification.



Typos



With the active use of copying mechanisms and automatic code completion, very often errors can occur in the names of various variables and constants. Such errors may lead to incorrect results of calculations or unexpected behavior of the program. In the Blender project, the analyzer found several examples at once. Consider them in detail.



A typo in the condition



CurvePoint::CurvePoint(CurvePoint *iA, CurvePoint *iB, float t3) { .... if ((iA->getPoint2D() - //<= iA->getPoint2D()).norm() < 1.0e-6) { //<= .... } .... }


I’m the operator: iA-> getPoint2D () - iA-> getPoint2D () curve.cpp 136



Inside the CurvePoint function, you work with two similar objects, iA and iB . Different methods of these objects constantly intersect in various operations in a rather long tree of conditions. In one of the conditional blocks, a typo was made. The result is a subtraction operation between the property of the same object. Without knowing the features of the code, it is impossible to say exactly which of the two operands is an error. I suggest two possible options for its correction: 

 if ((iA->getPoint2D()-iB->getPoint2D()).norm()<1.0e-6)....


or 

 if ((iB->getPoint2D()-iA->getPoint2D()).norm()<1.0e-6)....


The following error was also hidden inside the conditional operator. 

 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 the right! ”= 'Operator: m_cols! = M_cols jacobisvd.h 819



In the given code fragment, the equation for the number of rows and columns inside a certain matrix occurs. If the number is not equal, respectively, memory is allocated for new items or they are created. And after, if new cells were added, the matrix resizing operation takes place. But as a result of an error in the expression of the conditional operator, the operation will never occur, since the condition m_cols! = M_cols is always false. In this case, it does not matter which of the two parts of the expression needs to be changed, so I suggest this option: 

 if(m_cols!=m_rows) m_scaledMatrix.resize(rows,cols)


A few more problem areas detected by the V501 diagnostic:

Work with a null pointer



Here a typo in the name led to a more serious error. 

 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 'might take place. functions1d.cpp 107



The above function is quite short, but typos can trap us even in simple functions. From the code it is clear that two objects are created and checked sequentially. But after checking the second object, an error occurred, and if fe was successfully created, instead of it, the result is recorded the result of the function from the first object, which was not created according to early conditions. This will most likely lead to a program crash if the exception is not intercepted by a higher level handler.



Apparently, the second code fragment was written using Copy-Paste. And by chance in one place they forgot to change the variable name ve . The correct code, most likely, should have been like this: 

 FEdge *fe = dynamic_cast<FEdge*>(&inter); if (fe) { result = fe->qi(); return 0; }

Using the null pointer



 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 above fragment, you can see that checking the ibuf variable in case the object is created interrupts the function much earlier than this variable is used. Perhaps this could be stopped and confirm the fact of pointer dereference. However, a closer examination of the code and comments to it, it turns out the true cause of the error. And in fact, there is again a typo. In the places specified by the analyzer, the variable orig_ibuf was actually to be used instead of ibuf.



Invalid variable type



 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:

The example shows two types that are enumerations. And the fact that there was a typo when writing code in the type is quite expected for such types that are almost indistinguishable by name.



In fact, the code works correctly. But at the same time, he is misleading by the type mismatch. A variable gets the value of one enum and is compared with the constants of another. To correct the error in this case, it is enough to change the type of the event variable to eOutlinerLibOpTypes .



Operation Priority Error



 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 A = B <C 'kind. The expression is calculated as the following: 'A = (B <C)'. blf_font.c 414



Non-compliance with the priority of operations is one of the most common errors when working with complex expressions. In this case, this is just a typo, but it has led to a violation of the logic of the ternary operator. Due to an incorrectly set parenthesis, an error occurred with the priority of operations. In addition, the value of the alphatest variable deteriorates . Instead of the value that the ternary operator calculates, the alphatest variable is assigned the bool-type value obtained as a result of the comparison operation (<). And after that the ternary operator works with the value of the alphatest variable , and the result of its work is not saved. To correct the error, you must change the expression as follows: 

 cbuf[3] = (unsigned char)(alphatest = (((int)cbuf[3] + (int)(a * 255)) < 255) ? alphatest : 255);

Error in constant



 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 statement is senseless. Check lines: 1672, 1675. writeffmpeg.c 1675



The function performs a sequential check of the value of a variable against a flag using single-line conditions. As a result of a typo, one of the flags is checked twice. For certain, instead of repeated check another constant should be checked. But there are a lot of variants of these constants, so I can’t guess exactly how to fix this code.



Using one variable in outer and nested loops



 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 the loop. Check lines: 2204, 2212. bmesh_queries.c 2212



Using the same variable in the outer and nested loop may lead to incorrect execution of the outer loop. In this case, this is most likely not an error, since the cycle apparently searches for the necessary element and ends, and the second cycle only works in this case. But still, the use of a single variable is a dangerous place, and can lead to real mistakes if you need to optimize this piece of code.



Redundant code



Extra code snippets can be found in any program. Sometimes this is the old code left over after refactoring. And it happens that the extra fragments serve to maintain the style of the project code. Sometimes these pieces of code can be dangerous. In other words, duplicate code often indicates logical errors.



Double check



 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 of the operator. editmesh_knife.c 781



This is one of the options for an ill-considered condition. This, of course, is not an error, but an extra check, but this does not mean that the code does not need to be improved. The condition consists of several expressions. In this part of the second expression is fully consistent with the test of one of the variables in the first and is superfluous. To fix it, remove the extra lh2-> v check from the second expression. After that, the code will be much clearer.



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



Another version of the logical error. Here two absolutely identical expressions are checked inside an external and nested condition. Re-checking will always produce the same result and does not make sense. Of course, this code does not affect the operation of the program. But it is not known how the code will change over time, and unnecessary checks can be confusing.



Excessive checks are found not in the same place of the project. Here are some more places found by the analyzer:

And the third example is explicit redundancy 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 statement is senseless. Check lines: 331, 332. writefile.c 332



Here is the string if (wd-> error) return; just superfluous, and the function will complete before this condition is processed. Therefore, it is required to simply remove.



Opposite condition blocks



 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



From the fragment it is true that an additional condition is located inside the external conditional block. The nested condition is opposite to the main one and always gives the same result, and the variable sel will never get the value 1 . Therefore, it is sufficient to simply write sel = 0 without re-checking. Although, perhaps the error should be corrected by changing one of the conditions. I do not participate in the development of the project and it is difficult for me to judge what is what.



Overkill



 DerivedMesh *fluidsimModifier_do(....) { .... if (!fluidmd || (fluidmd && !fluidmd->fss)) return dm; .... }


V728 Anonymous check can be simplified. The '||' operator fluidmd and fluidmd. mod_fluidsim_util.c 528



Within one condition, the opposite values ​​of the same variable are checked. Such conditions are often found in different types and variations. They do not harm the work of the program, but in vain complicate the code. This expression can be simplified and lead to the following form: 

 if (!fluidmd || !fluidmd->fss)) ....


Similar places:

Another variant of this condition: 

 void ED_transverts_create_from_obedit(....) { .... if ((tipsel && rootsel) || (rootsel)) {....} .... }


V686 A pattern was detected: (rootsel) || ((rootsel) && ...). The expression is abundant or contains a logical error. ed_transverts.c 325



As in the example above, the same variable is checked twice within the same expression. Such an expression is not erroneous, but is clearly overloaded with an extra check. To make the code more compact and clear it needs to be simplified. 

 if ((tipsel || rootsel) {....}


Similar errors were encountered in other places of the project.

Reassignment



 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



Assignment within conditional blocks does not make sense, since its value is still assigned anew at the end of the cycle without any condition. The conclusion that the extra line is located just above helps the loop located in the code after the given fragment. It differs only in prev variables and the absence of this line in the condition. Moreover, if we assume that the extra line below and the condition CFRA == (int) aknext-> cfra turns out to be false, then the cycle will turn into an infinite one. This fragment obviously needs to be corrected, but how only the project developers know how to change it.



Unnecessary or unused variables



There are a lot of similar fragments with initialized, but not used as a result, variables. Some of them relate to examples of logical errors and redundant rechecks; similar fragments have already been mentioned above many times. There are also constants, which quite possibly should have changed inside functions. But in the end they remained only in the form of a test, 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 the conditional expression is always true: nearest == 0. boids.c 361



The rest of the fragments, I will just list. Perhaps many of them are controversial, but they are worth paying attention to.

Extra cleaning list



 int TileManager::gen_tiles(bool sliced) { .... state.tiles.clear(); //<= .... int tile_index = 0; state.tiles.clear(); state.tiles.resize(num); .... }


V586 The 'clear' function of the same resource. Check lines: 149, 156. tile.cpp 156



In this case, it may just be an extra line. It is possible that some other code was located between the two cleanings of the list earlier, but in this case it is another extra piece that is not useful and should be deleted so as not to clutter up the code. This line may also be due to the fact that it had to clear some other object that is not visible during a cursory inspection of the code. In this case, the fragment will be an obvious mistake, which may lead to unknown consequences for the program.



Very often, this seemingly redundant code can lead to really serious errors or help to avoid their appearance in the future when finalizing the code. That is why you should pay attention to such messages of the analyzer, and not dismiss them as minor.



Intrigue



The PVS-Studio team is now actively working on a new direction. And I cover the rear, filling the information field with articles about re-checking some open projects. What is this direction? I can not say. I will only leave a picture that everyone is free to interpret in his own way.



Picture 2



Conclusion



The analyzer indicated a lot of problem areas in the project. However, Blender sometimes uses a strange style of code, and it is impossible to accurately interpret such fragments as errors. I find dangerous mistakes often due to typos. The PVS-Studio analyzer copes with their finding well. But the errors reflected in the article are solely the opinion of the author, which is quite subjective. And for a full assessment of the capabilities of the analyzer it is worth downloading and trying it yourself.





If you want to share this article with an English-speaking audience, then please use the link to the translation: Alexander Chibisov. Let us recheck Blender .



Read the article and have a question?
Often our articles are asked the same questions. We collected answers to them here: Answers to questions from readers of articles about PVS-Studio, version 2015 . Please review the list.

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

More articles:



All Articles