# Good job, authors of the game 0 A\.D\! 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](https://import.viva64.com/docx/blog/0581_0ad/image1.png) ## 0 A\.D\. Game [0 A\.D\.](https://play0ad.com/) 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](https://en.wikipedia.org/wiki/0_A.D._(video_game)) is taken from Wikipedia\]\. Why 0 A\.D\.? I asked my colleague [Egor Bredikhin](https://pvs-studio.com/en/blog/posts/?author=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](https://pvs-studio.com/en/blog/posts/cpp/0579/) 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](https://pvs-studio.com/en/blog/posts/cpp/0523/) 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](https://pvs-studio.com/en/blog/posts/0158/)\)\. ## Errors Let's now look at 19 bugs that I found in the game\. To do the analysis, I used the [PVS\-Studio](https://pvs-studio.com/en/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](https://pvs-studio.com/en/blog/posts/0457/)\. **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\. ```cpp 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](https://pvs-studio.com/en/docs/warnings/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** ```cpp void CmpRallyPointRenderer::MergeVisibilitySegments( std::deque& segments) { .... segments.erase(segments.end()); .... } ``` PVS\-Studio warning: [V783](https://pvs-studio.com/en/docs/warnings/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: ```cpp segments.erase(segments.end() - 1); ``` Although, even such a simple variant could have been written: ```cpp 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\. ```cpp #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\. ```cpp 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](https://pvs-studio.com/en/docs/warnings/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\. ```cpp 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(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** ```cpp Status LoadHeightmapImageOs(....) { .... shared_ptr fileData = shared_ptr(new u8[fileSize]); .... } ``` PVS\-Studio warning: [V554](https://pvs-studio.com/en/docs/warnings/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: ```cpp shared_ptr fileData = shared_ptr(new u8[fileSize]); ``` **Error N6** ```cpp FUTrackedPtr(ObjectClass* _ptr = NULL) : ptr(_ptr) { if (ptr != NULL) FUTracker::TrackObject((FUTrackable*) ptr); ptr = ptr; } ``` PVS\-Studio warning: [V570](https://pvs-studio.com/en/docs/warnings/v570/) The 'ptr' variable is assigned to itself\. FUTracker\.h 122 **Errors N7, N8** ```cpp 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](https://pvs-studio.com/en/docs/warnings/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](https://pvs-studio.com/en/docs/warnings/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](https://pvs-studio.com/en/blog/examples/v595/)\. In the beginning the pointer is already used, and only then is checked\. ```cpp 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](https://pvs-studio.com/en/docs/warnings/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** ```cpp typedef int tbool; void MikkTSpace::setTSpace(...., const tbool bIsOrientationPreserving, ....) { .... m_NewVertices.push_back(bIsOrientationPreserving > 0.5 ? 1.0f : (-1.0f)); .... } ``` [V674](https://pvs-studio.com/en/docs/warnings/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** ```cpp virtual Status ReplaceFile(const VfsPath& pathname, const shared_ptr& 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](https://pvs-studio.com/en/docs/warnings/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** ```cpp CFsmEvent* CFsm::AddEvent( unsigned int eventType ) { .... pEvent = new CFsmEvent( eventType ); if ( !pEvent ) return NULL; .... } ``` PVS\-Studio warning: [V668](https://pvs-studio.com/en/docs/warnings/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\. ```cpp 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\. ```cpp 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: ```cpp 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** ```cpp 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](https://pvs-studio.com/en/docs/warnings/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](https://pvs-studio.com/en/blog/posts/0580/)\. Thank you for your attention\. I'll finish the article by inviting you to follow us in Twitter [@Code\_Analysis](https://twitter.com/Code_Analysis)\.