{"id":381104,"date":"2024-06-29T03:28:53","date_gmt":"2024-06-29T03:28:53","guid":{"rendered":"http:\/\/savepearlharbor.com\/?p=381104"},"modified":"-0001-11-30T00:00:00","modified_gmt":"-0001-11-29T21:00:00","slug":"","status":"publish","type":"post","link":"https:\/\/savepearlharbor.com\/?p=381104","title":{"rendered":"Yo, Ho, Ho, And a Bottle of Rum \u2014 Or How We Analyzed Storm Engine’s Bugs<\/span>"},"content":{"rendered":"
<\/div>\n
\n
\n
\n
\n

PVS-Studio is a static analysis tool that helps find errors in software source code. This time PVS-Studio looked for bugs in Storm Engine’s source code.<\/p>\n

<\/figcaption><\/figure>\n

Storm Engine<\/h3>\n

Storm Engine is a gaming engine that Akella has been developing since January 2000, for the Sea Dogs game series. The game engine became open-source on March 26th, 2021. The source code is available on GitHub<\/a> under the GPLv3 license. Storm Engine is written in C++.<\/p>\n

In total, PVS-Studio issued 235 high-level warnings and 794 medium-level warnings. Many of these warnings point to bugs that may cause undefined behavior. Other warnings reveal logical errors — the program runs well, but the execution’s result may be not what’s expected.<\/p>\n

Examining each of the 1029 errors PVS-Studio discovered — especially those that involve the project’s architecture — would take up an entire book that is difficult to write and read. In this article, I’ll review more obvious and on-the-surface-type errors that do not require delving deep into the project’s source code.<\/p>\n

Detected Errors<\/h3>\n

Redundant Checks<\/h4>\n

PVS-Studio warns: V547<\/a> Expression ‘nStringCode >= 0xffffff’ is always false. dstring_codec. h 84<\/p>\n

#define DHASH_SINGLESYM 255 .... uint32_t Convert(const char *pString, ....) {   uint32_t nStringCode;   ....   nStringCode = ((((unsigned char)pString[0]) << 8) & 0xffffff00) |                   (DHASH_SINGLESYM)   ....   if (nStringCode >= 0xffffff)   {     __debugbreak();   }   return nStringCode; }<\/code><\/pre>\n
Let’s evaluate the expression that the nStringCode<\/em> variable contains. The unsigned<\/em> char<\/em> type takes values in the range of *[0,255]*. Consequently, (unsigned char)pString[0]<\/em> is always less than 2^8<\/em>. After left shifting the result by 8<\/em>, we get a number that does not exceed 2^16<\/em>. The ‘&’ operator does not augment this value. Then we increase the expression’s value by no more than 255<\/em>. As a result, the nStringCode<\/em> variable’s value never exceeds 2^16+256<\/em>, and therefore, is always less than 0xffffff = 2^24-1<\/em>. Thus, the check is always false and is of no use. At first glance, it would seem that we can safely remove it:<\/p>\n
#define DHASH_SINGLESYM 255 .... uint32_t Convert(const char *pString, ....) {   uint32_t nStringCode;   ....   nStringCode = ((((unsigned char)pString[0]) << 8) & 0xffffff00) |                 (DHASH_SINGLESYM) ....   return nStringCode; }<\/code><\/pre>\n
But let’s not rush into anything. Obviously, the check is here for a reason. The developers may have expected the expression or the DHASH_SINGLESYM<\/em> constant to change in the future. This example demonstrates a case when the analyzer is technically correct, but the code fragment that triggered the warning might not require fixing.<\/p>\n
PVS-Studio warns: V560<\/a> A part of conditional expression is always true: 0x00 <= c. utf8.h 187<\/p>\n
inline bool IsValidUtf8(....) {   int c, i, ix, n, j;   for (i = 0, ix = str.length(); i < ix; i++)s   {     c = (unsigned char)str[i];     if (0x00 <= c && c <= 0x7f)       n = 0;     ....   }   .... }<\/code><\/pre>\n
The c<\/em> variable holds an unsigned type value and the 0x00 <= c<\/em> check can be removed as unnecessary. The fixed code:<\/p>\n
inline bool IsValidUtf8(....) {   int c, i, ix, n, j;   for (i = 0, ix = str.length(); i < ix; i++)s   {     c = (unsigned char)str[i];     if (c <= 0x7f)       n = 0;     ....   }   .... }<\/code><\/pre>\n
Reaching Outside Array Bounds<\/h4>\n
PVS-Studio warns: V557<\/a> Array overrun is possible. The value of ‘TempLong2 — TempLong1 + 1’ index could reach 520. internal_functions.cpp 1131<\/p>\n
DATA *COMPILER::BC_CallIntFunction(....) {   if (TempLong2 - TempLong1 >= sizeof(Message_string))   {     SetError(\"internal: buffer too small\");     pV = SStack.Push();     pV->Set(\"\");     pVResult = pV;     return pV;   }   memcpy(Message_string, pChar + TempLong1,           TempLong2 - TempLong1 + 1);   Message_string[TempLong2 - TempLong1 + 1] = 0;   pV = SStack.Push(); }<\/code><\/pre>\n
Here the analyzer helped find the off-by-one error.<\/p>\n
The function above first makes sure that the TempLong2 — TempLong1<\/em> value is less than the Message_string<\/em> length. Then the Message_string[TempLong2 — TempLong1 + 1]<\/em> element takes the 0 value. Note that if TempLong2 — TempLong1 + 1 == sizeof(Message_string)<\/em>, the check is successful and the internal error is not generated. However, the Message_string[TempLong2 — TempLong1 + 1]<\/em> element is of bounds. When this element is assigned a value, the function accesses unreserved memory. This causes undefined behavior. You can fix the check as follows:<\/p>\n
DATA *COMPILER::BC_CallIntFunction(....) {   if (TempLong2 - TempLong1 + 1 >= sizeof(Message_string))   {     SetError(\"internal: buffer too small\");     pV = SStack.Push();     pV->Set(\"\");     pVResult = pV;     return pV;   }   memcpy(Message_string, pChar + TempLong1,           TempLong2 - TempLong1 + 1);   Message_string[TempLong2 - TempLong1 + 1] = 0;   pV = SStack.Push(); }<\/code><\/pre>\n
Assigning a Variable to Itself<\/h4>\n
PVS-Studio warns: V570<\/a> The ‘Data_num’ variable is assigned to itself. s_stack.cpp 36<\/p>\n
uint32_t Data_num; .... DATA *S_STACK::Push(....) {   if (Data_num > 1000)   {     Data_num = Data_num;   }   .... }<\/code><\/pre>\n
Someone may have written this code for debugging purposes and then forgot to remove it. Instead of a new value, the Data_num<\/em> variable receives its own value. It is difficult to say what the developer wanted to assign here. I suppose Data_num<\/em> should have received a value from a different variable with a similar name, but the names got mixed up. Alternatively, the developer may have intended to limit the Data_num<\/em> value to the 1000 constant but made a typo. In any case there’s a mistake here that needs to be fixed.<\/p>\n
Dereferencing a Null Pointer<\/h4>\n
PVS-Studio warns: V595<\/a> The ‘rs’ pointer was utilized before it was verified against nullptr. Check lines: 163, 164. Fader.cpp 163<\/p>\n
uint64_t Fader::ProcessMessage(....) {   ....   textureID = rs->TextureCreate(_name);   if (rs)   {     rs->SetProgressImage(_name);     .... }<\/code><\/pre>\n
In the code above, the rs<\/em> pointer is first dereferenced, and then evaluated against nullptr<\/em>. If the pointer equals nullptr<\/em>, the null pointer’s dereference causes undefined behavior. If this scenario is possible, it is necessary to place the check before the first dereference:<\/p>\n
uint64_t Fader::ProcessMessage(....) {   ....   if (rs)   {     textureID = rs->TextureCreate(_name);     rs->SetProgressImage(_name);     .... }<\/code><\/pre>\n
If the scenario guarantees that rs != nullptr<\/em> is always true, then you can remove the unnecessary if (rs)<\/em> check:<\/p>\n
uint64_t Fader::ProcessMessage(....) {   ....   textureID = rs->TextureCreate(_name);   rs->SetProgressImage(_name);   .... }<\/code><\/pre>\n
There’s also a third possible scenario. Someone could have intended to check the textureID<\/em> variable.<\/p>\n
Overall, I encountered 14 of the V595 warnings in the project.<\/p>\n
If you are curious, download and start PVS-Studio<\/a>, analyze the project and review these warnings. Here I’ll limit myself to one more example:<\/p>\n
PVS-Studio warns: V595<\/a> The ‘pACh’ pointer was utilized before it was verified against nullptr. Check lines: 1214, 1215. sail.cpp 1214<\/p>\n
void SAIL::SetAllSails(int groupNum) {   ....   SetSailTextures(groupNum, core.Event(\"GetSailTextureData\",                   \"l\", pACh->GetAttributeAsDword(\"index\",  -1)));   if (pACh != nullptr){   .... }<\/code><\/pre>\n
When calculating the Event<\/em> method’s arguments, the author dereferences the pACh<\/em> pointer. Then, in the next line, the pACh<\/em> pointer is checked against nullptr<\/em>. If the pointer can take the null value, the if-statement that checks pACh<\/em> for nullptr<\/em> must come before the SetSailTextures<\/em> function call that prompts pointer dereferencing.<\/p>\n
void SAIL::SetAllSails(int groupNum) {   ....   if (pACh != nullptr){     SetSailTextures(groupNum, core.Event(\"GetSailTextureData\",                      \"l\", pACh->GetAttributeAsDword(\"index\",  -1)));   .... }<\/code><\/pre>\n
If pACh<\/em> can never be null, you can remove the check:<\/p>\n
void SAIL::SetAllSails(int groupNum) {   ....   SetSailTextures(groupNum, core.Event(\"GetSailTextureData\",                    \"l\", pACh->GetAttributeAsDword(\"index\",  -1)));   .... }<\/code><\/pre>\n
new[] \u2013 delete Error<\/h4>\n
PVS-Studio warns: V611<\/a> 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 [] pVSea;’. Check lines: 169, 191. SEA.cpp 169<\/p>\n
struct CVECTOR {   public:     union {       struct       {         float x, y, z;       };       float v[3];   }; }; .... struct SeaVertex {   CVECTOR vPos;   CVECTOR vNormal;   float tu, tv; }; .... #define STORM_DELETE (x) { delete x; x = 0; }  void SEA::SFLB_CreateBuffers() {     ....     pVSea = new SeaVertex[NUM_VERTEXS]; } SEA::~SEA() { .... STORM_DELETE(pVSea); .... }<\/code><\/pre>\n
Using macros requires special care and experience. In this case a macro causes an error: the incorrect delete<\/em> operator — instead of the correct delete[]<\/em> operator — releases the memory that the new[]<\/em> operator allocated. As a result, the code won’t call destructors for the pVSea<\/em> array elements. In some cases, this won’t matter — for example, when all destructors of both array elements and their fields are trivial.<\/p>\n
However, if the error does not show up at runtime — it does not mean there isn’t one. The key here is how the new[] operator is defined. In some cases calling the new[]<\/em> operator will allocate memory for the array, and will also write the memory section’s size and the number of elements at the beginning of the memory slot. If the developer then uses the delete<\/em> operator that is incompatible with new[]<\/em>, the delete operator is likely to misinterpret the information at the beginning of the memory block, and the result of such operation will be undefined. There is another possible scenario: memory for arrays and single elements is allocated from different memory pools. In that case, attempting to return memory allocated for arrays back to the pool that was intended for scalars will result in a crash.<\/p>\n
This error is dangerous, because it may not manifest itself for a long time, and then shoot you in the foot when you least expect it. The analyzer found a total of 15 errors of this type. Here are some of them:<\/p>\n