Checking Appleseed Source Code


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

    Introduction


    Appleseed is a state-of-the-art open source engine with physically sound rendering designed to play 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, which consists of approximately 700 files with source code, only a few interesting warnings of the 1st and 2nd levels were found.

    Validation 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 their 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 the uninitialized field 'm_s0_cache' is used to initialize the 'm_s1_element_swapper' field, but they do not fix it. According to the language standard, the order of initialization of class members in the constructor occurs in the order of their declaration 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 of '~ 0' is a value of -1, which is of type int. Then this number turns into the unsigned type size_t. Not scary, but not aesthetically pleasing. In such expressions, it is recommended that you immediately specify the constant SIZE_MAX.

    At first glance, there is no obvious error, 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 allowed value of type size_t (SIZE_MAX). These checks are recorded in different ways. Such code is very suspicious, perhaps there is a logical error.

    V668There 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
    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 where the value of the pointer returned by the 'new' operator is compared to zero. It should be noted that if the 'new' operator could not allocate memory, then according to the C ++ language standard, an exception std :: bad_alloc () is generated.

    Thus, in an Appleseed project that compiles in Visual Studio 2013, most likely checking for a pointer to zero is pointless. And someday using this function may lead to unexpected results. The duplicate_string () function is supposed to return nullptr if it cannot duplicate the string. Instead, it will throw an exception that other parts of the program might not be ready for.

    V719The switch statement does not cover all values ​​of the 'InputFormat' 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 the action for a variable with the value 'InputFormatEntity'. Is this a mistake or did the author consider it necessary to skip this value?

    Two more such places were found:
    • V719 The switch statement does not cover all values ​​of the 'InputFormat' enum: InputFormatEntity. appleseed inputarray.cpp 121
    • V719 The switch statement does not cover all values ​​of the 'InputFormat' enum: InputFormatEntity. appleseed inputarray.cpp 182

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

    V205 Explicit conversion of pointer type to 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 that manifests itself in the 64-bit version of the program. The project is cross-platform and compiles for Windows and Linux. Cmake is used to get project files. The assembly documentation for Windows suggests using “Visual Studio 12 Win64”, therefore, in addition to general-purpose diagnostics (GA, General Analysis), I also looked at the diagnostics of 64-bit errors (64, Viva64) of the PVS-Studio analyzer.

    The complete macro definition code for '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 able to safely store a pointer in itself regardless of the platform bit depth, but the type “unsigned long int” was defined for assembly on Windows. The type size depends on the data model , and unlike Linux, on Windows the type 'long' is always 32-bit, so on Win64 platform the pointer will not fit into a variable of this type.

    Conclusion


    For its size, the project verified using PVS-Studio contains few serious warnings from the analyzer, for this it receives the “Clear Code” medal and can 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 .



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

    Also popular now: