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'.
Похожие места:
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.
Второе такое место:
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' на ноль, а потом уже заполнять поля.
Ещё несколько таких мест:
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;".
И это место не единственное:
V614 Uninitialized pointer 'ndata' used. simulation.cpp 1688
void *Simulation::transform_save(....)
{
void *ndata;
....
//ndata = build_save(....); //TODO: IMPLEMENT
....
return ndata;
}До запланированной доработки этого места функция будет возвращать неинициализированный указатель.
Похожее место:
The Powder Toy - интересный кроссплатформенный проект, который можно использовать для игры, обучения и экспериментов. Несмотря на небольшой размер проекта, было интересно в нём разобраться. Надеюсь, авторы уделят внимание анализу исходного кода и проанализируют полный лог проверки.
Используя статический анализ регулярно, можно сэкономить массу времени на решение более полезных задач и TODO'шек.