Проверка симулятора 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
RequestBroker: Request::~Request ()
{
std: vector
#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: 0×00002. 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: 0×04000. 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.
