Checking The Powder Toy Simulator

    The Powder Toy is a free physics sandbox that simulates the pressure and speed of air, heat, gravity and countless interactions between different substances. The game provides a variety of building materials, liquids, gases and electronic components that can be used to build complex cars, weapons, bombs, realistic terrain, and just about anything else. You can view and play thousands of different buildings made. But the game turned out to be not so wonderful: for a small project of ~ 350 files, quite a lot of warnings were received from the static analyzer. This article will describe the most interesting places.

    The powder toytested with PVS-Studio 5.20. The project is built on Windows in msys using a Python script, so for verification 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 .

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

    Under a certain condition, three consecutive elements of the character array are checked, but due to a typo, the element s [3] was not checked, which, possibly, leads to incorrect behavior of the program 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);
      ....
    }

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

    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 The return value of function 'empty' is required to be utilized. requestbroker.cpp 309
    std::vector Children;
    RequestBroker::Request::~Request()
    {
      std::vector::iterator iter = Children.begin();
      while(iter != Children.end())
      {
        delete (*iter);
        iter++;
      }
      Children.empty();             //<==
    }

    Instead of clearing the vector, the function 'empty ()' was called, 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 ith 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 fulfilled, because the maximum value for '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 luck, part of the condition is always true. There is a high probability of 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 conditional expression is always true: 0x00002. frzw.cpp 34
    unsigned int Properties;
    Element_FRZW::Element_FRZW()
    {
      ....
      Properties = TYPE_LIQUID||PROP_LIFE_DEC;
      ....
    }

    Throughout the code with the variable 'Properties', bitwise 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 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 variable 'sandcolour_frame' is used twice at the same point in the sequence. As a result, it is impossible to predict the outcome of such an expression. Read more in the description of the V567 diagnostic .

    V570 The 'parts [i] .colour' 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 field initialization with an eigenvalue.

    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, use the% p specifier.

    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 first check the 'gameSave' pointer for zero, and then fill in the fields.

    A few more of these places:
    • 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;          //<==
      }
      ....
    }

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

    And this place is not the only one:
    • 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;
    }

    Prior to 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 can be used for games, training and experiments. Despite the small size of the project, it was interesting to figure it out. I hope that 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 in 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 The Powder Toy Simulator .

    Have you read the article and have a question?
    Often our articles are asked the same questions. We collected answers to them here: Answers to questions from readers of articles about PVS-Studio and CppCat, version 2014 . Please see the list.

    Also popular now: