# Captain Blood's adventures: would Arabella sink? We recently found out that the source code of the Captain Blood game \(formerly known as Age of Pirates: Captain Blood\) has been opened\. We couldn't pass it up so we checked the game's quality with the help of PVS\-Studio\. Will Captain Blood's legendary ship sink into the sea abyss because of the detected bugs? Let's find out\! ![1033_cap_blood/image1.png](https://import.viva64.com/docx/blog/1033_cap_blood/image1.png) ## Project overview Captain Blood is an action\-adventure game made by 1C Company and based on the novels by Rafael Sabatini\. The game follows the adventures of Captain Peter Blood, the main character of Sabatini's novel\. The action takes place in New England\. The protagonist of the game is an Irish doctor who has been unjustly convicted of treason against the king and exiled to the island of Barbados\. Peter Blood managed to escape from the island by capturing a Spanish ship\. Thus, he became the most famous pirate of that time\. The project was never released and was closed in 2010\. This happened because 1C and Playlogic were unable to decide who owns the rights to the game\. There were rumors that the final build of the game had been irretrievably lost\. Recently, some news came out proving the opposite\. A working build of the game appeared on a popular torrent tracker, and a walkthrough of the game was posted on YouTube\. Later, the [the game](https://github.com/storm-devs/captain-blood.git)'s source code was posted on GitHub under the GPLv3 license\. So, after we got learned more about the project, let's discuss the most interesting warnings that PVS\-Studio found when analyzing it\. ## Captain Blood's thirteen **Warning N1** [V1075](https://pvs-studio.com/en/docs/warnings/v1075/) The function expects the file to be opened for writing, but it was opened for reading\. Debugger\.cpp 172 We can congratulate our teammate Sergey Larin on the first "trophy" warning of his diagnostic rule \(it was released in PVS\-Studio 7\.15\)\. According to this diagnostic rule, data is written into files that are opened for reading and vice versa\. Honestly, we didn't even expect that this warning would ever be issued\. Here is this problematic function: ```cpp void appDebuger::CopyDump(const char * src, DWORD from, const char * dst) { FILE * file = fopen(src, "rb"); if(file) { fseek(file, 0, SEEK_END); DWORD size = ftell(file); fseek(file, from, SEEK_SET); if(size > from) { FILE * to = fopen(dst, "a+b"); if(to) { char buf[128]; while(from < size) { DWORD s = size - from; if(s > sizeof(buf)) s = sizeof(buf); memset(buf, ' ', sizeof(buf)); fread(buf, s, 1, file); fwrite(buf, s, 1, file); // <= from += s; } fclose(to); } } fclose(file); } } ``` So, the function should read data from a file at the _src_ path, starting at the _from_ position, and write it to another file at the _dst_ path\. The _file_ variable is responsible for the source file, the _to_ variable is responsible for the resulting file\. Unfortunately, we can see reading from the _file_ variable and writing to the _file_ variable in the code\. We can fix it this way: ```cpp fwrite(buf, s, 1, to); ``` **Warning N2** [V1065](https://pvs-studio.com/en/docs/warnings/v1065/) \[CWE\-682\] Expression can be simplified, check 'bi' and similar operands\. PreviewAnimation\.cpp 146 ```cpp PreviewAnimation::PreviewAnimation(....) { // .... const int bw = 30; const int bh = 30; const int bi = 3; const int cn = 9; bar.r.w = 7 + (bw + bi)*cn + 7 - bi + 1 + (bw + bi) + 7 - bi + 1; // .... } ``` That's one of my favorite diagnostic rules\! It indicates that an expression can be simplified by removing the _bi_ variable: ```cpp bar.r.w = 7 + (bw + bi)*cn + 7 - bi + 1 + bw + 7 + 1; ``` Maybe there should be another variable instead of one of the _bi_\. Or the _\(bw \+ bi\)_ expression was supposed to be multiplied by something\. I wonder how the game animation would change\.\.\. I would like to mention that it's a bad practice to give constants such names \(_bw, bh, bi, cn_\)\. They can make the code confusing to the person reading it\. It would be better to make the names of the variables more meaningful or leave a comment to identify them\. **Warning N3** [V570](https://pvs-studio.com/en/docs/warnings/v570/) \[CWE\-480\] The 'ldMaxChars' variable is assigned to itself\. StringAttr\.cpp 198 ```cpp void StringAttribute::LoadFromFile (....) { int ldMinChars, ldMaxChars; // .... minChars = ldMinChars; ldMaxChars = ldMaxChars; // .... } class StringAttribute : public BaseAttribute { int minChars; int maxChars; } ``` The usual assignment of a variable to itself takes place here\. Let's look at the definition of the _StringAttribute_ class to see what should be in place of the variable where the assignment occurs\. Here we can see the similar _maxChars_ data member\. Obviously, the developer made a mistake and actually should have assigned the _ldMaxChars_ value to the variable\. Here's the fixed version of the code: ```cpp void StringAttribute::LoadFromFile (....) { int ldMinChars, ldMaxChars; // .... minChars = ldMinChars; maxChars = ldMaxChars; // .... } ``` **Warning N4** [V781](https://pvs-studio.com/en/docs/warnings/v781/) \[CWE\-20, CERT\-API00\-C\] The value of the 'start' index is checked after it was used\. Perhaps there is a mistake in program logic\. coreloader\.h 136 ```cpp __forceinline void GetBootParams(const char * bootIni) { long start = 0; // .... if (bootIni[start] && start > 0) { // .... } // .... } ``` In this code fragment, the developer first used the _start_ value to index over the _bootIni_ array, and only then checked it for more than zero\. This check may be pointless, then it would be better to remove it\. The check can make sense, then it should be interchanged with the array subscripting: ```cpp if (start > 0 && bootIni[start]) { // .... } ``` Otherwise, the developer runs the risk of crashing the game\. In fact, quite a few warnings of this type are issued for this code\. Here are a few that are worth paying attention: * [V781](https://pvs-studio.com/en/docs/warnings/v781/) \[CWE\-20, CERT\-API00\-C\] The value of the 'i' index is checked after it was used\. Perhaps there is a mistake in program logic\. tinyxml\.h 352 * [V781](https://pvs-studio.com/en/docs/warnings/v781/) \[CWE\-20, CERT\-API00\-C\] The value of the 'start' index is checked after it was used\. Perhaps there is a mistake in program logic\. coreloader\.h 136 * [V781](https://pvs-studio.com/en/docs/warnings/v781/) \[CWE\-20, CERT\-API00\-C\] The value of the 'pathLen' index is checked after it was used\. Perhaps there is a mistake in program logic\. winworkpath\.h 50 * \.\.\.\. **Warning N5** [V700](https://pvs-studio.com/en/docs/warnings/v700/) \[CWE\-480\] Consider inspecting the 'mView \= mView \= chr\.Render\(\)\.GetView\(\)' expression\. It is odd that variable is initialized through itself\. CharacterItems\.cpp 705 ```cpp void CharacterItems::DrawFlares(float dltTime, const Matrix & toWorld) { // .... Matrix mView = mView = chr.Render().GetView(); // .... } ``` Here the _mView_ variable is assigned to itself, that's pretty weird\. There must have been a typo\. Anyway, this redundant assignment can be removed: ```cpp void CharacterItems::DrawFlares(float dltTime, const Matrix & toWorld) { // .... Matrix mView = chr.Render().GetView(); // .... } ``` **Warning N6** [V654](https://pvs-studio.com/en/docs/warnings/v654/) \[CWE\-834\] The condition 'n \>\= 0' of loop is always true\. resourceselect\.cpp 42 ```cpp typedef unsigned int dword; TResourceSelectorWindow::TResourceSelectorWindow () { string PakPath; // .... miss->EditorGetPackPath(PakPath); //.... for (dword n = PakPath.Size()-1; n >= 0; n--) { if (PakPath[n] == '\\') { PakPath.Delete(0, n+1); break; } } // .... } ``` In this case, we see a potentially infinite loop, if there is no _\\_ in the string\. The _n_ variable is of unsigned type, which means that it will always be greater than or equal to _0_\. But in fact, there will be no infinite loop, because when the _n_ variable overflows, there will be a failure\. Overflowing of an unsigned variable does not lead to undefined behavior, but accessing beyond the array boundaries will\. Actually, an [access violation](https://pvs-studio.com/en/blog/terms/0063/) is likely to occur\. Here the developer has written an algorithm, that searches for the first occurrence of _\\ _from the end\. Then the algorithm removes a substring from the beginning to this character inclusively\. The best way to fix this code is to use _rbegin_ and _rend_, but there's a nuance\. We are dealing with a self\-written string \(a _std::string_ analog with extra functionality\), that has no iterators\. Then, to fix the code, we can take a signed variable twice the size \(_int64\_t_\) and run a loop with it\. ```cpp for (int64_t n = PakPath.Size() - 1; n >= 0; n--) { if (PakPath[n] == '\\') { PakPath.Delete(0, n+1); break; } } ``` However, this will not work if you port the application to a 64\-bit system, where the _Size_ function returns a value of the _uint64\_t_ type\. In this case, using iterators would be a good way to write correct code\. For example, here's what the code in C\+\+14 would look like if the self\-written string still doesn't have iterators, but at least has functions to access the underlying buffer: ```cpp auto r_begin = std::make_reverse_iterator(PakPath.GetBuffer() + PakPath.Size()); auto r_end = std::make_reverse_iterator(PakPath.GetBuffer()); while (r_begin != r_end ) { if (*r_begin == '\\') { PakPath.Delete(0, std::distance(r_begin, r_end)); break; } ++r_begin; } ``` **Warning N7** [V611](https://pvs-studio.com/en/docs/warnings/v611/) \[CWE\-762, CERT\-MEM51\-CPP\] The memory was allocated using 'new T\[\]' operator but was released using the 'delete' operator\. Consider inspecting this code\. It's probably better to use 'delete \[\] ldName;'\. ColorAttr\.cpp 198 ```cpp void ColorAttribute::LoadFromFile (....) { DWORD slen = 0; // .... char* ldName = NEW char[slen+1]; // .... delete ldName; } ``` Here the developer has cleared the dynamically allocated memory, but in the wrong way\. Instead of calling the _operator delete\[\]_ to release previously allocated memory by the _operator new\[\]_, the developer called a simple _delete_\. Here is the fixed version: ```cpp void ColorAttribute::LoadFromFile (....) { DWORD slen = 0; // .... char* ldName = NEW char[slen+1]; // .... delete ldName[]; } ``` This error is discussed in more detail in the article written by my colleague: "[Why do arrays have to be deleted via delete\[\] in C\+\+](https://pvs-studio.com/en/blog/posts/cpp/0973/)"\. **Warning N8** [V607](https://pvs-studio.com/en/docs/warnings/v607/) Ownerless expression 'isSetPause ? 1 : 0'\. SoundsEngine\.cpp 741 ```cpp void SoundsEngine::SetPause(bool isSetPause) { //.... isSetPause ? 1 : 0; //.... } ``` The analyzer detects dead code\. I've noticed that the _SoundsEngine_ class contains the similar data member — _isPause_, but it has the _long_ type\. The code might have done a conversion of the _isSetPause_ parameter from _bool_ to _long_ type earlier, but then it was refactored\. This line is harmless and can be removed\. **Warning N9** [V583](https://pvs-studio.com/en/docs/warnings/v583/) \[CWE\-783\] The '?:' operator, regardless of its conditional expression, always returns one and the same value: 0xffff0000\. NoFatality\.cpp 63 ```cpp void _cdecl NoFatality::Draw(float dltTime, long level) { //.... for (....) { Render().DrawBox(...., IsActive() ? 0xffff0000 : 0xffff0000); } //.... } ``` As we can see, the second and the third operands of the conditional operator are identical\. So, the second argument of the _DrawBox_ function is always the _0xffff0000_ constant\. It's likely that a different value should have been passed in one of the branches, otherwise the code can be simplified\. **Warning N10** [V523](https://pvs-studio.com/en/docs/warnings/v523/) \[CWE\-691\] The 'then' statement is equivalent to the 'else' statement\. TaskViewer\.cpp 298 ```cpp void TaskViewer::SetText(const char * rawText) { // .... if (rawText[0] == '#') { // Let's replace the identifier with a real string // then str = rawText; } else { str = rawText; } // .... } ``` In this code fragment, the analyzer has warned us that the _if_ statement has identical _then_ and _else_ branches\. So, regardless the condition, the same code fragment is executed\. This may not be the behavior the developer expected\. **Warning N11** [V614](https://pvs-studio.com/en/docs/warnings/v614/) \[CWE\-457, CERT\-EXP53\-CPP\] Uninitialized variable 'color\.c' used\. Color\.h 1268 ```cpp mathinline dword mathcall Color::GetDword() const { DColor color; color.r = (byte)(r * 255.0f); color.g = (byte)(g * 255.0f); color.b = (byte)(b * 255.0f); color.a = (byte)(a * 255.0f); return color.c; } ``` Let's find out what is _DColor_: ```cpp class DColor { public: union { #ifndef _XBOX struct { ///Blue unsigned char b; ///Green unsigned char g; ///Red unsigned char r; ///Transparency unsigned char a; }; #else struct { ///Transparency unsigned char a; ///Red unsigned char r; ///Green unsigned char g; ///Blue unsigned char b; }; #endif union { ///Packed color dword c; ///Packed color dword color; }; }; ``` The four RGBA components are written byte\-by\-byte to the required data members, and then returned from the function as value of the _dword_ type \(the 4\-byte unsigned type\)\. If this code were written in the C language, it would be harmless and the developer would get the expected behavior\. In C\+\+ reading the _union_ inactive data member results in undefined behavior\. This is because C\+\+ works with objects that should start their lifetime\. Only one data member can be active at any given time\. The lifetime of the inactive data member \(the last _union_ data member\) has not started\. Some compilers allow such code to be written as a non\-standard compiler extension, and in most cases this code works as expected\. However, please note that this code is non\-portable to other compilers\. This code may work correctly on the current compiler and platform, but if we change a tiny little thing, it may break\. So, how do we write code in C\+\+ that would work the same way always and everywhere? There are several ways to do it\. We can keep the four RGBA components in four data members and \(if necessary\) get their combination as a _dword_ using bitwise operators: ```cpp class DColor { public: ///Blue uint8_t b; ///Green uint8_t g; ///Red uint8_t r; ///Transparency uint8_t a; dword getAsDword() const noexcept { #ifndef _XBOX return (static_cast(b) << 0) | (static_cast(g) << 8) | (static_cast(r) << 16) | (static_cast(a) << 24); #else return (static_cast(a) << 0) | (static_cast(r) << 8) | (static_cast(g) << 16) | (static_cast(b) << 24); #endif } }; ``` There is another way\. We can use a 4\-byte array that can be converted to a _dword_ with the _memcpy_ function or _std::bit\_cast_ since C\+\+20: ```cpp class DColor { private: uint8_t m_components[4]; public: DColor(uint8_t r, uint8_t g, uint8_t b, uint8_t a) noexcept; DColor(dword comp) noexcept; // To construct from dword DColor& operator=(dword) noexcept; // To be assigned to dword uint8_t& R() noexcept; uint8_t& G() noexcept; uint8_t& B() noexcept; uint8_t& A() noexcept; dword getAsDword() const noexcept { #if __cplusplus >= 202002L && __has_cpp_attribute(__cpp_lib_bit_cast) return std::bit_cast(m_components); #else dword result; memcpy(&result, m_components, sizeof(m_components)); return result; #endif } }; ``` **My colleague, Andrey Karpov, made a comment on this\.** The use of conditional compilation directives makes the code less comprehensible\. The nesting is broken and then we have to guess what the code is about by going through various code fragments in our mind\. We need to write code not to make it as short as possible, but to make it easier to read and understand\. I would make two different implementations of the function here: ```cpp #if __cplusplus >= 202002L && __has_cpp_attribute(__cpp_lib_bit_cast) dword getAsDword() const noexcept { return std::bit_cast(m_components); } #else dword getAsDword() const noexcept { dword result; memcpy(&result, m_components, sizeof(m_components)); return result; } #endif ``` The code is a little longer, but it's much more readable that way\. If we are dealing with long functions, this will not work with conditional compilation fragments\. In this case, we can enclose these conditional compilation fragments in separate functions, which are implemented differently in different cases\. It means that we need to separate the general code from the compilation conditions\. **Warning N12** [V579](https://pvs-studio.com/en/docs/warnings/v579/) \[CWE\-687, CERT\-ARR01\-C\] The memset function receives the pointer and its size as arguments\. It is possibly a mistake\. Inspect the third argument\. Debugger\.cpp 282 ```cpp void appDebuger::debuggerMakeThreadDump(....) { CONTEXT ct; memset (&ct, 0, sizeof(&ct)); // .... } typedef struct DECLSPEC_NOINITALL _CONTEXT { DWORD ContextFlags; // .... DWORD SegCs; // MUST BE SANITIZED DWORD EFlags; // MUST BE SANITIZED DWORD Esp; DWORD SegSs; BYTE ExtendedRegisters[MAXIMUM_SUPPORTED_EXTENSION]; } CONTEXT; ``` The developer intended to initialize all structure data members with zeros\. The size of the _ct_ object is _716_ bytes, and they passed the size of the pointer – _4_ bytes\. We can fix the _memset_ call this way: ```cpp memset (&ct, 0, sizeof(ct)); ``` P\.S\. Oddly enough, developers are still using this structure initialization pattern with the _memset_ function, although they could write this way: ```cpp CONTEXT ct {}; ``` **Warning N13** I would like to finish listing the errors with a few amusing typos: * [V501](https://pvs-studio.com/en/docs/warnings/v501/) \[CWE\-571\] There are identical sub\-expressions 'lleg\.to \>\= 0' to the left and to the right of the '&&' operator\. CharacterLegs\.cpp 51 * [V501](https://pvs-studio.com/en/docs/warnings/v501/) \[CWE\-571\] There are identical sub\-expressions 'lleg\.fo \>\= 0' to the left and to the right of the '&&' operator\. CharacterLegs\.cpp 51 ```cpp void CharacterLegs::Invalidate() { lleg.th = ani->FindBone("Bedro_left_joint" ,true); lleg.kn = ani->FindBone("Golen_left_joint" ,true); lleg.fo = ani->FindBone("Foolt_left_joint" ,true); lleg.to = ani->FindBone("Foolt_left_joint_2",true); rleg.th = ani->FindBone("Bedro_right_joint" ,true); rleg.kn = ani->FindBone("Golen_right_joint" ,true); rleg.fo = ani->FindBone( "Foot_right_joint" ,true); rleg.to = ani->FindBone( "Foot_right_joint_2",true); if ( lleg.th >= 0 && lleg.kn >= 0 && lleg.fo >= 0 && lleg.to >= 0 && rleg.th >= 0 && rleg.kn >= 0 && lleg.fo >= 0 && lleg.to >= 0 ) { // .... } // .... } ``` Here the analyzer found identical subexpressions in the condition of the _if_ statement — _lleg\.to \>\= 0_ and _lleg\.fo \>\= 0_\. As you can see, the developer intended to check all the data members but made a typo\. Instead of two identical subexpressions there must be checks for other variables: ```cpp if ( lleg.th >= 0 && lleg.kn >= 0 && rleg.th >= 0 && rleg.kn >= 0 && lleg.fo >= 0 && lleg.to >= 0 && rleg.fo >= 0 && rleg.to >= 0) { // .... } ``` ## Conclusion The analyzer found various kinds of errors in the project\. I have described the Captain Blood's thirteen in this article, yo\-ho\-ho\! You can find other bugs with the PVS\-Studio analyzer as well\. That's a good incentive to give the analyzer a [try](https://pvs-studio.com/en/pvs-studio/try-free/) \(if you have not yet\)\. And it would be even more compelling to see what you can find in your own code or that of your teammates :\) I'd like to thank people who made it possible to finally see the game, as well as those who developed it and supported the game with reviews, articles and discussions\. Well, whether the "Arabella" would fo down or not, let the readers decide for themselves\!