• 10/14/15 07:12 AM
    Sign in to follow this  

    Problems Found in Appleseed Source Code

    General and Gameplay Programming

    PVS-studio team
    The majority of the projects we report about in these articles contain dozens of PVS-Studio analyzer warnings. Of course we choose just a small portion of data from the analyzer report to be in our articles. There are some projects though, where the quantity of warnings is not that high and the number of some interesting "bloomers" is just not enough for an article. Usually these are small projects, which ceased developing. Today I'm going to tell you about Appleseed project check, the code of which we found quite high-quality, from the point of view of the analyzer.

    Introduction

    Appleseed is a modern, open source, physically-based rendering engine designed to produce photorealistic images, animations and visual effects. It provides individuals and small studios with an efficient, reliable suite of tools built on robust foundations and open technologies. This project contains 700 source code files. Our PVS-Studio analyzer found just several warnings of 1st and 2nd level that could be of interest to us.

    Check Results

    V670 The uninitialized class member [tt]m_s0_cache[/tt] is used to initialize the [tt]m_s1_element_swapper[/tt] member. Remember that members are initialized in the order of their declarations inside a class. animatecamera cache.h 1009 [code]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) { } [/code] The analyzer found a possible error in the constructor class initialization. Judging by the comment: "warning: referring to an uninitialized member", which has already been in the code, we see that the developers know that for the [tt]m_s1_element_swapper[/tt] field initialization another uninitialized [tt]m_s0_cache[/tt] field may be used. They are not correcting it though. According to the language standard, the order of initialization of the class members in the constructor goes in their declaration order in the class. V605 Consider verifying the expression: m_variation_aov_index [lessthan] ~0. An unsigned value is compared to the number -1. appleseed adaptivepixelrenderer.cpp 154 [code]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, ....); .... }[/code] The inversion result of [tt]~0[/tt] is -1, having the [tt]int[/tt] type. Then this number converts into an unsigned [tt]size_t[/tt] type. It's not crucial, but not really graceful. It is recommended to specify a [tt]SIZE_MAX[/tt] constant in such expression right away. At first glance there is no evident error here. But my attention was drawn by the usage of two different conditional operators, though both conditions check the same. The conditions are true if the variables are not equal to the maximum possible [tt]size_t[/tt] type value ([tt]SIZE_MAX[/tt]). These checks are differently written. Such a code looks very suspicious; perhaps there can be some logical error here. V668 There is no sense in testing the 'result' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. appleseed string.cpp 58 [code]char* duplicate_string(const char* s) { assert(s); char* result = new char[strlen(s) + 1]; if (result) strcpy(result, s); return result; }[/code] The analyzer detected a situation when the pointer value, returned by the [tt]new[/tt] operator, is compared to null. We should remember, that if the [tt]new[/tt] operator could not allocate the memory, then according to the C++ language standard, an exception [tt]std::bad_alloc()[/tt] would be generated. Thus in the Appleseed project, which is compiled into Visual Studio 2013, the pointer comparison with null will be meaningless. One day such function usage can lead to an unexpected result. It is assumed that the [tt]duplicate_string()[/tt] function will return [tt]nullptr[/tt] if it can't create a string duplicate. It will generate an exception instead, that other parts of the program may be not ready for. V719 The switch statement does not cover all values of the 'InputFormat' enum: InputFormatEntity. appleseed inputarray.cpp 92 [code]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; }[/code] And where is the case for [tt]InputFormatEntity[/tt]? This [tt]switch()[/tt] block contains neither a default section, nor a variable action with the [tt]InputFormatEntity[/tt] value. Is it a real error or did the author deliberately miss the value? There are two more fragments (cases) like that:
    • V719 The switch statement does not cover all values of the [tt]InputFormat[/tt] enum: InputFormatEntity. appleseed inputarray.cpp 121
    • V719 The switch statement does not cover all values of the [tt]InputFormat[/tt] enum: InputFormatEntity. appleseed inputarray.cpp 182
    If there is no [tt]default[/tt] section and handling of all variable values, you may possibly miss the code addition for a new [tt]InputFormat[/tt] value and not be aware of that for a very long time. V205 Explicit conversion of pointer type to 32-bit integer type: (unsigned long int) strvalue appleseed snprintf.cpp 885 [code]#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); .... }[/code] Finally we found quite a serious error that shows up in a 64-bit version of the program. Appleseed is a cross-platform project that can be compiled on Windows and Linux. To get the project files we use Cmake. In the Windows compilation documentation it is suggested to use "Visual Studio 12 Win64" that's why except the general diagnostics (GA, General Analysis), I've also looked through the diagnostics of 64-bit errors (64, Viva64) of the PVS-Studio analyzer. The full identification code of [tt]UINTPTR_T[/tt] macro looks like this: [code]/* 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) */[/code] The [tt]uintptr_t[/tt] is an unsigned, integer memsize-type that can safely hold a pointer no matter what the platform architecture is, although for Windows compilation was defined [tt]unsigned long int[/tt] type. The type size depends on the data model, and unlike Linux OS, the [tt]long[/tt] type is always 32-bits in Windows. That's why the pointer won't fit into this variable type on Win64 platform.

    Conclusion

    All in all the Appleseed project, which is quite a big one, contains only a few analyzer's warnings. That's why it proudly gets a medal "Clear Code" and can no longer be afraid of our unicorn.
    [attachment=29297:image2.png]


      Report Article
    Sign in to follow this  


    User Feedback

    Create an account or sign in to leave a review

    You need to be a member in order to leave a review

    Create an account

    Sign up for a new account in our community. It's easy!

    Register a new account

    Sign in

    Already have an account? Sign in here.

    Sign In Now


    Brain

    Report ·

      

    Share this review


    Link to review
    FranzB

    Report ·

      

    Share this review


    Link to review
    TheChubu

    Report ·

      

    Share this review


    Link to review
    Rattrap

    Report ·

      

    Share this review


    Link to review
    jbadams

    Report ·

      

    Share this review


    Link to review
    iedoc

    Report ·

      

    Share this review


    Link to review
    AthosVG

    Report ·

      

    Share this review


    Link to review