To get a trial key
fill out the form below
Team license
Enterprise license
** By clicking this button you agree to our Privacy Policy statement

Request our prices
New License
License Renewal
--Select currency--
USD
EUR
RUB
* By clicking this button you agree to our Privacy Policy statement

Free PVS-Studio license for Microsoft MVP specialists
** By clicking this button you agree to our Privacy Policy statement

To get the licence for your open-source project, please fill out this form
** By clicking this button you agree to our Privacy Policy statement

I am interested to try it on the platforms:
** By clicking this button you agree to our Privacy Policy statement

Message submitted.

Your message has been sent. We will email you at


If you haven't received our response, please do the following:
check your Spam/Junk folder and click the "Not Spam" button for our message.
This way, you won't miss messages from our team in the future.

>
>
>
Analysis of the The Powder Toy Simulator

Analysis of the The Powder Toy Simulator

Dec 24 2014

The Powder Toy is a free physics sandbox game, which simulates air pressure and velocity, heat, gravity and a countless number of interactions between different substances. The game provides you with various building materials, liquids, gases and electronic components which can be used to construct complex machines, guns, bombs, realistic terrains and almost anything else. You can browse and play thousands of different saves made by the community or upload your own. However, not everything is that good in the game: for a small project of about 350 files, it triggers too many warnings from our static analyzer. In this article, I'm going to show you the most interesting issues found in the project.

0298_The_Powder_Toy/image1.webp

The Powder Toy was checked by PVS-Studio 5.20. The project is built under Windows in msys with the help of a Python script - that's why I had to use a special utility PVS-Studio Standalone to do the check. To learn more about the standalone version, see the article: PVS-Studio Now Supports Any Build System under Windows and Any Compiler. Easy and Right Out of the Box.

Analysis results

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

At a certain condition, a series of three items of an array of characters are to be checked, but because of a typo, item s[3] can't be checked, which is probably the reason for the program's incorrect behavior in certain 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);
  ....
}

This is a function fragment with suspiciously similar code blocks. The conditional expression contains a series of logical operations, so I assume that it's not that this code fragment contains a pointless check, but there is a typo in the next to last function parameter 'draw_icon()'. That is, a value other than 255 should be written somewhere.

Similar fragments:

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

Instead of clearing the vector, the programmer called the 'empty()' function that doesn't change it. Since the code is inside a destructor, this error doesn't seem to affect program execution in any way. But I still thought this issue to be worth mentioning.

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

This code contains a suspicious piece only its author can conceive. Earlier, if the i-th item of the 'partsData' array was larger than or equal to 161, the value 28 was used to be written into the item. Now, the constant 161 is commented out and replaced with 256, which causes the condition to never be true as the maximum value of 'unsigned char' is 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 pure luck, part of the condition is always true. It is very probable that we are dealing with a typo here: perhaps it was the '||' operator that should have been used instead of '&&', or 'savePreview->Height' should be checked in one of the cases, for example.

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

Everywhere in the code, bit operations are performed over the 'Properties' variable but in two places '||' is used instead of '|'. It means that 1 will be written into Properties there.

Here's another issue of this kind:

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

The 'sandcolour_frame ' variable is used twice in one sequence point. It results in an unpredictable result of such an expression. To learn more, see the description of the V567 diagnostic.

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 a field to its own value.

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

To print a pointer, the %p specifier should be used.

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

It would be more logical to check the 'gameSave' pointer for being null first and only then fill the fields.

A few other similar issues:

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

Operators new, new[], delete, and delete[] should be used in corresponding pairs, i.e. a correct way to write this code is as follows: "delete[] userSession;".

It's not the only issue of this kind in the project:

  • 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;
}

Until the intended modification of this fragment is carried out, the function will keep returning an uninitialized pointer.

Another similar place:

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

Conclusion

The Powder Toy is an interesting cross-platform project that can be used for game, education and experiments. Despite its small size, I found it interesting to look into it. I hope the authors will find time to carry out analysis of the source code and study the complete analysis log.

Using static analysis regularly will help you save plenty of time to solve more serious tasks and TODO's.

Popular related articles
In the world of anthropomorphic animals: PVS-Studio checks Overgrowth

Date: Jun 23 2022

Author: Vladislav Stolyarov

Recently, Wolfire Games released Overgrowth's source code. We couldn't but check the game's quality with the help of PVS-Studio. Let's see where you can find the coolest action: in the game or in its…
Unreal baselining: PVS-Studio's enhancements for Unreal Engine projects

Date: Apr 26 2022

Author: Valery Komarov

The PVS-Studio static analyzer is constantly evolving. We enhance various mechanisms, integrate the analyzer with game engines, IDEs, CI/CD instruments, and other systems and services. A few years ag…
Checking the Ogre3D framework with the PVS-Studio static analyzer

Date: Mar 16 2022

Author: Grigory Semenchev

Developers like graphics engines because they are easy to work with. The PVS-Studio team likes graphics engines because we often find interesting code fragments. One of our readers asked us to analyz…
Thanks, Mario, but the code needs fixing — checking TheXTech

Date: Nov 22 2021

Author: Alexey Govorov

It's cool when enthusiastic developers create a working clone of a famous game. It's even cooler when people are ready to continue the development of such projects! In this article, we check TheXTech…
How the Carla car simulator helped us level up the static analysis of Unreal Engine 4 projects

Date: Nov 19 2021

Author: Konstantin Kochkin

One of the mechanisms of static analysis is method annotations of popular libraries. Annotations provide more information about functions during errors detecting. CARLA is an impressive open-source p…

Comments (0)

Next comments
Unicorn with delicious cookie
Our website uses cookies to enhance your browsing experience. Would you like to learn more?
Accept