The authors of the game 0 AD - well done

0 AD is a three-dimensional game in the genre of historical real-time strategy game, developed by a community of volunteers. The size of the code base is small and I decided to check out the game as a holiday from large projects such as Android and XNU Kernel. So, we have a project that contains 165,000 lines of C ++ code. Let's see what is interesting in it can be found with the help of the PVS-Studio static analyzer.

0 AD

0 AD (0 AD) is a free three-dimensional game in the genre of historical real-time strategy, developed by a community of volunteers (the main developers are united in the Wildfire Games team). The game allows you to control the civilizations that existed in the period 500 years BC. e. —1 year BC. e. As of the summer of 2018, the project is in alpha version. [ Description taken from Wikipedia].

Why exactly AD 0?

I asked a colleague Yegor Bredikhin ( EgorBredikhin) select and check for me some small open project with which I could work in breaks between other tasks. He sent me a log for the 0 AD project. To the question: “Why this particular project?” Was the answer: “Yes, I just played this game, a good real-time strategy.” Ok, then it will be 0 AD :).

Error density

Just want to praise the authors of 0 AD for good quality C ++ code. Well done, rarely able to meet such a low density of errors. Of course, not all errors are meant, but those that can be detected with PVS-Studio. As I said, although PVS-Studio does not find all the errors, nevertheless, we can safely talk about the relationship between the density of errors and the quality of the code as a whole.

Few numbers. The total number of non-empty lines of code is 231270. Of these, 28.7% are comments. Total, 165,000 lines of pure C ++ code.

The number of messages issued by the analyzer was small, and after reviewing them all, I wrote out 19 errors. All these errors I will discuss later in the article. Perhaps I missed something, considering the error innocuous, inaccurate code. However, in general, this picture does not change.

So, I found 19 errors on 165,000 lines of code. We calculate the density of errors: 19 * 1000/165000 = 0.115.

For simplicity, we round and assume that the PVS-Studio analyzer detects 0.1 errors per 1000 lines of code in the game code.

Excellent result! For comparison, in my recent article about Android, I thought that I detected at least 0.25 errors per 1000 lines of code. In fact, the density of errors there is even more, I just did not find the strength to carefully analyze the entire report.

Or take for example the EFL Core Libraries library, which I carefully studied and counted the number of defects. In it, PVS-Studio detects 0.71 errors per 1000 lines of code.

So, the authors of AD 0 are great, but for the sake of fairness, it should be noted that a small amount of code written in C ++ helps them. Unfortunately, the larger the project, the faster its complexity increases, and the density of errors increases nonlinearly ( more ).

Errors

Let's now look at the 19 errors I found in the game. To analyze the project, I used PVS-Studio version 6.24. I suggest trying to download the demo version and check the projects you are working on.

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

Error N1

We begin with the consideration of a complex error. In fact, it is not complicated, but you have to get acquainted with a fairly large code fragment.

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 first glance, the analyzer message seems strange. Why is the condition nbNeighb> = 2 always false? Indeed, in the body of the loop there is an increment of the variable nbNeighb !

Look below and you will see a break statement that terminates the loop. Therefore, if the variable nbNeighb is increased, the cycle will be stopped. Thus, the value of the variable nbNeighb will never reach a value greater than 1.

The code clearly contains some kind of logical error.

N2 error

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

Very, very strange code. Perhaps the programmer wanted to remove the last item from the container. In this case, the code should be like this:

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

Although, then it would be easier to write:

segments.pop_back();

To be honest, I do not quite understand what exactly was supposed to be written here.

Error N3, N4

I decided to consider two errors together, as they are connected with the leakage of resources and require you to first show what the macro WARN_RETURN is .

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

So, as you can see, the WARN_RETURN macro causes the function to exit the body. Now look at the sloppy ways to use 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 fread function cannot read the data, the sys_generate_random_bytes function will exit without releasing the file descriptor. In practice, this is hardly possible. It is doubtful that you will not be able to read the data from "/ dev / urandom". However, the code is inaccurate.

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. A memory leak is possible. x.cpp 421

If the cursor cannot load, then 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 is allocated with 'new []' will be cleaned using 'delete'. MapIO.cpp 54

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

Error N7, N8

std::wstring TraceEntry::EncodeAsText() const
{
  constwchar_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(),
             (unsignedlong)m_size);
  return buf;
}

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

Here we come across a confusing and vague history of an alternative implementation of the swprintf function in Visual C ++. I will not retell it, but refer to the V576 diagnostic documentation (see the “Wide lines” section).

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

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

Error N9, N10, N11

Classic . At the beginning, the pointer is used, and only then it is checked.

staticvoidTEST_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 used against nullptr. Check lines: 140, 143. test_secure_crt.h 140

I think the error needs no explanation. Similar warnings:

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

Error n12

typedefint 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 the value of the 'int' type. Consider inspecting the 'bIsOrientationPreserving> 0.5' expression. MikktspaceWrap.cpp 137

It makes no sense to compare an int variable with a constant of 0.5. Moreover, by its very meaning, this is generally a boolean type variable, which means comparing it with 0.5 looks quite strange. I suppose that instead of bIsOrientationPreserving , another variable should be used here.

Error N13

virtual Status ReplaceFile(const VfsPath& pathname,
                           constshared_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: CWE-675 Destructor V749 vfs.cpp 165

Before creating a file, you need an object of type ScopedLock to unlock an object. To do this, explicitly call the destructor. The trouble is that the destructor for the object s will be automatically called again when exiting the function. Those. The destructor will be called twice. I have not studied the device class ScopedLock , but in any case, this is not worth it. Often, such a double call to a destructor leads to undefined behavior or other unpleasant errors. Even if the code now works fine, it is very easy to break everything by changing the implementation of the ScopedLock class .

Error N14, N15, N16, N17

CFsmEvent* CFsm::AddEvent( unsignedint eventType )
{
  ....
  pEvent = new CFsmEvent( eventType );
  if ( !pEvent ) returnNULL;
  ....
}

PVS-Studio warning : V668 CWE-570, it’s not a problem. The exception will be generated in the case of memory allocation error. fsm.cpp 259

Checking the pointer does not make sense, since in case of a memory allocation error, an exception std :: bad_alloc will be generated .

So, the check is superfluous, but the error is not serious. However, everything is much worse when some logic is executed in the body of the if statement . Consider the following case:

CFsmTransition* CFsm::AddTransition(....)
{
  ....
  CFsmEvent* pEvent = AddEvent( eventType );
  if ( !pEvent ) returnNULL;
  // Create new transition
  CFsmTransition* pNewTransition = new CFsmTransition( state );
  if ( !pNewTransition )
  {
    delete pEvent;
    returnNULL;
  }
  ....
}

Analyzer Warning: VW C8E-570 V668 The exception will be generated in the case of memory allocation error. fsm.cpp 289 An

attempt is made to free the memory whose address is stored in the pEvent pointer . Naturally, this will not happen and a memory leak will occur.

In fact, when I started to deal with this code, it turned out that everything is more complicated and perhaps there is not one mistake, but two. Now I will explain what is wrong with this code. To do this, we need to familiarize yourself with the device function AddEvent .

CFsmEvent* CFsm::AddEvent( unsignedint 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 ) returnNULL;
    // Store new event into internal map
    m_Events[ eventType ] = pEvent;
  }
  return pEvent;
}

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

And then the question arises: who owns and must destroy the objects, the pointers to which are stored in the m_Events container ? I am not familiar with the project, but, most likely, somewhere there is code that destroys all these objects. Then deleting an object inside the CFsm :: AddTransition function is generally redundant.

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

if ( !pNewTransition )
{
  delete pEvent;
  returnNULL;
}

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

Ошибка N18, N19

staticvoiddir_scan_callback(struct de *de, void *data){
  structdir_scan_data *dsd = (structdir_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, the memory is reallocated using the realloc function . The error is that the value of the pointer to the original memory block is immediately overwritten by the new value returned by the realloc function .

If memory cannot be allocated, the realloc function will return NULL, and this NULL will be written to the dsd-> entries variable . After that, it will be impossible to free a block of memory whose address 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, the original pointer 'Buffer' is lost. Consider assigning realloc () to a temporary pointer. Preprocessor.cpp 84

Conclusion

I can not say that this time the article turned out to be fascinating, or that I managed to show a lot of terrible mistakes. What to do, just once is not necessary. I see what I write .

Thanks for attention. For a change, I’ll finish the article with an invitation to follow me on Instagram @pvs_studio_unicorn and on Twitter @Code_Analysis .

If you want to share this article with an English-speaking audience, then please use the link to the translation: Andrey Karpov. Good job, authors of the game 0 AD!

Tags:

The authors of the game 0 AD - well done

0 AD

Error density

Errors

Conclusion

Also popular now: