Checking the Ogre3D framework with the PVS-Studio static analyzer

• Introduction
• Bugs found by PVS-Studio
• Not everything is cut and dried
• Suspicious fragments
• Conclusion

Developers like graphics engines because they are easy to work with. The PVS-Studio team likes graphics engines because we often find interesting code fragments. One of our readers asked us to analyze the Ogre3D graphics framework. We did it and found some warnings — keep reading and choose the best one!

Introduction

Wizards, ogres, witchcraft, and castles of villains. Sounds like a great setting for a fantasy movie. But this is not a "Rescue the Princess" story, although we'll encounter some "Ogres".

Ogre3D (Object-Oriented Graphics Rendering Engine) is an open-source graphics engine. It is available on GitHub. The project is written in C++. It is designed to create games and 3D visualization.

Bugs found by PVS-Studio

Analyzing Ogre3D, PVS-Studio issued 562 warnings of High and Medium levels. Only General Analysis warnings (GA) were included. You can find more about the filtering mechanism in our documentation. 562 is not so much — moreover, the analyzer issued most of the warnings with the V730 diagnostic. According to this diagnostic rule, not all members of a class are initialized inside the constructor. But it's difficult to determine whether project developers intended to do this or not. We don't know the subtleties of the project implementation.

Not everything is cut and dried

I found some analyzer warnings quite interesting. Let's start with the best ones.

V1064 The '1' operand of integer division is less than the '100000' one. The result will always be zero. OgreAutoParamDataSource.cpp 1094

typedef Vector<4, Real> Vector4;

const Vector4& 
  AutoParamDataSource::getShadowSceneDepthRange(size_t index) const
{
  static Vector4 dummy(0, 100000, 100000, 1/100000);
  // ....
}

Here the dummy vector should store floating point numbers. In this case, the constructor receives 4 arguments of the float type. However, there are integer values to the left and right of the division operator. That's why the result of 1/100000 will be not a fraction but zero.

Let's fix this:

const Vector4& AutoParamDataSource::getShadowSceneDepthRange(size_t index) const
{
  static Vector4 dummy(0, 100000, 100000, 1.0f/100000);
  // ....
}

Now everything works properly.

V506 Pointer to local variable 'varyingName' is stored outside the scope of this variable. Such a pointer will become invalid. OgreGLES2RenderToVertexBuffer.cpp 268

typedef std::string String;

void GLES2RenderToVertexBuffer::bindVerticesOutput(Pass* pass)
{
  // ....

  const GLchar *names[64];
  for (unsigned short e = 0; e < elemCount; e++)
  {
    const VertexElement* element = declaration->getElement(e);
    String varyingName = getSemanticVaryingName(element->getSemantic(),
                                                element->getIndex());
    names[e] = varyingName.c_str(); // <=
  }

  // ....
}

In this code, we have an array of 64 pointers to the const GLchar type, storing pointers to internal storage containers of the String type. The problem is that the String type containers are declared and initialized inside the loop. After going out of the scope, they are destroyed along with internal storages. This makes the pointers stored in names invalid.

We can fix this error by allocating memory in the heap for new storage. To do that, we copy the string from the String container and save the pointer to the new storage. But it's easier to replace an array of pointers with an array of String type. That's exactly what we're going to do:

void GLES2RenderToVertexBuffer::bindVerticesOutput(Pass* pass)
{
  // ....

  String names[64];
  for (unsigned short e = 0; e < elemCount; e++)
  {
    const VertexElement* element = declaration->getElement(e);
    names[e] = getSemanticVaryingName(element->getSemantic(),
                                      element->getIndex());
  }

  // ....
}

V614 Uninitialized variable 'lodLevel.reductionValue' used. main.cpp 806

The LodLevel structure:

struct _OgreLodExport LodLevel
{
  // ....
  VertexReductionMethod reductionMethod;
  Real reductionValue;
  // ....
};

Here's the code that uses this structure:

numLod = opts.numLods;
LodLevel lodLevel;            // <=
lodLevel.distance = 0.0;
for (unsigned short iLod = 0; iLod < numLod; ++iLod)
{

  lodLevel.reductionMethod = opts.usePercent
    ? LodLevel::VRM_PROPORTIONAL
    : LodLevel::VRM_CONSTANT;

  if (opts.usePercent)
  {
    lodLevel.reductionValue += opts.lodPercent * 0.01f;    // <=
  }
  else
  {
    lodLevel.reductionValue += (Ogre::Real)opts.lodFixed;  // <=
  }

  lodLevel.distance += opts.lodDist;
  lodConfig.levels.push_back(lodLevel);
}

In this code fragment, the LodLevel structure is declared. It does not have a user-defined default constructor and default member initializers for non-static class data members. Thus, the non-static data member is not initialized. Then the data member is read.

If we want all data members to be default-initialized, we can use one of the following options:

• define a default constructor;
• add default member initializers (since C++11);
• use the value initialization when declaring an instance of the structure (since C++11).

The third option is the most preferable because it does not make the type non-trivial, and this may be important:

LodLevel lodlevel {};

V595 The 'params' pointer was utilized before it was verified against nullptr. Check lines: 95, 101. OgreGpuProgramManager.cpp 95

Resource* GpuProgramManager::createImpl(...., 
                                        const NameValuePairList* params)
{
  auto langIt = params->find("language");
  auto typeIt = params->find("type");

  if (langIt == params->end())
    langIt = params->find("syntax");

  if (!params || langIt == params->end() || typeIt == params->end())
  {
    OGRE_EXCEPT(Exception::ERR_INVALIDPARAMS,
      "You must supply 'language' or 'syntax' and 'type' parameters");
  }
}

In this code fragment, the passed params pointer had been dereferenced before it was checked against null. A classic error. The code works until someone passes nullptr into the function. Let's place the check in the beginning and rewrite the code as follows:

Resource* GpuProgramManager::createImpl(....,
                                        const NameValuePairList* params)
{
  if (!params)
  {
    OGRE_EXCEPT(Exception::ERR_INVALIDPARAMS,
      "Params can't be nullptr");
  }

  auto langIt = params->find("language");
  auto typeIt = params->find("type");

  if (langIt == params->end())
    langIt = params->find("syntax");

  if (langIt == params->end() || typeIt == params->end())
  {
    OGRE_EXCEPT(Exception::ERR_INVALIDPARAMS,
      "You must supply 'language' or 'syntax' and 'type' parameters");
  }

  // ....
}

V547 Expression 'x == 0' is always true/false. OgreTerrain.cpp 3750

Terrain::NeighbourIndex Terrain::getNeighbourIndex(long x, long y)
{
  if (x < 0)
  {
    if (y < 0)
      return NEIGHBOUR_SOUTHWEST;
    else if (y > 0)
      return NEIGHBOUR_NORTHWEST;
    else
      return NEIGHBOUR_WEST;
  }
  else if (x > 0)
  {
    if (y < 0)
      return NEIGHBOUR_SOUTHEAST;
    else if (y > 0)
      return NEIGHBOUR_NORTHEAST;
    else
      return NEIGHBOUR_EAST;
  }

  if (y < 0)
  {
    if (x == 0)               // <= 
      return NEIGHBOUR_SOUTH;
  }
  else if (y > 0)
  {
    if (x == 0)               // <=
      return NEIGHBOUR_NORTH;
  }

  return NEIGHBOUR_NORTH;
}

Here the x variable is checked for 0 after false checks: x > 0 and x < 0. This check is pointless. Why checking the x variable for 0 if we can access this part of the code only if x == 0 — simple math! Let's remove unnecessary checks and simplify the code:

Terrain::NeighbourIndex Terrain::getNeighbourIndex(long x, long y)
{
  if (x < 0)
  {
    // ....
  }
  else if (x > 0)
  {
    // ....
  }
  else if (y < 0)
    return NEIGHBOUR_SOUTH;
  else if (y > 0)
    return NEIGHBOUR_NORTH;
  else
    return NEIGHBOUR_NORTH;
}

Now the fragment looks much better. There are no obviously unnecessary checks.

V609. Possible division or mod by zero. OgreInstanceBatchHW_VTF.cpp 392

Get a good look at the following code:

static const uint16 c_maxTexWidthHW = 4096;
const size_t numBones = 
  std::max<size_t>(1, baseSubMesh->blendIndexToBoneIndexMap.size());

// ....

const size_t maxUsableWidth = c_maxTexWidthHW –
                             (c_maxTexWidthHW % (numBones * mRowLength));

// ....

size_t texHeight = numWorldMatrices * mRowLength / maxUsableWidth; // <=

The maxUsableWidth variable can have a value from 0 to 4096. Thus, if maxUsableWidth suddenly turns out to be zero, we will get a division by zero at the place specified by the comment. Boom! But the code seems to be clean. It even compiles and works until 0 slips into the maxUsableWidth variable. This can happen if the result of numBones * mRowLength is greater than 4096.

The size of the blendIndexToBoneIndexMap vector is used to initialize the numBones variable. Perhaps developers control the number of container elements outside the class. But maybe they're just lucky that the vector isn't big enough. However, if the vector is suddenly larger than 4096, the division by zero will happen — the program will crash.

V557 Array overrun is possible. The 'j' index is pointing beyond array bound. OgreAnimationTrack.cpp 219

A typical array overrun:

void AnimationTrack::_buildKeyFrameIndexMap(
  const std::vector<Real>& keyFrameTimes)
{

  // ....

  size_t i = 0, j = 0;
  while (j <= keyFrameTimes.size())                    // <=
  {
    mKeyFrameIndexMap[j] = static_cast<ushort>(i);
    while (i < mKeyFrames.size()
      && mKeyFrames[i]->getTime() <= keyFrameTimes[j]) // <=
      ++i;
    ++j;
  }
}

The j index that gives us access to the elements of the keyFrameTimes container is incremented to a value equal to the container size.

Let's fix this:

while (j < keyFrameTimes.size())
{
  // ....
}

The static analyzer found several similar errors in other places. The OgreSerializer.cpp file triggered the analyzer. The array has 255 elements, but we are trying to access the 256th element:

String Serializer::readString(const DataStreamPtr& stream, size_t numChars)
{
  OgreAssert(numChars <= 255, "");
  char str[255];
  stream->read(str, numChars);
  str[numChars] = '\0';
  return str;
}

This code looks very strange. It seems useless — like developers forgot to clean it during refactoring, but what if someone uses the function anyway? Let's analyze the errors. First, we try to assign the '\0' value to a nonexistent 256 character — that's why an out-of-bounds access of array occurs in the function. Secondly, the number of characters returned by the read function can be less than the size of the str buffer. In this case, there will be uninitialized memory between the '\0' character and the string read by the read function. We can rewrite this function as follows:

String Serializer::readString(const DataStreamPtr& stream, 
                              size_t numChars)
{
  OgreAssert(numChars <= 255, "");
  String str(numChars, '\0');
  numChars = stream->read(&str[0], numChars);
  str.erase(numChars);
  return str;
}

Now we don't have the array out of bounds error. We fill all uninitialized memory with the '\0' characters and use the erase function at the end. Also, in C++23, we will be able to overwrite such a pattern with the help of the resize_and_overwrite function.

V1048 The 'mVSOutPosition' variable was assigned the same value. OgreShaderExTriplanarTexturing.cpp 168

void TriplanarTexturing::copyFrom(....)
{
  const TriplanarTexturing& rhsTP =
    static_cast<const TriplanarTexturing&>(rhs);

  mPSOutDiffuse = rhsTP.mPSOutDiffuse;
  mPSInDiffuse = rhsTP.mPSInDiffuse;

  mVSInPosition = rhsTP.mVSInPosition;   // <=
  mVSOutPosition = rhsTP.mVSOutPosition; // <=

  mVSOutNormal = rhsTP.mVSOutNormal;
  mVSInNormal = rhsTP.mVSInNormal;
  mPSInNormal = rhsTP.mPSInNormal;

  mVSInPosition = rhsTP.mVSInPosition;   // <=
  mVSOutPosition = rhsTP.mVSOutPosition; // <=
}

A classic copy-paste typo. The same value is assigned to the member variables twice.

V560 Part of conditional expression is always true/false. OgreTerrainLodManager.cpp 62

void TerrainLodManager::open(const String& filename)
{
  if (!filename.empty() && filename.length() > 0)
       mDataStream = 
         Root::getSingleton()
              .openFileStream(filename, 
                              mTerrain->_getDerivedResourceGroup());
}

Here, the developer checks that the std::string container is empty and its length is greater than 0. We can remove one of the condition parts:

void TerrainLodManager::open(const String& filename)
{
  if (!filename.empty())
       mDataStream = 
         Root::getSingleton()
              .openFileStream(filename, 
                              mTerrain->_getDerivedResourceGroup());
}

Suspicious fragments

I also want to describe some suspicious places that the PVS-Studio analyzer found. With these places, it's difficult to say whether it's an error or not. Obviously, the analyzer worked properly. However, we don't know whether the developers intended to write code this way or not. But I will still show you these warnings.

V703 It is odd that the 'mProgramID' field in derived class 'GLGpuNvparseProgram' overwrites field in base class 'GLGpuProgram'. Check lines: OgreGLGpuNvparseProgram.h:63, OgreGLGpuProgram.h:60.

class _OgreGLExport GLGpuProgram : public GpuProgram, public GLGpuProgramBase
{
  // ....
protected:
  GLuint mProgramID; // <=
};

class _OgreGLExport GLGpuNvparseProgram : public GLGpuProgram
{
  // ....

  GLuint getProgramID(void) const
  {
    return mProgramID;            // <=
  } 

  // ....

private:
  GLuint mProgramID; // <=
};

Here, the descendant class declares a variable with the same name as the protected variable in the parent class. This leads to name hiding and errors. When mProgramID returns from the getProgramID function, we get the value from the descendant class, not from the base class. We don't know whether the developers intended to do so or not. However, the developers still should check this place.

They can rename one of the data members or explicitly specify the data member:

// Now we access the base class data member
GLuint getProgramID(void) const
{ return GLGpuProgram::mProgramID; }

The first method, of course, is preferable and more correct.

V547 Expression 'i != end' is always true. OgreScriptTranslator.cpp 787

bool ScriptTranslator::getMatrix4(
  AbstractNodeList::const_iterator i,
  AbstractNodeList::const_iterator end,
  Matrix4 *m)
{
  int n = 0;
  while (i != end && n < 16)
  {
    if (i != end)               // <=
    {
      Real r = 0;
      if (getReal(*i, &r))
        (*m)[n / 4][n % 4] = r;
      else
        return false;
    }
    else
    {
      return false;
    }
    ++i;
    ++n;
  }
  return true;
}

Very strange code. I notice at least two problems here:

• The i != end condition is checked twice. If the condition in while is true, then the condition in if will always be true. The check is unnecessary.
• The else branch is unreachable. At the same time, it returns false.

It's difficult to offer a solution without knowing what the function must do. However, we could simplify the code without changing the logic:

bool ScriptTranslator::getMatrix4(
  AbstractNodeList::const_iterator i,
  AbstractNodeList::const_iterator end,
  Matrix4 *m)
{
  int n = 0;
  while (i != end && n < 16)
  {
    Real r = 0;

    if (!getReal(*i, &r))
      return false;

    (*m)[n / 4][n % 4] = r;
    ++i;
    ++n;
  }
  return true;
}

V1053 Calling the 'destroyAllDeclarations' virtual function in the destructor may lead to unexpected result at runtime. OgreDefaultHardwareBufferManager.h 118

Declaring virtual class functions:

class _OgreExport HardwareBufferManagerBase : public BufferAlloc
{
protected:
  // ....
  /// Internal method for destroys all vertex declarations.
  virtual void destroyAllDeclarations(void);

  /// Internal method for destroys all vertex buffer bindings.
  virtual void destroyAllBindings(void);
  // ....    
}

Declaring a destructor:

class _OgreExport DefaultHardwareBufferManager : public HardwareBufferManager
{

  // ....

  ~DefaultHardwareBufferManager()
  {
    // have to do this before mImpl is gone
    destroyAllDeclarations();
    destroyAllBindings();
  }

  // ....
}

Here we call two virtual functions in the destructor. So far, it doesn't affect anything. However, if we inherit from this class and redefine these functions, the destructor of the DefaultHardwareBufferManager class will still use virtual functions from the base class. This can lead to unexpected results. Using virtual functions in destructors is considered bad practice — it can produce a dangerous place in the code. We even wrote an article about such a case.

V530 The return value of function 'back' is required to be utilized. OgreGLXConfigDialog.cpp 410

class GLXConfigurator
{
public:
  // ....
  std::list<ConfigCallbackData> mConfigCallbackData
  // ....
}

void GLXConfigurator::SetRenderer(RenderSystem *r)

  // ....
  mConfigCallbackData.back();
  // ....
}

Here, for some reason, we call the back function of the std::list container to get a reference to the last element. However, we do not use or save this reference. Such a strange place. Perhaps the developers intended to do something else.

V570 Variable is assigned to itself. OgreETCCodec.cpp 242

bool ETCCodec::decodePKM(const DataStreamPtr& stream,
                         DecodeResult& result) const
{
  // ....
  void *destPtr = output->getPtr();
  stream->read(destPtr, imgData->size);
  destPtr = static_cast<void*>(static_cast<uchar*>(destPtr)); // <=
  // ....
}

The destPtr pointer is cast to another pointer type, then to its own type, and is assigned to itself. A very strange place. Perhaps this is an old code that the developers forgot to remove.

V1065 Expression can be simplified: check similar operands. OgrePage.cpp 117

bool Page::isHeld() const
{
  unsigned long nextFrame = Root::getSingleton().getNextFrameNumber();
  unsigned long dist;
  if (nextFrame < mFrameLastHeld)
  {
    // we must have wrapped around
    dist = mFrameLastHeld +
      (std::numeric_limits<unsigned long>::max() - mFrameLastHeld); // <=
  }
  else
    dist = nextFrame - mFrameLastHeld;

  // 5-frame tolerance
  return dist <= 5;
}

Again, a very suspicious place. First, we can simplify the expression — it's enough to assign a value from std::numeric_limits to the dist variable. Secondly, if the condition is true, the dist variable is assigned a value that is obviously greater than 5. It would be much clearer and better to write something as follows:

bool Page::isHeld() const
{
  unsigned long nextFrame = Root::getSingleton().getNextFrameNumber();

  if (nextFrame >= mFrameLastHeld)
  {
    // 5-frame tolerance
    return (nextFrame – mFrameLastHeld) <= 5;
  }

  return false;
}

The code looks much nicer and cleaner.

Conclusion

To sum up, we can say that the code in the Ogre3D project is not perfect, but excellent. An overwhelming number of errors were in the same files. Moreover, the analyzer found no errors in other files. Perhaps this is the result of having junior developers in the team. The team lead asked them to write certain files, but code reviews were rare and inefficient.

The analyzer issued most of the warnings with the V730 diagnostic rule. It's difficult to say anything for sure. We do not know the details of the project implementation, perhaps the developers intended to do so. But one thing we can say for sure — the PVS-Studio analyzer helps to remove most of the errors listed above. With PVS-Studio, the developers could have fixed these errors before the release.