# PVS\-Studio Looked into the Red Dead Redemption's Bullet Engine Nowadays there is no need to implement the physics of objects from scratch for game development because there are a lot of libraries for this purpose\. Bullet was actively used in many AAA games, virtual reality projects, various simulations and machine learning\. And it is still used, being, for example, one of the Red Dead Redemption and Red Dead Redemption 2 engines\. So why not check the Bullet with PVS\-Studio to see what errors static analysis can detect in such a large\-scale physics simulation project\. ![0647_Bullet3D_physics/image1.png](https://import.viva64.com/docx/blog/0647_Bullet3D_physics/image1.png) This library is [freely distributed](https://github.com/bulletphysics), so everyone can use it in their own projects if they wish\. In addition to Red Dead Redemption, this physics engine is also used in the film industry to create special effects\. For example, it was used in the shooting of Guy Ritchie's "Sherlock Holmes" to calculate collisions\. If this is the first time you meet an article where PVS\-Studio checks projects, I will make a small digression\. [PVS\-Studio](https://pvs-studio.com/en/pvs-studio/) is a static code analyzer that helps you find errors, defects and potential vulnerabilities in the source code of C, C\+\+, C\#, Java programs\. Static analysis is a kind of automated code review process\. ## Warm\-up **Example 1:** Let's start with a funny mistake: [V624](https://pvs-studio.com/en/docs/warnings/v624/) There is probably a misprint in '3\.141592538' constant\. Consider using the M\_PI constant from \. PhysicsClientC\_API\.cpp 4109 ```cpp B3_SHARED_API void b3ComputeProjectionMatrixFOV(float fov, ....) { float yScale = 1.0 / tan((3.141592538 / 180.0) * fov / 2); .... } ``` A small typo in the Pi value \(3,141592653\.\.\.\)\. The 7th digit in the fractional part is missing \- it must be equal to 6\. ![0647_Bullet3D_physics/image2.png](https://import.viva64.com/docx/blog/0647_Bullet3D_physics/image2.png) Perhaps, an error in the ten millionth fraction after the decimal point will not lead to any significant consequences, but still you should use the already existing library constants which have no typos\. There is an _M\_PI_ constant for the Pi number from the _math\.h_ header\. ## Copy\-paste **Example 2:** Sometimes the analyzer allows you to find the error indirectly\. For example, three related arguments halfExtentsX, halfExtentsY, halfExtentsZ are passed into the function here but the latter is not used anywhere in the function\. You may notice that the halfExtentsY variable is used twice when calling the _addVertex_ method\. So maybe it is a copypaste error and the forgotten argument should be used here\. [V751](https://pvs-studio.com/en/docs/warnings/v751/) Parameter 'halfExtentsZ' is not used inside function body\. TinyRenderer\.cpp 375 ```cpp void TinyRenderObjectData::createCube(float halfExtentsX, float halfExtentsY, float halfExtentsZ, ....) { .... m_model->addVertex(halfExtentsX * cube_vertices_textured[i * 9], halfExtentsY * cube_vertices_textured[i * 9 + 1], halfExtentsY * cube_vertices_textured[i * 9 + 2], cube_vertices_textured[i * 9 + 4], ....); .... } ``` **Example 3:** The analyzer has also detected the following interesting fragment and I will show it first in the initial form\. ![0647_Bullet3D_physics/image3.png](https://import.viva64.com/docx/blog/0647_Bullet3D_physics/image3.png) See this looooooooooong line? ![0647_Bullet3D_physics/image4.png](https://import.viva64.com/docx/blog/0647_Bullet3D_physics/image4.png) It is very strange that the programmer decided to write such a long condition into one line\. But it is not surprising that an error has most likely slipped into it\. The analyzer generated the following warnings on this line\. [V501](https://pvs-studio.com/en/docs/warnings/v501/) There are identical sub\-expressions 'rotmat\.Column1\(\)\.Norm\(\) < 1\.0001' to the left and to the right of the '&&' operator\. LinearR4\.cpp 351 [V501](https://pvs-studio.com/en/docs/warnings/v501/) There are identical sub\-expressions '0\.9999 < rotmat\.Column1\(\)\.Norm\(\)' to the left and to the right of the '&&' operator\. LinearR4\.cpp 351 If we write it all down in a clear "tabular" form, we can see that all the same checks apply to _Column1_\. The last two comparisons show that there are _Column1_ and _Column2_\. Most likely, the third and fourth comparisons should have checked the value of _Column2_\. ```cpp Column1().Norm() < 1.0001 && 0.9999 < Column1().Norm() && Column1().Norm() < 1.0001 && 0.9999 < Column1().Norm() &&(Column1() ^ Column2()) < 0.001 && (Column1() ^ Column2()) > -0.001 ``` In this form, the same comparisons become much more noticeable\. **Example 4:** Error of the same kind: [V501](https://pvs-studio.com/en/docs/warnings/v501/) There are identical sub\-expressions 'cs\.m\_fJacCoeffInv\[0\] \=\= 0' to the left and to the right of the '&&' operator\. b3CpuRigidBodyPipeline\.cpp 169 ```cpp float m_fJacCoeffInv[2]; static inline void b3SolveFriction(b3ContactConstraint4& cs, ....) { if (cs.m_fJacCoeffInv[0] == 0 && cs.m_fJacCoeffInv[0] == 0) { return; } .... } ``` In this case, one and the same array element is checked twice\. Most likely, the condition must have looked like this: _cs\.m\_fJacCoeffInv\[0\] \=\= 0 && cs\.m\_fJacCoeffInv\[1\] \=\= 0_\. This is a classic example of a copy\-paste error\. **Example 5:** It was also discovered that there was such a defect: [V517](https://pvs-studio.com/en/docs/warnings/v517/) The use of 'if \(A\) \{\.\.\.\} else if \(A\) \{\.\.\.\}' pattern was detected\. There is a probability of logical error presence\. Check lines: 79, 112\. main\.cpp 79 ```cpp int main(int argc, char* argv[]) { .... while (serviceResult > 0) { serviceResult = enet_host_service(client, &event, 0); if (serviceResult > 0) { .... } else if (serviceResult > 0) { puts("Error with servicing the client"); exit(EXIT_FAILURE); } .... } .... } ``` The function _enet\_host\_service_, the result of which is assigned to _serviceResult_, returns 1 in case of successful completion and \-1 in case of failure\. Most likely, the _else if_ branch should have reacted to the negative value of _serviceResult_, but the check condition was duplicated\. Probably it is also a copy\-paste error\. There is a similar warning of the analyzer, but there is no point in looking at it more closely in this article\. [V517](https://pvs-studio.com/en/docs/warnings/v517/) The use of 'if \(A\) \{\.\.\.\} else if \(A\) \{\.\.\.\}' pattern was detected\. There is a probability of logical error presence\. Check lines: 151, 190\. PhysicsClientUDP\.cpp 151 ## Over the top: exceeding array boundaries **Example 6:** One of the unpleasant errors to search for is the array overrun\. This error often occurs because of a complex indexing in a loop\. Here, in the loop condition, the _dofIndex_ variable''s upper limit is 128 and _dof''s_ is 4 inclusive\. But _m\_desiredState_ also contains only 128 items\. As a result, the _\[dofIndex\+dof\]_ index may cause an array overrun\. [V557](https://pvs-studio.com/en/docs/warnings/v557/) Array overrun is possible\. The value of 'dofIndex \+ dof' index could reach 130\. PhysicsClientC\_API\.cpp 968 ```cpp #define MAX_DEGREE_OF_FREEDOM 128 double m_desiredState[MAX_DEGREE_OF_FREEDOM]; B3_SHARED_API int b3JointControl(int dofIndex, double* forces, int dofCount, ....) { .... if ( (dofIndex >= 0) && (dofIndex < MAX_DEGREE_OF_FREEDOM ) && dofCount >= 0 && dofCount <= 4) { for (int dof = 0; dof < dofCount; dof++) { command->m_sendState.m_desiredState[dofIndex+dof] = forces[dof]; .... } } .... } ``` **Example 7:** A similar error but now it is caused by summing up not when indexing an array but in a condition\. If the file has a name with maximal length, the terminal zero will be written outside the array \([Off\-by\-one Error](https://cwe.mitre.org/data/definitions/193.html)\)\. Of course, the _len_ variable will be equal to _MAX\_FILENAME\_LENGTH_ only in exceptional cases, but it doesn't eliminate the error but simply makes it rare\. [V557](https://pvs-studio.com/en/docs/warnings/v557/) Array overrun is possible\. The value of 'len' index could reach 1024\. PhysicsClientC\_API\.cpp 5223 ```cpp #define MAX_FILENAME_LENGTH MAX_URDF_FILENAME_LENGTH 1024 struct b3Profile { char m_name[MAX_FILENAME_LENGTH]; int m_durationInMicroSeconds; }; int len = strlen(name); if (len >= 0 && len < (MAX_FILENAME_LENGTH + 1)) { command->m_type = CMD_PROFILE_TIMING; strcpy(command->m_profile.m_name, name); command->m_profile.m_name[len] = 0; } ``` ## Measure it once, cut it seven times **Example 8:** In cases when you need to use the result of some function's work many times or use a variable which requires to pass through the whole chain of calls to get access to , you should use temporary variables for optimization and better code readability\. The analyzer has found more than 100 places in the code where you can make such a correction\. [V807](https://pvs-studio.com/en/docs/warnings/v807/) Decreased performance\. Consider creating a pointer to avoid using the 'm\_app\-\>m\_renderer\-\>getActiveCamera\(\)' expression repeatedly\. InverseKinematicsExample\.cpp 315 ```cpp virtual void resetCamera() { .... if (....) { m_app->m_renderer->getActiveCamera()->setCameraDistance(dist); m_app->m_renderer->getActiveCamera()->setCameraPitch(pitch); m_app->m_renderer->getActiveCamera()->setCameraYaw(yaw); m_app->m_renderer->getActiveCamera()->setCameraPosition(....); } } ``` The same call chain is used many times here and can be replaced with a single pointer\. **Example 9:** [V810](https://pvs-studio.com/en/docs/warnings/v810/) Decreased performance\. The 'btCos\(euler\_out\.pitch\)' function was called several times with identical arguments\. The result should possibly be saved to a temporary variable, which then could be used while calling the 'btAtan2' function\. btMatrix3x3\.h 576 [V810](https://pvs-studio.com/en/docs/warnings/v810/) Decreased performance\. The 'btCos\(euler\_out2\.pitch\)' function was called several times with identical arguments\. The result should possibly be saved to a temporary variable, which then could be used while calling the 'btAtan2' function\. btMatrix3x3\.h 578 ```cpp void getEulerZYX(....) const { .... if (....) { .... } else { .... euler_out.roll = btAtan2(m_el[2].y() / btCos(euler_out.pitch), m_el[2].z() / btCos(euler_out.pitch)); euler_out2.roll = btAtan2(m_el[2].y() / btCos(euler_out2.pitch), m_el[2].z() / btCos(euler_out2.pitch)); euler_out.yaw = btAtan2(m_el[1].x() / btCos(euler_out.pitch), m_el[0].x() / btCos(euler_out.pitch)); euler_out2.yaw = btAtan2(m_el[1].x() / btCos(euler_out2.pitch), m_el[0].x() / btCos(euler_out2.pitch)); } .... } ``` In this case, you can create two variables and save the values returned by the _btCos_ function for _euler\_out\.pitch_ and _euler\_out2\.pitch_ into them instead of calling the function four times for each argument\. ## Leak **Example 10:** A lot of errors of the following kind were detected in the project: [V773](https://pvs-studio.com/en/docs/warnings/v773/) Visibility scope of the 'importer' pointer was exited without releasing the memory\. A memory leak is possible\. SerializeSetup\.cpp 94 ```cpp void SerializeSetup::initPhysics() { .... btBulletWorldImporter* importer = new btBulletWorldImporter(m_dynamicsWorld); .... fclose(file); m_guiHelper->autogenerateGraphicsObjects(m_dynamicsWorld); } ``` Memory has not been released from the _importer_ pointer here\. This may result in a memory leak\. And for the physical engine it may be a bad trend\. To avoid a leak, it is enough to add _delete importer_ after the variable becomes unnecessary\. But, of course, it is better to use smart pointers\. ## C\+\+ lives by its own code **Example 11:** The next error appears in the code because C\+\+ rules do not always coincide with mathematical rules or "common sense"\. Will you notice where this small code fragment contains an error? ```cpp btAlignedObjectArray m_fractureBodies; void btFractureDynamicsWorld::fractureCallback() { for (int i = 0; i < numManifolds; i++) { .... int f0 = m_fractureBodies.findLinearSearch(....); int f1 = m_fractureBodies.findLinearSearch(....); if (f0 == f1 == m_fractureBodies.size()) continue; .... } .... } ``` The analyzer generates the following warning: [V709](https://pvs-studio.com/en/docs/warnings/v709/) Suspicious comparison found: 'f0 \=\= f1 \=\= m\_fractureBodies\.size\(\)'\. Remember that 'a \=\= b \=\= c' is not equal to 'a \=\= b && b \=\= c'\. btFractureDynamicsWorld\.cpp 483 It would appear that the condition checks that _f0_ is equal to _f1_ and is equal to the number of items in _m\_fractureBodies_\. It seems that this comparison should have checked whether _f0_ and _f1_ are located at the end of the _m\_fractureBodies_ array, since they contain the object position found by the _findLinearSearch\(\)_ method\. But in fact, this expression turns into a check to see if _f0_ and _f1_ are equal to _m\_fractureBodies\.size\(\)_ and then a check to see if _m\_fractureBodies\.size\(\)_ is equal to the result _f0 \=\= f1_\. As a result, the third operand here is compared to 0 or 1\. Beautiful mistake\! And, fortunately, quite rare\. So far, we have only [met](https://pvs-studio.com/en/blog/examples/v709/) it in two open source projects, and it is interesting that all of them were game engines\. **Example 12:** When working with strings, it is often better to use the features provided by the _string_ class\. So, for the next two cases it is better to replace _strlen\(MyStr\.c\_str\(\)\) and val \= ""_ with _MyStr\.length\(\) _and_ val\.clear\(\)_, respectively\. [V806](https://pvs-studio.com/en/docs/warnings/v806/) Decreased performance\. The expression of strlen\(MyStr\.c\_str\(\)\) kind can be rewritten as MyStr\.length\(\)\. RobotLoggingUtil\.cpp 213 ```cpp FILE* createMinitaurLogFile(const char* fileName, std::string& structTypes, ....) { FILE* f = fopen(fileName, "wb"); if (f) { .... fwrite(structTypes.c_str(), strlen(structTypes.c_str()), 1, f); .... } .... } ``` [V815](https://pvs-studio.com/en/docs/warnings/v815/) Decreased performance\. Consider replacing the expression 'val \= ""' with 'val\.clear\(\)'\. b3CommandLineArgs\.h 40 ```cpp void addArgs(int argc, char **argv) { .... std::string val; .... val = ""; .... } ``` There were other warnings, but I think we can stop here\. As you see, static code analysis can detect a wide range of various errors\. It is interesting to read about one\-time project checks, but it is not the proper way to use static code analyzers\. And we will speak about it below\. ## Errors found before us It was interesting to try to find bugs or defects which have already been fixed but which a static analyzer could detect in the light of the recent article "[Errors which are not found by static code analysis because it is not being used](https://habr.com/en/company/pvs-studio/blog/460121/)"\. ![0647_Bullet3D_physics/image5.png](https://import.viva64.com/docx/blog/0647_Bullet3D_physics/image5.png) There were not many pull requests in the repository and many of them are related to the internal logic of the engine\. But there were also errors that the analyzer could detect\. **Example 13:** ```cpp char m_deviceExtensions[B3_MAX_STRING_LENGTH]; void b3OpenCLUtils_printDeviceInfo(cl_device_id device) { b3OpenCLDeviceInfo info; b3OpenCLUtils::getDeviceInfo(device, &info); .... if (info.m_deviceExtensions != 0) { .... } } ``` The comment for the request says that you needed to check the array for the fact that it was not empty, but instead a meaningless pointer check was performed, which always returned true\. This is what PVS\-Studio's warning about the original check tells you: [V600](https://pvs-studio.com/en/docs/warnings/v600/) Consider inspecting the condition\. The 'info\.m\_deviceExtensions' pointer is always not equal to NULL\. b3OpenCLUtils\.cpp 551 **Example 14:** Can you find out what the problem is with the next function? ```cpp inline void Matrix4x4::SetIdentity() { m12 = m13 = m14 = m21 = m23 = m24 = m13 = m23 = m41 = m42 = m43 = 0.0; m11 = m22 = m33 = m44 = 1.0; ``` \} The analyzer generates the following warnings: [V570](https://pvs-studio.com/en/docs/warnings/v570/) The same value is assigned twice to the 'm23' variable\. LinearR4\.h 627 [V570](https://pvs-studio.com/en/docs/warnings/v570/) The same value is assigned twice to the 'm13' variable\. LinearR4\.h 627 Repeated assignments in this form of recording are difficult to track with the naked eye and, as a result, some of the matrix elements did not get the initial value\. This error was corrected by the tabular form of assignment recording: ```cpp m12 = m13 = m14 = m21 = m23 = m24 = m31 = m32 = m34 = m41 = m42 = m43 = 0.0; ``` **Example 15:** The following error in one of the conditions of the _btSoftBody function::addAeroForceToNode\(\)_ led to an evident bug\. According to the comment in the pull request, the forces were applied to the objects from the wrong side\. ```cpp struct eAeroModel { enum _ { V_Point, V_TwoSided, .... END }; }; void btSoftBody::addAeroForceToNode(....) { .... if (....) { if (btSoftBody::eAeroModel::V_TwoSided) { .... } .... } .... } ``` PVS\-Studio could also find this error and generate the following warning: [V768](https://pvs-studio.com/en/docs/warnings/v768/) The enumeration constant 'V\_TwoSided' is used as a variable of a Boolean\-type\. btSoftBody\.cpp 542 Fixed check looks like this: ```cpp if (m_cfg.aeromodel == btSoftBody::eAeroModel::V_TwoSided) { .... } ``` Instead of equivalence of an object's property to one of the enumerators, the _V\_TwoSided_ enumerator itself was checked\. It's clear that I didn't look at all the pull\-requests, because that wasn't the point\. I just wanted to show you that regular use of a static code analyzer can detect errors at the very early stage\. This is the right way to use static code analysis\. Static analysis must be built into the DevOps process and be the primary bug filter\. All this is well described in the article "[Introduce Static Analysis in the Process, Don't Just Search for Bugs with It](https://habr.com/en/post/440610/)"\. ## Conclusion ![0647_Bullet3D_physics/image6.png](https://import.viva64.com/docx/blog/0647_Bullet3D_physics/image6.png) Judging by some pull\-requests, a project is sometimes checked through various code analysis tools but corrections are made not gradually but in groups and with large intervals\. In some requests, the comment indicates that the changes were made only to suppress warnings\. This approach to using analysis significantly reduces its usefulness because it is the regular checks of the project that allow you to correct errors right away rather than waiting for any explicit bugs to appear\. Follow us and subscribe to our social media accounts and channels: [Twitter](https://twitter.com/pvs_studio), [Telegram](https://t.me/pvsstudio_en)\. We'd love to be with you wherever you are and to keep you on the loop\.