Проверка симулятора 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 : %dn", 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.

Автор: SvyatoslavMC

Источник