?

Log in

No account? Create an account

Previous Entry | Next Entry

Author: Svyatoslav Razmyslov

In order to demonstrate our analyzer's diagnostic capabilities, we analyze open-source projects and write articles to discuss any interesting bugs we happen to find. We always encourage our users to suggest interesting open-source projects for analysis, and note down all the suggestions we receive via e-mail. Sometimes they come from people closely related to the project we are asked to check. In this article, we will tell you about a check of the components of the OpenVZ project we have been asked to analyze by the project manager Sergey Bronnikov.



About PVS-Studio and OpenVZ



PVS-Studio is a static analyzer designed to detect errors in source code of C/C++ applications. It can be downloaded from the official website but is available for operating systems of the Windows family only. Therefore, to be able to analyze OpenVZ components in Linux, we had to use a PVS-Studio beta-version we once used to check the Linux Kernel.

OpenVZ is an operating system-level virtualization technology based on the Linux kernel and operating system. OpenVZ allows a physical server to run multiple isolated operating system instances, called containers, virtual private servers (VPSs), or virtual environments (VEs).

Analysis results

OpenVZ components are small-sized projects, so there were relatively few warnings yet they were very characteristic of software written in C++.

Troubles with pointers

V595 The 'plog' pointer was utilized before it was verified against nullptr. Check lines: 530, 531. CPackedProblemReport.cpp 530

void
CPackedProblemReport::appendSystemLog( CRepSystemLog * plog )
{
QString strPathInTemp = m_strTempDirPath + QString("/") +
QFileInfo( plog->getName() ).fileName();
if( !plog )
{
QFile::remove( strPathInTemp );
return;
}
....
}


It's a genuine pointer handling bug. The 'plog' pointer is dereferenced right after entering the function and only then is checked for being valid.

V595 The 'd' pointer was utilized before it was verified against nullptr. Check lines: 1039, 1041. disk.c 1039

int vzctl2_add_disk(....)
{
....
if (created)
destroydir(d->path);
if (d)
free_disk(d);
return ret;
}


In this case, when the 'created' flag is set, the 'd' variable will be dereferenced, though the next code line suggests that the pointer may be null. This code fragment can be rewritten in the following way:

int vzctl2_add_disk(....)
{
....
if (d)
{
if (created)
  destroydir(d->path);
free_disk(d);
}
return ret;
}


V595 The 'param' pointer was utilized before it was verified against nullptr. Check lines: 1874, 1876. env.c 1874

int
vzctl2_env_set_veth_param(....,
                      struct vzctl_veth_dev_param *param,
                      int size)
{
int ret;
struct vzctl_ip_param *ip = NULL;
struct vzctl_veth_dev_param tmp = {};
memcpy(&tmp, param, size);
if (param == NULL || tmp.dev_name_ve == NULL)
return VZCTL_E_INVAL;
....
}


The 'memcpy' function copies the contents of one memory area into another. The second function parameter is a pointer to the source address. The function contains a check of the 'param' pointer for null, but before that the pointer is used by the 'memcpy' function, which may cause a null-pointer dereferencing operation.

V595 The 'units' pointer was utilized before it was verified against nullptr. Check lines: 607, 610. wrap.c 607

int vzctl_set_cpu_param(....)
{
....
if (weight != NULL &&
  (ret = vzctl2_env_set_cpuunits(param, *units * 500000)))
goto err;
if (units != NULL &&
  (ret = vzctl2_env_set_cpuunits(param, *units)))
goto err;
....
}


The analyzer's warning about the 'units' pointer being checked for null before being used may hint at a typo in this code. In the first condition, the 'weight' pointer being checked is not used. It should have probably looked like this:

if (weight != NULL &&
(ret = vzctl2_env_set_cpuunits(param, *weight * 500000)))
goto err;


V668 There is no sense in testing the 'pRule' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. PrlHandleFirewallRule.cpp 59

PRL_RESULT PrlHandleFirewallRule::Create(PRL_HANDLE_PTR phRule)
{
PrlHandleFirewallRule* pRule = new PrlHandleFirewallRule;
if ( ! pRule )
return PRL_ERR_OUT_OF_MEMORY;
*phRule = pRule->GetHandle();
return PRL_ERR_SUCCESS;
}


The analyzer has detected an issue when a pointer value returned by the 'new' operator is compared to zero. If the 'new' operator fails to allocate a required amount of memory, the C++ standard forces the program to throw an std::bad_alloc() exception. Therefore, checking the pointer for null doesn't make sense. The developers need to check which kind of the 'new' operator is used in their code. If it is really set to throw an exception in case of memory shortage, then there are 40 more fragments where the program may crash.

Troubles with classes

V630 The 'malloc' function is used to allocate memory for an array of objects which are classes containing constructors and destructors. IOProtocol.cpp 527

/**
* Class describes IO package.
*/
class IOPackage {
public:
/** Common package type */
typedef quint32 Type;
....
};
IOPackage* IOPackage::allocatePackage ( quint32 buffNum )
{
return reinterpret_cast(
::malloc(IOPACKAGESIZE(buffNum))
);
}


The analyzer has detected a potential error related to dynamic memory allocation. Using malloc/calloc/realloc functions to allocate memory for C++ objects leads to a failure when calling the class constructor. The class fields are therefore left uninitialized, and probably some other important actions can't be accomplished as well. Accordingly, in some other place, the destructor can't be called when memory is freed through the free() function, which may cause resource leaks or other troubles.

V670 The uninitialized class member 'm_stat' is used to initialize the 'm_writeThread' member. Remember that members are initialized in the order of their declarations inside a class. SocketClient_p.cpp 145

class SocketClientPrivate:protected QThread,
                      protected SocketWriteListenerInterface
{
....
SocketWriteThread m_writeThread;  //<==line 204
....
IOSender::Statistics m_stat;      //<==line 246
....
}
SocketClientPrivate::SocketClientPrivate (....) :
....
m_writeThread(jobManager, senderType, ctx, m_stat, this),
....
{
....
}


The analyzer has detected a potential bug in the class constructor's initialization list. Under the C++ standard, class members are initialized in the constructor in the same order they were declared in the class. In this case, the m_writeThread variable will be the first to be initialized instead of m_stat.

So it may be unsafe to construct 'm_writeThread' using the 'm_stat' field as one of the arguments.

V690 The 'CSystemStatistics' class implements a copy constructor, but lacks the '=' operator. It is dangerous to use such a class. CSystemStatistics.h 632

class CSystemStatistics: public CPrlDataSerializer
{
public:
....
/** Copy constructor */
CSystemStatistics(const CSystemStatistics &_obj);
/** Initializing constructor */
CSystemStatistics(const QString &source_string);
....
}


There is a copy constructor for this class but its assignment operator has not been redefined.

The "Rule of three" (also known as the "Law of the Big Three" or "The Big Three") is violated here. This rule claims that if a class defines one (or more) of the following it should probably explicitly define all three:

destructor;
copy constructor;
copy assignment operator.


These three functions are special member functions automatically implemented by the compiler when they are not explicitly defined by the programmer. The Rule of Three claims that if one of these had to be defined by the programmer, it means that the compiler-generated version does not fit the needs of the class in one case and it will probably not fit in the other cases either and lead to runtime errors.

Other warnings

V672 There is probably no need in creating the new 'res' variable here. One of the function's arguments possesses the same name and this argument is a reference. Check lines: 367, 393. IORoutingTable.cpp 393

bool IOJobManager::initActiveJob (
SmartPtr& jobPool,
const IOPackage::PODHeader& pkgHeader,
const SmartPtr& package,
Job*& job,                               //<==
bool urgent )
{
....
while ( it != jobPool->jobList.end() ) {
    Job* job = *it;                      //<==
    if ( isJobFree(job) ) {
        ++freeJobsNum;
        // Save first job
        if ( freeJobsNum == 0 ) {
            freeJob = job;
            firstFreeJobIter = it;
        }
    }
    ++it;
}
....
}


It is strongly recommended not to declare variables bearing the same names as function arguments. You should generally avoid having identical names for local and global variables. Otherwise, it may cause a variety of errors due to careless use of such variables. Besides incorrect program execution logic, you may face an issue when, for instance, a global pointer points to a local object which will be destroyed in the future, and since it takes some time before a memory area is cleared, this error will take an irregular character.

Other issues of this kind:

V672 There is probably no need in creating the new 'job' variable here. One of the function's arguments possesses the same name and this argument is a reference. Check lines: 337, 391. IOSendJob.cpp 391

V672 There is probably no need in creating the new 'res' variable here. One of the function's arguments possesses the same name and this argument is a reference. Check lines: 367, 393. IORoutingTable.cpp 393

V610 Undefined behavior. Check the shift operator '<<'. The left operand '~0' is negative. util.c 1046

int parse_ip(const char *str, struct vzctl_ip_param **ip)
{
....
if (family == AF_INET)
    mask = htonl(~0 << (32 - mask));
....
}


The analyzer has detected a shift operation leading to undefined behavior. The reason behind it is that in the '~0' operation, the number will be inverted into signed int and therefore there will be a negative number shift, which, under the C++ standard, leads to undefined behavior. The unsigned type should be defined explicitly.

Correct code:

int parse_ip(const char *str, struct vzctl_ip_param **ip)
{
....
if (family == AF_INET)
    mask = htonl(~0u << (32 - mask));
....
}


Two more warnings of this kind:

V610 Undefined behavior. Check the shift operator '<<'. The left operand '~0' is negative. util.c 98

V610 Undefined behavior. Check the shift operator '<<'. The left operand '~0' is negative. vztactl.c 187

V547 Expression 'limit < 0' is always false. Unsigned type value is never < 0. io.c 80

int
vz_set_iolimit(struct vzctl_env_handle *h, unsigned int limit)
{
int ret;
struct iolimit_state io;
unsigned veid = eid2veid(h);
if (limit < 0)
return VZCTL_E_SET_IO;
....
}


The analyzer has detected an invalid conditional expression in this function. An unsigned variable can never be less than zero. This condition is probably just an excessive one, but it is also possible that identification of an incorrect state was meant to be implemented in some other way.

Another similar fragment:

V547 Expression 'limit < 0' is always false. Unsigned type value is never < 0. io.c 131

Conclusion

At first, the developers suggested checking an already existing beta-version to be released soon - the version ".... where we will at least rewrite all the planned product parts and fix the bugs found during the testing stage". But that would hugely contradict the static analysis ideology! Static analysis is most efficient and beneficial when used at the earlier development stages and on a regular basis. A single-time check of your project may help improve the code at some point but the overall quality will remain at the same low level. Delaying code analysis till the testing stage is the greatest mistake however you look at it - either as a manager or a developer. It is cheapest to fix a bug at the coding stage!

This idea is discussed in more detail by my colleague Andrey Karpov in the article "How Do Programs Run with All Those Bugs At All?" I especially recommend reading the sections "No need to use PVS-Studio then?" and "PVS-Studio is needed!" And may your code stay bugless!

P.S. Full logs from PVS Studio is here.

Latest Month

July 2016
S M T W T F S
     12
3456789
10111213141516
17181920212223
24252627282930
31      
Powered by LiveJournal.com
Designed by Tiffany Chow