Проверка кроссплатформенного фреймворка Cocos2d-x



    Cocos2d — открытое программное обеспечение, фреймворк. Он может быть использован для построения игр, приложений и графических интерфейсов интерактивных кроссплатформенных приложений. Cocos2d содержит множество бранчей, известные из них Cocos2d-iPhone, Cocos2d-x, Cocos2d-html5 и Cocos2d-XNA

    В данной статье будут рассмотрены результаты проверки Cocos2d-x, фреймворка для C++, полученные с помощью PVS-Studio 5.18. Проект достаточно качественный, но всё же на некоторые места стоит обратить внимание. Исходный код взят с GitHub.

    От malloc к new, от C к С++


    Работа с графическими объектами, как правило, связана с обработкой массивов и матриц, память под которые выделяют динамически. В данном проекте для выделения памяти используются как вызовы функции 'malloc', так и оператор 'new'. У этих способов есть существенные различия в использовании, которые необходимо учитывать при их замене в коде. Далее будут описаны места, не совсем корректно использующие 'malloc' и '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);
      ....
    }

    Обычно с выделенной памятью работают как с массивом объектов, имеющих конструктор или деструктор. При таком выделении памяти для класса не будет вызван конструктор. При освобождении памяти с помощью функции free также не будет вызван деструктор. Это крайне подозрительно. В результате переменные 'x' и 'y' будут не инициализированы. Конечно, можно вызвать конструктор для каждого объекта «вручную» или явно инициализировать поля, но более правильным будет использование оператора 'new':
    _pointVertexes = new Vec2[ _maxPoints];
    _vertices = new Vec2[_maxPoints * 2];

    Аналогичные места:
    • 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;               //<==
      ....
    }

    Здесь оператор 'new' уже возвращает типизированный указатель и приведение его к этому же типу является бессмысленным.

    Похожее место:
    • 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;
    }

    Если оператор 'new' не смог выделить память, то согласно стандарту языка Си++, генерируется исключение std::bad_alloc(). Таким образом проверять указатель на равенство нулю не имеет смысла, в отличии от возвращаемого значения функции 'malloc'. В проекте есть ещё 475 таких проверок!

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

    Условие (NULL == commonInfo->eventName) всегда будет ложным, так как массив 'eventName' объявлен локально. Если выделить память под массив фиксированного размера не удастся, то проблема будет выявлена ранее — при выделении памяти под структуру.

    Ещё проверки:
    • 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

    Страшный сон структурного программирования


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

    Анализатор обнаружил код, который может ввести в заблуждение программиста. Оператор continue в цикле «do {… } while(0)» остановит цикл, а не возобновит его. Таким образом, после вызова оператора 'continue' будет проверено условие (0), и цикл завершится так как условие ложно. Если именно так задумано и ошибки нет, то код всё равно лучше изменить. Можно воспользоваться оператором 'break'.

    Аналогичные циклы:
    • 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

    Форматированный вывод


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

    Функция gai_strerror может быть определена как gai_strerrorW и gai_strerrorA, в зависимости от определения директивы UNICODE. В Visual Studio 2012, где проверялся проект, была определена юникодная функция, которая возвращает широкую строку, для печати которой необходимо использовать спецификатор '%S' (большая S), иначе будет напечатан только первый символ строки или бессмысленный текст.

    Одинаковый результат условия


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

    Возможно так написали для красоты, но, тем не менее, возврат одного значения в условии выглядит очень подозрительно.

    Разыменование указателя


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

    Во многих местах кода, буквально сразу после разыменования, указатель проверяют на валидность. Вот некоторые из этих мест:
    • 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

    Неслучайный тест


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

    В исходных файлах для тестирования была обнаружена такая функция. Скорее всего планируется получать вещественные числа в диапазоне 0.0f до 1.0f, но результат функции rand() является целым числом, следовательно вещественная часть после деления отбрасывается. Функция возвращает только 0.0 или 1.0. Более того, так как функция rand() возвращает значение от 0 до RAND_MAX, вероятность получать результат 1.0 ничтожно мала.

    Скорее тесты, использующие функцию frand() на самом деле ничего не тестируют. Хороший пример, как статический анализ дополняет тестирование с помощью юнит-тестов.

    Заключение


    Как я уже сказал в самом начале, Cocos2d-x содержит довольно мало подозрительных мест. Проект сравнительно молодой и инновационный, не содержит унаследованного от старых времён кода. Наверняка разработчики используют различные способы контроля качества и стараются следовать современным стандартам, и методологиям программирования.

    Эта статья на английском


    Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Svyatoslav Razmyslov. Checking the Cross-Platform Framework Cocos2d-x.

    Прочитали статью и есть вопрос?
    Часто к нашим статьям задают одни и те же вопросы. Ответы на них мы собрали здесь: Ответы на вопросы читателей статей про PVS-Studio и CppCat, версия 2014. Пожалуйста, ознакомьтесь со списком.
    PVS-Studio
    Static Code Analysis for C, C++, C# and Java

    Comments 28

      +41
      А Вы сообщили разработчикам о найденных ошибках?
        +2
        Под спойлером в конце есть ссылка с ответом на ваш вопрос. Или вы тролите? :-)
          +11
          Вам не понять, какой букет эмоций я испытал, когда писал этот комментарий. :)
            +1
            На данный момент занят в создании специализированного статического анализатора кода PVS-Studio
            Теперь все понятно :-)
          +1
          Когда будет версия для FreeBSD?


          Какую версию кокоса вы проверяли, как я понимаю 3.x? Ее относительно недавно переписывали с нуля с учетом C++11, по сравнению с 2.x там все и должно быть хорошо. До 3.x, cocos2d-x представлял собой стремную смесь С, С++ с очень сильным влиянием Obj-C API, например, в управлении памятью.
            +1
            Напишите нам от имени компании и мы обсудим варианты сотрудничества. Мы можем заключить контракт на аудит вашего кода или интеграцию PVS-Studio в процесс разработки (адаптацию для FreeBSD и т.д.). Никаких публичных версий. Только контрактные работы.
              +1
              Ссылка с исходниками приведена в самом начале статьи. Каталог, взятый там, называется у меня cocos2d-x-3. Как видите, не всё ещё переписали. Но в целом да, тут всё хорошо. Но опять же, я не их разработчик и мог что-то упустить.
            +7
            более правильным будет использование оператора 'new':
            _pointVertexes = new Vec2[sizeof(Vec2) * _maxPoints];
            _vertices = new Vec2[sizeof(Vec2) * _maxPoints * 2];
            Копи-паст детектед;) правильно будет
            _pointVertexes = new Vec2[_maxPoints];
            _vertices = new Vec2[_maxPoints * 2];
              +8
              Поправил. К сожалению, анализатор статьи не проверяет)
              +3
              Если именно так задумано и ошибки нет, то код всё равно лучше изменить. Можно воспользоваться оператором 'break'.

              А как может быть задумано еще? Очевидно, что если цикл вида do {…} while(false), то он в любом случае может быть выполнен только один раз и используется вместо goto при ошибках. Довольно логично, если рассматривать со стороны смысловой нагрузки, что используется continue, но не логично со стороны стандартного выхода из цикла :)
                0
                Вообще по моему плохая практика использовать такие циклы ради break. Лучше уже выделить в отдельную функцию, намного понятней код будет.
                +2
                _pointVertexes = (Vec2*)malloc(sizeof(Vec2) * _maxPoints);
                _vertices = (Vec2*)malloc(sizeof(Vec2) * _maxPoints * 2);
                _texCoords = (Tex2F*)malloc(sizeof(Tex2F) * _maxPoints * 2);
                

                В результате переменные 'x' и 'y' будут не инициализированы.

                Они будут «инициализированы» далее пачкой. Зачем вызывать конструктор для 1000 элементов(который установит переменные в 0), если можно просто просто мемсетом(как пример) всё зачистить за 1 проход.

                PS: Читаю вторую Вашу статью, и второй раз нахожу ложные предупреждения.
                  0
                  Они будут «инициализированы» далее пачкой. Зачем вызывать конструктор для 1000 элементов(который установит переменные в 0), если можно просто просто мемсетом(как пример) всё зачистить за 1 проход.

                  Зачем тогда Си++?
                    0
                    Зачем тогда Си++?

                    Хорошо, пойдём другим путём — мы создаём буфер управляемый OpenGL.
                      +3
                      Там, где с ним удобнее. А игровой движок — такая штука, где микрооптимизации всюду. Если авторы на 100% знают, что конкретно для этих кусков памяти не нужна инициализация — то зачем её делать?
                        +1
                        Это даже не микрооптимизация.

                        Вот логика работы, если бы там был new:

                        1. Создаём буфер.
                        2. Инициализируем его элементы.
                        3. Передаём управление OpenGL.
                        4. Он берёт буфер и опять инициализирует его.

                        Какой смысл второго шага? Втупую потратить O(N) времени?
                          +1
                          Зачем было писать дефолтный конструктор тогда?
                          Лучше б убрали его и сделали статический метод Zero(), к примеру, который возвращает инициализированный объект.
                          Vec2 myVec = Vec2::Zero()
                        0
                        Зачем тогда Си++?

                        Чтоб не плодить сущности.
                        Если нам нужно поработать с 2х-мерным вектором — используем класс Vec2 по назначению.
                        Если нам нужно создать буффер вершин, которые (99% вероятности) просто будут отправлены далее в видеопамять — то почему бы не использовать уже существующую структуру данных?

                        П.С. У гейм-девелоперов другие задачи и заботы, здесь правила идеального мира не всегда подходят :)
                        +5
                        Смотря с какой точки зрения рассматривать предупреждения как ложные. То, что некотоыре из приведённых примеров являются рабочими, и так и задумывались — не спорю, но статья(и анализатор) обращает на них внимание, потому что не совсем хорошо так писать. Потому что это не всегда очевидно, и не всегда понятно что же задумывалось на самом деле. И если какой-либо другой разработчик придёт исправлять что-то в таком коде( а в статьях говорится об Open Source) он может потратить гораздо больше времени и сил, или вообще получить в итоге что-то в духе Heartbleed.
                          0
                          Кстати, соглашусь. malloc() как раз и существует для случаев, когда нужно выделить неинициализированную память. Теоретически всегда можно было бы применять calloc(), который заполняет память нулевыми байтами, но порой это неэффективно и не нужно. Инициализация будет следовать далее. Да, это не слишком c++-style, чревато последствиями, и, вероятно, не даёт большого прироста в производительности, но такой код имеет право жить.
                            –2
                            Мемсетом? А мемесет вам таблицу виртуальных функций в класс положит? А если в обьекте есть более сложные атрибуты чем поля с числами? Другие объекты и указатели например. Вырвать кусок памяти и потом забить нулями — это мягко говоря не самый универсальный подход, я бы даже сказал узкоспециализированый. Хотя по производительности может и быстрее (надо на досуге замер провести).
                              +1
                              Что за странная привычка в чужой огород со своим уставом ходить? Если типичный программист на Java не имеет представления о том как устроены объекты, с которым он работает, то это вовсе не значит, что программисты на других языках устроены так же.

                              По производительности сильно быстрее, а что про всякие атрибуты и виртуальные таблицы — ну так на то C++ и низкоуровневый язык. В нём даже есть понятие POD, которое на уровне стандарта отделяет структуры, которые можно memset'ить от тех, которые нельзя (строго говоря POD можно копировать с помощью memcpy, а memset'ить нельзя, но эти гарантии дают другие стандарты).
                                0
                                Провел замер (так чисто ради интереса), и так при memset'е выигрыш есть всегда, но ощутим лиш на больших массивах данных.
                                Тестовая структура:

                                struct Vector3D
                                {
                                private:
                                float x, y, z;
                                public:
                                Vector3D():x(0),y(0),z(0){;}
                                };

                                И так на моей машине массив из 1000000(!) обьектов был создан через new[] за 11-12 миллисекунд. С мемсетом 4-5 миллисекуд. В ограниченом ряде случаев такой подход имеет смысл.
                              0
                              хм. в третьей все же не все так страшно оказалось. отполировали-таки. Было бы интересно еще интересно последнюю стабильную 2.7.x версию посмотреть, но она уже заморожена.
                                +1
                                Как я уже сказал в самом начале, Cocos2d-x содержит довольно мало подозрительных мест. Проект сравнительно молодой и инновационный, не содержит унаследованного от старых времён кода. Наверняка разработчики используют различные способы контроля качества и стараются следовать современным стандартам, и методологиям программирования.

                                А тут вы не угадали. Проект родился из Cocos2d, написанном на Obj-C, почти 4 года назад. До третей, текущей, версии там было полно паттернов из мира Obj-C. На настоящие плюсы начали переписывать только в третей версии. И всё равно встречается рудиментарный код.
                                Статическими анализаторами они вообще не пользуются — конкретно задавал этот вопрос разработчикам. И вообще, сложилось впечатление, что о таком виде контроля качества они слышат впервые (заодно порекламировал PVS-Studio и CppCat). А вопрос такой у меня возник потому, что недавний билд не собирался на 64-битных системах по причине того, что в описании функции возвращаемый тип был указан как ssize_t, а в определении — как long int.
                                В общем, печально там всё. Особенно для тех, кому пришлось во внутренности фреймворка лезть.
                                  0
                                  Небольшой оффтопик: у вас нет в планах проверить популярные скриптовые языки, такие как php, ruby, python?
                                    0
                                    Ответил ниже, с окошком ошибся.
                                    +3
                                    php я на днях проверял, но статья может быть не раньше, чем через 2 недели, может даже общая про все языки.

                                    Only users with full accounts can post comments. Log in, please.