Clever Geek Handbook

PVS-Studio and Continuous Integration: TeamCity. Open RollerCoaster Tycoon 2 Project Analysis



One of the most relevant scenarios for using the PVS-Studio analyzer is its integration with CI systems. And although the analysis of a PVS-Studio project from almost any continuous integration system can be built into just a few commands, we continue to make this process even more convenient. PVS-Studio now supports converting the analyzer output to the format for TeamCity - TeamCity Inspections Type. Let's see how it works.



Information about the software used



PVS-Studio is a static analyzer of , ++, C # and Java code designed to facilitate the task of finding and fixing various kinds of errors. The analyzer can be used on Windows, Linux and macOS. In this article, we will actively use not only the analyzer itself, but also some utilities from its distribution kit.



CLMonitor is a monitoring server that monitors compiler launches. It must be run immediately before starting building your project. In monitor mode, the server will intercept the runs of all supported compilers. It should be noted that this utility can only be used to analyze C / C ++ projects.



PlogConverter is a utility for converting the analyzer report into different formats.



Information about the investigated project



Let's try this functionality with a practical example - let's analyze the OpenRCT2 project.



OpenRCT2 is an open source implementation of RollerCoaster Tycoon 2 (RCT2), extending it with new features and bug fixes. The gameplay revolves around the construction and maintenance of an amusement park, which houses attractions, shops and facilities. The player should try to make a profit and maintain a good reputation for the park while keeping the guests happy. OpenRCT2 allows you to play in both scenario and sandbox. Scenarios require the player to complete a specific task at a set time, while the sandbox allows the player to build a more flexible park without any restrictions or finances.



Setting up



In order to save time, I'll probably skip the installation process and start from the moment when the TeamCity server is running on my computer. We need to go to: localhost: {the port specified during the installation} (in my case, localhost: 9090) and enter the authorization data. After entering we will be met by:



image3.png


Click on the Create Project button. Next, select Manually, fill in the fields.



image5.png


After clicking on the Create button , we are greeted by a window with settings.



image7.png


Click Create build configuration .



image9.png


Fill in the fields, click Create . We see a window offering to choose a version control system. Since the sources are already located locally, click Skip .



image11.png


Finally, we move on to the project settings.



image13.png


Add build steps by clicking: Build steps -> Add build step .



image15.png


Here we select:



  • Runner type -> Command Line
  • Run -> Custom Script


Since we will be analyzing during compilation of the project, assembly and analysis should be one step, so fill in the Custom Script field :



image17.png


We will dwell on individual steps later. It is important that loading the analyzer, building the project, analyzing it, outputting the report and formatting it all take only eleven lines of code.



The last thing we need to do is set the environment variables, which I have indicated in some ways to improve their readability. To do this, go to: Parameters -> Add new parameter and add three variables:



image19.png


It remains to click on the Run button in the upper right corner. While the project is being assembled and analyzed, I will tell you about the script.



Directly script



First, we need to download the latest PVS-Studio distribution. For this we use the hocolatey package manager. For those who want to know more about this, there is a related article : 



choco install pvs-studio -y


Next, let's launch the CLMonitor project assembly tracking utility. 



%CLmon% monitor –-attach


Then we will build the project, the path to the version of MSBuild I need to build is used as the MSB environment variable 



%MSB% %ProjPath% /t:clean
%MSB% %ProjPath% /t:rebuild /p:configuration=release
%MSB% %ProjPath% /t:g2
%MSB% %ProjPath% /t:PublishPortable


Let's enter the PVS-Studio login and license key: 



%PVS-Studio_cmd% credentials --username %PVS_Name% --serialNumber %PVS_Key%


After the build is complete, run CLMonitor again to generate preprocessed files and static analysis: 



%CLmon% analyze -l "c:\ptest.plog"


Then we will use one more utility from our distribution kit. PlogConverter converts the report from standard to TeamCity specific format. Thanks to this, we will be able to see it right in the assembly window. 



%PlogConverter% "c:\ptest.plog" --renderTypes=TeamCity -o "C:\temp"


The last step is to output the formatted report to stdout , where it will be picked up by the TeamCity parser. 



type "C:\temp\ptest.plog_TeamCity.txt"


Full script code: 



choco install pvs-studio -y
%CLmon% monitor --attach
set platform=x64
%MSB% %ProjPath% /t:clean
%MSB% %ProjPath% /t:rebuild /p:configuration=release
%MSB% %ProjPath% /t:g2
%MSB% %ProjPath% /t:PublishPortable
%PVS-Studio_cmd% credentials --username %PVS_Name% --serialNumber %PVS_Key%
%CLmon% analyze -l "c:\ptest.plog"
%PlogConverter% "c:\ptest.plog" --renderTypes=TeamCity -o "C:\temp"
type "C:\temp\ptest.plog_TeamCity.txt"


In the meantime, the assembly and analysis of the project has been successfully completed, we can go to the Projects tab and verify this.



image21.png


Now click on Inspections Total to go to view the analyzer report:



image23.png


Warnings are grouped by diagnostic rule numbers. To navigate through the code, you need to click on the line number with the warning. Clicking on the question mark in the upper right corner will open a new documentation tab for you. You can also navigate through the code by clicking on the parser warning line number. Navigation from a remote computer is possible using the SourceTreeRoot marker. Those who are interested in this analyzer operation mode can familiarize themselves with the corresponding section of the documentation .



Viewing Analyzer Results



After we're done with deploying and configuring the build, I suggest looking at some interesting warnings found in the project under study.



Warning N1



V773 [CWE-401] The exception was thrown without releasing the 'result' pointer. A memory leak is possible. libopenrct2 ObjectFactory.cpp 443 



Object* CreateObjectFromJson(....)
{
  Object* result = nullptr;
  ....
  result = CreateObject(entry);
  ....
  if (readContext.WasError())
  {
    throw std::runtime_error("Object has errors");
  }
  ....
}

Object* CreateObject(const rct_object_entry& entry)
{
  Object* result;
  switch (entry.GetType())
  {
    case OBJECT_TYPE_RIDE:
      result = new RideObject(entry);
      break;
    case OBJECT_TYPE_SMALL_SCENERY:
      result = new SmallSceneryObject(entry);
      break;
    case OBJECT_TYPE_LARGE_SCENERY:
      result = new LargeSceneryObject(entry);
      break;
    ....
    default:
      throw std::runtime_error("Invalid object type");
  }
  return result;
}


The analyzer noticed an error that after dynamic memory allocation in CreateObject , when an exception is thrown, the memory is not cleared; accordingly, a memory leak occurs.



Warning N2



V501 There are identical sub-expressions '(1ULL << WIDX_MONTH_BOX)' to the left and to the right of the '|' operator. libopenrct2ui Cheats.cpp 487 



static uint64_t window_cheats_page_enabled_widgets[] = 
{
  MAIN_CHEAT_ENABLED_WIDGETS |
  (1ULL << WIDX_NO_MONEY) |
  (1ULL << WIDX_ADD_SET_MONEY_GROUP) |
  (1ULL << WIDX_MONEY_SPINNER) |
  (1ULL << WIDX_MONEY_SPINNER_INCREMENT) |
  (1ULL << WIDX_MONEY_SPINNER_DECREMENT) |
  (1ULL << WIDX_ADD_MONEY) |
  (1ULL << WIDX_SET_MONEY) |
  (1ULL << WIDX_CLEAR_LOAN) |
  (1ULL << WIDX_DATE_SET) |
  (1ULL << WIDX_MONTH_BOX) |  // <=
  (1ULL << WIDX_MONTH_UP) |
  (1ULL << WIDX_MONTH_DOWN) |
  (1ULL << WIDX_YEAR_BOX) |
  (1ULL << WIDX_YEAR_UP) |
  (1ULL << WIDX_YEAR_DOWN) |
  (1ULL << WIDX_DAY_BOX) |
  (1ULL << WIDX_DAY_UP) |
  (1ULL << WIDX_DAY_DOWN) |
  (1ULL << WIDX_MONTH_BOX) |  // <=
  (1ULL << WIDX_DATE_GROUP) |
  (1ULL << WIDX_DATE_RESET),
  ....
};


Few other than a static analyzer could pass this attentiveness test. This example of copy-paste is good for exactly that.



N3



V703 It is odd that the 'flags' field in derived class 'RCT12BannerElement' overwrites field in base class 'RCT12TileElementBase'. Check lines: RCT12.h: 570, RCT12.h: 259. libopenrct2 RCT12.h 570 



struct RCT12SpriteBase
{
  ....
  uint8_t flags;
  ....
};
struct rct1_peep : RCT12SpriteBase
{
  ....
  uint8_t flags;
  ....
};


Of course, using a variable with the same name in the base class and in the inheritor is not always a mistake. However, the inheritance technology itself assumes the presence of all fields of the parent class in the child. By declaring a field with the same name in the inheritor, we introduce confusion.



Warning N4



V793 It is odd that the result of the 'imageDirection / 8' statement is a part of the condition. Perhaps this statement should have been compared with something else. libopenrct2 ObservationTower.cpp 38 



void vehicle_visual_observation_tower(...., int32_t imageDirection, ....)
{
  if ((imageDirection / 8) && (imageDirection / 8) != 3)
  {
    ....
  }
  ....
}


Let's take a closer look. The imageDirection / 8 expression will be false if the imageDirection is in the range -7 to 7. The second part: (imageDirection / 8)! = 3 checks the imageDirection to be out of range: -31 to -24 and 24 to 31 respectively. It seems rather strange to me to check numbers for entering a certain range in this way, and even if there is no error in this code fragment, I would recommend rewriting these conditions for more explicit ones. This would make life much easier for people who will read and maintain this code.



Warning N5



V587An odd sequence of assignments of this kind: A = B; B = A ;. Check lines: 1115, 1118.libopenrct2ui MouseInput.cpp 1118 



void process_mouse_over(....)
{
  ....
  switch (window->widgets[widgetId].type)
  {
    case WWT_VIEWPORT:
      ebx = 0;
      edi = cursorId;                                 // <=
      // Window event WE_UNKNOWN_0E was called here,
      // but no windows actually implemented a handler and
      // it's not known what it was for
      cursorId = edi;                                 // <=
      if ((ebx & 0xFF) != 0)
      {
        set_cursor(cursorId);
        return;
      }
      break;
      ....
  }
  ....
}


This piece of code was most likely obtained by decompilation. Then, judging by the comment left, some of the non-working code was removed. However, a couple of operations on cursorId remain , which also do not make much sense.



N6 Warning



V1004 [CWE-476] The 'player' pointer was used unsafely after it was verified against nullptr. Check lines: 2085, 2094.libopenrct2 Network.cpp 2094 



void Network::ProcessPlayerList()
{
  ....
  auto* player = GetPlayerByID(pendingPlayer.Id);
  if (player == nullptr)
  {
    // Add new player.
    player = AddPlayer("", "");
    if (player)                                          // <=
    {
      *player = pendingPlayer;
       if (player->Flags & NETWORK_PLAYER_FLAG_ISSERVER)
       {
         _serverConnection->Player = player;
       }
    }
    newPlayers.push_back(player->Id);                    // <=
  }
  ....
}


It is quite simple to fix this code, you need to either check the player for a null pointer a third time , or add it to the body of the conditional operator. I would suggest the second option: 



void Network::ProcessPlayerList()
{
  ....
  auto* player = GetPlayerByID(pendingPlayer.Id);
  if (player == nullptr)
  {
    // Add new player.
    player = AddPlayer("", "");
    if (player)
    {
      *player = pendingPlayer;
      if (player->Flags & NETWORK_PLAYER_FLAG_ISSERVER)
      {
        _serverConnection->Player = player;
      }
      newPlayers.push_back(player->Id);
    }
  }
  ....
}


N7 Warning



V547 [CWE-570] Expression 'name == nullptr' is always false. libopenrct2 ServerList.cpp 102 



std::optional<ServerListEntry> ServerListEntry::FromJson(...)
{
  auto name = json_object_get(server, "name");
  .....
  if (name == nullptr || version == nullptr)
  {
    ....
  }
  else
  {
    ....
    entry.name = (name == nullptr ? "" : json_string_value(name));
    ....
  }
  ....
}


You can get rid of the hard-to-read line of code in one fell swoop and solve the problem with checking for nullptr . I suggest changing the code as follows: 



std::optional<ServerListEntry> ServerListEntry::FromJson(...)
{
  auto name = json_object_get(server, "name");
  .....
  if (name == nullptr || version == nullptr)
  {
    name = ""
    ....
  }
  else
  {
    ....
    entry.name = json_string_value(name);
    ....
  }
  ....
}


Warning N8



V1048 [CWE-1164] The 'ColumnHeaderPressedCurrentState' variable was assigned the same value. libopenrct2ui CustomListView.cpp 510 



void CustomListView::MouseUp(....)
{
  ....
  if (!ColumnHeaderPressedCurrentState)
  {
    ColumnHeaderPressed = std::nullopt;
    ColumnHeaderPressedCurrentState = false;
    Invalidate();
  }
}


The code looks pretty weird. I think there was a typo or be subject to, or when re-assigning variable ColumnHeaderPressedCurrentState value to false .



Output



As we can see, integrating the PVS-Studio static analyzer into your TeamCity project is quite simple. It is enough to write just one small configuration file for this. Code review will allow you to identify problems immediately after assembly, which will help to eliminate them when the complexity and cost of edits are still small.





If you want to share this article with an English-speaking audience, please use the translation link: Vladislav Stolyarov. PVS-Studio and Continuous Integration: TeamCity. Analysis of the Open RollerCoaster Tycoon 2 project .


More articles:


All Articles