PVS-Studio glanced at Red Dead Redemption engine - Bullet

Nowadays, for, for example, developing games, there is no longer any need to independently implement the physics of objects from scratch, since there are a large number of libraries for this. Bullet was once actively used in many AAA games, virtual reality projects, various simulations and machine learning. Yes, and it is still used today, being, for example, one of the engines of Red Dead Redemption and Red Dead Redemption 2. So why not check Bullet with PVS-Studio to see what errors static analysis can detect in such a large-scale project, related to physics simulation.

This library is freely distributed , so that everyone can optionally use it in their projects. In addition to Red Dead Redemption, this physics engine is also used in the film industry to create special effects. For example, he was involved in the filming of Sherlock Holmes by Guy Ritchie to calculate collisions.

If this is your first encounter with an article where PVS-Studio checks projects, then I will make a small digression. PVS-Studio is a static code analyzer that helps find errors, shortcomings and potential vulnerabilities in the source code of C, C ++, C #, Java programs. Static analysis is a kind of automated code review process.

For warming up

Example 1:
')
Let's start with a funny mistake:

V624 There is probably a misprint in '3.141592538' constant. Consider using the M_PI constant from <math.h>. PhysicsClientC_API.cpp 4109

B3_SHARED_API void b3ComputeProjectionMatrixFOV(float fov, ....) { float yScale = 1.0 / tan((3.141592538 / 180.0) * fov / 2); .... } 

A small typo in the number Pi (3.141592653 ...), missing the number "6" at the 7th position in the fractional part.

Perhaps the error in the ten-millionth fraction after the decimal point will not lead to tangible consequences, but you should still use the existing library constants without typos. For Pi, there is an M_PI constant from the math.h header.

Copy paste

Example 2:

Sometimes the analyzer allows you to find the error indirectly. So, for example, here three related arguments halfExtentsX, halfExtentsY, halfExtentsZ are passed to the function, but the latter is not used anywhere in the function. You may notice that when calling the addVertex method, the variable halfExtentsY is used twice. So perhaps this is a copy-paste error, and here a forgotten argument should be used.

V751 Parameter 'halfExtentsZ' is not used inside function body. TinyRenderer.cpp 375

 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 also found the following interesting fragment, I will cite it first in its original form.


See this last line?


It is very strange that the programmer decided to write such a long condition in one line. But the fact that a mistake most likely crept into it is not at all surprising.

The analyzer issued the following warnings on this line.

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 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 everything in a visual “tabular” form, it will become clear that 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 .

  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 comparison becomes much more noticeable.

Example 4:

An error of a similar kind:

V501 There are identical sub-expressions 'cs.m_fJacCoeffInv [0] == 0' to the left and to the right of the '&&' operator. b3CpuRigidBodyPipeline.cpp 169

 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, the same array element is double checked. Most likely, the condition should have looked like this: cs.m_fJacCoeffInv [0] == 0 && cs.m_fJacCoeffInv [1] == 0 . This is a classic example of copy-paste error.

Example 5:

Another shortcoming was discovered:

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

 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 enet_host_service function, whose result is assigned to serviceResult , returns one if it succeeds and -1 if it fails. Most likely the else if branch should have responded to the negative value of serviceResult , but the verification condition was duplicated. Most likely this is also a copy-paste error.

A similar analyzer warning, which does not make sense to describe in the article:

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

Beyond the limits: going beyond the boundaries of the array

Example 6:

One of the unpleasant things to look for errors is going out of the array. Often this error occurs due to complex indexing in the loop.

Here, in the loop condition, the dofIndex variable is limited to 128 from above and dof to 4, inclusive. But m_desiredState also contains only 128 elements. As a result, the index [dofIndex + dof] can lead to the outflow of the array.

V557 Array overrun is possible. The value of 'dofIndex + dof' index could reach 130. PhysicsClientC_API.cpp 968

 #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 summation leads to it not when indexing an array, but in a condition. If the file name is as long as possible, then the terminal zero will be written outside the array ( Off-by-one Error ). Naturally, the len variable will be equal to MAX_FILENAME_LENGTH only in exceptional cases, but this does not eliminate the error, but simply makes its occurrence rare.

V557 Array overrun is possible. The value of 'len' index could reach 1024. PhysicsClientC_API.cpp 5223

 #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 once - cut seven times

Example 8:

In cases where you need to repeatedly use the result of a function or reuse a variable that you need to access through a series of calls, you should use temporary variables to optimize and read the code better. The analyzer found more than 100 places in the code where such an amendment can be made.

V807 Decreased Performance. Consider creating a pointer to avoid using the 'm_app-> m_renderer-> getActiveCamera ()' expression repeatedly. InverseKinematicsExample.cpp 315

 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(....); } } 

Here the same call chain is reused, which can be replaced with a single pointer.

Example 9:

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 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

 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 store the values ​​returned by the btCos function for euler_out.pitch and euler_out2.pitch in them , instead of calling the function four times for each argument.

A leak

Example 10:

A lot of errors of the following type were found in the project:

V773 Visibility scope of the 'importer' pointer was exited without releasing the memory. A memory leak is possible. SerializeSetup.cpp 94

 void SerializeSetup::initPhysics() { .... btBulletWorldImporter* importer = new btBulletWorldImporter(m_dynamicsWorld); .... fclose(file); m_guiHelper->autogenerateGraphicsObjects(m_dynamicsWorld); } 

No memory has been freed from the importer pointer here. This may cause a memory leak. And for the physical engine, this may be a bad trend. To avoid leakage, it is enough after the variable is no longer needed to add delete importer . But of course, it’s better to use smart pointers.

Here are your laws

Example 11:

The following error appears in the code due to the fact that C ++ rules do not always coincide with mathematical rules or “common sense”. Notice for yourself where a small code snippet contains an error?

 btAlignedObjectArray<btFractureBody*> 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 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 seem that the condition checks that f0 is equal to f1 and equal to the number of elements in m_fractureBodies . It looks like this comparison should have checked if f0 and f1 are at the end of the m_fractureBodies array, since they contain the position of the object found by the findLinearSearch () method. However, in fact, this expression turns into a test to see if f0 and f1 are equal, and then to a check if m_fractureBodies.size () is equal to the result of f0 == f1 . As a result, the third operand here is compared with 0 or 1.

Beautiful mistake! And, fortunately, quite rare. So far we have met her only in two open projects, and interestingly, all of them were just game engines.

Example 12:

When working with strings, it is often better to use the facilities provided by the string class. So, for the following two cases, it is better to replace strlen (MyStr.c_str ()) and val = "" with MyStr.length () and val.clear () respectively.

V806 Decreased Performance. The expression of strlen (MyStr.c_str ()) kind can be rewritten as MyStr.length (). RobotLoggingUtil.cpp 213

 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 Decreased Performance. Consider replacing the expression 'val = ""' with 'val.clear ()'. b3CommandLineArgs.h 40

 void addArgs(int argc, char **argv) { .... std::string val; .... val = ""; .... } 

There were other warnings, but I think you can stop. As you can see, static code analysis can reveal a wide range of various errors.

It is interesting to read about one-time checks of projects, but this is not the right way to use static code analyzers. And about this we will talk below.

Bugs Found Before Us

It was interesting in the spirit of the recent article “ Errors that static code analysis does not find because it is not used ” to try to find bugs or shortcomings that have already been fixed, but which the static analyzer would be able to detect.

There were not so 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:

 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) { .... } } 

A comment on the edit suggests that it was necessary to check the array that it is not empty, but instead a pointless pointer check was performed, which always returned true. This is indicated by the warning PVS-Studio on the initial type of check:

V600 Consider inspecting the condition. The 'info.m_deviceExtensions' pointer is always not equal to NULL. b3OpenCLUtils.cpp 551

Example 14:

Can you immediately find out what the problem is in the next function?

 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 The same value is assigned twice to the 'm23' variable. LinearR4.h 627

V570 The same value is assigned twice to the 'm13' variable. LinearR4.h 627

Repeated assignments with this form of recording are difficult to track with the naked eye and as a result, some of the matrix elements did not receive the initial value. This error was corrected in the tabular form of the assignment record:

 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 :: addAeroForceToNode () function led to an explicit bug. According to the comment in the pull request, forces were applied to objects from the wrong side.

 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 display the following warning:

V768 The enumeration constant 'V_TwoSided' is used as a variable of a Boolean-type. btSoftBody.cpp 542

The corrected check is as follows:

 if (m_cfg.aeromodel == btSoftBody::eAeroModel::V_TwoSided) { .... } 

Instead of the equivalence of the object 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, since I didn’t set myself such a task. I just wanted to show that regular use of a static code analyzer can detect errors at an early stage. This is the correct way to use static code analysis. Static analysis should be built into the DevOps process and be the primary filter of bugs. All this is well described in the article " Embed static analysis into the process, and not look for bugs with it ."

Conclusion

Judging by some pull requests, a project is sometimes checked through various code analysis tools, however, edits are made not gradually, but in groups and at large intervals in time. In some requests, a comment indicates that the changes were made only to suppress warnings. Such an approach to the use of analysis significantly reduces its benefit, since it is a constant check of the project that allows you to fix errors immediately, and not wait until any obvious bugs appear.

If you want to always be in the know about the news and events of our team, subscribe to our social services. Networks: Instagram , Twitter , Vkontakte , Telegram .



If you want to share this article with an English-speaking audience, then please use the translation link: PVS-Studio Looked into the Red Dead Redemption's Bullet Engine