# Let's check the qdEngine game engine, part three: 10 more bugs

In the first article about qdEngine, we've discussed 10 errors selected by the PVS\-Studio plugin\. However, we have 10 more bugs that are worth mentioning\. As they say, it's better to learn from other people's mistakes\. The bugs also demonstrate well the capability of PVS\-Studio to detect various defect patterns\.

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

Make sure to catch up on the previous articles about checking the qdEngine game engine:

1. [Top 10 warnings issued by PVS\-Studio](https://pvs-studio.com/en/blog/posts/cpp/1119/)
1. [Simplifying C\+\+ code](https://pvs-studio.com/en/blog/posts/cpp/1121/)

Without further delay, let's move on to looking at the collection of bugs :\)

## Warning N1: error in error handler

Error handlers are usually not tested\. Writing unit tests for them is difficult and uninteresting\. In manual testing mode, they're difficult to access \(it's difficult to create a scenario that will cause the particular error to occur in the application\)\.

As a result, error handlers often contain errors\. I'll write a separate article about it sometime\.

This is where static code analysis tools such as [PVS\-Studio](https://pvs-studio.com/en/pvs-studio/) come in\. Analyzers check code regardless of how often \(with what probability\) it's executed while the application is running\. That's why analyzers can find errors in rarely used code\. Let's look at such a case:

```cpp
class CAVIGenerator  
{
  ....
  _bstr_t m_sFile;
  ....
};

HRESULT CAVIGenerator::InitEngine()
{
  ....
  TCHAR szBuffer[1024];
  ....
  if (hr != AVIERR_OK)
  {
    _tprintf(szBuffer,
             _T("AVI Engine failed to initialize. Check filename %s."),m_sFile);
    m_sError=szBuffer;
  ....
};
```

The PVS\-Studio warning: [V510](https://pvs-studio.com/en/docs/warnings/v510/) The 'printf' function is not expected to receive class\-type variable as third actual argument\. AVIGenerator\.cpp 132

The point is that _m\_sFile_ is a class of the [_\_bstr\_t_](https://learn.microsoft.com/en-us/cpp/cpp/bstr-t-class?view=msvc-170) type\. An attempt to display the class contents as a simple string leads to undefined behavior\. In reality, garbage characters are likely to be displayed instead of the file name\. If there are too many garbage characters, the _szBuffer\[1024\]_ buffer overflows\. A buffer overflow, in its turn, can be seen as a potential vulnerability\. 

So, use PVS\-Studio to prevent such classic bugs in error handlers\.

We can fix the code using overloaded operators of the _\_bstr\_t_ class:

```cpp
// Extract the pointers to the encapsulated Unicode or multibyte BSTR object.
operator wchar_t*
operator char*
```

The fixed code looks like this:

```cpp
_tprintf(szBuffer,
         _T("AVI Engine failed to initialize. Check filename %s."),
         (LPCSTR)m_sFile);
```

Depending on the compilation mode \(whether it's a UNICODE application or not\), the _LPCSTR_ type is expanded to _const_ _wchar\_t\*_ or _const_ _char\*_\.

## Warning N2: unreachable code

```cpp
bool qdGameScene::follow_path_seek(qdGameObjectMoving* pObj,
                                   bool lock_target)
{
  if (qdGameObjectMoving::FOLLOW_UPDATE_PATH == pObj->follow_condition())
    selected_object_->set_grid_zone_attributes(sGridCell::CELL_SELECTED);

  return pObj->move(selected_object_->last_move_order(), lock_target);

  if (qdGameObjectMoving::FOLLOW_UPDATE_PATH == pObj->follow_condition())
    selected_object_->drop_grid_zone_attributes(sGridCell::CELL_SELECTED);
}
```

The PVS\-Studio warning: [V779](https://pvs-studio.com/en/docs/warnings/v779/) Unreachable code detected\. It is possible that an error is present\. qd\_game\_scene\.cpp 1245

The code after the _return_ operator isn't executed\.

The block of unreachable code is exactly the same as the one before the _return_ statement\. Most likely, programmers accidentally created the repeated code as a result of careless code editing or branch merging\. I think they could just delete the code below _return_\.

## Warning N3: new \[\] / delete

The qdEngine engine code has a lot of manual memory management\. As a result, the code contains many errors related to memory allocation and deallocation\. I'd recommend those who will further develop the engine pay particular attention to the revision of the memory handling code\.

```cpp
bool grFont::load_index(XStream& fh)
{
  int buf_sz = fh.size();
  char* buf = new char[buf_sz];
  ....
  delete buf;
  return true;
}
```

The PVS\-Studio warning: [V611](https://pvs-studio.com/en/docs/warnings/v611/) The memory was allocated using the 'operator new\[\]' but was released using the 'operator delete'\. The 'delete\[\] buf;' statement should be used instead\. gr\_font\.cpp 72

An array is allocated, so the operator _delete\[\]_ should be used to deallocate it\. The fixed code:

```cpp
bool grFont::load_index(XStream& fh)
{
  int buf_sz = fh.size();
  char* buf = new char[buf_sz];
  ....
  delete[] buf;
  return true;
}
```

No, though\. Such code is incorrect, too\. The right way is to use smart pointers:

```cpp
bool grFont::load_index(XStream& fh)
{
  int buf_sz = fh.size();
  auto buf = std::make_unique<char[]>(buf_sz);
  ....
  return true;
}
```

P\.S\.: More code fragments with the same error:

* V611 The memory was allocated using the 'operator new\[\]' but was released using the 'operator delete'\. The 'delete\[\] buf;' statement should be used instead\. gr\_font\.cpp 101
* V611 The memory was allocated using the 'operator new\[\]' but was released using the 'operator delete'\. The 'delete\[\] alpha\_buffer\_;' statement should be used instead\. Check lines: 25, 168\. gr\_font\.cpp 25
* V611 The memory was allocated using the 'operator new\[\]' but was released using the 'operator delete'\. The 'delete\[\] buffer;' statement should be used instead\. qda\_editor\.cpp 1012

## Warning N4: function doesn't return value

```cpp
class qdSound : public qdNamedObject, public qdResource
{
  ....
  //! Returns the sound duration in seconds.
  float length() const { 
#ifndef __QD_SYSLIB__
    return sound_.length(); 
#endif
  }
  ....
}
```

The PVS\-Studio warning: [V591](https://pvs-studio.com/en/docs/warnings/v591/) Non\-void function should return a value\. qd\_sound\.h 70

If the _\_\_QD\_SYSLIB\_\__ macro is defined, the _length_ function looks like this:

```cpp
float length() const { 
}
```

Calling such a function results in undefined behavior\.

**Note\.** Some programmers believe that undefined behavior in such a case results in the function returning a random value\. Nope\. When undefined behavior is invoked, anything can happen\. You can see an interesting example in this [article](https://pvs-studio.com/en/blog/posts/cpp/0917/)\.

I'd suggest fixing the code like this:

```cpp
#ifndef __QD_SYSLIB__
  float length() const { 
    return sound_.length(); 
  }
#endif
```

This causes some of the external code to stop compiling\. Well, that's great\. We need to rewrite this code too\. It makes no sense as it calls to the "dead" _length_ function\.

If such a radical approach isn't a good option for some reason, we can settle for a compromise:

```cpp
#ifndef __QD_SYSLIB__
  float length() const { 
    return sound_.length(); 
  }
#else
  [[noreturn]] float length() const { 
    throw std::logic_error { "Function not implemented" };
  }
#endif
```

## Warning N5: memory leak

```cpp
template<class FilterClass>
LineContribType* C2PassScale<FilterClass>::AllocContributions(UINT uLineLength, 
                                                              UINT uWindowSize)
{
  static LineContribType line_ct;

  LineContribType *res = new LineContribType; 

  line_ct.WindowSize = uWindowSize; 
  line_ct.LineLength = uLineLength; 

  if (contribution_buffer_.size() < uLineLength)
    contribution_buffer_.resize(uLineLength);

  line_ct.ContribRow = &*contribution_buffer_.begin();

  if (weights_buffer_.size() < uLineLength * uWindowSize)
    weights_buffer_.resize(uLineLength * uWindowSize);

  double* p = &*weights_buffer_.begin();

  for (UINT u = 0; u < uLineLength; u++) {
    line_ct.ContribRow[u].Weights = p;
    p += uWindowSize;
  }
  return &line_ct;
}
```

The PVS\-Studio warning: [V773](https://pvs-studio.com/en/docs/warnings/v773/) The function was exited without releasing the 'res' pointer\. A memory leak is possible\. 2PassScale\.h 107

The following line in the code is redundant:

```cpp
LineContribType *res = new LineContribType;
```

The created object isn't used or deleted anywhere\. The only thing that happens is a memory leak\.

## Warning N6: error in logic

```cpp
bool qdConditionGroup::remove_condition(int condition_id)
{
  ....
  conditions_container_t::iterator it1 = std::find(conditions_.begin(),
                                                   conditions_.end(),
                                                   condition_id);
  if (it1 != conditions_.end())
    return false;

  conditions_.erase(it1);
  return true;
}
```

The PVS\-Studio warning: [V783](https://pvs-studio.com/en/docs/warnings/v783/) Dereferencing of the invalid iterator 'it1' might take place\. qd\_condition\_group\.cpp 58

An attempt is made to delete a nonexistent element:

```cpp
conditions_.erase(conditions_.end());
```

Most likely, the condition contains a logic error and should look like this:

```cpp
if (it1 == conditions_.end())
  return false;

conditions_.erase(it1);
```

## Warning N7: dangerous handling of the HRESULT type

```cpp
class winVideo
{
  ....
  struct IGraphBuilder*  graph_builder_;
  ....
};

bool winVideo::open_file(const char *fname)
{
  ....
  if (graph_builder_ -> RenderFile(wPath,NULL))
  {
    close_file(); return false;
  }
  ....
}
```

At first glance, the code looks fine\. However, the _RenderFile_ function returns the [_HRESULT_](https://en.wikipedia.org/wiki/HRESULT) type:

```cpp
HRESULT RenderFile(
  [in] LPCWSTR lpcwstrFile,
  [in] LPCWSTR lpcwstrPlayList
);
```

So, the check is unsafe\.

The PVS\-Studio warning: [V545](https://pvs-studio.com/en/docs/warnings/v545/) Such conditional expression of 'if' statement is incorrect for the HRESULT type value 'graph\_builder\_\-\>RenderFile\(wPath, 0\)'\. The SUCCEEDED or FAILED macro should be used instead\. WinVideo\.cpp 139

_HRESULT_ is a 32\-bit value divided into three different fields: an error severity code, a facility code, and an error code\. Macros such as _SUCCEEDED_ and _FAILED_ are used to check values of the _HRESULT_ type\.

The _HRESULT_ type has many states: 0L \(_S\_OK_\), 0x80000002L \(Ran out of memory\), 0x80004005L \(unspecified failure\), etc\. Note that the _S\_OK_ state is coded as 0\.

So, any status other than _S\_OK_ is considered an error and the function terminates\. Overall, this code seems to work correctly now but it's unsafe\. A little refactoring, in case someone thinks that the _RenderFile_ function returns the _bool_ type, results in a bug\.

The correct check looks like this:

```cpp
if (FAILED(graph_builder_ -> RenderFile(wPath,NULL)))
{
  close_file(); return false;
}
```

## Warning N8: false instead of nullptr

This code fragment, like the one we've discussed above, works\. However, I can't say it's correct\.

```cpp
const char* qdInterfaceDispatcher::get_save_title() const
{
  if (!cur_screen_)
    return false;
  ....
  return false;
}
```

PVS\-Studio warnings for the "return false" lines:

* [V601](https://pvs-studio.com/en/docs/warnings/v601/) The 'false' value is implicitly cast to the pointer\. qd\_interface\_dispatcher\.cpp 772
* V601 The 'false' value is implicitly cast to the pointer\. qd\_interface\_dispatcher\.cpp 783

The _false_ value is implicitly converted to a null pointer, so the code works as intended\. However, for the sake of beauty and decency, let's use _nullptr_:

```cpp
const char* qdInterfaceDispatcher::get_save_title() const
{
  if (!cur_screen_)
    return nullptr;
  ....
  return nullptr;
}
```

## Warning N9: something's wrong, I can feel it

I'm sorry for the long code fragment below\. I haven't worked out how to shorten it without making it even more confusing\.

```cpp
Vect2s
qdGameObjectMoving::get_nearest_walkable_point(const Vect2s& target) const
{
  ....
  // Go from end. If we reach passable point different from starting one
  bool fir_step = true;
  if (abs(x2 - x1) > abs(y2 - y1)) {
    int dx = int(float(x2 - x1)/dr.x);
    do {
      if (false == is_walkable(Vect2s(r.xi(),r.yi())))
      {
        // If there's only the first step, then it's a failure
        if (fir_step) return Vect2s(-1, -1);
        r -= dr;
        return Vect2s(r.xi(),r.yi());
      }

      fir_step = false;
      r += dr;
    } while (--dx >= 0);
  }
  else {
    int dy = int(float(y2 - y1)/dr.y);
    do {
      if (false == is_walkable(Vect2s(r.xi(),r.yi())))
      {
        if (fir_step) return Vect2s(-1, -1);
        r -= dr;
        return Vect2s(r.xi(),r.yi());
      }
      
      fir_step = false;
      r += dr;
    } while (--dy >= 0);
  }

  // If the step was never taken
  if (fir_step) return trg;
  ....
}
```

Take a look at the loops\. If they don't end with exiting the function, the _fir\_step_ variable is always _false_ at the end of the loops\.

So, the following code doesn't make any sense:

```cpp
// If the step was never taken
if (fir_step) return trg;
```

The PVS\-Studio analyzer warns us about that: [V547](https://pvs-studio.com/en/docs/warnings/v547/) Expression 'fir\_step' is always false\. qd\_game\_object\_moving\.cpp 1724

The check line is redundant, or there's a logic error in the loops\. Unfortunately, I can't say for sure as I'm not familiar with the project code\.

## Warning N10: strictly aligned

```cpp
bool zipContainer::find_index()
{
  const int buf_sz = 1024;
  char buf[buf_sz];

  stream_.seek(0,XS_END);

  while (stream_.tell() >= buf_sz) {
    stream_.seek(-buf_sz,XS_CUR);
    stream_.read(buf,buf_sz);

    const unsigned long idx_signature = 0x06054b50L;

    for (int i = 0; i < buf_sz - sizeof(long) * 3; i++) {
      if (*((unsigned long*)(buf + i)) == idx_signature) {
        XBuffer xbuf(buf + i + sizeof(long) + sizeof(unsigned short) * 4,
                     sizeof(long) * 2);
        xbuf > index_size_ > index_offset_;
        return true;
      }
    }
  }

  return false;
}
```

The PVS\-Studio warning: [V1032](https://pvs-studio.com/en/docs/warnings/v1032/) The pointer 'buf' is cast to a more strictly aligned pointer type\. zip\_container\.cpp 237

The _0x06054b50L_ signature is searched for in the byte buffer\. It's done in an unportable and dangerous way\.

To begin with, we can't be sure what signature is being searched for\. The thing is that the _long_ type has [different sizes](https://pvs-studio.com/en/blog/terms/0012/) on different platforms\. In most cases, _long_ can be either 32\-bit or 64\-bit\. Depending on the amount of memory, the following things can be searched for:

* the 4\-byte signature — 0x06054b50;
* or the 8\-byte signature — 0x0000000006054b50\.

The signature is hardly expected to have different sizes\. It's most likely always be the 4\-byte one\. So, the programmer would be more correct to use the _uint32\_t_ type of the fixed size\.

However, the analyzer warns us about a different thing\. The point is that a pointer of the _char\* _type is used to search for the signature\. To perform the check, the pointer address \(_char \*_\) is interpreted as a block of 4/8 bytes \(_unsigned long \*_\) at each iteration\. Here's a picture for demonstration:

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

The issue is that alignment is not considered\. Values of the _unsigned long_ type should be located at addresses that are multiples of 4 or 8 bytes \(or some other value, depending on the architecture\)\.

Accessing an unaligned address results in undefined behavior\. It's impossible to predict how it can show itself\. It depends on the architecture of the microprocessor and compiler\. Here are a few possible scenarios:

1. The program crashes\.
1. An incorrect value is read\. For example, the two low\-order pointer bits may be ignored, and reading is still done within boundaries that are multiples of 4 bytes\.
1. The reading of data will be slowed down because the processor has to perform additional operations\.
1. Everything works fine, at least for now\.

We can fix the issue in a number of ways\. The easiest way is to use the _memcmp_ function, which works at the byte\-array level\.

```cpp
if (memcmp(buf + i, &idx_signature, sizeof(idx_signature)) == 0)
```

Is it possible to write the code in modern C\+\+ without using the _memcmp_ function from the C world, though?

Yes, but honestly, the code gets a little longer\. So, I bring such an option just for the fun of it:

```cpp
const uint32_t idx_sig = 0x06054b50L;

const std::ranges::subrange
  haystack { buf, buf + buf_sz - sizeof(idx_sig) * 3 };
const auto needle = std::bit_cast<std::array<char, sizeof(idx_sig)>>(idx_sig);

if (auto res = std::ranges::search(haystack, needle); !std::empty(res))
{
  XBuffer xbuf(it + sizeof(uint32_t) + sizeof(uint16_t) * 4,
               sizeof(uint32_t) * 2);
  xbuf > index_size_ > index_offset_;
  return true;
}
```

## Is there anything else left?

We've got another error that's worth looking into\. It's related to object\-oriented programming and requires detailed analysis, so I'll put it in a separate, last article\.

Thank you for reading\. Subscribe to the monthly [article digest](https://pvs-studio.com/en/subscribe/) to make sure you don't miss out on something interesting\.