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

Author: Andrey Karpov

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.

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

1. Top 10 warnings issued by PVS-Studio
2. Simplifying C++ code

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

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

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

The fixed code looks like this:

_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

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 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.

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

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:

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

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 Non-void function should return a value. qd_sound.h 70

If the __QD_SYSLIB__ macro is defined, the length function looks like this:

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.

I'd suggest fixing the code like this:

#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:

#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

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

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

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 Dereferencing of the invalid iterator 'it1' might take place. qd_condition_group.cpp 58

An attempt is made to delete a nonexistent element:

conditions_.erase(conditions_.end());

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

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

conditions_.erase(it1);

Warning N7: dangerous handling of the HRESULT type

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 type:

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

So, the check is unsafe.

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

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.

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

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

• 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:

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.

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:

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

The PVS-Studio analyzer warns us about that: 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

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

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.
2. 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.
3. The reading of data will be slowed down because the processor has to perform additional operations.
4. 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.

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:

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 to make sure you don't miss out on something interesting.