Linux version of PVS-Studio gave itself a tour of Disney

Recently, the Linux version of the PVS-Studio analyzer has been published. With its help, a number of open source projects were tested. Among them are Chromium, GCC, LLVM (Clang) and others. And today, projects that were developed by Walt Disney Animation Studios for the virtual reality community will join this list. Let's proceed to the review of the found warnings of the analyzer.

Something about Disney

For many years, Walt Disney has been making television audiences all over the world delighted with amazing stories and characters, giving it unforgettable impressions. From year to year Disney releases more and more fascinating, entertaining and challenging films, and cartoons. Accordingly, there is an increasing need for the development of various programs that will contribute to the realization of the artists' creative ideas on visual effects.

Walt Disney Animation Studios programmers support animation and visual effects specialists by creating technologies that are available as open source C and C ++ programs for everyone in the virtual reality industry. These programs include:

• Partio (allows you to work with standard particle file formats through a single interface, implemented on the same principle as unified image libraries)
• Alembic (an open interchange format that is becoming the industry standard for exchanging animated computer graphics between digital content creation packages)
• Universal Scene Description (an efficient system capable of reading and transmitting scene data for the exchange between different graphic applications)
• OpenSubdiv (provides detailed rendering of surfaces (subdivision surface) based on reduced models)
• Dinamica (plugin for Autodesk Maya, developed on the basis of the real-time physics engine Bullet Physics Library)
• PTex (texture mapping system)

Open source programs from Disney can be downloaded from https://disney.imtqy.com/ .
')

Test results

The projects reviewed by Walt Disney are small and have only a few tens of thousands of lines of C and C ++ code. From here and such a small amount of errors on projects.

Project partio

PVS-Studio warning : 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++; } .... } 

The analyzer issued a message that the condition is always true. This will lead to the fact that the action that is defined in the else branch will never be executed. I think this situation arose because of the negligence of the programmer, and the conditions that will not lead to such an error 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 warning : V528 It is odd that the pointer to the '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; } 

If I understand correctly, the CharArrayLen function counts the number of characters in the charArray string. But is this really the case? In my opinion, in the condition of the while loop there is an error related to the fact that the pointer to the type char is compared with the value of '\ 0' . There is a high probability that the pointer dereference operation is forgotten. Therefore, the while loop condition should look like this:

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

By the way, the check located slightly higher is also quite strange:

 if(charArray != false) 

Verification, of course, works, but it will be much better to replace it with this:

 if(charArray != nullptr) 

In general, it seems that the function was developed by the trainee, or it was not added. It’s not clear why not just write code using the strlen () function:

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

PVS-Studio warning : 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 dangerous use of realloc in the code. The construction foo = realloc (foo, ...) is dangerous because if the memory allocation is impossible, the function will return nullptr , thus overwriting the previous pointer value, which can lead to a memory leak, or even a program crash. Perhaps this situation is extremely rare for many cases, but to be safe, I think, is still worth it. To prevent such a situation, it is recommended to save the pointer value in an additional variable before using realloc .

Similar analyzer warnings:

• V701 realloc () possible leak: when realloc () fails in allocating memory, the 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, the 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, the original pointer 'data' is lost. Consider assigning realloc () to a temporary pointer. ParticleSimpleInterleave.cpp 292

Alembic Project

PVS-Studio warning : V501 There are identical sub-expressions. 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; .... } 

And again the error associated with the absentmindedness of the programmer. It is easy to guess that instead of repeating the m_uKnot field, the condition should be m_vKnot .

PVS-Studio warning : 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 } .... } 

PVS-Studio detected the if..else statement in the code, in which the same thing is done in both outcomes, despite the different comments. It is likely that this section of code is languishing in a queue of immediate tasks for a team of programmers, but in the meantime, this section of code is wrong and needs some work.

PVS-Studio warning : V629 Consider inspecting the '1 << iStreamID' expression. Bit shifting of the 32-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 has detected a potential error in an expression that contains a shift operation.

In the expression newVal = oldVal | (1 << iStreamID) the unit represented as int is shifted, and then the result of the shift is converted to the 64-bit type. The potential error here is that if the value of the variable iStreamID may be greater than 32, then this part of the code will not work correctly due to the occurrence of undefined behavior.

The code will become safer if the number 1 is represented by a 64-bit unsigned data type:

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

The analyzer issued another such warning:

• V629 Consider inspecting the '1 << (val - 1)' expression. Bit shifting of the 32-bit type. StreamManager.cpp 148

Project Universal Scene Description

PVS-Studio warning : V668 it was no use. 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; } 

According to the modern standard of the language, new in case of unsuccessful allocation of memory throws an exception, but does not return nullptr . This code is a kind of archaic programming. For modern compilers, these checks are no longer meaningful and can be removed.

PVS-Studio Warning: V501 There are identical sub-expressions of 'HdChangeTracker :: DirtyPrimVar' 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; } 

To determine the mask used many fields, among which are duplicate. It should not be so, therefore, the programmer, or once again because of inattention, uses the same field, or, most likely, there should be another field instead of the repeating DirtyPrimVar field.

Similar warning:

• V501 There are identical sub-expressions "HdChangeTracker :: DirtyPrimVar" operator. mesh.cpp 1199

OpenSubdiv project

PVS-Studio warning : V595 The 'destination' pointer was used 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; } .... } 

Perhaps the V595 is the most common warning given by the analyzer. PVS-Studio considers the code dangerous if the pointer is dereferenced, and then checked below the code. If the pointer is checked, then assume that it can be zero.

This is what happens in the above code section. To initialize the opposite pointer, the destination pointer is dereferenced , and then these pointers are checked for NULL equality.

And a couple more warnings:

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

PVS-Studio warning : 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; // <= .... } .... } 

The programmer made a mistake in writing the condition, which leads to the fact that the condition is always false . Most likely, the programmer wanted to make it so that if end-of-line markers such as \ n or \ r \ n are encountered, it is necessary to exit the while loop . Therefore, the erroneous condition should be written as follows:

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

PVS-Studio warning : V593 Consider reviewing the expression A = B! = C 'kind. The expression is calculated as the 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 potential error in the expression GLenum r = glewInit ()! = GLEW_OK , which, most likely, does not work as the programmer intended. When creating such code, the programmer, as a rule, wants to perform actions in the following order:

 (GLenum r = glewInit()) != GLEW_OK 

But the priority of the '! =' Operator is higher than the priority of the assignment operator. Therefore, the expression is calculated as:

 GLenum r = (glewInit() != GLEW_OK) 

Therefore, if the glewInit () function does not work correctly, an incorrect error code will be printed on the screen. More precisely, the unit will always be printed.

To correct the error, you can use brackets or make the creation of an object outside the condition, which will give the code a more readable look. See also chapter 16 in the book The Main Question of Programming, Refactoring, and All That .

PVS-Studio discovered several more similar places:

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

PVS-Studio warning : 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; // <= .... } .... } 

And again the dangerous use of the realloc function. And why it is dangerous - described above in the section 'Project Partio'.

Dynamica Project

PVS-Studio warning : V512 A call of the 'memset' function will lead you to. 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 has detected a potential error related to filling the memory buffer header.padding . Through memset, the programmer clears 36 bytes in the header.padding buffer consisting of only 32 bytes . At first glance, such use is erroneous, but, in fact, the programmer turned out to be a trickster and, together with header.padding, reset the data variable . After all, the padding and data fields of the pdb_header_t structure are arranged sequentially, and therefore sequentially located in memory. Yes! There is no error in this situation, but due to such tricks, an error may potentially occur in this place. For example, if another programmer changes the pdb_header_t structure, adding padding and data between the fields, and will not notice the tricks of his colleague. Therefore, it is better to reset each variable separately.

Ptex Project

PVS-Studio warning : 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(); } 

In the above function, there is a suspicious while loop , in which on the first pass a pointer to entries is returned. Do not you think that there is something confusing? This section of the code requires more detailed consideration.

Conclusion

Static code analysis when writing high-quality software plays a very important role, as using static analysis on a regular basis, you reduce labor costs to eliminate stupid or hard-to-find errors and can spend more time on something useful.

If you haven’t checked your project for errors yet and didn’t go into an exciting search for bugs, then I advise you to download PVS-Studio for Linux and be sure to do it.


If you want to share this article with an English-speaking audience, then please use the link to the translation: Maxim Stefanov. PVS-Studio for Linux Went on Tour Around Disney .