# Checking the Ogre3D framework with the PVS\-Studio static analyzer

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\!

![0927_ogre/image1.png](https://import.viva64.com/docx/blog/0927_ogre/image1.png)

## 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](https://www.ogre3d.org/) \(Object\-Oriented Graphics Rendering Engine\) is an open\-source graphics engine\. It is available on [GitHub](https://github.com/OGRECave/ogre)\. 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](https://pvs-studio.com/en/docs/manual/6532/)\. 562 is not so much — moreover, the analyzer issued most of the warnings with the [V730](https://pvs-studio.com/en/docs/warnings/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](https://pvs-studio.com/en/docs/warnings/v1064/) The '1' operand of integer division is less than the '100000' one\. The result will always be zero\. OgreAutoParamDataSource\.cpp 1094

```cpp
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:

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

Now everything works properly\.

[V506](https://pvs-studio.com/en/docs/warnings/v506/) Pointer to local variable 'varyingName' is stored outside the scope of this variable\. Such a pointer will become invalid\. OgreGLES2RenderToVertexBuffer\.cpp 268

```cpp
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:

```cpp
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](https://pvs-studio.com/en/docs/warnings/v614/) Uninitialized variable 'lodLevel\.reductionValue' used\. main\.cpp 806

The _LodLevel_ structure:

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

Here's the code that uses this structure:

```cpp
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](https://en.cppreference.com/w/cpp/named_req/TrivialType), and this may be important:

```cpp
LodLevel lodlevel {};
```

[V595](https://pvs-studio.com/en/docs/warnings/v595/) The 'params' pointer was utilized before it was verified against nullptr\. Check lines: 95, 101\. OgreGpuProgramManager\.cpp 95

```cpp
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:

```cpp
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](https://pvs-studio.com/en/docs/warnings/v547/) Expression 'x \=\= 0' is always true/false\. OgreTerrain\.cpp 3750

```cpp
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:

```cpp
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](https://pvs-studio.com/en/docs/warnings/v609/)\. Possible division or mod by zero\. OgreInstanceBatchHW\_VTF\.cpp 392

Get a good look at the following code:

```cpp
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](https://pvs-studio.com/en/docs/warnings/v557/) Array overrun is possible\. The 'j' index is pointing beyond array bound\. OgreAnimationTrack\.cpp 219

A typical array overrun:

```cpp
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\.

![0927_ogre/image2.png](https://import.viva64.com/docx/blog/0927_ogre/image2.png)

Let's fix this:

```cpp
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:

```cpp
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:

```cpp
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_](http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2021/p1072r10.html) function\.

[V1048](https://pvs-studio.com/en/docs/warnings/v1048/) The 'mVSOutPosition' variable was assigned the same value\. OgreShaderExTriplanarTexturing\.cpp 168

```cpp
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](https://pvs-studio.com/en/docs/warnings/v560/) Part of conditional expression is always true/false\. OgreTerrainLodManager\.cpp 62

```cpp
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:

```cpp
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](https://pvs-studio.com/en/docs/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\.

```cpp
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:

```cpp
// 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](https://pvs-studio.com/en/docs/warnings/v547/) Expression 'i \!\= end' is always true\. OgreScriptTranslator\.cpp 787

```cpp
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:

1. 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\.
1. 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:

```cpp
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](https://pvs-studio.com/en/docs/warnings/v1053/) Calling the 'destroyAllDeclarations' virtual function in the destructor may lead to unexpected result at runtime\. OgreDefaultHardwareBufferManager\.h 118

Declaring virtual class functions:

```cpp
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:

```cpp
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](https://pvs-studio.com/en/blog/posts/cpp/0891/) about such a case\.

[V530](https://pvs-studio.com/en/docs/warnings/v530/) The return value of function 'back' is required to be utilized\. OgreGLXConfigDialog\.cpp 410

```cpp
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](https://pvs-studio.com/en/docs/warnings/v570/) Variable is assigned to itself\. OgreETCCodec\.cpp 242

```cpp
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](https://pvs-studio.com/en/docs/warnings/v1065/) Expression can be simplified: check similar operands\. OgrePage\.cpp 117

```cpp
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:

```cpp
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](https://pvs-studio.com/en/docs/warnings/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](https://pvs-studio.com/en/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\.