Проверка симулятора The Powder Toy

    The Powder Toy является песочницей со свободной физикой, которая имитирует давление и скорость воздуха, тепла, тяжести и бесчисленное количество взаимодействий между различными веществами. Игра предоставляет различные строительные материалы, жидкости, газы и электронные компоненты, которые могут быть использованы для построения сложных машин, оружия, бомб, реалистичной местности и почти всего, что угодно. Вы можете просматривать и воспроизводить тысячи различных сделанных построек. Вот только в игре оказалось не всё так замечательно: для небольшого проекта размером в ~350 файлов было получено довольно много предупреждений статического анализатора. В этой статье будут описаны наиболее интересные места.

    The Powder Toy проверялся с помощью PVS-Studio 5.20. Проект собирается под Windows в msys с помощью скрипта на Python, поэтому для проверки я воспользовался специальной утилитой PVS-Studio Standalone, которая описана в статье: PVS-Studio теперь поддерживает любую сборочную систему на Windows и любой компилятор. Легко и «из коробки».

    Результаты проверки


    V501 There are identical sub-expressions to the left and to the right of the '||' operator: !s[1] ||!s[2] ||!s[1] graphics.cpp 829
    void Graphics::textsize(const char* s, int& width, int& height)
    {
      ....
      else if (*s == '\x0F')
      {
        if(!s[1] || !s[2] || !s[1]) break;     //<==
        s+=3;                                  //<==
      }
      ....
    }

    При определённом условии выполняется проверка трёх последовательных элементов массива символов, но из-за опечатки элемент s[3] не был проверен, что, возможно, приводит к неправильному поведению программы в некоторых ситуациях.

    V523 The 'then' statement is equivalent to the 'else' statement. button.cpp 142
    void Button::Draw(const Point& screenPos)
    {
      ....
      if(Enabled)
        if(isButtonDown || (isTogglable && toggle))
        {
          g->draw_icon(Position.X+iconPosition.X,
                       Position.Y+iconPosition.Y,
                       Appearance.icon, 255, iconInvert);
        }
        else
        {
          g->draw_icon(Position.X+iconPosition.X,
                       Position.Y+iconPosition.Y,
                       Appearance.icon, 255, iconInvert);
        }
      else
        g->draw_icon(Position.X+iconPosition.X,
                     Position.Y+iconPosition.Y,
                     Appearance.icon, 180, iconInvert);
      ....
    }

    Фрагмент функции с подозрительно одинаковым кодом. Условное выражение содержит ряд логических операций, поэтому можно предположить, что данный фрагмент кода не содержит бесполезную проверку, а допущена опечатка в предпоследнем параметре функции 'draw_icon()'. Т.е. где-то должно быть написано не значение '255'.

    Похожие места:
    • V523 The 'then' statement is equivalent to the 'else' statement. luascriptinterface.cpp 2758
    • V523 The 'then' statement is equivalent to the 'else' statement. searchview.cpp 305

    V530 The return value of function 'empty' is required to be utilized. requestbroker.cpp 309
    std::vector<Request*> Children;
    
    RequestBroker::Request::~Request()
    {
      std::vector<Request*>::iterator iter = Children.begin();
      while(iter != Children.end())
      {
        delete (*iter);
        iter++;
      }
      Children.empty();             //<==
    }

    Вместо очистки вектора вызвали функцию 'empty()', которая не изменяет вектор. Так как код находится в деструкторе, то ошибка, пожалуй, никак не влияет на исполнение программы. Но, тем не менее, решил озвучить этот момент.

    V547 Expression 'partsData[i] >= 256' is always false. The value range of unsigned char type: [0, 255]. gamesave.cpp 816:

    #define PT_DMND 28
    //#define PT_NUM  161
    #define PT_NUM 256
    
    unsigned char *partsData = NULL,
    
    void GameSave::readOPS(char * data, int dataLength)
    {
      ....
      if(partsData[i] >= PT_NUM)
        partsData[i] = PT_DMND; //Replace all invalid elements....
      ....
    }

    Код содержит подозрительное место, которое понятно только автору. Раньше, если i-й элемент массива 'partsData' был больше или равен 161, то в элемент записывалось значение 28. Сейчас же константа 161 закомментирована и заменена на 256, вследствие чего условие никогда не выполняется, т.к. максимальное значение 'unsigned char': 255.

    V547 Expression is always false. Probably the '||' operator should be used here. previewview.cpp 449:

    void PreviewView::NotifySaveChanged(PreviewModel * sender)
    {
      ....
      if(savePreview && savePreview->Buffer &&
         !(savePreview->Width == XRES/2 &&           //<==
           savePreview->Width == YRES/2))            //<==
      {
        pixel * oldData = savePreview->Buffer;
        float factorX = ((float)XRES/2)/((float)savePreview->Width);
        float factorY = ((float)YRES/2)/((float)savePreview->Height);
        float scaleFactor = factorY < factorX ? factorY : factorX;
        savePreview->Buffer = Graphics::resample_img(....);
        delete[] oldData;
        savePreview->Width *= scaleFactor;
        savePreview->Height *= scaleFactor;
      }
      ....
    }

    Благодаря везению часть условия всегда истинна. С большой вероятностью тут имеет место опечатка: возможно, должен быть использован оператор '||' вместо '&&', или в одном случае необходимо проверять, например, 'savePreview->Height'.

    V560 A part of conditional expression is always true: 0x00002. frzw.cpp 34
    unsigned int Properties;
    
    Element_FRZW::Element_FRZW()
    {
      ....
      Properties = TYPE_LIQUID||PROP_LIFE_DEC;
      ....
    }

    Во всём коде с переменной 'Properties' выполняются битовые операции, но в двух местах используется '||' вместо '|'. Получается, что в Properties будет записано значение 1.

    Второе такое место:
    • V560 A part of conditional expression is always true: 0x04000. frzw.cpp 34

    V567 Undefined behavior. The 'sandcolour_frame' variable is modified while being used twice between sequence points. simulation.cpp 4744
    void Simulation::update_particles()
    {
      ....
      sandcolour_frame = (sandcolour_frame++)%360;
      ....
    }

    Переменная 'sandcolour_frame ' дважды используется в одной точке следования. В результате невозможно предсказать результат работы такого выражения. Подробнее – в описании диагностики V567.

    V570 The 'parts[i].dcolour' variable is assigned to itself. fwrk.cpp 82
    int Element_FWRK::update(UPDATE_FUNC_ARGS)
    {
      ....
      parts[i].life=rand()%10+18;
      parts[i].ctype=0;
      parts[i].vx -= gx*multiplier;
      parts[i].vy -= gy*multiplier;
      parts[i].dcolour = parts[i].dcolour;              //<==
      ....
    }

    Подозрительная инициализация поля собственным значением.

    V576 Incorrect format. Consider checking the third actual argument of the 'printf' function. To print the value of pointer the '%p' should be used. powdertoysdl.cpp 3247
    int SDLOpen()
    {
      ....
      SDL_SysWMinfo SysInfo;
      SDL_VERSION(&SysInfo.version);
      if(SDL_GetWMInfo(&SysInfo) <= 0) {
          printf("%s : %d\n", SDL_GetError(), SysInfo.window);
          exit(-1);
      }
      ....
    }

    Для печати указателя необходимо использовать спецификатор %p.

    V595 The 'gameSave' pointer was utilized before it was verified against nullptr. Check lines: 1063, 1070. gamecontroller.cpp 1063
    void GameController::OpenLocalSaveWindow(bool asCurrent)
    {
      Simulation * sim = gameModel->GetSimulation();
      GameSave * gameSave = sim->Save();                        //<==
      gameSave->paused = gameModel->GetPaused();
      gameSave->gravityMode = sim->gravityMode;
      gameSave->airMode = sim->air->airMode;
      gameSave->legacyEnable = sim->legacy_enable;
      gameSave->waterEEnabled = sim->water_equal_test;
      gameSave->gravityEnable = sim->grav->ngrav_enable;
      gameSave->aheatEnable = sim->aheat_enable;
      if(!gameSave)                                             //<==
      {
        new ErrorMessage("Error", "Unable to build save.");
      }
      ....
    }

    Логичнее будет сначала проверить указатель 'gameSave' на ноль, а потом уже заполнять поля.

    Ещё несколько таких мест:
    • V595 The 'newSave' pointer was utilized before it was verified against nullptr. Check lines: 972, 973. powdertoysdl.cpp 972
    • V595 The 'gameSave' pointer was utilized before it was verified against nullptr. Check lines: 1271, 1278. gamecontroller.cpp 1271
    • V595 The 'gameSave' pointer was utilized before it was verified against nullptr. Check lines: 1323, 1330. gamecontroller.cpp 1323
    • V595 The 'state_' pointer was utilized before it was verified against nullptr. Check lines: 220, 232. engine.cpp 220

    V611 The memory was allocated using 'new T[]' operator but was released using the 'delete' operator. Consider inspecting this code. It's probably better to use 'delete [] userSession;'. apirequest.cpp 106
    RequestBroker::ProcessResponse
    APIRequest::Process(RequestBroker & rb)
    {
      ....
      if(Client::Ref().GetAuthUser().ID)
      {
        User user = Client::Ref().GetAuthUser();
        char userName[12];
        char *userSession = new char[user.SessionID.length() + 1];
        ....
        delete userSession;          //<==
      }
      ....
    }

    Операторы new, new[], delete, delete[] должны использоваться соответствующими парами, т.е. здесь правильно будет: «delete[] userSession;».

    И это место не единственное:
    • V611 The memory was allocated using 'new T[]' operator but was released using the 'delete' operator. Consider inspecting this code. It's probably better to use 'delete [] userSession;'. webrequest.cpp 106
    • V611 The memory was allocated using 'new T[]' operator but was released using the 'delete' operator. Consider inspecting this code. It's probably better to use 'delete [] workingDirectory;'. optionsview.cpp 228

    V614 Uninitialized pointer 'ndata' used. simulation.cpp 1688
    void *Simulation::transform_save(....)
    {
      void *ndata;
      ....
      //ndata = build_save(....); //TODO: IMPLEMENT
      ....
      return ndata;
    }

    До запланированной доработки этого места функция будет возвращать неинициализированный указатель.

    Похожее место:
    • V614 Potentially uninitialized pointer 'tempThumb' used. saverenderer.cpp 150

    Заключение


    The Powder Toy — интересный кроссплатформенный проект, который можно использовать для игры, обучения и экспериментов. Несмотря на небольшой размер проекта, было интересно в нём разобраться. Надеюсь, авторы уделят внимание анализу исходного кода и проанализируют полный лог проверки.

    Используя статический анализ регулярно, можно сэкономить массу времени на решение более полезных задач и TODO'шек.

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


    Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Svyatoslav Razmyslov. Analysis of the The Powder Toy Simulator.

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

    Similar posts

    Comments 12

      +2
      Стандартное нытьё про проверку freeswitch ;)
        +1
        В статье описаны по большому счету маленькие, нефатальные ошибки, поэтому с The Powder Toy не все так уж и плохо.
          +1
          Как вы это определили? V567 и V611 указывают на дефекты, провоцирующие неопределенное поведение, код с такими дефектами непереносим не только на другие компиляторы, но и на другие версии того же компилятора.
            +1
            V501 — неприятная бага в данном случае. Подайте на вход строку с текстом "\x0Fqq" и вуаля — мы уже читаем за пределами буфера.

            Понятно, что фатальные баги исправили на этапе тестирования, иначе приложение бы не запускалось :-)
            –2
            А вот мне интересно рекурсивное поведение — проверить запущенный инстанс самого себя.
            Такое делалось?
              +2
              В смысле проверить исходные коды PVS-Studio с помощью PVS-Studio? Или что Вы имели в виду?
                0
                Вы прочитали то, что написано под «Прочитали статью и есть вопрос?»? :-)
                +1
                Вот мне интересны такие вещи. Проект относительно небольшой, всего 2.1Мб кода (*.c, *.h, *.cpp). Сколько всего сообщений выдало PVS-Studio на этом проекте? В статье упомянуто около двух десятков. Какая это доля от всех сообщений? Остальные слишком скучны для статьи или это ложные сработки? Можете ли вы оценить долю ложных сработок? Понятно, что не всегда возможно сказать наверняка, ложная она или нет, но приблизительно же можно посчитать? Для простоты можно считать, что 64битные сообщения (V101-V303) нас пока не интересуют. Приведите примеры характерных ложных сработок.
                  0
                  Мы сознательно не приводим информацию о ложных срабатываниях, чтобы почём зря не создать негативное впечатление. Часто анализатор выдаёт много ложных срабатываний, многие, которые можно легко и быстро изничтожить. Занимаясь проверкой сторонних проектов, мы этого не делаем. Наша цель написать статью, поэтому можно и потерпеть мусор среди сообщений. Но человек, работая со своим проектом, думаю может весьма хорошо настроить анализатор и ложных срабатываний будет не так уж и много.

                  Что-бы пояснить данный момент на практике, приведу фрагмент из статьи "Третья проверка кода проекта Chromium с помощью анализатора PVS-Studio".

                  Мне часто задают вопрос:

                  В статьях вы очень ловко приводите примеры найденных ошибок. Но вы не говорите, каково общее количество выдаваемых сообщений. Часто статические анализаторы выдают очень много ложных срабатываний и среди них практически невозможно отыскать настоящие ошибки. Как дело обстоит с анализатором PVS-Studio?

                  И я всегда не знаю, что сходу ответить на этот вопрос. У меня есть два противоположных ответа: первый — много, второй — мало. Все зависит, как подойти к рассмотрению выданного списка сообщения. Сейчас на примере Chromium я попробую объяснить суть такой двойственности.

                  Анализатор PVS-Studio выдал 3582 предупреждений первого уровня (набор правил общего назначения GA). Это очень много. И в большинстве этих сообщений являются ложными. Если подойти «в лоб» и начать сразу просматривать весь список, то это очень быстро надоест. И впечатление будет ужасное. Одни сплошные однотипные ложные срабатывания. Ничего интересного не попадается. Плохой инструмент.

                  Ошибка такого пользователя в том, что не выполнена даже минимальная настройка инструмента. Да, мы стараемся сделать инструмент PVS-Studio таким, чтобы он работал сразу после установки. Мы стараемся, чтобы ничего не надо было настраивать. Человек должен просто проверить свой проект и изучить список выданных предупреждений.

                  Однако так не всегда получается. Так не получилось и с Chromium. Например, огромное количество ложных сообщений возникло из-за макроса 'DVLOG'. Этот макрос что-то логирует. Он написан хитро и PVS-Studio ошибочно посчитал, что в нем может быть ошибка. Поскольку, макрос очень активно используется, ложных сообщений получилось очень много. Можно сказать, что сколько раз используется макрос DVLOG, столько ложных предупреждений попадет в отчет. А именно, о макросе было выдано около 2300 ложных сообщений «V501 There are identical sub-expressions.....».

                  Можно подавить эти предупреждения, вписав в заголовочный файл, рядом с объявлением макроса, вот такой вот комментарий:

                  //-V:DVLOG:501

                  Смотрите, таким простым действием мы вычтем из 3582 сообщений, 2300 ложных. Мы сразу отсеяли 65% сообщений. И теперь нам не надо их зря просматривать.

                  Без особых усилий можно сделать ещё несколько подобных уточнений и настроек. В результате, большинство ложных срабатываний исчезнут. В некоторых случаях после настройки следует перезапускать анализ, в некоторых нет. Подробнее всё это описывает в документации раздел "Подавление ложных предупреждений". Конкретно, в случае с ошибками в макросах, придется перезапускать анализ.

                  Надеюсь теперь понятно. Почему первый ответ — ложных срабатываний много. И почему второй ответ — ложных срабатываний мало. Всё зависит, готов ли человек потратить хоть немного времени на изучение продукта и способов избавить себя от лишних сообщений.

                    0
                    Здравствуйте, Андрей! Спасибо за развёрнутый ответ, хотя я всё же хотел увидеть конкретные цифры. Я же не как потенциальный пользователь интересуюсь, а скорее как коллега ;-) Впрочем, раздел документации частично проясняет ситуацию. Про массовую фильтрацию, конечно, понятно. К примеру, IntelliJ IDEA скомпилирована с какой-то хитрой штуковиной вроде Lombok, которая на выходе функции, помеченной Notnull автоматом вставляет в байт-код проверку и assert, если мы пытаемся вернуть null. С точки зрения FindBugs подавляющее большинство таких проверок лишнее, в результате имеется свыше 11000 ложных сообщений Redundant nullcheck of value known to be non-null (а всего 28000 сообщений на всём проекте). Если перекомпилировать без этой штуки или отключить этот варнинг, то доля ложных сработок резко падает.

                    Кстати, тут не ошибка (это из вашей документации)?
                    //-V:ABC<<:501
                    AB C = x == x; // Есть предупреждение
                    AB y = ABC == ABC; // Нет предупреждения

                    Кажется, << лишнее в комментарии.

                    Всё же я думаю, что самое лучшее впечатление — это когда оно соответствует истине. В любой научной статье новый метод или алгоритм не только расхваливается, но и указываются пределы его применимости и возможные проблемы. Ваш продукт всё же на умных людей рассчитан. Взвешенные статьи с указаниями не только преимуществ, но и характерных недостатков (в виде пары интересных ложных срабатываний) смотрелись бы весомее. Может, конечно, я ничего не понимаю в маркетинге :-)
                      +1
                      Предупреждения общего назначения:
                      Первый уровень: 29
                      Второй уровень: 72
                      Третий уровень: 208 (по умолчанию отключены)

                      Что есть ложное, а что нет, сказать сложно. Например, на третьем уровне почти 100 предупреждений V688. Вроде и не ошибка, но сообщение по делу. Пример:

                      class LuaWindow
                      {
                        ....
                        lua_State * l;
                        ....
                      };
                      
                      int LuaWindow::size(lua_State * l)
                      {
                        ....
                      };
                      

                      V688 The 'l' function argument possesses the same name as one of the class members, which can result in a confusion. luawindow.cpp 179

                      Плохая идея делать член класса с именем 'l' и при этом также называть аргумент функции. Легко запутаться, с каким 'l' нужно работать.

                      По поводу документации. Да, ошибка. Спасибо, исправим.
                        0
                        Ясно, спасибо. В целом цифры разумные. Про V688 никаких вопросов. Понятно, что есть прямо ошибки, а есть неаккуратность или плохой стиль. Если пользователю не нравится конкретная диагностика, он может её отключить. Ложными срабатываниями я считаю ситуации, когда текст сообщения об ошибке неверен, именно такой код и задумывался и исправлять нечего. Например, в случае с V688 если бы на самом деле не было члена класса с таким именем, это было бы ложное срабатывание (понятно, что конкретно эта диагностика простая и вряд ли для неё вообще возможны ложные срабатывания).

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