Test simulator The Powder Toy

The Powder Toy is a free physics sandbox that simulates the pressure and velocity of air, heat, gravity, and countless interactions between different substances. The game provides various building materials, liquids, gases and electronic components that can be used to build complex machines, weapons, bombs, realistic terrain and almost everything. You can view and play thousands of different buildings made. That's just the game was not so great: for a small project with a size of ~ 350 files, quite a few warnings of the static analyzer were received. This article will describe the most interesting places.

The Powder Toy was tested with PVS-Studio 5.20. The project is built under Windows in msys using a Python script, so for testing I used the special utility PVS-Studio Standalone, which is described in the article: PVS-Studio now supports any build system on Windows and any compiler. Easy and out of the box .

Test results

V501 There are identical to the left and 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; //<== } .... } 

Under certain conditions, three consecutive elements of the character array are checked, but due to a typo, the element s [3] has not been verified, which may lead to incorrect program behavior in some situations.

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); .... } 

Function fragment with suspiciously identical code. The conditional expression contains a series of logical operations, so we can assume that this code fragment does not contain a useless check, but a typo in the penultimate parameter of the 'draw_icon ()' function is made. Those. somewhere it should be written not the value '255'.
')
Similar places:

• 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 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(); //<== } 

Instead of cleaning the vector, they called the function 'empty ()', which does not change the vector. Since the code is in the destructor, the error, perhaps, does not affect the execution of the program. But, nevertheless, I decided to voice this moment.

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.... .... } 

The code contains a suspicious place that is clear only to the author. Previously, if the i-th element of the 'partsData' array was greater than or equal to 161, then the value 28 was written to the element. Now, the constant 161 is commented out and replaced by 256, as a result of which the condition is never satisfied, since maximum value of '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; } .... } 

Thanks to luck, part of the condition is always true. There is probably a typo here: perhaps the '||' operator should be used instead of '&&', or in one case it is necessary to check, for example, 'savePreview-> Height'.

V560 A part of the conditional expression is always true: 0x00002. frzw.cpp 34

 unsigned int Properties; Element_FRZW::Element_FRZW() { .... Properties = TYPE_LIQUID||PROP_LIFE_DEC; .... } 

In the whole code, with the variable 'Properties', bit operations are performed, but in two places, '||' is used. instead of '|'. It turns out that the value 1 will be written in Properties.

The second such place:

• V560 A part of the conditional expression is always true: 0x04000. frzw.cpp 34

V567 Undefined behavior. The 'sandcolour_frame' variable is modified simulation.cpp 4744

 void Simulation::update_particles() { .... sandcolour_frame = (sandcolour_frame++)%360; .... } 

The variable 'sandcolour_frame' is used twice in one sequence point. As a result, it is impossible to predict the result of the operation of such an expression. For details, see the V567 diagnostic description .

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; //<== .... } 

Suspicious initialization of the field with its own value.

V576 Incorrect format. Consider checking the third printf function. To print the value of 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); } .... } 

To print the pointer, you must use the% p specifier.

V595 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."); } .... } 

It would be more logical to first check the 'gameSave' pointer to zero, and then fill in the fields.

A few more places like this:

• V595 The 'newSave' pointer was used before it was verified against nullptr. Check lines: 972, 973. powdertoysdl.cpp 972
• V595 Check lines: 1271, 1278. gamecontroller.cpp 1271
• V595 Check lines: 1323, 1330. gamecontroller.cpp 1323
• V595 The 'state_' pointer was used before it was nullptr. Check lines: 220, 232. engine.cpp 220

V611 The memory was allocated using the 'new T []' 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; //<== } .... } 

The operators new, new [], delete, delete [] should be used in the appropriate pairs, i.e. here it will be correct: "delete [] userSession;".

And this place is not the only one:

• V611 The memory was allocated using the 'new T []' operator. Consider inspecting this code. It's probably better to use 'delete [] userSession;'. webrequest.cpp 106
• V611 The memory was allocated using the 'new T []' 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; } 

Before the planned completion of this place, the function will return an uninitialized pointer.

Similar place:

• V614 Potentially uninitialized pointer 'tempThumb' used. saverenderer.cpp 150

Conclusion

The Powder Toy is an interesting cross-platform project that you can use to play, learn, and experiment. Despite the small size of the project, it was interesting to understand it. I hope the authors will pay attention to the analysis of the source code and analyze the full verification log.

Using static analysis regularly, you can save a lot of time on solving more useful tasks and TODO's.

This article is in English.

If you want to share this article with an English-speaking audience, then please use the link to the translation: Svyatoslav Razmyslov. Analysis of the Powder Toy Simulator .