Cocos2d is an open source software framework. It can be used to build games, apps and other cross-platform GUI based interactive programs. Cocos2d contains many branches with the best known being Cocos2d-Swift, Cocos2d-x, Cocos2d-html5 and Cocos2d-XNA.
In this article, we are going to discuss results of the check of Cocos2d-x, the framework for C++, done by PVS-Studio 5.18. The project is pretty high-quality, but there are still some issues to consider. The source code was downloaded from GitHub.
Working with graphic objects is usually about processing arrays and matrices, memory being allocated dynamically. In this project, both the 'malloc' function and the 'new' operator are used to allocate memory. These techniques are very different in use, so you have to take these differences into account when replacing one with another in the code. Further in this article, I'll show you those fragments which don't quite correctly use 'malloc' and 'new'.
V630 The 'malloc' function is used to allocate memory for an array of objects which are classes containing constructors and destructors. 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);
....
}Allocated memory is usually handled as an array of objects with a constructor or destructor. In the above cited fragment, the constructor won't be called when allocating memory for the class. When freeing it through the free function, the destructor won't be called as well. This is very strange. This code will cause the variables 'x' and 'y' to remain uninitialized. Of course, we can call the constructor for each object "manually" or initialize the fields explicitly, but a more correct way is to use the 'new' operator:
_pointVertexes = new Vec2[_maxPoints];
_vertices = new Vec2[_maxPoints * 2];Other similar fragments:
V572 It is odd that the object which was created using 'new' operator is immediately casted to another type. 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; // <=
....
}This time, the 'new' operator returns a typed pointer, so casting it to the same type doesn't make any sense.
Another similar fragment:
V668 There is no sense in testing the 'pRet' pointer against null, as the memory was 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 has failed to allocate memory, the std::bad_alloc() exception will be generated, according to the C++ language standard. So, checking the pointer for being null doesn't make sense, unlike the return value of the 'malloc' function. And there are 475 more checks like that 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 (NULL == commonInfo->eventName) condition will always be false as the 'eventName' array is a local one. If the program fails to allocate memory for a fixed-size array, the issue will reveal itself even earlier - when allocating memory for the structure.
Other similar checks:
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 has detected code that may mislead the programmer. The continue operator in the "do { ... } while(0)" loop will terminate it instead of continuing it. So, after calling the 'continue' operator, condition (0) will be checked and the loop will terminate as it is false. Even if it was done purposefully and there is no error here, the code still should be improved. For example, you can use the 'break' operator.
Other similar loops:
V576 Incorrect format. Consider checking the fourth actual argument of the 'fprintf' function. The pointer to string of char type 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 UNICODE directive. In Visual Studio 2012 that we were working in when checking the project, a Unicode function was declared that returned a wide string that should be printed using the '%S' specifier (capital S), otherwise only the first character of the string or simply meaningless text would be printed.
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); // <=
....
}Maybe the programmer wrote it that way just for the sake of aesthetics, but returning one and the same value in a condition looks too suspicious.
V595 The 'values' pointer was utilized before it was verified against nullptr. 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;
}Very often in the project, pointers are checked for being valid literally right after being dereferenced. Here are some of these fragments:
V636 The 'rand() / 0x7fff' expression was implicitly casted from 'int' type to 'float' type. Consider utilizing an explicit type cast to avoid the loss of a fractional part. An example: double A = (double)(X) / Y;. cpp-tests physicstest.cpp 307
static inline float frand(void)
{
return rand()/RAND_MAX;
}This function was discovered among the source files referring to tests. The programmer most likely wanted real numbers to be returned within the range 0.0f - 1.0f, but the rand() function's return value is an integer number, which means its real component is truncated after division. So the function returns only 0.0 or 1.0. Moreover, since the rand() function returns a value between 0 and RAND_MAX, getting number 1.0 is almost improbable.
Seems like the tests using the frand() function don't actually test anything. That's a good example of how static analysis complements unit testing.
Like I've already told you in the very beginning, there are pretty few suspicious fragments in the Cocos2d-x project. This framework is relatively young and innovative and doesn't contain any legacy code from the old times. The project developers seem to be using various means of code quality control and trying to conform to modern standards and programming methodologies.