To get a trial key
fill out the form below
Team License (standard version)
Enterprise License (extended version)
* By clicking this button you agree to our Privacy Policy statement

** This site is protected by reCAPTCHA and the Google Privacy Policy and Terms of Service apply.
Request our prices
New License
License Renewal
--Select currency--
USD
EUR
GBP
RUB
* By clicking this button you agree to our Privacy Policy statement

** This site is protected by reCAPTCHA and the Google Privacy Policy and Terms of Service apply.
To get the licence for your open-source project, please fill out this form
* By clicking this button you agree to our Privacy Policy statement

** This site is protected by reCAPTCHA and the Google Privacy Policy and Terms of Service apply.
To get the licence for your open-source project, please fill out this form
* By clicking this button you agree to our Privacy Policy statement

** This site is protected by reCAPTCHA and the Google Privacy Policy and Terms of Service apply.
I am interested to try it on the platforms:
* By clicking this button you agree to our Privacy Policy statement

** This site is protected by reCAPTCHA and the Google Privacy Policy and Terms of Service apply.
Message submitted.

Your message has been sent. We will email you at


If you haven't received our response, please do the following:
check your Spam/Junk folder and click the "Not Spam" button for our message.
This way, you won't miss messages from our team in the future.

>
>
>
Good job, authors of the game 0 A.D!

Good job, authors of the game 0 A.D!

Aug 15 2018
Author:

0 A.D. is a 3D game in the genre of historical real-time strategy, developed by a community of volunteers. The size of the code base is small and I decided to perform checking of this game as a break from larger projects, such as Android and the XNU Kernel. So, we have a project containing 165000 lines of C++ code. Let's see what interesting things we can find in it using the PVS-Studio static analyzer.

0581_0ad/image1.png

0 A.D. Game

0 A.D. is a free, open-source real-time strategy game of ancient warfare, developed by a community of volunteers (Wildfire Games unites a team of main developers). The game lets controlling civilizations that existed between 500 BC - 1 BC. As at summer 2018, the project is at the state of alpha-version. [Description is taken from Wikipedia].

Why 0 A.D.?

I asked my colleague Egor Bredikhin to choose and check for me a small open source project, which I could investigate in between my other tasks. He sent me the log of the project 0 A.D. After the question "Why this project?" he answered: "I just played this game, a good real-time strategy". OK, then let it be 0 A.D.:).

Error Density

I would like to compliment the authors of 0 A.D. for good quality of C++ code. Well done, I seldom meet such low density of errors. I mean, of course, not all errors, but those that can be detected with the help of PVS-Studio. As I have already said, although PVS-Studio finds not all errors, despite this, you can safely speak about the connection between the density of errors and code quality in general.

A few numbers. The total number of nonblank lines of code is 231270. 28.7% from them are comments. In total, 165000 lines of pure C++ code.

The number of warnings issued by the analyzer was small and having reviewed them all I wrote down 19 errors. I'll consider all these error later in this article. Maybe I skipped something, considering the bug as harmless sloppy code. However, this does not change the whole picture.

So, I found 19 errors per 165000 lines of code. Let's calculate the density of errors: 19*1000/165000 = 0.115.

For simplicity, we will round up and will assume that the PVS-Studio analyzer detects 0.1 error per 1000 lines of code in the game's code.

A great result! For comparison, in my recent article about Android I figured out I discovered at least 0.25 errors per 1000 lines of code. In fact, the density of errors was even greater there, I just haven't found enough energy to review carefully the whole report.

On the other hand, we can take for example the library Core EFL Libraries, which I thoroughly analyzed and counted the number of defects. PVS-Studio detects 0.71 errors per 1000 lines of code in it.

So, the 0 A.D. authors - well done! However, for the sake of fairness it should be noted that the small amount of code written in C++ works in authors' favor. Unfortunately, the larger the project, the faster its complexity is growing and density of errors increases nonlinearly (more info).

Errors

Let's now look at 19 bugs that I found in the game. To do the analysis, I used the PVS-Studio analyzer version 6.24. I suggest trying to download the demo version, and test the projects you are working on.

Note. We position PVS-Studio as a B2B solution. For small projects and individual developers, we have a free license option: How to use PVS-Studio for free.

Error N1

Let's start with considering a complex error. Actually, it is not complicated, but we'll have to get acquainted with a large fragment of code.

void WaterManager::CreateWaveMeshes()
{
  ....
  int nbNeighb = 0;
  ....
  bool found = false;
  nbNeighb = 0;
  for (int p = 0; p < 8; ++p)
  {
    if (CoastalPointsSet.count(xx+around[p][0] +
                               (yy + around[p][1])*SideSize))
    {
      if (nbNeighb >= 2)
      {
        CoastalPointsSet.erase(xx + yy*SideSize);
        continue;
      }
      ++nbNeighb;
      // We've found a new point around us.
      // Move there
      xx = xx + around[p][0];
      yy = yy + around[p][1];
      indexx = xx + yy*SideSize;
      if (i == 0)
        Chain.push_back(CoastalPoint(indexx,CVector2D(xx*2,yy*2)));
      else
        Chain.push_front(CoastalPoint(indexx,CVector2D(xx*2,yy*2)));
      CoastalPointsSet.erase(xx + yy*SideSize);
      found = true;
      break;
    }
  }
  if (!found)
    endedChain = true;
  ....
}

PVS-Studio warning: V547 CWE-570 Expression 'nbNeighb >= 2' is always false. WaterManager.cpp 581

At a first glance the analyzer message seems strange. Why is the condition nbNeighb >= 2 always false? In the body of the loop there is an increment of the nbNeighb variable!

Look below and you'll see the operator break that interrupts the execution of the loop. Therefore, if the variable nbNeighb is incremented, then the loop will be stopped. Thus, the value of the variable nbNeighb will never reach the value greater than 1.

The code obviously contains a logical error.

Error N2

void
CmpRallyPointRenderer::MergeVisibilitySegments(
  std::deque<SVisibilitySegment>& segments)
{
  ....
  segments.erase(segments.end());
  ....
}

PVS-Studio warning: V783 CWE-119 Dereferencing of the invalid iterator 'segments.end()' might take place. CCmpRallyPointRenderer.cpp 1290

This code is very strange. Perhaps, a developer wanted to remove the last element from the container. In this case the correct code should be as follows:

segments.erase(segments.end() - 1);

Although, even such a simple variant could have been written:

segments.pop_back();

Honestly, I don't quite understand what exactly was supposed to be written here.

Errors N3, N4

I decided to consider jointly two bugs, as they relate to the resources leak and require showing what is a WARN_RETURN macro.

#define WARN_RETURN(status)\
  do\
  {\
    DEBUG_WARN_ERR(status);\
    return status;\
  }\
  while(0)

So, as you can see, the macro WARN_RETURN leads to the exit from the function body. Now we'll look at the messy ways of using this macro.

The first fragment.

Status sys_generate_random_bytes(u8* buf, size_t count)
{
  FILE* f = fopen("/dev/urandom", "rb");
  if (!f)
    WARN_RETURN(ERR::FAIL);

  while (count)
  {
    size_t numread = fread(buf, 1, count, f);
    if (numread == 0)
      WARN_RETURN(ERR::FAIL);
    buf += numread;
    count -= numread;
  }

  fclose(f);
  return INFO::OK;
}

PVS-Studio warning: V773 CWE-401 The function was exited without releasing the 'f' handle. A resource leak is possible. unix.cpp 332

If the function fread is unable to read the data, the function sys_generate_random_bytes will terminate without releasing the file descriptor. In practice, this is hardly possible. It is doubtful that it will not be possible to read data from "/dev/urandom". However, the code is poorly written.

The second fragment.

Status sys_cursor_create(....)
{
  ....
  sys_cursor_impl* impl = new sys_cursor_impl;
  impl->image = image;
  impl->cursor = XcursorImageLoadCursor(wminfo.info.x11.display, image);
  if(impl->cursor == None)
    WARN_RETURN(ERR::FAIL);

  *cursor = static_cast<sys_cursor>(impl);
  return INFO::OK;
}

PVS-Studio warning: V773 CWE-401 The function was exited without releasing the 'impl' pointer. A memory leak is possible. x.cpp 421

If it is not possible to load the cursor, a memory leak occurs.

Error N5

Status LoadHeightmapImageOs(....)
{
  ....
  shared_ptr<u8> fileData = shared_ptr<u8>(new u8[fileSize]);
  ....
}

PVS-Studio warning: V554 CWE-762 Incorrect use of shared_ptr. The memory allocated with 'new []' will be cleaned using 'delete'. MapIO.cpp 54

Here is the correct version:

shared_ptr<u8[]> fileData = shared_ptr<u8>(new u8[fileSize]);

Error N6

FUTrackedPtr(ObjectClass* _ptr = NULL) : ptr(_ptr)
{
  if (ptr != NULL) FUTracker::TrackObject((FUTrackable*) ptr);
  ptr = ptr;
}

PVS-Studio warning: V570 The 'ptr' variable is assigned to itself. FUTracker.h 122

Errors N7, N8

std::wstring TraceEntry::EncodeAsText() const
{
  const wchar_t action = (wchar_t)m_action;
  wchar_t buf[1000];
  swprintf_s(buf, ARRAY_SIZE(buf), L"%#010f: %c \"%ls\" %lu\n",
             m_timestamp, action, m_pathname.string().c_str(),
             (unsigned long)m_size);
  return buf;
}

PVS-Studio warning: V576 CWE-628 Incorrect format. Consider checking the fifth actual argument of the 'swprintf_s' function. The char type argument is expected. trace.cpp 93

Here we face a confusing and strange story of an alternative implementation of the swprintf function in Visual C++. I will not retell it, you can refer to the documentation on the diagnostic V576 (see the section "Wide strings").

In this case, most likely, this code will work correctly when compiled in Visual C++ for Windows and incorrectly when building for Linux or macOS.

A similar error: V576 CWE-628 Incorrect format. Consider checking the fourth actual argument of the 'swprintf_s' function. The char type argument is expected. vfs_tree.cpp 211

Errors N9, N10, N11

Classic. In the beginning the pointer is already used, and only then is checked.

static void TEST_CAT2(char* dst, ....)
{
  strcpy(dst, dst_val);                                 // <=
  int ret = strcat_s(dst, max_dst_chars, src);
  TS_ASSERT_EQUALS(ret, expected_ret);
  if(dst != 0)                                          // <=
    TS_ASSERT(!strcmp(dst, expected_dst));
}

PVS-Studio warning: V595 CWE-476 The 'dst' pointer was utilized before it was verified against nullptr. Check lines: 140, 143. test_secure_crt.h 140

I think the error does not require explanation. Similar warnings:

  • V595 CWE-476 The 'dst' pointer was utilized before it was verified against nullptr. Check lines: 150, 153. test_secure_crt.h 150
  • V595 CWE-476 The 'dst' pointer was utilized before it was verified against nullptr. Check lines: 314, 317. test_secure_crt.h 314

Error N12

typedef int tbool;

void MikkTSpace::setTSpace(....,
                           const tbool bIsOrientationPreserving,
                           ....)
{
  ....
  m_NewVertices.push_back(bIsOrientationPreserving > 0.5 ? 1.0f : (-1.0f));
  ....
}

V674 CWE-682 The '0.5' literal of the 'double' type is compared to a value of the 'int' type. Consider inspecting the 'bIsOrientationPreserving > 0.5' expression. MikktspaceWrap.cpp 137

It makes no sense to compare a variable of the int type with the constant 0.5. Moreover, by meaning this is generally a boolean variable, and thus, comparing it with 0.5 looks very strange. I suppose that instead of bIsOrientationPreserving other variable should be used here.

Error N13

virtual Status ReplaceFile(const VfsPath& pathname,
                           const shared_ptr<u8>& fileContents, size_t size)
{
  ScopedLock s;
  VfsDirectory* directory;
  VfsFile* file;
  Status st;
  st = vfs_Lookup(pathname, &m_rootDirectory, directory,
                  &file, VFS_LOOKUP_ADD|VFS_LOOKUP_CREATE);

  // There is no such file, create it.
  if (st == ERR::VFS_FILE_NOT_FOUND)
  {
    s.~ScopedLock();
    return CreateFile(pathname, fileContents, size);
  }
  ....
}

PVS-Studio warning: V749 CWE-675 Destructor of the 's' object will be invoked a second time after leaving the object's scope. vfs.cpp 165

Before creating the file, we need the ScopedLock object to unlock something. To do this, the destructor is explicitly called. The trouble is that the destructor for the s object will be called automatically again when exiting the function. I.e., the destructor will be called twice. I have not investigated the configuration of the ScopedLock class but anyway it's not worth doing it. Often such double call of the destructor causes undefined behavior or other unpleasant errors. Even if now the code works fine, everything is very easy to break by changing the implementation of the ScopedLock class.

Errors N14, N15, N16, N17

CFsmEvent* CFsm::AddEvent( unsigned int eventType )
{
  ....
  pEvent = new CFsmEvent( eventType );
  if ( !pEvent ) return NULL;
  ....
}

PVS-Studio warning: V668 CWE-570 There is no sense in testing the 'pEvent' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. fsm.cpp 259

Checking of the pointer does not make sense, as in the case of memory allocation error, the exception std::bad_alloc will be thrown.

So, the check is redundant, but it is not a serious error. However, everything is much worse when in the body of the operator if unclear logic is executed. Let's consider such a case.

CFsmTransition* CFsm::AddTransition(....)
{
  ....
  CFsmEvent* pEvent = AddEvent( eventType );
  if ( !pEvent ) return NULL;

  // Create new transition
  CFsmTransition* pNewTransition = new CFsmTransition( state );
  if ( !pNewTransition )
  {
    delete pEvent;
    return NULL;
  }
  ....
}

Analyzer warning: V668 CWE-570 There is no sense in testing the 'pNewTransition' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. fsm.cpp 289

Here happens the attempt to release memory, an address to which is stored in the pEvent pointer. Naturally, this will not happen and there will be a memory leak.

In fact, when I started to deal with this code, it turned out that everything is more complicated and perhaps there is not one error, but two. Now I'm going to explain what's wrong with this code. For this, we will need to be acquainted with the configuration of the AddEvent function.

CFsmEvent* CFsm::AddEvent( unsigned int eventType )
{
  CFsmEvent* pEvent = NULL;

  // Lookup event by type
  EventMap::iterator it = m_Events.find( eventType );
  if ( it != m_Events.end() )
  {
    pEvent = it->second;
  }
  else
  {
    pEvent = new CFsmEvent( eventType );
    if ( !pEvent ) return NULL;

    // Store new event into internal map
    m_Events[ eventType ] = pEvent;
  }

  return pEvent;
}

Note that the function does not always return a pointer to the new object created using the new operator. Sometimes it takes an existing object from the container m_Events. A pointer to the newly created object, by the way, will be also placed in m_Events.

Here the question arises: who owns, and has to destroy objects, pointers to which are stored in the container m_Events? I'm not familiar with the project, but most likely, somewhere there is a code, which destroys all the objects. Then removing of the object inside the function CFsm::AddTransition is superfluous.

I got the impression that you can simply delete the following code fragment:

if ( !pNewTransition )
{
  delete pEvent;
  return NULL;
}

Other errors:

  • V668 CWE-571 There is no sense in testing the 'ret' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. TerrainTextureEntry.cpp 120
  • V668 CWE-571 There is no sense in testing the 'answer' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. SoundManager.cpp 542

Errors N18, N19

static void dir_scan_callback(struct de *de, void *data) {
  struct dir_scan_data *dsd = (struct dir_scan_data *) data;

  if (dsd->entries == NULL || dsd->num_entries >= dsd->arr_size) {
    dsd->arr_size *= 2;
    dsd->entries = (struct de *) realloc(dsd->entries, dsd->arr_size *
                                         sizeof(dsd->entries[0]));
  }
  if (dsd->entries == NULL) {
    // TODO(lsm): propagate an error to the caller
    dsd->num_entries = 0;
  } else {
    dsd->entries[dsd->num_entries].file_name = mg_strdup(de->file_name);
    dsd->entries[dsd->num_entries].st = de->st;
    dsd->entries[dsd->num_entries].conn = de->conn;
    dsd->num_entries++;
  }
}

PVS-Studio warning: V701 CWE-401 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'dsd->entries' is lost. Consider assigning realloc() to a temporary pointer. mongoose.cpp 2462

If the size of the array becomes insufficient, memory reallocation happens using the function realloc. The bug is that the value of a pointer to the source memory block is immediately overwritten with the new value returned by the realloc function.

If it is not possible to allocate memory, the realloc function will return NULL and this NULL will be stored in the dsd->entries variable. After that it will become impossible to release a block of memory, the address to which was previously stored in dsd->entries. A memory leak will occur.

Another error: V701 CWE-401 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'Buffer' is lost. Consider assigning realloc() to a temporary pointer. Preprocessor.cpp 84

Conclusion

I can't say that this time the article turned out to be fascinating or that I managed to show a lot of terrible errors. It depends. I write what I see.

Thank you for your attention. I'll finish the article by inviting you to follow us in Instagram @pvsstudio and in Twitter @Code_Analysis.

Popular related articles
Technologies used in the PVS-Studio code analyzer for finding bugs and potential vulnerabilities

Date: Nov 21 2018

Author: Andrey Karpov

A brief description of technologies used in the PVS-Studio tool, which let us effectively detect a large number of error patterns and potential vulnerabilities. The article describes the implementati…
The Evil within the Comparison Functions

Date: May 19 2017

Author: Andrey Karpov

Perhaps, readers remember my article titled "Last line effect". It describes a pattern I've once noticed: in most cases programmers make an error in the last line of similar text blocks. Now I want t…
Appreciate Static Code Analysis!

Date: Oct 16 2017

Author: Andrey Karpov

I am really astonished by the capabilities of static code analysis even though I am one of the developers of PVS-Studio analyzer myself. The tool surprised me the other day as it turned out to be sma…
The Last Line Effect

Date: May 31 2014

Author: Andrey Karpov

I have studied many errors caused by the use of the Copy-Paste method, and can assure you that programmers most often tend to make mistakes in the last fragment of a homogeneous code block. I have ne…
Characteristics of PVS-Studio Analyzer by the Example of EFL Core Libraries, 10-15% of False Positives

Date: Jul 31 2017

Author: Andrey Karpov

After I wrote quite a big article about the analysis of the Tizen OS code, I received a large number of questions concerning the percentage of false positives and the density of errors (how many erro…
PVS-Studio for Java

Date: Jan 17 2019

Author: Andrey Karpov

In the seventh version of the PVS-Studio static analyzer, we added support of the Java language. It's time for a brief story of how we've started making support of the Java language, how far we've co…
The way static analyzers fight against false positives, and why they do it

Date: Mar 20 2017

Author: Andrey Karpov

In my previous article I wrote that I don't like the approach of evaluating the efficiency of static analyzers with the help of synthetic tests. In that article, I give the example of a code fragment…
Free PVS-Studio for those who develops open source projects

Date: Dec 22 2018

Author: Andrey Karpov

On the New 2019 year's eve, a PVS-Studio team decided to make a nice gift for all contributors of open-source projects hosted on GitHub, GitLab or Bitbucket. They are given free usage of PVS-Studio s…
The Ultimate Question of Programming, Refactoring, and Everything

Date: Apr 14 2016

Author: Andrey Karpov

Yes, you've guessed correctly - the answer is "42". In this article you will find 42 recommendations about coding in C++ that can help a programmer avoid a lot of errors, save time and effort. The au…
Static analysis as part of the development process in Unreal Engine

Date: Jun 27 2017

Author: Andrey Karpov

Unreal Engine continues to develop as new code is added and previously written code is changed. What is the inevitable consequence of ongoing development in a project? The emergence of new bugs in th…

Comments (0)

Next comments

This site is protected by reCAPTCHA and the Google Privacy Policy and Terms of Service apply.
This website uses cookies and other technology to provide you a more personalized experience. By continuing the view of our web-pages you accept the terms of using these files. If you don't want your personal data to be processed, please, leave this site.
Learn More →
Accept