Analyzing the Blender project with PVS-Studio

• Introduction
• About the Blender project
• False positives
• Defects and odd code fragments we've found
  • Error in a macro
  • Error while using a macro
  • Null pointer dereferencing
  • Identical functions
  • Misprint in a homogeneous code block
  • Unspecified behavior
  • Odd comparisons
  • Identical actions
  • Incorrect array filling
  • Misprint in code clearing an array
  • Double check
  • Odd code
  • Uncreated file
  • Using a pointer before verifying it against being a null pointer
• Conclusions
• References

We go on analyzing open source projects and making the software world better. This time we have checked the Blender 2.62 package intended for creating 3D computer graphics.

Introduction

We regularly check various open source projects in C/C++ and make reports on check results. It allows the world of open source programs to become better and us to tell programmers about the PVS-Studio tool. Reports usually contain far not all the issues we find: since we are not familiar with projects, it might be difficult for us to tell if certain fragments are real errors or just intricate code. It's OK. We always give the authors of open source projects a free registration key for some time so that they could analyze their source code more thoroughly. If a project is small, the trial version of PVS-Studio will be quite enough to check it, since it provides the full functionality.

Readers often say in comments that checking open source projects is just advertisement of our tool. They also give Coverity as an example of a tool that supports open source projects much more intensively.

This comparison is not fair. Improving quality of open source products' codes has become a result of realizing the Vulnerability Discovery and Remediation Open Source Hardening Project campaign. Within the framework of this initiative, the Coverity company was granted $297,000 to support open source projects [1]. That's not too much, of course, but if we were sponsored at least a bit too, we could be more active analyzing open source projects.

About the Blender project

Blender is an open source package for creating 3D computer graphics which includes tools of designing, animation, rendering, video postprocessing and also tools of making interactive games. Starting with 2002, Blender is an open source project (GNU GPL) and develops under active support by Blender Foundation [2].

The Blender package is written in C, C++ and Python. We naturally checked parts in C and C++. The size of the source code together with additional libraries is 68 Mbytes (2105 KLOC).

In this project, by the way, I seem to have met a function with the highest cyclomatic complexity I've ever seen. This is the fast9_corner_score() function which can be found in the fast_9.c file. Its cyclomatic complexity is 1767. But the function is actually simple, so you won't see anything incredible here.

Analysis was performed by the PVS-Studio static analyzer version 4.60.

False positives

The programming style used in Blender causes the PVS-Studio analyzer to generate a lot of false positives among which real messages get lost. As a result, you cannot start working with Blender without preliminarily customizing the analyzer. It's not that bad, however, as it may seem at first. It will take you few efforts to greatly simplify your work when reading the report.

Let me clarify the above stated idea using numerical data. All in all, PVS-Studio generates 574 first-level warnings referring to general analysis diagnostic rules. Just glancing through the report helps you understand that most of the false positives refer to macros BLI_array_append, BLI_array_growone and other macros starting with "BLI_array_".

These macros are safe but they are used quite often. The analyzer generates warnings V514 and V547 for the places where they are used. To get rid of these warnings you can add a special comment in the BLI_array.h file that contains definitions of all these macros:

//-V:BLI_array_:514,547

This comment can be added anywhere in the text. After that you'll have to relaunch analysis but the result will be quite noticeable: about 280 false positives will be eliminated.

All in all, the amount of first-level messages will be cut from 574 to 294 after adding one single comment! This example shows very well that presence of a large number of false positives doesn't mean that the report is difficult to analyze. The most part of noise can be often removed through quite little effort.

To learn more about methods of suppressing false alarms, please read the corresponding documentation section about suppression of false alarms.

Defects and odd code fragments we've found

Error in a macro

The sample given above shows how one can significantly reduce the number of false positives suppressing warnings related to certain macros. But before suppressing a warning, make sure that there is no real error. I know it from my own experience that when some warning concerns a macro, you feel an urge not to investigate the reasons and ignore it right away. But don't hurry.

For example, consider the DEFAULT_STREAM macro that is used more than once in the Blender project. It is long, so we'll cite only part of it here:

#define  DEFAULT_STREAM  \
  m[dC] = RAC(ccel,dC); \
  \
  if((!nbored & CFBnd)) { \
  \
  ....

PVS-Studio's warning: V564 The '&' operator is applied to bool type value. You've probably forgotten to include parentheses or intended to use the '&&' operator. bf_intern_elbeem solver_main.cpp 567

Parentheses are arranged in a wrong way here. As a result, it is "!nbored" which is calculated first, and only then the & operator is applied to a Boolean value. This is the correct code:

if(!(nbored & CFBnd)) { \

Error while using a macro

An error here occurs not because of the macro, but because of a misprint when using it:

#define MAX2(x,y) ( (x)>(y) ? (x) : (y) )
static Scene *preview_prepare_scene(....)
{
  ...
  int actcol = MAX2(base->object->actcol > 0, 1) - 1;
  ...
}

PVS-Studio's warning: V562 It's odd to compare 0 or 1 with a value of 1: (base->object->actcol > 0) > (1). bf_editor_render render_preview.c 361

If you expand the macro, this is what you'll get:

int actcol = ( ( (base->object->actcol > 0) > (1) ) ?
  (base->object->actcol > 0) : (1) ) - 1;

The "base->object->actcol > 0" expression always gives 0 or 1. The "[0..1] > 1" condition is always false. It means that the statement can be simplified to:

int actcol = 0;

This is obviously not what the programmer intended. The fragment "> 0" must have been taken along by accident when copying the "base->object->actcol" fragment.

This is the correct code:

int actcol = MAX2(base->object->actcol, 1) - 1;

Null pointer dereferencing

static int render_new_particle_system(...)
{
  ParticleSettings *part, *tpart=0;
  ...
  // tpart don't used
  ...
  psys_particle_on_emitter(psmd,tpart->from,
    tpa->num,pa->num_dmcache,tpa->fuv,
    tpa->foffset,co,nor,0,0,sd.orco,0);
  ...
}

PVS-Studio's warning: V522 Dereferencing of the null pointer 'tpart' might take place. bf_render convertblender.c 1788

The 'tpart' pointer in the render_new_particle_system() function is initialized by zero and never changed until the moment of dereferencing. The function is quite complex and contains variables with similar names. This is most likely a misprint and a different pointer should be used.

Identical functions

The analyzer has found a lot of functions with identical bodies. I didn't investigate these messages too closely but I seemed to have found at least one error. Perhaps if Blender's authors use PVS-Studio, they can find other similar fragments.

float uiLayoutGetScaleX(uiLayout *layout)
{
  return layout->scale[0];
}

float uiLayoutGetScaleY(uiLayout *layout)
{
  return layout->scale[0];
}

PVS-Studio's warning: V524 It is odd that the body of 'uiLayoutGetScaleY' function is fully equivalent to the body of 'uiLayoutGetScaleX' function (interface_layout.c, line 2410). bf_editor_interface interface_layout.c 2415

Intuition tells me that the uiLayoutGetScaleY() function should return the second item of the 'scale' array:

float uiLayoutGetScaleY(uiLayout *layout)
{
  return layout->scale[1];
}

Misprint in a homogeneous code block

void tcd_malloc_decode(....) {
  ...
  x0 = j == 0 ? tilec->x0 :
    int_min(x0, (unsigned int) tilec->x0);
  y0 = j == 0 ? tilec->y0 :
    int_min(y0, (unsigned int) tilec->x0);
  x1 = j == 0 ? tilec->x1 :
    int_max(x1, (unsigned int) tilec->x1);      
  y1 = j == 0 ? tilec->y1 :
    int_max(y1, (unsigned int) tilec->y1);
  ...
}

PVS-Studio's warning: V537 Consider reviewing the correctness of 'x0' item's usage. extern_openjpeg tcd.c 650

If you look attentively, you can notice an error occurring when assigning a new value to the 'y0' variable. At the very end of the line, a member of the 'tilec->x0' class is used instead of 'tilec->y0'.

This code was most likely created through the Copy-Paste technology and the programmer forgot to change the name of one variable during editing. This is the correct code:

y0 = j == 0 ? tilec->y0 :
  int_min(y0, (unsigned int) tilec->y0);

Unspecified behavior

#define cpack(x) \
  glColor3ub( ((x)&0xFF), (((x)>>8)&0xFF), (((x)>>16)&0xFF) )
static void star_stuff_init_func(void)
{
  cpack(-1);
  glPointSize(1.0);
  glBegin(GL_POINTS);
}

PVS-Studio's warning: V610 Unspecified behavior. Check the shift operator '>>. The left operand '(- 1)' is negative. bf_editor_space_view3d view3d_draw.c 101

According to the C++ language standard, right shift of a negative value leads to unspecified behavior. In practice this method is often used but you shouldn't do that: it cannot be guaranteed that the code will always work as intended. This issue was discussed in the article "Wade not in unknown waters. Part three".

I suggest rewriting this code in the following way:

cpack(UINT_MAX);

Similar dangerous fragments can be found in other functions:

V610 Undefined behavior. Check the shift operator '<<. The left operand '-1' is negative. bf_intern_ghost ghost_ndofmanager.cpp 289

V610 Undefined behavior. Check the shift operator '<<. The left operand '(~0)' is negative. extern_bullet btquantizedbvh.h 82

V610 Undefined behavior. Check the shift operator '<<. The left operand '(~0)' is negative. extern_bullet btsoftbodyconcavecollisionalgorithm.h 48

Odd comparisons

static PyObject *bpy_bmlayercollection_subscript_slice(
  BPy_BMLayerCollection *self,
  Py_ssize_t start, Py_ssize_t stop)
{
  ...
  if (start >= start) start = len - 1;
  if (stop >= stop)   stop  = len - 1;
  ...
}

PVS-Studio's warnings:

V501 There are identical sub-expressions to the left and to the right of the '>=' operator: start >= start bf_python_bmesh bmesh_py_types_customdata.c 442

V501 There are identical sub-expressions to the left and to the right of the '>=' operator: stop >= stop bf_python_bmesh bmesh_py_types_customdata.c 443

The two conditions shown above never hold. I cannot tell for sure what exactly the developer intended to write here. Perhaps the correct code should look as follows:

if (start >= len) start = len - 1;
if (stop >= len)   stop  = len - 1;

Here is one more odd comparison:

typedef struct opj_pi_resolution {
  int pdx, pdy;
  int pw, ph;
} opj_pi_resolution_t;

static bool pi_next_rpcl(opj_pi_iterator_t * pi) {
  ...
  if ((res->pw==0)||(res->pw==0)) continue;
  ...
}

PVS-Studio's warning: V501 There are identical sub-expressions to the left and to the right of the '||' operator: (res->pw == 0) || (res->pw == 0) extern_openjpeg pi.c 219

Most likely, it is not only the 'pw' variable that should be checked here, but 'ph' too:

if ((res->pw==0)||(res->ph==0)) continue;

Similar incorrect checks can be found here:

V501 There are identical sub-expressions to the left and to the right of the '||' operator: (res->pw == 0) || (res->pw == 0) extern_openjpeg pi.c 300

V501 There are identical sub-expressions to the left and to the right of the '||' operator: (res->pw == 0) || (res->pw == 0) extern_openjpeg pi.c 379

Identical actions

EIGEN_DONT_INLINE static void run(....)
{
  ...
  if ((size_t(lhs0+alignedStart)%sizeof(LhsPacket))==0)
    for (Index i = alignedStart;i<alignedSize;
         i+=ResPacketSize)
      pstore(&res[i],
             pcj.pmadd(ploadu<LhsPacket>(&lhs0[i]),
                       ptmp0, pload<ResPacket>(&res[i])));
  else
    for (Index i = alignedStart;i<alignedSize;
         i+=ResPacketSize)
      pstore(&res[i],
             pcj.pmadd(ploadu<LhsPacket>(&lhs0[i]),
                       ptmp0, pload<ResPacket>(&res[i])));
  ...
}

PVS-Studio's warning: V523 The 'then' statement is equivalent to the 'else' statement. bf_ikplugin generalmatrixvector.h 268

Regardless of a condition, identical actions will be performed in the program. Perhaps it should be that way. But this is most likely an error and the actions should be different.

Incorrect array filling

static int imb_read_tiff_pixels(....)
{
  float *fbuf=NULL;
  ...
  memset(fbuf, 1.0, sizeof(fbuf));
  ...
}

PVS-Studio's warning: V579 The memset function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third argument. bf_imbuf tiff.c 442

The analyzer generates one warning, but the programmer actually managed to make 2 mistakes at once in one line. We have noted it down for ourselves to implement a rule to find the second error - it should be easy.

The first error. The 'fbuf' variable is a pointer, which means that sizeof(fbuf) will return the pointer size instead of the array size. As a result, the memset() function will fill only the first several bytes in the array.

The second error. The array consisting of items of the float type was intended to be filled with ones. But the memset function handles bytes, so the array will be filled with trash.

A similar error can be found here:

V579 The memset function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third argument. bf_imbuf tiff.c 450

Misprint in code clearing an array

int ntlGeometryObjModel::initModel(....)
{
  ...
  ntlSetVec3f averts; averts.mVerts.clear();
  ntlSetVec3f anorms; averts.mVerts.clear();
  ...
}

PVS-Studio's warning: V586 The 'clear' function is called twice for deallocation of the same resource. Check lines: 176, 177. bf_intern_elbeem ntl_geometrymodel.cpp 177

I find it meaningless to clear an array in just now created objects. But I'm not familiar with the project, so perhaps there is some sense in this operation. A misprint causes one and the same array to be cleared both times. This is the correct code:

ntlSetVec3f averts; averts.mVerts.clear();
ntlSetVec3f anorms; anorms.mVerts.clear();

Double check

In Blender's code, we have found two identical checks written besides each other. The second condition should be probably replaced with another. Or maybe this code is correct and the second check is extraneous.

static void fcurve_add_to_list (....)
{
  ...
  if (agrp == NULL) {
    if (agrp == NULL) {
  ...
}

PVS-Studio's warning: V571 Recurring check. The 'if (agrp == ((void *) 0))' condition was already verified in line 1108. bf_blenkernel ipo.c 1110

Odd code

void CcdPhysicsController::RelativeRotate(
  const float rotval[9], bool local)
{
  ...
  btMatrix3x3 drotmat(
    rotval[0],rotval[4],rotval[8],
    rotval[1],rotval[5],rotval[9],
    rotval[2],rotval[6],rotval[10]);
  ...
}

PVS-Studio's warnings:

V557 Array overrun is possible. The '9' index is pointing beyond array bound. ge_phys_bullet ccdphysicscontroller.cpp 867

V557 Array overrun is possible. The '10' index is pointing beyond array bound. ge_phys_bullet ccdphysicscontroller.cpp 868

The 'rotval' pointer can refer to an array of any size. Perhaps the code is correct, and number [9] is just a prompt for a human.

I cannot tell for sure if there is an error here or not. If the rotval array really consists of 9 items, an array overrun will occur.

Uncreated file

void LogFileObject::Write(....) {
  ...
  // If there's no destination file, make one before outputting
  if (file_ == NULL) {
    ...
    // file_ don't used
    ...
    fwrite(file_header_string, 1, header_len, file_);
    ...
}

PVS-Studio's warning: V575 The null pointer is passed into 'fwrite' function. Inspect the fourth argument. extern_libmv logging.cc 870

According to the comment, if the file descriptor equals NULL, a new file shall be created. However, before the fwrite() function is called, the 'filxe_' variable is not used anywhere. As a result, a null pointer will be passed into the fwrite() function as a descriptor.

Using a pointer before verifying it against being a null pointer

PVS-Studio has an interesting rule V595. This diagnostic rule can be briefly posed in this way:

V595 is generated if:

1) a pointer is dereferenced;

2) the pointer is not changed anywhere further;

3) the pointer is compared to 0.

There are some exceptions to this rule, but let's not go into details.

This rule has both its advantage and disadvantage. The former is that you can find interesting errors with its help. The latter is that it produces quite many false positives.

False positives are in most cases determined by the presence of unnecessary checks in macros. We cannot fight this issue yet. Here is a typical example where a false positive is generated:

#define SAFE_RELEASE(p) { if (p) { Release(p); delete p; } }
X *p = ....;
p->Foo(); // <= V595
SAFE_RELEASE(p);

The 'p' pointer is always not equal to NULL. But the code contains a check and the analyzer gets suspicious about it.

We've made such a long introduction because the V595 warning is generated very often in Blender. All in all, PVS-Studio has produced 119 warnings of this type. More than half of them are most likely to be false positives. But authors should study the PVS-Studio-generated report themselves.

Let me give you only one example:

static struct DerivedMesh *dynamicPaint_Modifier_apply(....)
{
  ...
  for (; surface; surface=surface->next) {
    PaintSurfaceData *sData = surface->data;
    if (surface &&
        surface->format !=
          MOD_DPAINT_SURFACE_F_IMAGESEQ &&
        sData)
    {
      ...
}

PVS-Studio's warning: V595 The 'surface' pointer was utilized before it was verified against nullptr. Check lines: 1585, 1587. bf_blenkernel dynamicpaint.c 1585

The 'surface' pointer is used in the beginning to initialize the 'sData' variable. And only then the 'surface' pointer is verified against being a null pointer.

Conclusions

1) Static analyzers are useful. Don't forget that they are most useful when you use them regularly. They help you detect a lot of errors at the earliest stage and therefore avoid numerous painful debuggings, but-reports from testers and users' complaints.

2) PVS-Studio sometimes produces a whole lot of false positives. But they usually can be eliminated through quite little effort.

3) The trial version of PVS-Studio that can be downloaded from the website has the full functionality. It will be sufficient to check small projects. Developers of large free open source programs will receive a free key for some time from us.

References

• Open Source Hardening Project. http://searchsecurity.techtarget.com/definition/Open-Source-Hardening-Project
• Wikipedia. Blender.
• Wikipedia. Cyclomatic complexity.