Geekly Articles each Day
📜 ⬆️ ⬇️

The authors of the game 0 AD - well done

PVS-Studio & 0 A.D.

0 AD is a three-dimensional 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 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's 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 strategy, developed by a community of volunteers (the main developers are united in the Wildfire Games team). The game allows you to control civilizations that existed in the period of 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 0 AD?

I asked my colleague Yegor Bredikhin ( EgorBredikhin ) to 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 have already 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 on 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 error density 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 are positioning 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

Let's start with 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 message analyzer 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 !

Take a look below and you will see a break statement that interrupts 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 element 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 related to the leakage of resources and require you first to show what the WARN_RETURN macro 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 terminate without releasing the file descriptor. In practice, this is hardly possible. It is doubtful that data from "/ dev / urandom" cannot be read. 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

The correct option is:

 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 { 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 argument of the swprintf_s function. The char type argument is expected. trace.cpp 93

Here we are confronted with a confusing and unintelligible history of the 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.

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

I think the error does not require clarification. 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 the value of the 'int' type. Consider inspecting the 'bIsOrientationPreserving> 0.5' expression. MikktspaceWrap.cpp 137

It makes no sense to compare a variable of type int with a constant of 0.5. Moreover, by its very meaning, this is generally a boolean 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, 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: CWE-675 Destructor's V749 Destructor will be taken away after leaving the object's scope. vfs.cpp 165

Before you create 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 the 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( unsigned int eventType ) { .... pEvent = new CFsmEvent( eventType ); if ( !pEvent ) return NULL; .... }

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, the 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 ) return NULL; // Create new transition CFsmTransition* pNewTransition = new CFsmTransition( state ); if ( !pNewTransition ) { delete pEvent; return NULL; } .... }

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 ourselves with the device function AddEvent .

 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 a new object created with the new operator. Sometimes it takes an 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; return NULL; }

Other errors:


Error 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 allocation is performed 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 the memory cannot be allocated, the realloc function will return NULL, and this NULL will be written to the variable dsd-> entries . After that, it will be impossible to free a block of memory whose address was previously stored in dsd-> entries . There is a memory leak.

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!

Source: https://habr.com/ru/post/420267/

More articles:


All Articles