This is a new piece of our series of articles about using the PVS-Studio static analyzer with cloud CI systems. Today we are going to look at another service, CircleCI. We'll take the Kodi media player application as a test project and see if we can find any interesting bugs in its source code.
Note. The previous articles on integrating PVS-Studio with cloud CI systems:
Before setting up the work environment and examining the analysis report, I'd like to say a few words about the software we are going to use and check.
CircleCI is a cloud CI service for automated software building, testing, and deployment. It supports project building both in containers and on virtual machines on Windows, Linux, and macOS.
Kodi is a free and open-source cross-platform media player application. It allows users to play and view most streaming media, such as videos, music, podcasts, and videos from the Internet, as well as all common digital media files from local and network storage media. It supports the use of themes and skins and functionality extensions through plugins. Kodi is available for Windows, Linux, macOS, and Android.
PVS-Studio is a static analyzer for detecting bugs and potential vulnerabilities in the source code of applications written in C, C++, C#, and Java. The analyzer runs on Windows, Linux, and macOS.
Setting up
First we need to go to the CircleCI main page and click «Sign Up»
On the next page, we are offered to authorize with a GitHub or Bitbucket account. We choose GitHub and get to the CircleCI authorization page.
After authorizing the application (by clicking the green button «Authorize circleci»), we are redirected to the «Welcome to CircleCI!» page:
Here we can specify right away which projects we want CircleCI to build. We tick our repository and click «Follow».
After adding the repository, CircleCI will automatically start the build process, but since we don't have a configuration file in our repository yet, the build job will be aborted with an error message.
Before adding a configuration file, we need to add a couple of variables containing analyzer license data. To do that, we click «Settings» on the left sidebar, select «Projects» in the «ORGANIZATION» section, and click the cogwheel button to the right of our project's name. A settings window will appear.
We go to the «Environment Variables» page. Here we create two variables, PVS_USERNAME and PVS_KEY, which contain the username and analyzer license key.
When starting the build, CircleCI reads the job configuration from the file stored in the repository at .circleci/config.yml. Let's add it.
First we need to specify the image of the virtual machine that the analyzer will be running on. The complete list of images is available here.
version: 2
jobs:
build:
machine:
image: ubuntu-1604:201903-01Next, we add the necessary repositories to apt and install the project's dependencies:
steps:
- checkout
- run: sudo -- sh -c "
add-apt-repository -y ppa:team-xbmc/xbmc-ppa-build-depends
&& add-apt-repository -y ppa:wsnipex/vaapi
&& add-apt-repository -y ppa:pulse-eight/libcec
&& apt-get update"
- run: sudo apt-get install -y
automake autopoint build-essential cmake
curl default-jre gawk gdb gdc gettext git-core
gperf libasound2-dev libass-dev libbluray-dev
libbz2-dev libcap-dev libcdio-dev libcec4-dev
libcrossguid-dev libcurl3 libcurl4-openssl-dev
libdbus-1-dev libegl1-mesa-dev libfmt3-dev
libfontconfig-dev libfreetype6-dev libfribidi-dev
libfstrcmp-dev libgif-dev libgl1-mesa-dev
libglu1-mesa-dev libiso9660-dev libjpeg-dev
liblcms2-dev libltdl-dev liblzo2-dev libmicrohttpd-dev
libmysqlclient-dev libnfs-dev libpcre3-dev libplist-dev
libpng-dev libpulse-dev libsmbclient-dev libsqlite3-dev
libssl-dev libtag1-dev libtinyxml-dev libtool libudev-dev
libusb-dev libva-dev libvdpau-dev libxml2-dev libxmu-dev
libxrandr-dev libxrender-dev libxslt1-dev libxt-dev
mesa-utils nasm pmount python-dev python-imaging
python-sqlite rapidjson-dev swig unzip uuid-dev yasm
zip zlib1g-dev wgetAdding the PVS-Studio repository and installing the analyzer:
- run: wget -q -O - https://files.viva64.com/etc/pubkey.txt
| sudo apt-key add -
&& sudo wget -O /etc/apt/sources.list.d/viva64.list
https://files.viva64.com/etc/viva64.list
- run: sudo -- sh -c "apt-get update
&& apt-get install pvs-studio -y"Then we are building the dependencies:
- run: sudo make -C tools/depends/target/flatbuffers PREFIX=/usr/localAfter that, we generate Makefiles in the build directory:
- run: mkdir build && cd build && cmake -DCMAKE_BUILD_TYPE=Debug ..
The next step is setting up and starting an analysis of the project.
First we create an analyzer license file. Another command will start tracing the project build by the compiler.
The next command following the tracing runs the analysis as such. If you use a demo version of PVS-Studio, launch it with the parameter:
--disableLicenseExpirationCheck.
The final command converts the analyzer's report file into an html report:
- run: pvs-studio-analyzer credentials -o PVS.lic ${PVS_USER} ${PVS_KEY}
- run: pvs-studio-analyzer trace -- make -j2 -C build/
- run: pvs-studio-analyzer analyze -j2 -l PVS.lic
-o PVS-Studio.log --disableLicenseExpirationCheck
- run: plog-converter -t html -o PVS-Studio.html PVS-Studio.logOnce the tests are over, we save the reports:
- run: mkdir PVS_Result && cp PVS-Studio.* ./PVS_Result/
- store_artifacts:
path: ./PVS_ResultHere's the complete text of the .circleci/config.yml file:
version: 2.1
jobs:
build:
machine:
image: ubuntu-1604:201903-01
steps:
- checkout
- run: sudo -- sh -c "
add-apt-repository -y ppa:team-xbmc/xbmc-ppa-build-depends
&& add-apt-repository -y ppa:wsnipex/vaapi
&& add-apt-repository -y ppa:pulse-eight/libcec
&& apt-get update"
- run: sudo apt-get install -y automake autopoint
build-essential cmake curl default-jre gawk gdb
gdc gettext git-core gperf libasound2-dev libass-dev
libbluray-dev libbz2-dev libcap-dev libcdio-dev
libcec4-dev libcrossguid-dev libcurl3 libcurl4-openssl-dev
libdbus-1-dev libegl1-mesa-dev libfmt3-dev libfontconfig-dev
libfreetype6-dev libfribidi-dev libfstrcmp-dev libgif-dev
libgl1-mesa-dev libglu1-mesa-dev libiso9660-dev libjpeg-dev
liblcms2-dev libltdl-dev liblzo2-dev libmicrohttpd-dev
libmysqlclient-dev libnfs-dev libpcre3-dev libplist-dev
libpng-dev libpulse-dev libsmbclient-dev libsqlite3-dev
libssl-dev libtag1-dev libtinyxml-dev libtool libudev-dev
libusb-dev libva-dev libvdpau-dev libxml2-dev libxmu-dev
libxrandr-dev libxrender-dev libxslt1-dev libxt-dev mesa-utils
nasm pmount python-dev python-imaging python-sqlite
rapidjson-dev swig unzip uuid-dev yasm zip zlib1g-dev wget
- run: wget -q -O - https://files.viva64.com/etc/pubkey.txt
| sudo apt-key add –
&& sudo wget -O /etc/apt/sources.list.d/viva64.list
https://files.viva64.com/etc/viva64.list
- run: sudo -- sh -c "apt-get update && apt-get install pvs-studio -y"
- run: sudo make -C tools/depends/target/flatbuffers PREFIX=/usr/local
- run: mkdir build && cd build && cmake -DCMAKE_BUILD_TYPE=Debug ..
- run: pvs-studio-analyzer credentials -o PVS.lic ${PVS_USER} ${PVS_KEY}
- run: pvs-studio-analyzer trace -- make -j2 -C build/
- run: pvs-studio-analyzer analyze -j2 -l PVS.lic
-o PVS-Studio.log --disableLicenseExpirationCheck
- run: plog-converter -t html -o PVS-Studio.html PVS-Studio.log
- run: mkdir PVS_Result && cp PVS-Studio.* ./PVS_Result/
- store_artifacts:
path: ./PVS_ResultOnce this file is uploaded to the repository, CircleCI will automatically start the build.
Once the job is finished, the files with the analysis results can be downloaded on the «Artifacts» tab.
Analysis results
OK, now let's take a look at some of the warnings output by the analyzer.
PVS-Studio warning: V504 It is highly probable that the semicolon ';' is missing after 'return' keyword. AdvancedSettings.cpp:1476
void CAdvancedSettings::SetExtraArtwork(const TiXmlElement* arttypes,
std::vector<std::string>& artworkMap)
{
if (!arttypes)
return
artworkMap.clear();
const TiXmlNode* arttype = arttypes->FirstChild("arttype");
....
}The code formatting suggests the following execution logic:
- if arttypes is a null pointer, the method returns;
- if arttypes is a non-null pointer, the artworkMap vector gets cleared and some actions are then performed.
But the missing ';' character breaks it all, and the actual execution logic is as follows:
- if arttypes is a null pointer, the artworkMap vector gets cleared and the method returns;
- if arttypes is a non-null pointer, the program executes whatever actions come next but the artworkMap vector doesn't get cleared.
To cut a long story short, this situation does look like a bug. After all, you hardly expect anyone to write expressions like return artworkMap.clear(); :).
PVS-Studio warnings:
- V547 Expression 'lastsector' is always false. udf25.cpp:636
- V547 Expression 'lastsector' is always false. udf25.cpp:644
- V571 Recurring check. The 'if (lastsector)' condition was already verified in line 636. udf25.cpp:644
int udf25::UDFGetAVDP( struct avdp_t *avdp)
{
....
uint32_t lastsector;
....
lastsector = 0; // <=
....
for(;;) {
....
if( lastsector ) { // <= V547
lbnum = lastsector;
terminate = 1;
} else {
//! @todo Find last sector of the disc (this is optional).
if( lastsector ) // <= V547
lbnum = lastsector - 256;
else
return 0;
}
}
....
}
Note the spots marked with // <=. The lastsector variable is assigned the value 0 and then used as a conditional expression in two if statements. Since the value doesn't change either in the loop or between the assignments, control will never get into the else branches of both if statements.
However, it could also mean that the developers simply haven't implemented the intended functionality yet (note the todo remark).
By the way, as you probably noticed, this snippet triggered three warnings at once. But even that many warnings for one piece of code wouldn't look convincing enough to some users, and they would keep believing that the analyzer is wrong… This aspect is discussed in detail in a post by one of my teammates: "One Day from PVS-Studio User Support" :).
PVS-Studio warning: V547 Expression 'values.size() != 2' is always false. GUIControlSettings.cpp:1174
bool CGUIControlRangeSetting::OnClick()
{
....
std::vector<CVariant> values;
SettingConstPtr listDefintion = settingList->GetDefinition();
switch (listDefintion->GetType())
{
case SettingType::Integer:
values.push_back(m_pSlider->
GetIntValue(CGUISliderControl::RangeSelectorLower));
values.push_back(m_pSlider->
GetIntValue(CGUISliderControl::RangeSelectorUpper));
break;
case SettingType::Number:
values.push_back(m_pSlider->
GetFloatValue(CGUISliderControl::RangeSelectorLower));
values.push_back(m_pSlider->
GetFloatValue(CGUISliderControl::RangeSelectorUpper));
break;
default:
return false;
}
if (values.size() != 2)
return false;
SetValid(CSettingUtils::SetList(settingList, values));
return IsValid();
}The values.size() != 2 check is redundant here since this conditional expression will always evaluate to false. Indeed, if execution enters one of the case branches of the switch statement, two elements will be added to the vector, and since it was initially empty, its size will naturally become equal to 2; otherwise (i.e. if the default branch is executed), the method will return.
PVS-Studio warning: V547 Expression 'prio == 0x7fffffff' is always true. DBusReserve.cpp:57
bool CDBusReserve::AcquireDevice(const std::string& device)
{
....
int prio = INT_MAX;
....
res =
dbus_bus_request_name(
m_conn,
service.c_str(),
DBUS_NAME_FLAG_DO_NOT_QUEUE |
(prio == INT_MAX ? 0 : DBUS_NAME_FLAG_ALLOW_REPLACEMENT), // <=
error);
....
}The prio variable is initialized to the INT_MAX value and then used as an operand of the ternary operator in the prio == INT_MAX comparison, though its value doesn't change after the initialization. It means the prio == INT_MAX expression is true and the ternary operator will always return 0.
PVS-Studio warnings:
- V575 The potential null pointer is passed into 'memcpy' function. Inspect the first argument. Check lines: 39, 38. DVDOverlayImage.h:39
- V575 The potential null pointer is passed into 'memcpy' function. Inspect the first argument. Check lines: 44, 43. DVDOverlayImage.h:44
CDVDOverlayImage(const CDVDOverlayImage& src)
: CDVDOverlay(src)
{
Data = (uint8_t*)malloc(src.linesize * src.height);
memcpy(data, src.data, src.linesize * src.height); // <=
if(src.palette)
{
palette = (uint32_t*)malloc(src.palette_colors * 4);
memcpy(palette, src.palette, src.palette_colors * 4); // <=
}
....
}Both warnings have the same pattern: a pointer returned by the malloc function is used further in the memcpy function without being checked for NULL first.
Some may argue that malloc will never return a null pointer, and if it does, it would be better for the application to crash. It's a subject of a separate discussion, but whatever your opinion is, I recommend reading this post by my teammate: "Why it is important to check what the malloc function returned".
If you wish, you can customize the analyzer so that it doesn't assume that malloc could return a null pointer – this will keep it from outputting this type of warnings. More details can be found here.
PVS-Studio warning: V522 There might be dereferencing of a potential null pointer 'entry'. Check lines: 985, 981. emu_msvcrt.cpp:985
struct dirent *dll_readdir(DIR *dirp)
{
....
struct dirent *entry = NULL;
entry = (dirent*) malloc(sizeof(*entry));
if (dirData->curr_index < dirData->items.Size() + 2)
{
if (dirData->curr_index == 0)
strncpy(entry->d_name, ".\0", 2);
....
}This example is similar to the previous one. The pointer returned by the malloc function is stored to the entry variable, and this variable is then used without a prior null check (entry->d_name).
PVS-Studio warning: V773 Visibility scope of the 'progressHandler' pointer was exited without releasing the memory. A memory leak is possible. PVRGUIChannelIconUpdater.cpp:94
void CPVRGUIChannelIconUpdater::SearchAndUpdateMissingChannelIcons() const
{
....
CPVRGUIProgressHandler* progressHandler =
new CPVRGUIProgressHandler(g_localizeStrings.Get(19286));
for (const auto& group : m_groups)
{
const std::vector<PVRChannelGroupMember> members = group->GetMembers();
int channelIndex = 0;
for (const auto& member : members)
{
progressHandler->UpdateProgress(member.channel->ChannelName(),
channelIndex++, members.size());
....
}
progressHandler->DestroyProgress();
}The value of the progressHandler pointer was returned by the new operator. But there is no delete operator for this pointer. This means a memory leak.
PVS-Studio warning: V557 Array overrun is possible. The 'idx' index is pointing beyond array bound. PlayerCoreFactory.cpp:240
std::vector<CPlayerCoreConfig *> m_vecPlayerConfigs;
bool CPlayerCoreFactory::PlaysVideo(const std::string& player) const
{
CSingleLock lock(m_section);
size_t idx = GetPlayerIndex(player);
if (m_vecPlayerConfigs.empty() || idx > m_vecPlayerConfigs.size())
return false;
return m_vecPlayerConfigs[idx]->m_bPlaysVideo;
}The if statement restricts the m_vecPlayerConfigs vector's size to a certain range by having the method return if the size-checking condition is true. As a result, when execution reaches the last return statement, the m_vecPlayerConfigs vector's size will be within the specified range, [1; idx]. But a couple of lines later, the program is indexing the vector at idx: m_vecPlayerConfigs[idx]->m_bPlaysVideo. It means that if idx is equal to the vector's size, we'll be indexing beyond the valid range.
Let's wind up this article with a couple of examples from the code of the Platinum library.
PVS-Studio warning: V542 Consider inspecting an odd type cast: 'bool' to 'char *'. PltCtrlPoint.cpp:1617
NPT_Result PLT_CtrlPoint::ProcessSubscribeResponse(...)
{
....
bool subscription = (request.GetMethod().ToUppercase() == "SUBSCRIBE");
....
NPT_String prefix = NPT_String::Format("
PLT_CtrlPoint::ProcessSubscribeResponse %ubscribe for
service \"%s\" (result = %d, status code = %d)",
(const char*)subscription?"S":"Uns", // <=
(const char*)service->GetServiceID(),
res,
response?response->GetStatusCode():0);
....
}The developers have had wrong assumptions about the operations' precedence. What gets cast to const char* is not the result returned by the ternary operator (subscription? «S»: «Uns») but the subscription variable. This looks strange, at the very least.
PVS-Studio warning: V560 A part of conditional expression is always false: c == '\t'. NptUtils.cpp:863
NPT_Result NPT_ParseMimeParameters(....)
{
....
case NPT_MIME_PARAMETER_PARSER_STATE_NEED_EQUALS:
if (c < ' ') return NPT_ERROR_INVALID_SYNTAX; // END or CTLs are invalid
if (c == ' ' || c == '\t') continue; // ignore leading whitespace
....
}The code of the space character is 0x20, and the code of the tab character is 0x09. Therefore, the c == '\t' subexpression will always evaluate to false as this case is already covered by the c < ' ' check (which, if true, will cause the function to return).
Conclusion
As this article demonstrates, we successfully set up an analysis by PVS-Studio on yet another CI system (CircleCI). I invite you to download and try the analyzer on your own project. If you have any questions about the setting-up or use of PVS-Studio, don't hesitate to contact us – we'll be glad to help.
And, of course, we wish you bugless code. :)
