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 in just a few commands, we continue to make this process even more convenient. PVS-Studio has added support for converting the analyzer output into a format for TeamCity β TeamCity Inspections Type. Let's see how it works.
Information about the software used
is a static analyzer of C, C++, C# and Java code, designed to facilitate the task of finding and correcting 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.
- is a monitoring server that monitors compiler launches. It must be run just before you start building your project. In watch mode, the server will intercept runs of all supported compilers. It should be noted that this utility can only be used to analyze C/C++ projects.
β a utility for converting the analyzer report into different formats.
Information about the researched project
Let's try this functionality on a practical example - let's analyze the OpenRCT2 project.
β an open implementation of the game RollerCoaster Tycoon 2 (RCT2), expanding it with new features and fixing bugs. The gameplay revolves around building and maintaining an amusement park that contains rides, shops, and facilities. The player must try to make a profit and maintain the good reputation of the park while keeping the guests happy. OpenRCT2 allows you to play both in scenarios and in the sandbox. The scenarios require the player to complete a certain task within a set time, while the sandbox allows the player to build a more flexible park without any restrictions or finances.
Setting
In order to save time, I will 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:{port specified during the installation process} (in my case, localhost:9090) and enter the data for authorization. Upon entering, we will be greeted by:
Click on the Create Project button. Next, select Manually, fill in the fields.
After pressing the button Create, we are greeted by a window with settings.
Let's press Create build configuration.
Fill in the fields, click Create. We see a window with a proposal to select a version control system. Since the sources are already local, click Skip.
Finally, we move on to the project settings.
Let's add assembly steps, for this we press: Build steps -> Add build step.
Here we choose:
- Runner type -> Command Line
- Run -> Custom Script
Since we will be doing analysis at compile time, build and analysis should be one step, so fill in the field Custom Script:
We will discuss the individual steps later. It is important that loading the analyzer, building the project, analyzing it, outputting the report and formatting it takes only eleven lines of code.
The last thing we need to do is set the environment variables, which I have outlined in some ways to improve their readability. To do this, let's go: Parameters -> Add new parameter and add three variables:
It remains to press the button Run in the upper right corner. While the assembly and analysis of the project is in progress, I will tell you about the script.
Script directly
First, we need to download a fresh PVS-Studio distribution. We use the Chocolatey package manager for this. For those who want to know more about this, there is a corresponding :
choco install pvs-studio -yNext, run the CLMonitor project build tracking utility.
%CLmon% monitor β-attachThen 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:PublishPortableLet's enter the login and PVS-Studio 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 perform static analysis:
%CLmon% analyze -l "c:ptest.plog"After we 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 stdout, where the TeamCity parser will pick it up.
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 been successfully completed, we can go to the tab Projects and make sure of it.
Now click on Inspections Totalto view the analyzer report:
Warnings are grouped by diagnostic rules numbers. To navigate through the code, click on the line number with the warning. Clicking on the question mark in the upper right corner will open a new documentation tab. You can also navigate through the code by clicking on the line number with the analyzer warning. Navigation from a remote computer is possible using SourceTreeRoot marker. Anyone who is interested in this mode of analyzer operation can read the corresponding section. .
Viewing analyzer results
Now that we are done with deploying and configuring the build, let's take a look at some interesting warnings found in the project under investigation.
Warning N1
[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, respectively, a memory leak occurs.
Warning N2
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 test of attentiveness. This copy-paste example is good for just that.
N3 warnings
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 descendant 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 fields with the same name in the successor, we introduce confusion.
Warning N4
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 is in the range from -7 to 7. Second part: (imageDirection / 8) != 3 checks imageDirection for being out of range: from -31 to -24 and from 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 to more explicit ones. This would make life much easier for the people who will read and maintain this code.
Warning N5
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 code snippet was most likely obtained by decompiling. Then, judging by the comment left, part of the non-working code was removed. However, a couple of operations remained on cursorId, which also don't make much sense.
Warning N6
[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); // <=
}
....
}This code is quite easy to fix, 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
[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 a hard-to-read line of code in one fell swoop and solve the problem with checking for nullptr. I suggest changing the code like this:
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
[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.
Hack and predictor Aviator
As we can see, integrating the PVS-Studio static analyzer into your TeamCity project is quite simple. To do this, it is enough to write just one small configuration file. Checking the code, on the other hand, will allow you to identify problems immediately after the build, which will help you fix them when the complexity and cost of changes are still small.
If you want to share this article with an English-speaking audience, then please use the link to the translation: Vladislav Stolyarov. .
Source: habr.com