Checking Appleseed source code

Most of the projects described by us in the articles contain dozens of warnings from the PVS-Studio analyzer. Of course, this is a small part of the analyzer report selected for the article, but there are projects where there are not so many warnings and there are no interesting “blunders” for the article. Usually these are small or non-developing projects. In this article I will talk about the verification of the Appleseed project, where from the point of view of PVS-Studio, the code is very high-quality.

Introduction

Appleseed is a modern open source engine with physically sound rendering, designed to reproduce photorealistic images, animations and visual effects. It offers a set of tools built on reliable and open technologies, both for individual users and for small studios.

Using the PVS-Studio analyzer in the Appleseed project, consisting of approximately 700 source code files, only a few interesting warnings of the 1st and 2nd levels were found.

Test results

V670 The uninitialized class member 'm_s0_cache' is used to initialize the 'm_s1_element_swapper' member. Remember that members are initialized in the order of declarations inside a class. animatecamera cache.h 1009

class DualStageCache : public NonCopyable { .... S1ElementSwapper m_s1_element_swapper; //<==Line 679 S1Cache m_s1_cache; S0ElementSwapper m_s0_element_swapper; S0Cache m_s0_cache; //<==Line 683 }; FOUNDATION_DSCACHE_TEMPLATE_DEF(APPLESEED_EMPTY) DualStageCache( KeyHasherType& key_hasher, ElementSwapperType& element_swapper, const KeyType& invalid_key, AllocatorType allocator) : m_s1_element_swapper(m_s0_cache, element_swapper)//warning... // warning: referring to an uninitialized member , m_s1_cache(m_s1_element_swapper, allocator) , m_s0_element_swapper(m_s1_cache) , m_s0_cache(key_hasher, m_s0_element_swapper, invalid_key) { } 

The analyzer detected a possible error in the initialization list of the class constructor. Judging by the comment "warning: referring to an uninitialized member", which was already present in the code, the developers know that they still use the uninitialized field 'm_s0_cache' to initialize the 'm_s1_element_swapper' field, but do not correct it. According to the language standard, the order of initialization of class members in the constructor occurs in the order they are declared in the class.
')
V605 Consider verifying the expression: m_variation_aov_index <~ 0. An unsigned value is compared to the number -1. appleseed adaptivepixelrenderer.cpp 154

 size_t m_variation_aov_index; size_t m_samples_aov_index; virtual void on_tile_end( const Frame& frame, Tile& tile, TileStack& aov_tiles) APPLESEED_OVERRIDE { .... if (m_variation_aov_index < ~0) //<== aov_tiles.set_pixel(x, y, m_variation_aov_index, ....); if (m_samples_aov_index != ~0) //<== aov_tiles.set_pixel(x, y, m_samples_aov_index, ....); .... } 

The result of the inversion '~ 0' is the value -1, which is of type int. Then this number becomes the size_t unsigned type. Not scary, but not aesthetically pleasing. In such expressions it is recommended to immediately indicate the constant SIZE_MAX.

At first glance, there is no obvious error here, but my attention was drawn to the use of two different conditional operators, although both conditions check the same thing. Conditions are true if the variables are not equal to the maximum allowable value of type size_t (SIZE_MAX). These checks are recorded in different ways. This code is very suspicious, perhaps there is a logical error.

V668 allocated allocated allocated allocated allocated allocated allocated allocated allocated allocated The exception will be generated in the case of memory allocation error. appleseed string.cpp 58

 char* duplicate_string(const char* s) { assert(s); char* result = new char[strlen(s) + 1]; if (result) strcpy(result, s); return result; } 

The analyzer detected a situation when the value of the pointer returned by the 'new' operator is compared with zero. It should be noted that if the 'new' operator could not allocate memory, then according to the C ++ standard of the language, an exception std :: bad_alloc () is generated.

Thus, in the Appleseed project, which is compiled in Visual Studio 2013, most likely checking the pointer to zero is meaningless. And ever use of this function can lead to unexpected results. It is assumed that the duplicate_string () function will return nullptr if it cannot create a duplicate string. Instead, it will generate an exception for which other parts of the program may not be ready.

V719 Enum : InputFormatEntity. appleseed inputarray.cpp 92

 enum InputFormat { InputFormatScalar, InputFormatSpectralReflectance, InputFormatSpectralIlluminance, InputFormatSpectralReflectanceWithAlpha, InputFormatSpectralIlluminanceWithAlpha, InputFormatEntity }; size_t add_size(size_t size) const { switch (m_format) { case InputFormatScalar: .... case InputFormatSpectralReflectance: case InputFormatSpectralIlluminance: .... case InputFormatSpectralReflectanceWithAlpha: case InputFormatSpectralIlluminanceWithAlpha: .... } return size; } 

And where is the case for InputFormatEntity? This switch () block contains neither the default section nor actions for a variable with the value 'InputFormatEntity'. Is this a mistake or did the author consider it necessary to skip this value?

Such places found two more:

• V719 Enum: InputFormatEntity. appleseed inputarray.cpp 121
• V719 Enum: InputFormatEntity. appleseed inputarray.cpp 182

In the absence of the 'default' section and processing of all values ​​of a variable, you can in some place potentially skip adding the code for the new value of 'InputFormat' and for a long time not knowing about it.

32-bit integer type: (unsigned long int) strvalue appleseed snprintf.cpp 885

 #define UINTPTR_T unsigned long int int portable_vsnprintf(char *str, size_t size, const char *format, va_list args) { const char *strvalue; .... fmtint(str, &len, size, (UINTPTR_T)strvalue, 16, width, //<== precision, flags); .... } 

Finally, a serious error was found in Appleseed, which manifests itself in the 64-bit version of the program. The project is cross-platform and compiled under Windows and Linux. To obtain project files, use Cmake. The build documentation for Windows suggests using “Visual Studio 12 Win64”, therefore, in addition to general-purpose diagnostics (GA, General Analysis), I looked at diagnosing 64-bit errors (64, Viva64) of the PVS-Studio analyzer.

The complete code for defining the macro 'UINTPTR_T' is as follows:

 /* Support for uintptr_t. */ #ifndef UINTPTR_T #if HAVE_UINTPTR_T || defined(uintptr_t) #define UINTPTR_T uintptr_t #else #define UINTPTR_T unsigned long int #endif /* HAVE_UINTPTR_T || defined(uintptr_t) */ #endif /* !defined(UINTPTR_T) */ 

The uintptr_t type is an unsigned integer memsize type and is capable of safely storing the pointer in itself, regardless of the platform's bitness, but the type “unsigned long int” was defined for the build in Windows. The size of the type depends on the data model , and unlike Linux, on Windows the type of 'long' is always 32-bit, so in the Win64 platform the pointer will not fit into a variable of this type.

Conclusion

For its volume, the project tested with PVS-Studio contains few serious warnings from the analyzer, for which it receives the Clear Code medal and may no longer be afraid of the unicorn)



If you want to share this article with an English-speaking audience, then please use the link to the translation: Svyatoslav Razmyslov. Checking Appleseed source code .