top 10 warnings PVS-Studio

The bugs that we managed to find in the qdEngine engine turned out to be very diverse, so I don’t want to mix everything together in one publication. Readers may miss interesting topics related to writing quality code. Therefore, the analysis of the project will be published in the form of a series of publications, the first of which is devoted to the most interesting triggers from the point of view of the PVS-Studio plugin.

Best button

The KD Lab association has opened the source code of the game engine qdEngineintended for creating quests (news on the opennet website). The engine is mainly written in C++ and looks like an interesting object to study using the PVS-Studio code analyzer.

And indeed, how can you ignore the engine, on the basis of which such games as:

• Pilot Brothers 3D. The Case of Garden Pests;
• Pilot Brothers 3D-2. Secrets of the Kennel Club;
• Brothers pilots. The Other Side of the Earth;
• Wait for it! Issue 3. Song for the hare;
• The adventures of the good soldier Schweik.

The errors that were found in the project are varied, and I would like to consider them from different points of view. Therefore, there will be several publications on different topics, and the first of them is dedicated to the button Best in plugins PVS-Studio.

The button helps the user when getting acquainted with the PVS-Studio tool for the first time. The analyzer selects 10 warnings that are most likely to indicate real errors and at the same time be varied and interesting.

All static code analyzers produce a large number of warnings when first launched, many of which are likely to be irrelevant or false. This is not a problem, since analyzers, including PVS-Studio, have flexible settings.

However, a programmer who is just getting acquainted with static analysis methodology may be demotivated by a lot of warnings that seem irrelevant to him. I would like to immediately see some interesting errors. Button Best just allows you to do this.

When you click on it, the analyzer uses an empirical algorithm to select 10 warnings. When choosing, he is guided not only by the probability that we are faced with an error, but also tries to select warnings of different types.

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

For the qdEngine project, this button worked well:

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

In my opinion, all these warnings well demonstrate the capabilities of the analyzer, and now we will look at them. I hope this will encourage those who have only read about PVS-Studio to try it out. Downloadinstall, check the project and press the button Best. If something goes wrong, just write to us in support. We will always advise and help.

Warnings

Let's look at the promised warnings and at the same time practice some refactoring here and there.

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);
  ....
}

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

Constant used twice QD_OBJ_STATE_CHANGE_FLAGdeclared in the file qd_game_object.h next to many others:

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

Most likely, they forgot to use some other constant to create the mask. It's possible it's just a duplicate and the duplicate constant can be removed from the expression. It’s hard for me to say exactly here, since I’m not familiar with the project code.

Warning N2: typo (lost # symbol)

A very beautiful mistake, in my opinion. I agree, I have strange taste and professional deformation, but I hope you will also appreciate this bug.

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

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

The analyzer notices that, regardless of the condition, the same action will be performed:

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

Please note that the code is formatted somewhat strangely. I mean that's the key word else is at the beginning of the line.

If you look at the code next to it and think a little, it becomes clear that what they really wanted to write was not the operator elseand the preprocessor directive #else. However, the programmer made a typo and forgot the hash symbol (#).

Corrected 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;
    }
  }
  ....
}

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

The loop does some work with a list of files. As I understand it, for debugging purposes this code fragment was added, which the analyzer does not like:

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

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

1. It’s convenient to set a breakpoint in the debugger and immediately see the file name in the variable str.
2. Assigning an object to itself can additionally remove a warning from some compiler/analyzer that the variable str not used.

There is no mistake here. However, this is code smell and should be improved. In other words, this analyzer warning is a good reason to refactor. To begin with, I would change the function interface file in class qdSprite:

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

It's strange to return content std::string as just a pointer, and then also check it in different places on nullptr. In fact, the function will never return nullptr. If somewhere in the code you need a pointer, then you can always call c_str. Changed function:

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

Naturally, changing the interface of a function will affect many places in the project code. Moreover, for symmetry it makes sense to change the function set_file. However, I have a suspicion that all this would benefit the code.

Let's continue. Let's rewrite the previously discussed code fragment.

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);  
}

Improvements:

1. The code has become shorter and simpler.
2. The pointless pointer check has disappeared.
3. No temporary object creation std::string in a loop just to make it easier to debug. These are relatively resource-intensive operations due to memory allocation and deallocation. However, there is no guarantee that the compiler will remove all unnecessary things during the optimization process.
4. The PVS-Studio analyzer no longer issues a warning V570.
5. For debugging purposes, it is still easy to set a breakpoint (on the line with the macro QD_ADJUST_TO_REL_FILE_MEMBER) and look at the file name.

Warning N4: Potential Null Pointer Use

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

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

Pointer ddobj dereferenced to call a function. Below is a check of this pointer. Based on this check, the analyzer concludes that the variable ddobj may contain nullptrwhich is what he warns us about.

The code can be rewritten, for example, like this:

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;
};

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

In the class code PtrHandle there is no error. However, the problem is that it stores an object of type ResourceUser. Let's look at the declaration of this class:

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;
};

This class has virtual functions. Therefore, other classes are expected to inherit from this class. And if so, then the destructor should be declared as virtual. Otherwise the class destructor PtrHandle will not completely destroy objects that inherit 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_;
}

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

In variable temp_buffer_ pointers to arrays created using the operator are stored new[]. Therefore, they must be destroyed using the operator delete[].

In the code under consideration, to destroy an array, simply use delete, intended for single objects. As a result, this code produces undefined behavior.

Note. Someone might argue that in practice everything will work, since the array consists of elements of the type charand the work of operators new/delete comes down to calling functions malloc/free. This is wrong. The implementation of operators can be quite varied. For example, for optimization purposes, single objects and arrays can be created in different pre-allocated (reserved) memory pools.

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);
  }
  ....
}

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

To synchronize threads, an ordinary global variable is used. This is a bad idea because the compiler, during the optimization process, may conclude that it does not change and create an eternal loop. In addition, changing a variable may not be atomic.

In this case, you should replace the type int on 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;
}

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

If an error occurs in loading resources, the function will terminate early. In this case, the object whose pointer is stored in the variable will not be deleted fp. A memory leak will occur.

Correct option:

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

However, even after fixing the error, the code cannot be called good. Manual memory management provokes errors, which is what we see here. It's much better to use smart pointers.

Interesting point. Please note the comment:

// IMPORTANT(pabdulin): auto_ptr usage was removed

This code may have been correct at one time as long as it used a smart pointer std::auto_ptr. This class was then deprecated. Instead of replacing it with std::unique_ptrthe programmer decided to work with pointers “manually”, but was unable to cope with the task.

Warning N9: false alarm

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;
};

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 alerts selected by the analyzer that turned out to be false. Diagnostics V794 warns that the object must be protected from being copied into itself. To do this, the analyzer looks for a construction of the form in the body of the function:

if (this != &p)

But here a more complex check is written through function calls get:

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

Perhaps the author wanted to protect himself not just from copying a smart pointer into itself, but from copying two different smart pointers that refer to the same object. However, this makes no sense. If two smart pointers (without reference counting) refer to the same object, then this is a disaster. A situation of double destruction of an object is possible.

So such a check is actually no better. It only further confuses the analyzer and the people reading the code.

Well, the analyzer did not understand the situation. To resolve a false alert, you can do one of the following:

Option 1. Add a comment to suppress the warning.

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

The comment clearly indicates to the PVS-Studio analyzer that the code is safe.

Option 2. Rewrite the check.

if (this != &p)

I probably like this method better. The option with calling functions, in my opinion, looks cumbersome.

If you want more control, you can add the following check:

PtrHandle& operator=(PtrHandle& p) 
{ 
  if (this != &p)
  { 
    if (get() == p.get())
    {
       // Всё плохо. Один объект в двух умных указателях.
       // Время отлаживаться.
       assert(false);
       throw std::logic_error("Multiple ownership of an object.");
    }
    delete ptr; 
    ptr = p.release(); 
  } 
  return *this; 
}

The check will help identify situations where two smart pointers suddenly control the same object.

Warning N10: Reassignment

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);
  }
  ....
}

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

The analyzer doesn't like what's in the branch MOVEMENT_MODE_MOVE variable offs_type the value that is already in it is assigned.

Formally, the analyzer is right. Re-assignment makes no sense and is sometimes not just extra code, but an error.

In this case there is no bug. Let's look at how you can get rid of the warning.

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

Option 2. Rewrite the code a little by adding 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;
  defaule:
    offs_type = qdGameObjectStateWalk::OFFSET_WALK;
  }

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

The warning will go away, but I don't like that variable offs_type is no longer initialized at the declaration location. Let's take another refactoring step and move part 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 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 has become shorter and still easy to read. Great. Moreover, now the variable offs_type is not needed at all, and the code can be shortened a little more.

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

A good example: an analyzer warning prompted us to refactor the code, making it simpler and better.

Conclusion

The article showed:

1. How the button can help Best when you first get acquainted with the PVS-Studio analyzer. I propose without delay, get a trial and try to analyze your projects.
2. What errors does PVS-Studio help you find and eliminate at the coding stage? The earlier an error is found, the easier and cheaper it is to fix it.
3. How analyzer warnings prompt code improvement (refactoring).

In subsequent articles about qdEngine, I will analyze a number of other errors found in the project. Thank you for your attention.

Additional links

1. Development of tools for C++ programmers: static code analyzers.
2. Warnings help you write concise code.
3. The static analyzer encourages you to write clean code.
4. Calling virtual functions in constructors and destructors (C++).
5. How to implement a static code analyzer in a legacy project without demotivating the team.

If you want to share this article with an English-speaking audience, please use the translation link: Andrey Karpov. Let's check the qdEngine game engine, part one: top 10 warnings issued by PVS-Studio.