PVS-Studio: Analyzing Pull Requests In Azure DevOps Using Self-Hosted Agents
PVS-Studio: Analyzing Pull Requests In Azure DevOps Using Self-Hosted Agents
Let's show you how to configure pull request analysis using self-hosted agents in Microsoft Azure DevOps, using the examples of the Minetest game.
Join the DZone community and get the full member experience.Join For Free
Static code analysis is most effective when changing a project, as errors are always more difficult to fix in the future than at an early stage. We continue expanding the options for using PVS-Studio in continuous development systems. This time, we'll show you how to configure pull request analysis using self-hosted agents in Microsoft Azure DevOps, using the example of the Minetest game.
Briefly About What We Are Dealing With
Minetest is an open-source cross-platform game engine containing about 200,000 lines of code in C, C++, and Lua. It allows you to create different game modes in voxel space. Supports multiplayer, and a lot of mods from community. The project repository is located here: https://github.com/minetest/minetest.
The following tools are used to configure regular error detection:
PVS-Studio is a static code analyzer of the code written in C, C++, C#, and Java to search for errors and security defects.
Azure DevOps is a cloud platform that allows you to develop, run applications, and store data on remote servers.
You can use Windows and Linux agent VMs to perform development tasks in Azure. However, running agents on the local equipment has several important advantages:
- The local host may have more resources than an Azure VM;
- The agent doesn't "disappear" after completing its task;
- Ability to directly configure the environment and more flexible management of build processes;
- Local storage of intermediate files has a positive effect on build speed;
- You can complete more than 30 tasks per month for free.
Preparation to Using a Self-Hosted Agent
The process of getting started with Azure is described in detail in the article " PVS-Studio in the Clouds: Azure DevOps", so I will go straight to creating a self-hosted agent.
In order for agents to be able to connect to project pools, they need a special Access Token. You can get it on the "Personal Access Tokens" page, in the "User settings" menu.
After clicking on "New token", you must specify a name and select Read & manage Agent Pools (you may need to expand the full list via "Show all scopes").
You need to copy the token, because Azure will not show it again, and you will have to make a new one.
A Docker container based on Windows Server Core will be used as the agent. The host is my desktop computer on Windows 10 x64 with Hyper-V.
First, you will need to expand the amount of disk space available to Docker containers.
To do this, in Windows, you need to modify the file 'C:\ProgramData\Docker\config\daemon.json' as follows:
To create a Docker image for agents with the build system and everything necessary, let's add a Docker file with the following content in the directory 'D:\docker-agent':
The result is a build system based on MSBuild for C++, with Chocolatey for installing PVS-Studio, CMake, and Git. Vcpkg is built for convenient management of the libraries that the project depends on. Also, we have to download the latest version of the Azure Pipelines Agent.
To initialize the agent from the ENTRYPOINT Docker file, the PowerShell script 'entrypoint.ps1' is called, to which you need to add the URL of the project's "organization", the token of the agent pool, and the PVS-Studio license parameters:
Commands for building an image and starting the agent:
The agent is running and ready to perform tasks.
Running Analysis On a Self-Hosted Agent
For PR analysis, a new pipeline is created with the following script:
This script will work when a PR is received and will be executed on the agents assigned to the pool by default. You only need to give it a permission to work with this pool.
The script saves the list of modified files obtained using git diff. Then the dependencies are updated, the project solution is generated via CMake, and it is built.
If the build was successful, analysis of the changed files is started (the flag '-f diff-files.txt'), ignoring the auxiliary projects created by CMake (select only the necessary project with the '-S minetest ' flag). To make determining relations between header and source C++ files faster, a special cache is created, which will be stored in a separate directory (the flag '-D C:\caches').
This way we can now get reports on analyzing changes in the project.
As mentioned at the beginning of the article, a nice bonus of using self-hosted agents is a noticeable acceleration of task execution, due to local storage of intermediate files.
Some Errors Found In Minetest
Overwriting the result
V519 The 'color_name' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 621, 627. string.cpp 627
This function should parse the color name with the transparency parameter (for example, Green#77) and return its code. Depending on the result of checking the condition, the color_name variable is passed the result of splitting the string or a copy of the function argument. However, the original argument is then converted to lowercase instead of the resulting string itself. As a result, it can't be found in the color dictionary if the transparency parameter is present. We can fix this line like this:
Redundant checks of conditions
V547 Expression 'nearest_emergefull_d == - 1' is always true. clientiface.cpp 363
The nearest_emergefull_d variable doesn't change during the loop operation, and its checking doesn't affect the algorithm execution progress. Either this is the result of a sloppy copy-paste, or they forgot to perform some calculations with it.
V560 A part of conditional expression is always false: y > max_spawn_y. mapgen_v7.cpp 262
The value of the ' y' variable is checked before the next iteration of the loop. A subsequent, opposite comparison will always return false and actually doesn't affect the result of checking the condition.
Missed pointer check
V595 The 'm_client' pointer was utilized before it was verified against nullptr. Check lines: 183, 187. game.cpp 183
Before accessing the m_client pointer, it is checked whether it is null using the assert macro. But this only applies to the debug build. So, this precautionary measure is replaced with a dummy when building to release, and there is a risk of dereferencing the null pointer.
Bit or not bit?
V616 The '(FT_RENDER_MODE_NORMAL)' named constant with the value of 0 is used in the bitwise operation. CGUITTFont.h 360
The FT_LOAD_TARGET_NORMAL macro is deployed to zero, and the bitwise "OR" will not set any flags in load_flags, the else branch can be removed.
Rounding integer division
V636 The 'rect.getHeight() / 16' expression was implicitly cast from 'int' type to 'float' type. Consider utilizing an explicit type cast to avoid the loss of a fractional part. An example: double A = (double)(X) / Y;. hud.cpp 771
Rect getters return integer values. The result of dividing integer numbers is written to a floating-point variable, and the fractional part gets lost. It looks like there are mismatched data types in these calculations.
Suspicious sequence of branching operators
V646 Consider inspecting the application's logic. It's possible that 'else' keyword is missing. treegen.cpp 413
There are else-if sequences in the tree generation algorithm here. In the middle the next if block is on the same line with the closing brace of the previous else statement. Perhaps, the code works correctly: before this if statement, blocks of the trunk are created, followed by leaves. On the other hand, it's possible that else is missed. Only the author can say this for sure.
Incorrect memory allocation check
V668 There is no sense in testing the 'clouds' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. game.cpp 1367
If new can't create an object, an std::bad_alloc exception is thrown, and it must be handled by the try-catch block. A check like this is useless.
Reading outside the array bound
V781 The value of the 'i' index is checked after it was used. Perhaps there is a mistake in program logic. irrString.h 572
Array elements are accessed before checking the index, which may lead to an error. Perhaps the author should rewrite the loop like this:
This article covers the analysis of pull requests in Azure DevOps and doesn't aim to provide a detailed overview of errors found in the Minetest project. Only some code fragments that I found interesting are written here. We suggest that the project authors don't follow this article to correct errors, but perform a more thorough analysis of the warnings that PVS-Studio will issue.
Thanks to its flexible command-line configuration, PVS-Studio analysis can be integrated into a wide variety of CI/CD scenarios. And the correct use of available resources pays off by increasing productivity.
Note that the pull request checking mode is only available in the Enterprise version of the analyzer. To get a demo Enterprise license, specify this in the comments when requesting a license on the download page. You can learn more about the difference between licenses on the Buy PVS-Studio page.
Published at DZone with permission of Alexey Govorov . See the original article here.
Opinions expressed by DZone contributors are their own.