Checking the Cocos2d-x cross-platform framework

Cocos2d is an open source software framework. It can be used to build games, applications and graphical interfaces of interactive cross-platform applications. Cocos2d contains many brunches, the famous ones are Cocos2d-iPhone, Cocos2d-x, Cocos2d-html5 and Cocos2d-XNA

This article will examine the results of the Cocos2d-x verification, a framework for C ++, obtained using PVS-Studio 5.18. The project is of sufficient quality, but still some places should be paid attention to. Source code is from GitHub .

From malloc to new, from C to C ++

Working with graphic objects, as a rule, is associated with processing arrays and matrices, memory for which is allocated dynamically. In this project, memory allocation uses both calls to the 'malloc' function and the 'new' operator. These methods have significant differences in use that must be considered when replacing them in the code. Next will be described the place, not quite correctly using 'malloc' and 'new'.
')
V630 The 'malloc' function is to allocate memory. ccmotionstreak.cpp 122

Vec2::Vec2() : x(0.0f), y(0.0f) { } Vec2::Vec2(float xx, float yy) : x(xx), y(yy) { } bool MotionStreak::initWithFade(...) { .... _pointVertexes = (Vec2*)malloc(sizeof(Vec2) * _maxPoints); _vertices = (Vec2*)malloc(sizeof(Vec2) * _maxPoints * 2); _texCoords = (Tex2F*)malloc(sizeof(Tex2F) * _maxPoints * 2); .... } 

Usually, memory is allocated as an array of objects that have a constructor or destructor. With such a memory allocation for the class will not be called constructor. When freeing memory using the free function, the destructor will not be called either. This is extremely suspicious. As a result, the variables 'x' and 'y' will not be initialized. Of course, you can call the constructor for each object "manually" or explicitly initialize the fields, but it would be more appropriate to use the 'new' operator:

 _pointVertexes = new Vec2[ _maxPoints]; _vertices = new Vec2[_maxPoints * 2]; 

Similar places:

• V630 The 'malloc' function is to allocate memory. ccmotionstreak.cpp 124
• V630 The 'malloc' function is to allocate memory. ccmotionstreak.cpp 125

V572 It is an odd one that has been created by the operator. ccactiontiledgrid.cpp 322

 struct Tile { Vec2 position; Vec2 startPosition; Size delta; }; Tile* _tiles; void ShuffleTiles::startWithTarget(Node *target) { .... _tiles = (struct Tile *)new Tile[_tilesCount]; //<== Tile *tileArray = (Tile*) _tiles; //<== .... } 

Here the operator 'new' already returns a typed pointer and bringing it to the same type is meaningless.

Similar place:

• V572 It is an odd one that has been created by the operator. luabasicconversions.cpp 1301

V668 It has been allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. ccfloat.h 48

 static __Float* create(float v) { __Float* pRet = new __Float(v); //<== if (pRet) //<== { pRet->autorelease(); } return pRet; } 

If the 'new' operator could not allocate memory, then, according to the C ++ standard of the language, the exception std :: bad_alloc () is generated. Thus, checking the pointer to equality to zero does not make sense, unlike the return value of the function 'malloc'. There are 475 such checks in the project!

V547 Expression '0 == commonInfo-> eventName' is always false. Pointer 'commonInfo-> eventName'! = NULL. ccluaengine.cpp 436

 struct CommonScriptData { // Now this struct is only used in LuaBinding. int handler; char eventName[64]; //<== .... }; int LuaEngine::handleCommonEvent(void* data) { .... CommonScriptData* commonInfo = static_cast<....*>(data); if (NULL == commonInfo->eventName || //<== 0 == commonInfo->handler) return 0; .... } 

The condition (NULL == commonInfo-> eventName) will always be false, since the array 'eventName' is declared locally. If you fail to allocate memory for an array of a fixed size, the problem will be revealed earlier - when allocating memory for the structure.

More checks:

• V547 Expression '0! = CommonInfo-> eventSourceClassName' is always true. Pointer 'commonInfo-> eventSourceClassName'! = NULL. ccluaengine.cpp 442
• V600 Consider inspecting the condition. The 'commonInfo-> eventName' pointer is not always equal to NULL. ccluaengine.cpp 436
• V600 Consider inspecting the condition. The 'commonInfo-> eventSourceClassName' pointer is not always equal to NULL. ccluaengine.cpp 442

Nightmare structured programming

V696 The 'continue' operator will terminate 'do {...} while (FALSE)' loop because the condition is always false. Check lines: 125, 153. cccomaudio.cpp 125

 bool ComAudio::serialize(void* r) { bool ret = false; do { .... if (file != nullptr) { if (strcmp(file, "") == 0) { continue; //<== } .... } }while(0); return ret; } 

The analyzer detected a code that could mislead a programmer. The continue statement in the “do {...} while (0)” loop will stop the loop, rather than resume it. Thus, after calling the 'continue' operator, the condition (0) will be checked, and the loop will end as the condition is false. If this is the way the error is conceived, then the code is still better to change. You can use the operator 'break'.

Similar cycles:

• V696 The 'continue' operator will terminate 'do {...} while (FALSE)' loop because the condition is always false. Check lines: 188, 341. cccomrender.cpp 188
• V696 The 'continue' operator will terminate 'do {...} while (FALSE)' loop because the condition is always false. Check lines: 276, 341. cccomrender.cpp 276
• V696 The 'continue' operator will terminate 'do {...} while (FALSE)' loop because the condition is always false. Check lines: 281, 341. cccomrender.cpp 281
• V696 The 'continue' operator will terminate 'do {...} while (FALSE)' loop because the condition is always false. Check lines: 323, 341. cccomrender.cpp 323

Formatted output

V576 Incorrect format. Consider checking the fourth argument of the fprintf function. The type of symbols is expected. ccconsole.cpp 341

 #ifdef UNICODE #define gai_strerror gai_strerrorW //<== #else #define gai_strerror gai_strerrorA #endif /* UNICODE */ bool Console::listenOnTCP(int port) { .... fprintf(stderr,"net_listen error for %s: %s", //<== serv, gai_strerror(n)); //<== .... } 

The gai_strerror function can be defined as gai_strerrorW and gai_strerrorA, depending on the definition of the UNICODE directive. In Visual Studio 2012, where the project was checked, a Unicode function was defined that returns a wide string for printing of which you need to use the '% S' specifier (large S), otherwise only the first character of the string or meaningless text will be printed.

Same condition result

V583 The '?:' Operator, regardless of its conditional expression, always returns one and the same value: ATLAS_REPEAT. atlas.cpp 219

 spAtlas* spAtlas_readAtlas (....) { .... page->uWrap = *str.begin == 'x' ? ATLAS_REPEAT : (*str.begin == 'y' ? ATLAS_CLAMPTOEDGE : ATLAS_REPEAT); page->vWrap = *str.begin == 'x' ? ATLAS_CLAMPTOEDGE : (*str.begin == 'y' ? ATLAS_REPEAT : ATLAS_REPEAT); //<== .... } 

Perhaps it was written for beauty, but, nevertheless, the return of one value in the condition looks very suspicious.

Pointer dereference

V595 The values ​​of the pointer Check lines: 188, 189. ccbundlereader.h 188

 template<> inline bool BundleReader::readArray<std::string>( unsigned int *length, std::vector<std::string> *values) { .... values->clear(); //<== if (*length > 0 && values) //<== { for (int i = 0; i < (int)*length; ++i) { values->push_back(readString()); } } return true; } 

In many places of the code, literally right after dereference, the pointer is checked for validity. Here are some of these places:

• V595 The '_openGLView' pointer was used before it was verified against nullptr. Check lines: 410, 417. ccdirector.cpp 410
• V595 Check lines: 365, 374. cctween.cpp 365
• V595 The "rootEle" pointer was used before it was verified against nullptr. Check lines: 378, 379. ccfileutils.cpp 378
• V595 The 'tolua_ret' pointer was used before it was verified against nullptr. Check lines: 429, 433. lua_cocos2dx_manual.cpp 429
• V595 The 'tolua_ret' pointer was used before it was verified against nullptr. Check lines: 1858, 1861. lua_cocos2dx_manual.cpp 1858
• V595 The 'tolua_ret' pointer was used before it was verified against nullptr. Check lines: 4779, 4781. lua_cocos2dx_manual.cpp 4779
• V595 The '_fontAtlas' pointer was used before it was verified against nullptr. Check lines: 384, 396. cclabel.cpp 384
• V595 The '_glprogramstate' pointer was used before it was verified against nullptr. Check lines: 216, 218. shadertest2.cpp 216
• V595 The '_sprite' pointer was used before it was verified against nullptr. Check lines: 530, 533. sprite3dtest.cpp 530

Non-random test

V636 The 'rand () / 0x7fff' expression was implicitly casted from 'int' type to 'float' type. Consider using a fractional part. An example: double A = (double) (X) / Y ;. cpp-tests physicstest.cpp 307

 static inline float frand(void) { return rand()/RAND_MAX; } 

In the source files for testing was found such a function. Most likely, it is planned to obtain real numbers in the range of 0.0f to 1.0f, but the result of the rand () function is an integer, therefore the real part after division is discarded. The function returns only 0.0 or 1.0. Moreover, since the rand () function returns a value from 0 to RAND_MAX, the probability of getting a result of 1.0 is negligible.

Rather, tests using the frand () function do not actually test anything. A good example of how static analysis complements testing with unit tests.

Conclusion

As I said at the beginning, Cocos2d-x contains quite a few suspicious places. The project is relatively young and innovative, does not contain the code inherited from the old times. Surely, developers use various methods of quality control and try to follow modern standards and programming methodologies.

This article is in English.

If you want to share this article with an English-speaking audience, then please use the link to the translation: Svyatoslav Razmyslov. Checking the Cross-Platform Framework Cocos2d-x .