Problems found in Appleseed source code

Majority of the projects we report about in the 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 1-st and 2-nd level that could be of interest to us.

Check 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 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 'm_s1_element_swapper' field initialization another uninitialized 'm_s0_cache' filed 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 < ~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 inversion result of '~0' is -1, having the int type. Then this number converts into an unsigned size_t type. It's not crucial, but not really graceful. It is recommended to specify a SIZE_MAX 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 size_t type value (SIZE_MAX). 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

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

Thus in the Appleseed project, which is compiled into Visual Studio 2013, the pointer comparison with null will be meaningless. And one day such function usage can lead to an unexpected result. It is assumed that the duplicate_string() function will return nullptr 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

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 a default section, nor a variable action with the 'InputFormatEntity' value. Is it a real error or the author deliberately missed the value?

There are two more fragments (cases) like that:

If there is no 'default' section and handling of all variable values, you may possibly miss the code addition for a new 'InputFormat' 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

#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 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 'UINTPTR_T' macro looks like this:

/* 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 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 "unsigned long int" type. The type size depends on the data model, and unlike Linux OS, the 'long' 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.