Checking the Cocos2d-x Cross-Platform Framework

Cocos2d - 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 quite high quality, but still it is worth paying attention to some places. The source code is taken from GitHub .

From malloc to new, from C to C ++

Work with graphic objects, as a rule, is associated with the processing of arrays and matrices, the memory for which is allocated dynamically. In this project, both malloc function calls and the new operator are used to allocate memory. These methods have significant differences in use, which must be taken into account when replacing them in code. The following will describe places that do not 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);
  ....
}

Usually, dedicated memory is treated like an array of objects that have a constructor or destructor. With this allocation of memory for the class, the constructor will not be called. 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 using the 'new' operator would be more correct:

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

Similar places:

• V630 The 'malloc' function is used to allocate memory for an array of objects which are classes containing constructors and destructors. ccmotionstreak.cpp 124
• V630 The 'malloc' function is used to allocate memory for an array of objects which are classes containing constructors. ccmotionstreak.cpp 125

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;               //<==
  ....
}

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

Similar place:

• V572 It is odd that the object which was created using 'new' operator is immediately casted to another type. luabasicconversions.cpp 1301

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 could not allocate memory, then according to the C ++ language standard, an exception std :: bad_alloc () is thrown. Thus, checking the pointer to equality to zero does not make sense, unlike the return value of the 'malloc' function. There are 475 more 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 cannot allocate memory for a fixed-size array, then the problem will be identified 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 always not equal to NULL. ccluaengine.cpp 436
• V600 Consider inspecting the condition. The 'commonInfo-> eventSourceClassName' pointer is always not equal to NULL. ccluaengine.cpp 442

The nightmare of structural 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 be confusing for the programmer. The continue statement in the "do {...} while (0)" loop will stop the loop, not resume it. Thus, after calling the 'continue' operator, condition (0) will be checked, and the cycle will end as the condition is false. If this is exactly what you intended and there is no error, then the code is still better to change. You can use the 'break' operator.

Similar loops:

• 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 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 definition of the UNICODE directive. In Visual Studio 2012, where the project was tested, a Unicode function was defined that returns a wide string, for printing which you need to use the '% S' (large S) specifier, otherwise only the first character of the line or meaningless text will be printed.

The same result of the condition

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, returning one value in a condition looks very suspicious.

Pointer dereferencing

V595 The 'values' pointer was utilized before it was verified against nullptr. Check lines: 188, 189. ccbundlereader.h 188

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

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

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

Random test

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

In the source files for testing, such a function was detected. Most likely it is planned to get real numbers in the range 0.0f to 1.0f, but the result of the rand () function is an integer, therefore the real part is discarded after division. 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 very beginning, Cocos2d-x contains quite a few suspicious places. The project is relatively young and innovative, does not contain code inherited from old times. Surely the 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 .