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.
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:
The source files of all these Disney tools can be downloaded from https://disney.github.io/ .
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.
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:
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:
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:
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:
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_OKHowever, 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:
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.
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.
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.
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.