PVS-Studio for Linux Went on a Tour Around Disney

Maxim Stefanov
Articles: 2



Recently we released a Linux version of PVS-Studio analyzer, which we had used before to check a number of open-source projects such as Chromium, GCC, LLVM (Clang), and others. Now this list includes several projects developed by Walt Disney Animation Studios for the community of virtual-reality developers. Let's see what bugs and defects the analyzer found in these projects.

Picture 1

Couple of words about Disney

For many years, the Walt Disney Company has been bringing joy and unforgettable experience to the audience across the world through their lovely stories and characters. Every year, they release new films and computer-animated films that grow even more fascinating, spectacular, and technically challenging, so the need for various software tools to help visual-effects artists fulfill their ideas, is also growing.

Walt Disney Animation Studios programmers assist animation and visual-effects specialists by developing software tools available as open-source applications in C and C++ to anyone working in the virtual-reality industry. The list of such tools includes the following products:

  • Partio (The goal of Partio is to provide a unified interface akin to unified image libraries that makes it easier to load, save, and manipulate particle files)
  • Alembic (an open file format widely adopted across the industry as a standard for the interchange of animated computer graphics between digital-content development packages)
  • Universal Scene Description (an efficient tool for loading and transmitting scene descriptions between different graphics applications)
  • OpenSubdiv (implements high-performance subdivision surface evaluation based on scaled-down models)
  • Dinamica (Autodesk Maya plugin based on the Bullet Physics Library engine)
  • PTex (texture mapping system)

The source files of all these Disney tools can be downloaded from https://disney.github.io/ .

Analysis results

The projects by Walt Disney that we have analyzed for this article are quite small and make a total of just a few dozens of thousands LOC in C and C++, hence the number of bugs across the projects is also small.

Partio

Picture 4

PVS-Studio diagnostic message: V547 Expression '"R"' is always true. PDA.cpp 90

ParticlesDataMutable* readPDA(....)
{
  ....
  while(input->good())
  {
    *input>>word;
    ....
    if(word=="V"){
        attrs.push_back(simple->addAttribute(....);
    }else if("R"){                                 // <=
        attrs.push_back(simple->addAttribute(....);
    }else if("I"){                                 // <=
        attrs.push_back(simple->addAttribute(....);
    }
    index++;
  }
  ....
}

This code triggered a warning saying that the conditional expression is always true, so the statement in the else branch will never execute. The cause of this mistake must be the programmer's inattention, and then the fixed code should look like this:

....
if(word=="V"){
    attrs.push_back(simple->addAttribute(....);
}else if(word=="R"){                                // <=
    attrs.push_back(simple->addAttribute(....);
}else if(word=="I"){                                // <=
    attrs.push_back(simple->addAttribute(....);
}
....

PVS-Studio diagnostic message: V528 It is odd that pointer to 'char' type is compared with the '\0' value. Probably meant: *charArray[i] != '\0'. MC.cpp 109

int CharArrayLen(char** charArray)
{
  int i = 0;
  if(charArray != false)
  {
    while(charArray[i] != '\0')   // <=
    {
      i++;
    }
  }
  return i;
}

As far as I understand, the CharArrayLen() function counts the number of characters in the charArray string, but does it really? There seems to be an error in the while loop's condition that has to do with the pointer to type char being compared with the value '\0'. It is very likely that a pointer dereferencing operation is missing, and then the loop condition should look something like this:

while ((*charArray)[i] != '\0')

By the way, the check before that one looks strange too:

if(charArray != false)

It does work, of course, but it's much better to use the following check instead:

if(charArray != nullptr)

This function looks like it was developed by a novice programmer or was just left unfinished. Why not simply use the strlen() function:

int CharArrayLen(const char** charArray)
{
  if (charArray == nullptr)
    return 0;
  return strlen(*charArray);
}

PVS-Studio diagnostic message: V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'attributeData[i]' is lost. Consider assigning realloc() to a temporary pointer. ParticleSimple.cpp 266

ParticleIndex ParticlesSimple::
addParticle()
{
  ....
  for(unsigned int i=0;i<attributes.size();i++)
    attributeData[i]=
                  (char*)realloc(attributeData[i],       // <=
                                (size_t)attributeStrides[i]*
                                (size_t)allocatedCount);
  ....
}

The analyzer detected a dangerous construct with realloc in the code above. What makes the foo = realloc(foo, ...) construct dangerous is that the function will return nullptr if the storage fails to be allocated, and thus rewrite the pointer's previous value, causing a memory leak or even a crash. It may never happen in most cases, but it's still better to play safe. To avoid the problem, it is recommended that you save the pointer's value to an auxiliary variable before using realloc.

Other similar warnings:

  • V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'attributeData[i]' is lost. Consider assigning realloc() to a temporary pointer. ParticleSimple.cpp 280
  • V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'data' is lost. Consider assigning realloc() to a temporary pointer. ParticleSimpleInterleave.cpp 281
  • V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'data' is lost. Consider assigning realloc() to a temporary pointer. ParticleSimpleInterleave.cpp 292

Alembic

Picture 3

PVS-Studio diagnostic message: V501 There are identical sub-expressions 'm_uKnot' to the left and to the right of the '||' operator. ONuPatch.h 253

class Sample
{
  public:
    ....
    bool hasKnotSampleData() const
    {
      if( (m_numU != ABC_GEOM_NUPATCH_NULL_INT_VALUE) ||
          (m_numV != ABC_GEOM_NUPATCH_NULL_INT_VALUE) ||
          (m_uOrder != ABC_GEOM_NUPATCH_NULL_INT_VALUE) ||
          (m_vOrder != ABC_GEOM_NUPATCH_NULL_INT_VALUE) ||
           m_uKnot || m_uKnot)                            // <=
           return true;
      else
          return false;
    }
    ....
  protected:
    ....
    int32_t m_numU;
    int32_t m_numV;
    int32_t m_uOrder;
    int32_t m_vOrder;
    Abc::FloatArraySample m_uKnot;
    Abc::FloatArraySample m_vKnot;
    ....
}

Another error caused by inattention. As you can easily guess, field m_vKnot should be used instead of the duplicate field m_uKnot in the condition.

PVS-Studio diagnostic message: V523 The 'then' statement is equivalent to the 'else' statement. OFaceSet.cpp 230

void OFaceSetSchema::set( const Sample &iSamp )
{
  ....
  if ( iSamp.getSelfBounds().hasVolume() )
  {
      // Caller explicity set bounds for this sample of the faceset.
      
      m_selfBoundsProperty.set( iSamp.getSelfBounds() );   // <=
  }
  else                                       
  {
      m_selfBoundsProperty.set( iSamp.getSelfBounds() );   // <=
      
      // NYI compute self bounds via parent mesh's faces
  }
  ....
} 

Both branches of the if..else statement in the code above have the same logic despite different comments. Maybe this code snippet is languishing waiting for its turn to be finished as one of the authors' top-priority tasks, but until then, it's faulty and needs to be fixed.

PVS-Studio diagnostic message: V629 Consider inspecting the '1 << iStreamID' expression. Bit shifting of the 32-bit value with a subsequent expansion to the 64-bit type. StreamManager.cpp 176

void StreamManager::put( std::size_t iStreamID )
{
  ....
  // CAS (compare and swap) non locking version
  Alembic::Util::int64_t oldVal = 0;
  Alembic::Util::int64_t newVal = 0;

  do
  {
    oldVal = m_streams;
    newVal = oldVal | ( 1 << iStreamID );             // <=
  }
  while ( ! COMPARE_EXCHANGE( m_streams, oldVal, newVal ) );
}

The analyzer detected a potential error in an expression with a shift operation.

In the newVal = oldVal | (1 << iStreamID ) expression, the value 1 of type int is shifted, and the resulting value is cast to a 64-bit type. The potential error here is that if the value of the iStreamID variable happens to be larger than 32, undefined behavior will occur resulting in the program's incorrect operation.

To make it safer, it is better to handle the value 1 as a 64-bit unsigned type:

 newVal = oldVal | (  Alembic::Util::int64_t(1) << iStreamID );

There was one more warning of this type:

  • V629 Consider inspecting the '1 << (val - 1)' expression. Bit shifting of the 32-bit value with a subsequent expansion to the 64-bit type. StreamManager.cpp 148

Universal Scene Description

Picture 5

PVS-Studio diagnostic message: V668 There is no sense in testing the '_rawBuffer' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. uvTextureStorageData.cpp 118

bool GlfUVTextureStorageData::Read(....) 
{
  ....
  _rawBuffer = new unsigned char[_size];                   // <=
  if (_rawBuffer == nullptr) {                             // <=
      TF_RUNTIME_ERROR("Unable to allocate buffer.");
      return false;
  }
  ....
  return true; 
}

As specified by the modern language standard, new throws an exception rather than returning nullptr when memory allocation fails. The code above is kind of a programming archaism; checks like that don't make sense to modern compilers anymore and can be safely removed.

PVS-Studio diagnostic message: V501 There are identical sub-expressions 'HdChangeTracker::DirtyPrimVar' to the left and to the right of the '|' operator. basisCurves.cpp 563

HdBasisCurves::_GetInitialDirtyBits() const
{
  int mask = HdChangeTracker::Clean;

  mask |= HdChangeTracker::DirtyPrimVar     // <=
       |  HdChangeTracker::DirtyWidths
       |  HdChangeTracker::DirtyRefineLevel
       |  HdChangeTracker::DirtyPoints
       |  HdChangeTracker::DirtyNormals
       |  HdChangeTracker::DirtyPrimVar     // <=
       |  HdChangeTracker::DirtyTopology
       ....
      ;

  return (HdChangeTracker::DirtyBits)mask;
}

mask is declared using multiple fields, one of which is used twice. It surely wasn't intended that way, so either the programmer added one extra field by mistake, or, which is more likely, there should be some other field instead of the second instance of the DirtyPrimVar member.

Another similar warning:

  • V501 There are identical sub-expressions 'HdChangeTracker::DirtyPrimVar' to the left and to the right of the '|' operator. mesh.cpp 1199

OpenSubdiv

Picture 7

PVS-Studio diagnostic message: V595 The 'destination' pointer was utilized before it was verified against nullptr. Check lines: 481, 483. hbr_utils.h 481

template <class T> void
createTopology(....) 
{
  ....
  OpenSubdiv::HbrVertex<T> * destination = 
                        mesh->GetVertex( fv[(j+1)%nv] );
  OpenSubdiv::HbrHalfedge<T> * opposite  = 
                        destination->GetEdge(origin);  // <=

  if(origin==NULL || destination==NULL)                // <=
  {              
    printf(....);
    valid=false;
    break;
  }
  ....
}

V595 is probably the most common warning issued by the analyzer. PVS-Studio considers it a dangerous situation when a pointer is first dereferenced and then checked: if a pointer is checked, then the programmer assumes it may be null.

That's just what happens in the code above: the destination pointer is dereferenced to initialize the opposite pointer and then both are tested for null.

Two more warnings of this type:

  • V595 The 'destination' pointer was utilized before it was verified against nullptr. Check lines: 145, 148. hbr_tutorial_1.cpp 145
  • V595 The 'destination' pointer was utilized before it was verified against nullptr. Check lines: 215, 218. hbr_tutorial_2.cpp 215

PVS-Studio diagnostic message: V547 Expression 'buffer[0] == '\r' && buffer[0] == '\n ' ' is always false. Probably the '||' operator should be used here. hdr_reader.cpp 84

unsigned char *loadHdr(....)
{
  ....
  char buffer[MAXLINE];
  // read header
  while(true) 
  {
    if (! fgets(buffer, MAXLINE, fp)) goto error;
    if (buffer[0] == '\n') break;
    if (buffer[0] == '\r' && buffer[0] == '\n') break;   // <=
    ....
  }
  ....
}

There is a mistake in the marked condition that causes it to be false all the time. What the programmer actually intended was probably to exit the while loop on encountering end-of-line character sequences such as \n or \r\n. In that case, the erroneous condition should be rewritten in the following way:

 if (buffer[0] == '\r' && buffer[1] == '\n') break;   

PVS-Studio diagnostic message: V593 Consider reviewing the expression of the 'A = B != C' kind. The expression is calculated as following: 'A = (B != C)'. main.cpp 652

main(int argc, char ** argv) 
{
  ....
  #if defined(OSD_USES_GLEW)
  if (GLenum r = glewInit() != GLEW_OK) {                 // <=
      printf("Failed to initialize glew. error = %d\n", r);
      exit(1);
  }
  #endif
....
}

The analyzer detected a possible error in the GLenum r = glewInit() != GLEW_OK expression, which does not seem to work as expected. In code like that, programmers generally expect the following order of evaluation:

(GLenum r = glewInit()) != GLEW_OK

However, the '!=' operation's precedence is higher than that of the assignment operator, so the expression is actually evaluated in this order:

GLenum r = (glewInit() != GLEW_OK)

Therefore, if the glewInit() function hasn't returned correctly, the program will output an incorrect error code. To be more exact, it will always output 1.

The error can be fixed by using additional parentheses or taking the code responsible for object creation outside the condition to make it easier to read. See Chapter 16 of "The Ultimate Question of Programming, Refactoring, and Everything".

PVS-Studio detected a few other issues of this type:

  • V593 Consider reviewing the expression of the 'A = B != C' kind. The expression is calculated as following: 'A = (B != C)'. glEvalLimit.cpp 1419
  • V593 Consider reviewing the expression of the 'A = B != C' kind. The expression is calculated as following: 'A = (B != C)'. glStencilViewer.cpp 1128
  • V593 Consider reviewing the expression of the 'A = B != C' kind. The expression is calculated as following: 'A = (B != C)'. farViewer.cpp 1406

PVS-Studio diagnostic message: V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'm_blocks' is lost. Consider assigning realloc() to a temporary pointer. allocator.h 145

template <typename T>
T*
HbrAllocator<T>::Allocate() 
{
  if (!m_freecount) 
  {
    ....
    // Keep track of the newly allocated block
    if (m_nblocks + 1 >= m_blockCapacity) {
        m_blockCapacity = m_blockCapacity * 2;
        if (m_blockCapacity < 1) m_blockCapacity = 1;
        m_blocks = (T**) realloc(m_blocks,                // <=
                                 m_blockCapacity * sizeof(T*));
    }
    m_blocks[m_nblocks] = block;                          // <=
    ....
  }
  ....
}

This is another example of a dangerous use of the realloc function. For details, see the 'Partio' section above.

Dynamica

Picture 9

PVS-Studio diagnostic message: V512 A call of the 'memset' function will lead to overflow of the buffer 'header.padding'. pdbIO.cpp 249

struct pdb_header_t
{
  int       magic;
  unsigned short swap;
  float       version;
  float       time;
  unsigned int data_size;
  unsigned int num_data;
  char      padding[32];
  //pdb_channel_t   **data;
  int             data;
};

bool pdb_io_t::write(std::ostream &out)
{
  pdb_header_t            header;
  ....
  header.magic = PDB_MAGIC;
  header.swap = 0;
  header.version = 1.0;
  header.time = m_time;
  header.data_size = m_num_particles;
  header.num_data = m_attributes.size();
  memset(header.padding, 0, 32 * sizeof(char) + sizeof(int));
  ....
}

The analyzer detected a possible error that has to do with filling the memory buffer header.padding. The programmer uses memset to clear 36 bytes in the header.padding buffer whose size is only 32 bytes. It looks like a mistake, but the programmer averts it in a smart way by clearing the data variable as well. The fields padding and data of the pdb_header_t structure go in sequence, so they are allocated in memory in sequence as well. That's right! There's no error here, but the programmer's trick is potentially dangerous and might cause one later, for example when another developer fails to notice that trick and changes the pdb_header_t structure by adding their own fields between padding and data. For that reason, it's better to clear each variable separately.

Ptex

Picture 10

PVS-Studio diagnostic message: V612 An unconditional 'return' within a loop. PtexHashMap.h 292

Entry* lockEntriesAndGrowIfNeeded(size_t& newMemUsed)
{
  while (_size*2 >= _numEntries) {
      Entry* entries = lockEntries();
      if (_size*2 >= _numEntries) {
          entries = grow(entries, newMemUsed);
      }
      return entries;
  }
  return lockEntries();
} 

The function above contains a strange while loop that returns a pointer to entries after the very first iteration. Don't you think it's somewhat intricate? This code needs to be examined more closely.

Conclusion

Static analysis plays a significant role in development of high-quality software, as, when used regularly, it helps focus on really important tasks rather than spending time on fixing silly or elusive bugs.

If you haven't checked your project for errors and set out on an engrossing bug-hunt yet, I strongly recommend downloading PVS-Studio for Linux and doing it now.



Use PVS-Studio to search for bugs in C, C++, and C# code

We offer you to check your project code with PVS-Studio. Just one bug found in the project will show you the benefits of the static code analysis methodology better than a dozen of the articles.

goto PVS-Studio;

Maxim Stefanov
Articles: 2


Do you make errors in the code?

Check your code
with PVS-Studio

Static code analysis
for C, C++, and C#

goto PVS-Studio;
On our website we use a cookie to collect information of a technical nature.
If you do not agree, please leave the site. Learn More →
Do not show again