Святослав Размыслов

24 Дек 2014

Теги:

#Cpp #GameDev

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

24 Дек 2014

Автор: Святослав Размыслов

Результаты проверки
Заключение

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'шек.

#Cpp #GameDev