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.
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.:).
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).
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:
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:
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
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 Twitter @Code_Analysis.