Andrey Karpov

Apr 27 2024

Tags:

#Cpp #GameDev

Let's check the qdEngine game engine, part one: top 10 warnings issued by PVS-Studio

Apr 27 2024

Author: Andrey Karpov

Best button
Warnings
Conclusion
Additional links

The bugs found in the qdEngine game engine are quite diverse, so I don't want to put them all in one publication. Our readers may miss out on interesting topics on writing high-quality code. That's why the project analysis will be a series of articles, and the first one is dedicated to the most interesting warnings issued by the PVS-Studio plugin.

Best button

K-D Lab has released the source code of qdEngine, a game engine for creating quests. The engine is mostly written in C++ and looks like a good candidate for checking with the PVS-Studio code analyzer.

The analyzer has found various types of errors in the project. So, we'd like to look at them from different angles. That's why I'll publish several articles on different topics. The first one is dedicated to the Best button in the PVS-Studio plugins.

The button is a great help for a user to get started with the PVS-Studio tool for the first time. The analyzer selects 10 warnings that are most likely to indicate real errors. They should also be diverse and interesting.

All static code analyzers issue a lot of warnings on the first run, many of these warnings are likely to be irrelevant or false. It's not a big deal because analyzers, including PVS-Studio, have flexible settings.

However, programmers new to the static analysis methodology may be discouraged by a bunch of warnings that seem irrelevant to them. They'd like to see some interesting bugs right away. The Best button makes that possible.

When you click it, the analyzer selects 10 warnings using the empirical algorithm. When making a choice, the analyzer isn't only guided by the probability that it's a real error, it also tries to select warnings of different types.

About error certainty. open icon

Let me explain why picking 10 warnings that are definitely real errors isn't the best option.

Let's look at the V668 diagnostic rule as an example. It indicates a pointless check of the pointer returned by the operator new. Here's the real code fragment from the Minetest project:

clouds = new Clouds(smgr, -1, time(0));
if (!clouds) {
 *error_message = "Memory allocation error (clouds)";
  errorstream << *error_message << std::endl;
  return false;
}

If the analyzer has detected such code, we're dealing with a real error in code. Of course, there's still a nothrow version of the operator new, but the analyzer considers it, too.

There are many such errors. In the whole time we've been writing articles, we've already found 1630 similar bugs in open-source projects! It's very likely that the analyzer would find similar fragments in a project that you'd use to get started with it.

However, issuing too many such warnings is undesirable:

It's not that interesting. You wouldn't like to see seven identical warnings about new, for example.
Certainty doesn't mean criticality. That's exactly the case. Here's an unreachable code fragment. It's not good, but I'm almost sure that the application works as intended. More complex diagnostic rules usually detect more interesting and critical errors. The more complex the error we're looking for, the harder it's to guarantee that we found a real bug. Even a human has to puzzle over such code.

For the qdEngine project, the button has worked well:

eight warnings indicate bugs or bad code;
one warning is technically correct, but there is no real error;
one warning is a false positive.

I think all these warnings show the analyzer capabilities really well. We're going to look at them. I hope this will encourage people who have only read about PVS-Studio to give it a try. Download the analyzer, install it, check the project, and click Best. If something goes wrong, just contact our support team. We're always ready to help you.

Warnings

Let's have a look at the promised warnings and practice some refactoring a bit.

Warning N1: duplicate

bool qdGameObject::init()
{
  ....
  drop_flag(QD_OBJ_STATE_CHANGE_FLAG | QD_OBJ_IS_IN_TRIGGER_FLAG |
            QD_OBJ_STATE_CHANGE_FLAG | QD_OBJ_IS_IN_INVENTORY_FLAG);
  ....
}

The PVS-Studio warning: V501 There are identical sub-expressions 'QD_OBJ_STATE_CHANGE_FLAG' to the left and to the right of the '|' operator. qd_game_object.cpp 176

The QD_OBJ_STATE_CHANGE_FLAG constant, declared in qd_game_object.h next to many others, is used twice:

....
const int QD_OBJ_HAS_BOUND_FLAG           = 0x80;
const int QD_OBJ_DISABLE_MOVEMENT_FLAG    = 0x100;
const int QD_OBJ_DISABLE_MOUSE_FLAG       = 0x200;
const int QD_OBJ_IS_IN_TRIGGER_FLAG       = 0x400;
const int QD_OBJ_STATE_CHANGE_FLAG        = 0x800;
const int QD_OBJ_IS_IN_INVENTORY_FLAG     = 0x1000;
const int QD_OBJ_KEYBOARD_CONTROL_FLAG    = 0x2000;
....

Developers probably forgot to use some other constant to create a mask. Perhaps it's just a duplicate, and we can remove the repeating constant from the expression. It's hard to say for sure, as I'm not familiar with the project code.

Warning N2: typo (lost # character)

A very beautiful error, in my opinion. I admit I may have weird tastes and professional deformations, but I hope you can appreciate this bug, too.

const qdGameObjectStateWalk* qdGameObjectMoving::current_walk_state() const
{
  const qdGameObjectState* st = get_cur_state();
  if(!st || st -> state_type() != qdGameObjectState::STATE_WALK){
#ifndef _QUEST_EDITOR
    st = last_walk_state_;
    if(!st || st -> state_type() != qdGameObjectState::STATE_WALK)
      st = get_default_state();
else
    st = get_default_state();
    if(!st) st = get_state(0);
#endif
  }
  ....
}

The PVS-Studio warning: V523 The 'then' statement is equivalent to the 'else' statement. qd_game_object_moving.cpp 2781

The analyzer points out that the same action is performed regardless of the condition:

  if(!st || st -> state_type() != qdGameObjectState::STATE_WALK)
    st = get_default_state();
else
  st = get_default_state();

Note that the code is formatted in a strange way. The else keyword is at the beginning of the string.

If we look at the code next to it and think about it a bit, we can see that the developers really wanted to write the #else preprocessor directive but not the else statement. However, the programmer made a typo and forgot to include the hash symbol (#).

Here's the fixed code:

const qdGameObjectState* st = get_cur_state();
if(!st || st -> state_type() != qdGameObjectState::STATE_WALK){
#ifndef _QUEST_EDITOR
  st = last_walk_state_;
  if(!st || st -> state_type() != qdGameObjectState::STATE_WALK)
    st = get_default_state();
#else
  st = get_default_state();
  if(!st) st = get_state(0);
#endif

Warning N3: unused string

bool qdGameScene::adjust_files_paths(....)
{
  ....
  for(qdGameObjectList::const_iterator it = object_list().begin();
      it != object_list().end(); ++it)
  {
    if((*it) -> named_object_type() == QD_NAMED_OBJECT_STATIC_OBJ)
    {
      qdGameObjectStatic* obj = static_cast<qdGameObjectStatic*>(*it);
      if(obj -> get_sprite() -> file())
        QD_ADJUST_TO_REL_FILE_MEMBER(pack_corr_dir, 
                                     obj -> get_sprite() -> file, 
                                     obj -> get_sprite() -> set_file, 
                                     can_overwrite, 
                                     all_ok);  
      std::string str = obj -> get_sprite() -> file();
      str = str;
    }
  }
  ....
}

The PVS-Studio warning: V570 The 'str' variable is assigned to itself. qd_game_scene.cpp 1799

In the loop, the file list is operated in some way. As I understand it, the developers added this code fragment for debugging purposes, and the analyzer doesn't like it:

std::string str = obj -> get_sprite() -> file();
str = str;

In the first line, the file name is taken. The second line, in my opinion, serves two purposes:

It's convenient to set a breakpoint on it in the debugger and immediately see the file name in the str variable.
Assigning an object to itself may also remove some compiler/analyzer warnings that the str variable is unused.

There's no error here. However, this is a code smell, and it's worth eliminating. In other words, the analyzer warning is a good reason to do some refactoring. I'd start by changing the interface of the file function in the qdSprite class:

const char* file() const { return file_.c_str(); }

It's strange to return the contents of std::string as just a pointer and then check its parts for nullptr. In reality, the function never returns nullptr. If we need a pointer somewhere in the code, we can always call c_str there. The changed function looks like this:

const std::string& file() const { return file_; }

Of course, a change to the function interface affects a large amount of code in the project. For symmetry, it also makes sense to change the set_file function. I have a feeling that all this would be good for the code.

We're moving on. Let's rewrite the code fragment we've discussed earlier.

if((*it) -> named_object_type() == QD_NAMED_OBJECT_STATIC_OBJ)
{
  qdGameObjectStatic* obj = static_cast<qdGameObjectStatic*>(*it);
  const std::string &file = obj -> get_sprite() -> file();
  QD_ADJUST_TO_REL_FILE_MEMBER(pack_corr_dir, 
                               file, 
                               obj -> get_sprite() -> set_file, 
                               can_overwrite, 
                               all_ok);  
}

We've made the following enhancements:

The code is now shorter and simpler.
The pointless pointer check is gone.
There's no creation of a temporary std::string object in the loop just to make debugging easier. These are relatively resource-intensive operations due to memory allocation and deallocation. Although, there's no guarantee that the compiler will remove all the unnecessary stuff during optimization.
The PVS-Studio analyzer no longer issues the V570 warning.
For debugging purposes, we can also set a breakpoint (on the line with the QD_ADJUST_TO_REL_FILE_MEMBER macro) and see the file name.

Warning N4: potential use of null pointer

bool DDraw_grDispatcher::Finit()
{
  ....
  ddobj_ -> SetCooperativeLevel((HWND)Get_hWnd(),DDSCL_NORMAL);
  if(fullscreen_ && ddobj_) ddobj_ -> RestoreDisplayMode();
  ....
}

The PVS-Studio warning: V595 The 'ddobj_' pointer was utilized before it was verified against nullptr. Check lines: 211, 212. ddraw_gr_dispatcher.cpp 211

The ddobj pointer is dereferenced for the function call. There's also a check for the pointer below. Based on this check, the analyzer deduced that the ddobj variable may contain nullptr and warns us about it.

We can rewrite the code as follows:

if (ddobj_)
{
  ddobj_ -> SetCooperativeLevel((HWND)Get_hWnd(),DDSCL_NORMAL);
  if(fullscreen_) ddobj_ -> RestoreDisplayMode();
}

Warning N5: destructor is not virtual

template <class T>
class PtrHandle 
{
  ....
  ~PtrHandle() { delete ptr; }
  ....
private:
  T *ptr;
};

The PVS-Studio warning: V599 Instantiation of PtrHandle < ResourceUser >: The virtual destructor is not present, although the 'ResourceUser' class contains virtual functions. Handle.h 14

There's no error in the code of the PtrHandle class. However, the issue is that it stores an object of the ResourceUser type. Let's look at the class declaration:

class ResourceUser
{        
  int ID;
  static int IDs;
public:
  ResourceUser(time_type period) { dtime = period; time = 0; ID = ++IDs; }
  virtual int quant() { return 1; } 

protected:
  time_type time;
  time_type dtime;  

  virtual void init_time(time_type time_) { time = time_ + time_step(); } 
  virtual time_type time_step() { return dtime; } 

  friend class ResourceDispatcher;
};

The class contains virtual functions. So, other classes are expected to be inherited from this class. In such a case, the destructor should be declared as virtual. Otherwise, the PtrHandle class destructor won't completely destroy objects that are inherited from ResourceUser. This leads to undefined behavior and resource leaks.

Warning N6: invalid delete operator

char* grDispatcher::temp_buffer(int size)
{
  if(size <= 0) size = 1;

  if(size > temp_buffer_size_){
    delete temp_buffer_;
    temp_buffer_ = new char[size];
    temp_buffer_size_ = size;
  }

  return temp_buffer_;
}

The PVS-Studio warning: V611 The memory was allocated using the 'operator new[]' but was released using the 'operator delete'. The 'delete[] temp_buffer_;' statement should be used instead. Check lines: 1241, 1242. gr_dispatcher.cpp 1241

The temp_buffer_ variable stores pointers to arrays created using the operator new[]. So, the operator delete[] should destroy them.

In the code we're looking at, delete, which is designed for single objects, is used to destroy an array. The code results in undefined behavior.

Note. Someone may argue that everything will work because the array consists of char-type items and the operators new/delete only call the malloc/free functions. That's not true. The implementation of operators can be quite diverse. For example, individual objects and arrays can be created in different pre-allocated (reserved) memory pools for optimization purposes.

Warning N7: wrong variable type for synchronization

static int b_thread_must_stop=0;

void MpegDeinitLibrary()
{
  ....
  if (hThread!=INVALID_HANDLE_VALUE)
  {
    b_thread_must_stop=1;
    while(b_thread_must_stop==1)
      Sleep(10);
  }
  ....
}

The PVS-Studio warning: V712 Be advised that compiler may delete this cycle or make it infinity. Use volatile variable(s) or synchronization primitives to avoid this. PlayOgg.cpp 293

A global variable of unsafe type is used to synchronize threads. This is a bad idea, as the compiler, during optimization, may decide that the variable doesn't change and create an infinite loop. Also, the change in the variable may not be atomic.

In such a case, replace the int type with atomic_int:

#include <atomic>
static atomic_int b_thread_must_stop { 0 };

Warning N8: memory leak

bool qdAnimationMaker::insert_frame(....)
{
  // IMPORTANT(pabdulin): auto_ptr usage was removed
  qdAnimationFrame* fp = new qdAnimationFrame;
  fp -> set_file(fname);
  fp -> set_length(default_frame_length_);

  if (!fp -> load_resources())
    return false;
  ....
  delete fp;
  return true;
}

The PVS-Studio warning: V773 The function was exited without releasing the 'fp' pointer. A memory leak is possible. qd_animation_maker.cpp 40

If a resource loading error occurs, the function returns early. The object whose pointer is stored in the fp variable isn't deleted. That's a memory leak.

Here's the fixed code:

if (!fp -> load_resources())
{
  delete fp;
  return false;
}

However, even after fixing the error, we can't call the code good. As we can see here, manual memory management causes errors. It's much better to use smart pointers.

Here's an interesting thing. Take a look at the comment:

// IMPORTANT(pabdulin): auto_ptr usage was removed

Maybe the code was correct once, as long as it used a smart pointer of the std::auto_ptr type. Since then, the class has become obsolete. Instead of replacing it with std::unique_ptr, the developer decided to work with pointers "in manual mode" but failed.

Warning N9: false positive

class PtrHandle 
{
  ....
  PtrHandle& operator=(PtrHandle& p) 
  { 
    if (get() != p.get()) 
    { 
      delete ptr; 
      ptr = p.release(); 
    } 
    return *this; 
  }
  ....

  T* get() const { return ptr; }
  ....
private:
  T *ptr;
};

The PVS-Studio warning: V794 The assignment operator should be protected from the case of 'this == &p'. Handle.h 19

This is the only one of the 10 warnings selected by the analyzer that has turned out to be false. The V794 diagnostic rule warns that the object should be protected from being copied into itself. The analyzer looks for the following construct in the function body:

if (this != &p)

However, developers have written a more complex check here using the get function call:

if (get() != p.get())

Perhaps the code author wanted to create protection not only against copying a smart pointer to itself, but also against copying two different smart pointers referring to the same object. That doesn't make any sense, though. If two smart pointers (without counting references) refer to the same object, that's bad. A double destruction of the object is possible.

So, such a check is really no better. It only further confuses the analyzer and the programmers reading the code.

Well, the analyzer didn't understand what was going on. To fix the false positive, we can do one of the following things.

Option 1. Add a comment to suppress the warning.

PtrHandle& operator=(PtrHandle& p) 
{ //-V794
  if (get() != p.get())

The comment explicitly informs the PVS-Studio analyzer that the code is safe.

Option 2. Rewrite the check.

if (this != &p)

I prefer this option over the other. The option with function calls looks heavy in my opinion.

If we want to have more control, we can add a check like this one:

PtrHandle& operator=(PtrHandle& p) 
{ 
  if (this != &p)
  { 
    if (get() == p.get())
    {
       // It's bad. One object in two smart pointers.
       // It's debugging time.
       assert(false);
       throw std::logic_error("Multiple ownership of an object.");
    }
    delete ptr; 
    ptr = p.release(); 
  } 
  return *this; 
}

The check helps detect cases when two smart pointers suddenly control the same object.

Warning N10: re-assignment

bool qdGameObjectMoving::update_screen_pos()
{
  ....
  if(get_cur_state() -> state_type() == qdGameObjectState::STATE_WALK){
    qdGameObjectStateWalk::OffsetType offs_type=
      qdGameObjectStateWalk::OFFSET_WALK;               // <=

    switch(movement_mode_){
    case MOVEMENT_MODE_STOP:
      offs_type = qdGameObjectStateWalk::OFFSET_STATIC;
      break;
    case MOVEMENT_MODE_TURN:
      offs_type = qdGameObjectStateWalk::OFFSET_STATIC;
      break;
    case MOVEMENT_MODE_START:
      offs_type = qdGameObjectStateWalk::OFFSET_START;
      break;
    case MOVEMENT_MODE_MOVE:
      offs_type = qdGameObjectStateWalk::OFFSET_WALK;   // <=
      break;
    case MOVEMENT_MODE_END:
      offs_type = qdGameObjectStateWalk::OFFSET_END;
      break;
    }

    offs += static_cast<qdGameObjectStateWalk*>(
      get_cur_state()) -> center_offset(direction_angle_, offs_type);
  }
  ....
}

The PVS-Studio warning: V1048 The 'offs_type' variable was assigned the same value. qd_game_object_moving.cpp 1094

The analyzer issues the warning because in the MOVEMENT_MODE_MOVE branch, the offs_type variable is assigned a value that is already in it.

Technically, the analyzer is correct. Re-assignment makes no sense. Also, sometimes it's not just extra code, but an error.

In this case, there's no bug. Let's take a look at how we can get rid of the warning.

Option 1. Add the //-V1048 comment to suppress the warning.

Option 2. Rewrite the code a bit by adding the default branch:

if(get_cur_state() -> state_type() == qdGameObjectState::STATE_WALK){
  qdGameObjectStateWalk::OffsetType offs_type;
  switch(movement_mode_){
  case MOVEMENT_MODE_STOP:
    offs_type = qdGameObjectStateWalk::OFFSET_STATIC;
    break;
  case MOVEMENT_MODE_TURN:
    offs_type = qdGameObjectStateWalk::OFFSET_STATIC;
    break;
  case MOVEMENT_MODE_START:
    offs_type = qdGameObjectStateWalk::OFFSET_START;
    break;
  case MOVEMENT_MODE_MOVE:
    offs_type = qdGameObjectStateWalk::OFFSET_WALK;
    break;
  case MOVEMENT_MODE_END:
    offs_type = qdGameObjectStateWalk::OFFSET_END;
    break;
  default:
    offs_type = qdGameObjectStateWalk::OFFSET_WALK;
  }

  offs += static_cast<qdGameObjectStateWalk*>(
    get_cur_state()) -> center_offset(direction_angle_, offs_type);
}

The warning is gone, but I don't like the fact that the offs_type variable is now not initialized where it's declared. Let's take another step in refactoring and move some of the code into a separate function.

qdGameObjectStateWalk::OffsetType qdGameObjectMoving::GetOffsetType()
{
  switch(movement_mode_){
  case MOVEMENT_MODE_STOP:    return qdGameObjectStateWalk::OFFSET_STATIC;
  case MOVEMENT_MODE_TURN:    return qdGameObjectStateWalk::OFFSET_STATIC;
  case MOVEMENT_MODE_START:   return qdGameObjectStateWalk::OFFSET_START;
  case MOVEMENT_MODE_MOVE:    return qdGameObjectStateWalk::OFFSET_WALK;
  case MOVEMENT_MODE_END:     return qdGameObjectStateWalk::OFFSET_END;
  }
  return qdGameObjectStateWalk::OFFSET_WALK;
}
....
if(get_cur_state() -> state_type() == qdGameObjectState::STATE_WALK){
  qdGameObjectStateWalk::OffsetType offs_type = GetOffsetType();
  offs += static_cast<qdGameObjectStateWalk*>(
    get_cur_state()) -> center_offset(direction_angle_, offs_type);}
....

The code is now shorter and easier to read. Cool. Also, we don't need the offs_type variable at all now, and we can shorten the code a bit.

if(get_cur_state() -> state_type() == qdGameObjectState::STATE_WALK){
  offs += static_cast<qdGameObjectStateWalk*>(
    get_cur_state()) -> center_offset(direction_angle_, GetOffsetType());
}

This is a good example: the analyzer warning prompted us to refactor the code to make it simpler and better.

Conclusion

The article covered the following topics:

How can the Best button help when you start using the PVS-Studio analyzer for the first time? I suggest you get a trial version and try to analyze your projects.
What errors does PVS-Studio help you find and fix at the coding stage? The sooner an error is found, the easier and less expensive it is to fix it.
How do the analyzer warnings encourage you to enhance (refactor) your code?

In future articles about qdEngine, I'll cover other errors found in the project. Thank you for reading.