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. You need to run it just before starting building your project. In watch 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 – 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 – open source implementation of RollerCoaster Tycoon 2 (RCT2), extending it with new features and fixing bugs. The gameplay revolves around the construction and maintenance of an amusement park that 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 pressing the button Create, we are greeted by a window with settings.

image7.png

Press Create build configuration…

image9.png

Fill in the fields, click Create… We see a window offering to choose a version control system. Since the source code is already located locally, click Skip…

image11.png

Finally, we move on to the project settings.

image13.png

Let’s add build steps, for this we press: Build steps -> Add build step…

image15.png

Here we select:

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

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

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 some ways to improve their readability. To do this, let’s go: Parameters -> Add new parameter and add three variables:

image19.png

It remains to press the button Run 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 corresponding 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, as an environment variable MSB is the path to the version of MSBuild I need to build

%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 view it directly in the assembly window.

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

The last step is to display the formatted report in stdoutwhere it will be picked up by the TeamCity parser.

type "C:tempptest.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:tempptest.plog_TeamCity.txt"

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

image21.png

Now click on Inspections Totalto go to viewing 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 analyzer warning line number. Navigation from a remote computer is possible when using SourceTreeRoot marker. Those who are interested in this analyzer operation mode can familiarize themselves with the corresponding section. documentation…

Viewing Analyzer Results

After we have finished 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 occurs, the memory is not cleared, and 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 people, except for a static analyzer, could pass this attentiveness test. This example of copy-paste is good for exactly that.

N3 warnings

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 the fields of the parent class in the child. By declaring a field with the same name in the inheritor, we create 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. Expression imageDirection / 8 will be false if imageDirection ranges from -7 to 7. Second part: (imageDirection / 8)! = 3 checks imageDirection 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

V587 An 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 remained on cursorIdwhich also don't make much sense.

Warning N6

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 easy to fix this code, you need to check it a third time player to a null pointer, or insert it into 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);
    }
  }
  ....
}

Warning N7

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

std::optional 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::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. It seems to me that there was a typo either in the condition or when re-assigning the variable ColumnHeaderPressedCurrentState meaning false...

Output

As we can see, integrating the PVS-Studio static analyzer into your TeamCity project is quite simple. All you need to do is write one small configuration file. Code review will allow you to identify problems immediately after the build, 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.

Similar Posts

Leave a Reply

Your email address will not be published. Required fields are marked *