Sviatoslav Razmyslov

Dec 24 2014

Tags:

#Cpp #GameDev

Analysis of the The Powder Toy Simulator

• Analysis results
• Conclusion

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.

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.

#Cpp #GameDev

0

0

0

0

SHARE

Tags:

#Cpp #GameDev

We can email you a selection of our best articles once a month