The following blog post, unless otherwise noted, was written by a member of Gamasutras community.

The thoughts and opinions expressed are those of the writer and not Gamasutra or its parent company.

Abstract

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: