To get a trial key
fill out the form below
Team License (a basic version)
Enterprise License (an extended version)
* By clicking this button you agree to our Privacy Policy statement

Request our prices
New License
License Renewal
--Select currency--
USD
EUR
GBP
RUB
* By clicking this button you agree to our Privacy Policy statement

Free PVS-Studio license for Microsoft MVP specialists
* By clicking this button you agree to our Privacy Policy statement

To get the licence for your open-source project, please fill out this form
* By clicking this button you agree to our Privacy Policy statement

I am interested to try it on the platforms:
* By clicking this button you agree to our Privacy Policy statement

Message submitted.

Your message has been sent. We will email you at


If you haven't received our response, please do the following:
check your Spam/Junk folder and click the "Not Spam" button for our message.
This way, you won't miss messages from our team in the future.

>
>
>
Checking the Cross-Platform Framework C…

Checking the Cross-Platform Framework Cocos2d-x

Aug 21 2014

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.

0275_cocos2d-x/image1.png

From malloc to new, from C to C++

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:

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

This time, the 'new' operator returns a typed pointer, so casting it to the same type doesn't make any sense.

Another similar fragment:

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

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

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

Identical condition results

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.

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

  • 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

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

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.

Conclusion

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.

Popular related articles
PVS-Studio for Java

Date: Jan 17 2019

Author: Andrey Karpov

In the seventh version of the PVS-Studio static analyzer, we added support of the Java language. It's time for a brief story of how we've started making support of the Java language, how far we've co…
The Evil within the Comparison Functions

Date: May 19 2017

Author: Andrey Karpov

Perhaps, readers remember my article titled "Last line effect". It describes a pattern I've once noticed: in most cases programmers make an error in the last line of similar text blocks. Now I want t…
Characteristics of PVS-Studio Analyzer by the Example of EFL Core Libraries, 10-15% of False Positives

Date: Jul 31 2017

Author: Andrey Karpov

After I wrote quite a big article about the analysis of the Tizen OS code, I received a large number of questions concerning the percentage of false positives and the density of errors (how many erro…
Appreciate Static Code Analysis!

Date: Oct 16 2017

Author: Andrey Karpov

I am really astonished by the capabilities of static code analysis even though I am one of the developers of PVS-Studio analyzer myself. The tool surprised me the other day as it turned out to be sma…
Free PVS-Studio for those who develops open source projects

Date: Dec 22 2018

Author: Andrey Karpov

On the New 2019 year's eve, a PVS-Studio team decided to make a nice gift for all contributors of open-source projects hosted on GitHub, GitLab or Bitbucket. They are given free usage of PVS-Studio s…
The way static analyzers fight against false positives, and why they do it

Date: Mar 20 2017

Author: Andrey Karpov

In my previous article I wrote that I don't like the approach of evaluating the efficiency of static analyzers with the help of synthetic tests. In that article, I give the example of a code fragment…
The Ultimate Question of Programming, Refactoring, and Everything

Date: Apr 14 2016

Author: Andrey Karpov

Yes, you've guessed correctly - the answer is "42". In this article you will find 42 recommendations about coding in C++ that can help a programmer avoid a lot of errors, save time and effort. The au…
Static analysis as part of the development process in Unreal Engine

Date: Jun 27 2017

Author: Andrey Karpov

Unreal Engine continues to develop as new code is added and previously written code is changed. What is the inevitable consequence of ongoing development in a project? The emergence of new bugs in th…
How PVS-Studio Proved to Be More Attentive Than Three and a Half Programmers

Date: Oct 22 2018

Author: Andrey Karpov

Just like other static analyzers, PVS-Studio often produces false positives. What you are about to read is a short story where I'll tell you how PVS-Studio proved, just one more time, to be more atte…
The Last Line Effect

Date: May 31 2014

Author: Andrey Karpov

I have studied many errors caused by the use of the Copy-Paste method, and can assure you that programmers most often tend to make mistakes in the last fragment of a homogeneous code block. I have ne…

Comments (0)

Next comments
This website uses cookies and other technology to provide you a more personalized experience. By continuing the view of our web-pages you accept the terms of using these files. If you don't want your personal data to be processed, please, leave this site.
Learn More →
Accept