How to avoid mistakes using modern C ++ / PVS-Studio Blog

PavelBelikov September 15, 2016 at 12:42

How to avoid mistakes using modern C ++

One of the problems with C ++ is the large number of constructs whose behavior is undefined or simply unexpected for the programmer. We often encounter such errors when using a static code analyzer on different projects. But, as you know, it is best to find errors at the compilation stage. Let's see what techniques from modern C ++ allow us to write not only simpler and more expressive code, but also make our code more secure and reliable.

What is Modern C ++?

The term Modern C ++ became very popular after the release of C ++ 11. What does he mean? First of all, Modern C ++ is a set of patterns and idioms that are designed to eliminate the shortcomings of the good old “C with classes”, which many C ++ programmers are used to, especially if they started to program in C. In many cases, C ++ 11 code looks like more concise and understandable, which is very important.

What do people usually remember when they talk about Modern C ++? Parallelism, compile-time calculations, RAII, lambdas, ranges, concepts, modules, and other equally important components of the standard library (for example, the API for working with the file system). These are very cool innovations, and we are waiting for them in the following standards. At the same time, I would like to draw attention to how new standards allow writing more secure code. When developing a static code analyzer, we encounter a large number of different types of errors and sometimes the thought arises: "But in modern C ++, this could have been avoided." Therefore, I propose to consider a series of errors that we found using PVS-Studio in various Open Source projects. At the same time, we’ll see how to better fix them.

Auto Type Inference

In C ++ 11, the keywords auto and decltype were added . You certainly know how they work:

std::map m;
auto it = m.find(42);
//C++98: std::map::iterator it = m.find(42);

Using auto, you can very conveniently shorten long types without losing code readability. However, these keywords are really revealed in combination with templates: with auto or decltype, you do not need to explicitly specify the type of the return value.

But back to our topic. Here is an example of a 64-bit error :

string str = .....;
unsigned n = str.find("ABC");
if (n != string::npos)

In a 64-bit application, the value of string :: npos is greater than the maximum value of UINT_MAX that an unsigned variable holds . It would seem that this is the case where auto can save us from this kind of problem: the type of the variable n is not important to us , the main thing is that it contains all possible values of string :: find . Indeed, if we rewrite this example with auto , the error will disappear:

string str = .....;
auto n = str.find("ABC");
if (n != string::npos)

But it is not so simple. Using auto is not a panacea, and there are many errors associated with it. For example, you could write code like this:

auto n = 1024 * 1024 * 1024 * 5;
char* buf = new char[n];

auto will not save you from overflow and less than 5GiB will be allocated for the buffer.

In a common mistake with an incorrectly recorded cycle, auto is also not an assistant to us. Consider an example:

std::vector bigVector;
for (unsigned i = 0; i < bigVector.size(); ++i)
{ ... }

For large arrays, this cycle turns into an endless one. The presence of such errors in the code is not surprising: they appear in rather rare situations, for which most likely the tests did not write.

Is it possible to rewrite this fragment through auto ?

std::vector bigVector;
for (auto i = 0; i < bigVector.size(); ++i)
{ ... }

No, the mistake has not gone away. It got worse.

With simple types, auto behaves badly. Yes, in the simplest cases (auto x = y) it works, but as soon as additional constructs appear, the behavior can become more unpredictable. And worst of all, the error will be more difficult to notice, since the types of variables will not be obvious at first glance. Fortunately for static analyzers, calculating the type is not a problem: they do not get tired and do not lose attention. But it’s better for mere mortals to specify simple types explicitly. Fortunately, narrowing ghosts can be eliminated in other ways, but about them a little later.

Dangerous countof

One of the “dangerous” types in C ++ is an array. Often when passing it to a function, they forget that it is passed as a pointer, and try to calculate the number of elements through sizeof :

#define RTL_NUMBER_OF_V1(A) (sizeof(A)/sizeof((A)[0]))
#define _ARRAYSIZE(A) RTL_NUMBER_OF_V1(A)
int GetAllNeighbors( const CCoreDispInfo *pDisp,
                     int iNeighbors[512] ) {
  ....
  if ( nNeighbors < _ARRAYSIZE( iNeighbors ) ) 
    iNeighbors[nNeighbors++] = pCorner->m_Neighbors[i];
  .... 
}

Note. The code is taken from the Source Engine SDK.

PVS-Studio warning : V511 The sizeof () operator returns size of the pointer, and not of the array, in 'sizeof (iNeighbors)' expression. Vrad_dll disp_vrad.cpp 60

Such confusion may arise due to specifying the size of the array in the argument: this number means nothing to the compiler and is just a hint to the programmer.

The trouble is that such code is compiled and the programmer does not suspect that something is amiss. The obvious solution would be to use metaprogramming:

template < class T, size_t N > constexpr size_t countof( const T (&array)[N] ) {
  return N;
}
countof(iNeighbors); //compile-time error

In the case when we do not pass an array to this function, we get a compilation error. In C ++ 17, you can use std :: size .

In C ++ 11, they added the function std :: extent , but it does not work as countof , since it returns 0 for inappropriate types.

std::extent(); //=> 0

You can make a mistake not only with countof , but also with sizeof .

VisitedLinkMaster::TableBuilder::TableBuilder(
    VisitedLinkMaster* master,
    const uint8 salt[LINK_SALT_LENGTH])
    : master_(master),
      success_(true) {
  fingerprints_.reserve(4096);
  memcpy(salt_, salt, sizeof(salt));
}

Note. The code is taken from Chromium.

PVS-Studio warnings:

V511 The sizeof () operator returns size of the pointer, and not of the array, in 'sizeof (salt)' expression. browser visitedlink_master.cc 968
V512 A call of the 'memcpy' function will lead to underflow of the buffer 'salt_'. browser visitedlink_master.cc 968

As you can see, standard arrays in C ++ have a lot of problems. Therefore, in modern C ++ it is worth using std :: array : its API is similar to std :: vector and other containers and it is more difficult to make mistakes when using it.

void Foo(std::array array)
{
  array.size(); //=> 16
}

How wrong in a simple for

Another source of errors is the simple for loop . It would seem, where can one be mistaken? Is there really something to do with a difficult exit condition or savings on the lines? No, they are mistaken in the simplest cycles.

Let's look at fragments from the projects:

const int SerialWindow::kBaudrates[] = { 50, 75, 110, .... };
SerialWindow::SerialWindow() : ....
{
  ....
  for(int i = sizeof(kBaudrates) / sizeof(char*); --i >= 0;)
  {
    message->AddInt32("baudrate", kBaudrateConstants[i]); 
    ....
  }
}

Note. The code is taken from the Haiku Operation System.

PVS-Studio warning : V706 Suspicious division: sizeof (kBaudrates) / sizeof (char *). Size of every element in 'kBaudrates' array does not equal to divisor. SerialWindow.cpp 162

We considered these errors in detail in the previous paragraph: again, the size of the array was incorrectly calculated. You can easily fix the situation using std :: size :

const int SerialWindow::kBaudrates[] = { 50, 75, 110, .... };
SerialWindow::SerialWindow() : ....
{
  ....
  for(int i = std::size(kBaudrates); --i >= 0;) {
    message->AddInt32("baudrate", kBaudrateConstants[i]); 
    ....
  }
}

But there is a better way. In the meantime, look at another fragment.

inline void CXmlReader::CXmlInputStream::UnsafePutCharsBack(
  const TCHAR* pChars, size_t nNumChars)
{
  if (nNumChars > 0)
  {
    for (size_t nCharPos = nNumChars - 1;
         nCharPos >= 0;
         --nCharPos)
      UnsafePutCharBack(pChars[nCharPos]);
  }
}

Note. The code is taken from Shareaza.

PVS-Studio warning : V547 Expression 'nCharPos> = 0' is always true. Unsigned type value is always> = 0. BugTrap xmlreader.h 946

Typical error in writing the reverse loop: they forgot that the iterator is of an unsigned type and the check always returns true . You may have thought: “How so? So only beginners and students are wrong. We, professionals, do not have such mistakes. ” Unfortunately, this is not entirely true. Of course, everyone understands that (unsigned> = 0) is true . Where then did such errors come from? Often they arise as a result of refactoring. Imagine this situation: the project is moving from a 32-bit platform to a 64-bit one. Used to be used for indexingint / unsigned , and it was decided to replace them with size_t / ptrdiff_t . And in one place they overlooked and used the unsigned type instead of the sign type.

What to do to avoid such a situation in your code? Some advise using signed types, as in C # or Qt. Maybe this is a good way, but if we want to work with large amounts of data, then the use of size_t cannot be avoided. Is there any safer way to get around an array in C ++? Of course have. Let's start with the simplest: non-member functions. To work with collections, arrays and initializer_list, there are unified functions, the principle of which you should be familiar with:

char buf[4] = { 'a', 'b', 'c', 'd' };
for (auto it = rbegin(buf);
     it != rend(buf);
     ++it) {
  std::cout << *it;
}

Great, now we do not need to remember the difference between the forward and reverse cycles. No need to think about whether we use a simple array or array - the loop will work anyway. Using iterators is a good way to get rid of a headache, but even it is not good enough. Best to use range for :

char buf[4] = { 'a', 'b', 'c', 'd' };
for (auto it : buf) {
  std::cout << it;
}

Of course, there are some drawbacks to the range for : it does not allow you to control the course of the loop so flexibly and if more complex work with indexes is required, then this for will not help us. But such situations should be considered separately. Our situation is quite simple: you need to go through the elements of the array in the reverse order. However, difficulties already arise at this stage. There are no helper classes for range-based for in the standard library . Let's see how it could be implemented:

template 
struct reversed_wrapper {
  const T& _v;
  reversed_wrapper (const T& v) : _v(v) {}
  auto begin() -> decltype(rbegin(_v))
  {
    return rbegin(_v);
  }
  auto end() -> decltype(rend(_v))
  {
    return rend(_v);
  }
};
template 
reversed_wrapper reversed(const T& v)
{
  return reversed_wrapper(v);
}

In C ++ 14, you can simplify the code by removing decltype . You can see how auto helps write template functions - reversed_wrapper will work with both an array and std :: vector .

Now you can rewrite the fragment as follows:

char buf[4] = { 'a', 'b', 'c', 'd' };
for (auto it : reversed(buf)) {
  std::cout << it;
}

What is this code good for? Firstly, it is very easy to read. We immediately see that here the array of elements is reversed. Secondly, to make a mistake is much more difficult. And thirdly, it works with any type. This is significantly better than what it was.

In boost, you can use boost :: adapters :: reverse (arr) .

But back to the original example. There the array is passed in a pair of pointer-size. Obviously, our reversed solution will not work for him. What to do? Use classes like span / array_view . In C ++ 17 there is string_view , I suggest to use it:

void Foo(std::string_view s);
std::string str = "abc";
Foo(std::string_view("abc", 3));
Foo("abc");
Foo(str);

string_view does not own the string, in fact it is a wrapper over const char * and length. Therefore, in the sample code, the string is passed by value, not by reference. A key feature of string_view is compatibility with different ways of representing strings: const char * , std :: string and non-null terminated const char * .

As a result, the function takes the following form:

inline void CXmlReader::CXmlInputStream::UnsafePutCharsBack(
  std::wstring_view chars)
{
  for (wchar_t ch : reversed(chars))
    UnsafePutCharBack(ch);
}

When passing to a function, it is important not to forget that the constructor string_view (const char *) is implicit, so you can write this:

Foo(pChars);

Not like that:

Foo(wstring_view(pChars, nNumChars));

The string pointed to by string_view does not have to be null-terminated, as the name of the string_view :: data method hints , and this should be borne in mind when using it. When passing its value to some function from cstdlib that expects a C string, you can get undefined behavior. And you can easily skip this if most of the cases you are testing use std :: string or null-terminated strings.

Enum

Let’s digress from C ++ and remember the good old C. How is it with security? Indeed, there are no problems with implicit calls to constructors and conversion operators, and there are no problems with different types of strings. In practice, errors are often found in the simplest designs: the most complex are already carefully scanned and debugged, as they cause suspicion. At the same time, simple designs are often forgotten to check. Here is an example of a dangerous construct that came to us from C:

enum iscsi_param {
  ....
  ISCSI_PARAM_CONN_PORT,
  ISCSI_PARAM_CONN_ADDRESS,
  ....
};
enum iscsi_host_param {
  ....
  ISCSI_HOST_PARAM_IPADDRESS,
  ....
};
int iscsi_conn_get_addr_param(....,
  enum iscsi_param param, ....)
{
  ....
  switch (param) {
  case ISCSI_PARAM_CONN_ADDRESS:
  case ISCSI_HOST_PARAM_IPADDRESS:
  ....
  }
  return len;
}

Linux kernel example. PVS-Studio warning : V556 The values of different enum types are compared: switch (ENUM_TYPE_A) {case ENUM_TYPE_B: ...}. libiscsi.c 3501

Note the values in the switch-case : one of the named constants is taken from another enumeration. In the original, of course, the code and possible values are much larger and the error is not as obvious. The reason for this is the weak typing of enum - they can be implicitly cast to int , and this gives excellent scope for various errors.

In C ++ 11, you can and should use the enum class : such a trick will not work with them, and the error will appear during compilation. As a result, the code below does not compile, which is what we need:

enum class ISCSI_PARAM {
  ....
  CONN_PORT,
  CONN_ADDRESS,
  ....
};
enum class ISCSI_HOST {
  ....
  PARAM_IPADDRESS,
  ....
};
int iscsi_conn_get_addr_param(....,
 ISCSI_PARAM param, ....)
{
  ....
  switch (param) {
  case ISCSI_PARAM::CONN_ADDRESS:
  case ISCSI_HOST::PARAM_IPADDRESS:
  ....
  }
  return len;
}

The following snippet is not completely related to enum , but has similar symptoms:

void adns__querysend_tcp(....) {
  ...
  if (!(errno == EAGAIN || EWOULDBLOCK || 
        errno == EINTR || errno == ENOSPC ||
        errno == ENOBUFS || errno == ENOMEM)) {
  ...
}

Note. The code is taken from ReactOS.

Yes, errno values are declared by macros, which in itself is a bad practice in C ++ (and in C too), but even if enum were used , it would not have been easier. The lost comparison will not manifest itself in the case of enum (and even more so the macro). But the enum class would not allow this, since there will not be an implicit cast to bool .

Initialization in the constructor

But back to the original C ++ problems. One of them manifests itself when you need to initialize an object in a similar way in several constructors. A simple situation: there is a class, there are two constructors, one of them calls the other. Everything looks logical: the general code is placed in a separate method - no one likes to duplicate the code. What's the catch?

Guess::Guess() {
  language_str = DEFAULT_LANGUAGE;
  country_str = DEFAULT_COUNTRY;
  encoding_str = DEFAULT_ENCODING;
}
Guess::Guess(const char * guess_str) {
  Guess();
  ....
}

Note. The code is taken from LibreOffice.

PVS-Studio Warning: V603 The object was created but it is not being used. If you wish to call constructor, 'this-> Guess :: Guess (....)' should be used. guess.cxx 56

A catch in the syntax of calling the constructor. Often they forget about it and create another instance of the class, which will be immediately destroyed. That is, initialization of the original instance does not occur. Naturally there are 1000 and 1 way to fix it. For example, you can explicitly call the constructor via this or take everything out into a separate function:

Guess::Guess(const char * guess_str)
{
  this->Guess();
  ....
}
Guess::Guess(const char * guess_str)
{
  Init();
  ....
}

By the way, an explicit repeated call to the constructor, for example, through this is a dangerous game and you need to understand what is happening. The option with the Init () function is much better and more understandable. For those who want to deal with tricks in more detail, I propose to get acquainted with the 19th chapter "How to properly call one constructor from another" from this book .

But it’s best to use a delegation of constructors. So we can explicitly call one constructor from another:

Guess::Guess(const char * guess_str) : Guess()
{
  ....
}

Such constructors have several limitations. First: the delegated constructor takes full responsibility for initializing the object. That is, with it, to initialize another field of the class in the initialization list will not work:

Guess::Guess(const char * guess_str)
  : Guess(),           
    m_member(42)
{
  ....
}

And naturally, it is necessary to ensure that the delegation does not form a cycle, since it will not work out. Unfortunately, this code compiles:

Guess::Guess(const char * guess_str)
  : Guess(std::string(guess_str))
{
  ....
}
Guess::Guess(std::string guess_str)
  : Guess(guess_str.c_str())
{
  ....
}

About virtual functions

Virtual functions are fraught with a potential problem: the fact is that it is very easy in an inherited class to make a mistake in the signature and, as a result, not redefine the function, but declare a new one. Consider this situation using an example:

class Base {
  virtual void Foo(int x);
}
class Derived : public class Base {
  void Foo(int x, int a = 1);
}

The Derived :: Foo method cannot be called with a pointer / link to Base . But this example is simple and we can say that no one is wrong. And they usually make a mistake like this:

Note. The code is taken from MongoDB.

class DBClientBase : .... {
public:
  virtual auto_ptr query(
    const string &ns,
    Query query,
    int nToReturn = 0
    int nToSkip = 0,
    const BSONObj *fieldsToReturn = 0,
    int queryOptions = 0,
    int batchSize = 0 );
};
class DBDirectClient : public DBClientBase {
public:
  virtual auto_ptr query(
    const string &ns,
    Query query,
    int nToReturn = 0,
    int nToSkip = 0,
    const BSONObj *fieldsToReturn = 0,
    int queryOptions = 0);
};

PVS-Studio Warning: V762 Consider inspecting virtual function arguments. See seventh argument of function 'query' in derived class 'DBDirectClient' and base class 'DBClientBase'. dbdirectclient.cpp 61

There are many arguments and there is no last argument in the function of the derived class. These are two different unrelated functions. Very often, such an error occurs with arguments that have a default value.

In the following fragment, the situation is trickier. Such code will work if it is compiled as 32-bit, but will not work in the 64-bit version. Initially, the parameter in the base class was of the DWORD type , but then it was corrected to DWORD_PTR . But in the inherited classes have not changed. Long live the sleepless night, debugging and coffee!

class CWnd : public CCmdTarget {
  ....
  virtual void WinHelp(DWORD_PTR dwData, UINT nCmd = HELP_CONTEXT);
  ....
};
class CFrameWnd : public CWnd { .... };
class CFrameWndEx : public CFrameWnd {
  ....
  virtual void WinHelp(DWORD dwData, UINT nCmd = HELP_CONTEXT);
  ....
};

To make a mistake in the signature is possible in more extravagant ways. You can forget const for a function or argument. You may forget that the function in the base class is not virtual. You can confuse the sign / unsigned type.

In C ++ 11, several keywords have been added that can govern the redefinition of virtual functions. Override will help us . Such code simply does not compile.

class DBDirectClient : public DBClientBase {
public:
  virtual auto_ptr query(
    const string &ns,
    Query query,
    int nToReturn = 0,
    int nToSkip = 0,
    const BSONObj *fieldsToReturn = 0,
    int queryOptions = 0) override;
};

NULL vs nullptr

Using NULL to indicate a null pointer leads to a number of unexpected situations. The fact is that NULL is a regular macro that expands to 0, which has type int . From here it is easy to understand why the second function is selected in this example:

void Foo(int x, int y, const char *name);
void Foo(int x, int y, int ResourceID);
Foo(1, 2, NULL);

But although this is understandable, it is definitely not logical. Therefore, there is a need for nullptr , which has its own type nullptr_t . Therefore, it is absolutely impossible to use NULL (and even more so 0 ) in modern C ++.

Another example: NULL can be used to compare with other integer types. Imagine that there is a certain WinAPI function that returns HRESULT . This type is in no way associated with a pointer, so comparing it with NULL does not make sense. And nullptr emphasizes this with a compilation error, while NULL works:

if (WinApiFoo(a, b) != NULL)    // Плохо
if (WinApiFoo(a, b) != nullptr) // Ура, ошибка
                                // компиляции

va_arg

There are situations when it is necessary to pass an indefinite number of arguments to a function. A typical example is the formatted I / O function. Yes, it can be designed so that a variable number of arguments is not needed, but I see no reason to refuse such a syntax, since it is much more convenient and visual. What do the old C ++ standards offer us? They suggest using va_list . What problems can arise in this? It is very easy to pass an argument of the wrong type to such a function. Or do not pass an argument. Let's look at the fragments in more detail.

typedef std::wstring string16; 
const base::string16& relaunch_flags() const;
int RelaunchChrome(const DelegateExecuteOperation& operation)
{
  AtlTrace("Relaunching [%ls] with flags [%s]\n",
           operation.mutex().c_str(),
           operation.relaunch_flags());
  ....
}

Note. The code is taken from Chromium.

PVS-Studio Warning: V510 The 'AtlTrace' function is not expected to receive class-type variable as third actual argument. delegate_execute.cc 96

They wanted to print the string std :: wstring , but they forgot to call the c_str () method . That is, the type wstring will be interpreted in the function as const wchar_t * . Naturally, nothing good will come of it.

cairo_status_t
_cairo_win32_print_gdi_error (const char *context)
{
  ....
  fwprintf (stderr, L"%s: %S", context,
            (wchar_t *)lpMsgBuf);
  ....
}

Note. The code is taken from Cairo.

Warning PVS-Studio: V576 Incorrect format. Consider checking the third actual argument of the 'fwprintf' function. The pointer to string of wchar_t type symbols is expected. cairo-win32-surface.c 130

In this fragment, format specifiers for strings are confused. The thing is that in Visual C ++ for wprintf % s expects wchar_t * , and% S expects char * . It is noteworthy that these errors are in the lines designed to display errors or debugging information - for sure these are rare situations, so they were missed.

static void GetNameForFile(
  const char* baseFileName,
  const uint32 fileIdx,
  char outputName[512] )
{
  assert(baseFileName != NULL);
  sprintf( outputName, "%s_%d", baseFileName, fileIdx );
}

Note. The code is taken from the CryEngine 3 SDK.

Warning PVS-Studio: V576 Incorrect format. Consider checking the fourth actual argument of the 'sprintf' function. The SIGNED integer type argument is expected. igame.h 66

It is equally easy to mix up integer types. Especially when their size depends on the platform. Here, however, everything is more banal: they mixed up the sign and unsigned types. Larger numbers will be printed as negative.

ReadAndDumpLargeSttb(cb,err)
  int     cb;
  int     err;
{
  ....
  printf("\n - %d strings were read, "
         "%d were expected (decimal numbers) -\n");
  ....
}

Note. The code is taken from Word for Windows 1.1a.

Warning PVS-Studio: V576 Incorrect format. A different number of actual arguments is expected while calling 'printf' function. Expected: 3. Present: 1. dini.c 498

An example found in an archaeological study . The string implies three arguments, but none. Maybe they wanted to print the data lying on the stack, but making such assumptions about what lies there is still not worth it. Unambiguously, you need to pass arguments explicitly.

BOOL CALLBACK EnumPickIconResourceProc(
  HMODULE hModule, LPCWSTR lpszType, 
  LPWSTR lpszName, LONG_PTR lParam)
{
  ....
  swprintf(szName, L"%u", lpszName);
  ....
}

Note. The code is taken from ReactOS.

Warning PVS-Studio: V576 Incorrect format. Consider checking the third actual argument of the 'swprintf' function. To print the value of pointer the '% p' should be used. dialogs.cpp 66

Example of a 64-bit error. The size of the pointer is architecture dependent and using% u for it is a bad idea. What to use instead? The analyzer itself tells us the correct answer -% p. It’s good if the pointer is simply printed for debugging. It will be much more interesting if they then try to read and use it from the buffer.

Why are functions with a variable number of arguments bad? Almost everyone! Neither the type of the argument nor the number of arguments can be checked in them. Step left, step right - undefined behavior.

It’s good that there are more reliable alternatives. Firstly, there are variadic templates . With the help of them we get all the information about the types passed in at compile time and we can use it however we want. For example, write the same printf , but a little more secure:

void printf(const char* s) {
  std::cout << s;
}
template
void printf(const char* s, T value, Args... args) {
  while (s && *s) {
    if (*s=='%' && *++s!='%') {
      std::cout << value;
      return printf(++s, args...);
    }
    std::cout << *s++;
  }
}

Naturally, this is just an example: it makes no sense to use it in practice. But with variadic templates, you are limited only by the flight of imagination, and not the means of the language.

Another design that can be considered as a variant of passing a variable number of arguments is std :: initializer_list . It does not allow passing arguments of different types. But if this is enough, then you can use it:

void Foo(std::initializer_list a);
Foo({1, 2, 3, 4, 5});

At the same time, it is very convenient to bypass it, since you can use the same begin , end and range for .

Narrowing

Narrow ghosts caused a lot of headache to programmers. Especially when the transition to 64-bit architecture became relevant. It’s good if the code used the correct types everywhere. But not everywhere is so rosy: often used various dirty hacks and extravagant ways of storing pointers. Not one liter of coffee was drunk to find all such places.

char* ptr = ...;
int n = (int)ptr;
....
ptr = (char*) n;

But let's digress from 64-bit errors. Here is a simpler example: there are two integer values and want to find their relationship. Do it like this:

virtual int GetMappingWidth( ) = 0;
virtual int GetMappingHeight( ) = 0;
void CDetailObjectSystem::LevelInitPreEntity()
{
  ....
  float flRatio = pMat->GetMappingWidth() /
                  pMat->GetMappingHeight();
  ....
}

Note. The code is taken from the Source Engine SDK.

PVS-Studio Warning: V636 The 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 ;. Client (HL2) detailobjectsystem.cpp 1480

Unfortunately, you cannot completely protect yourself from such errors - there is always another way to implicitly cast one type to another. But the new initialization method in C ++ 11 has one nice feature: it prohibits narrowing casts. In this code, an error will occur during compilation and it can be easily fixed.

float flRatio { pMat->GetMappingWidth() /
                pMat->GetMappingHeight() };

No news is good news

There are so many possibilities to make mistakes in managing memory and resources. Convenience when working with them is an important requirement for the modern language. Modern C ++ does not lag behind and offers a number of tools for automatic control of resources. And although such errors are more likely to be the domain of dynamic analysis, static analysis can also reveal some problems. Let's look at some of them:

void AccessibleContainsAccessible(....)
{
  auto_ptr child_array(
           new VARIANT[child_count]);
  ...
}

Note. The code is taken from Chromium.

PVS-Studio Warning: V554 Incorrect use of auto_ptr. The memory allocated with 'new []' will be cleaned using 'delete'. interactive_ui_tests accessibility_win_browsertest.cc 171

Naturally, the idea of smart pointers is not new: for example, there was such a class std :: auto_ptr . In the past tense, I’m talking about it because it is declared deprecated in C ++ 11, and removed in C ++ 17. The error appeared in this fragment due to the fact that auto_ptr was used incorrectly: the class has no specialization for arrays, and the standard delete will be called , not delete [] . Unique_ptr came to replace auto_ptr, which has both specialization for arrays, and the ability to pass the deleter functor , which will be called instead of delete , and full support for moving semantics. It seemed that what could be wrong here?

void text_editor::_m_draw_string(....) const
{
  ....
  std::unique_ptr pxbuf_ptr(
       new unsigned[len]);
  ....
}

Note. The code is taken from nana.

PVS-Studio Warning: V554 Incorrect use of unique_ptr. The memory allocated with 'new []' will be cleaned using 'delete'. text_editor.cpp 3137

It turns out that you can make exactly the same mistake. Yes, just write unique_ptrand it will disappear, however, in this form, the code also compiles. That is, in this way one can also be mistaken, but as practice shows, if you can make a mistake somewhere, they will be mistaken there. The code snippet only confirms this. So, using unique_ptr with arrays, be extremely careful: shooting yourself in the foot is easier than it sounds. Maybe then it is better to use std :: vector according to the precepts of Modern C ++?

Consider another kind of accident.

template
static FString GetShaderStageSource(TOpenGLStage* Shader)
{
  ....
  ANSICHAR* Code = new ANSICHAR[Len + 1];
  glGetShaderSource(Shaders[i], Len + 1, &Len, Code);
  Source += Code;
  delete Code;
  ....
}

Note. The code is taken from Unreal Engine 4.

PVS-Studio Warning: V611 The memory was allocated using 'new T []' operator but was released using the 'delete' operator. Consider inspecting this code. It's probably better to use 'delete [] Code;'. openglshaders.cpp 1790 The

same error can easily be made without smart pointers: the memory allocated with new [] is freed via free .

bool CxImage::LayerCreate(int32_t position)
{
  ....
  CxImage** ptmp = new CxImage*[info.nNumLayers + 1];
  ....
  free(ptmp);
  ....
}

Note. The code is taken from CxImage.

PVS-Studio Warning: V611 The memory was allocated using 'new' operator but was released using the 'free' function. Consider inspecting operation logics behind the 'ptmp' variable. ximalyr.cpp 50

And in this fragment malloc / free and new / delete are confused . This can happen during refactoring: there were everywhere functions from C, decided to change, got UB.

int settings_proc_language_packs(....)
{
  ....
  if(mem_files) {
    mem_files = 0;
    sys_mem_free(mem_files);
  }
  ....
}

Note. The code is taken from Fennec Media.

PVS-Studio Warning: V575 The null pointer is passed into 'free' function. Inspect the first argument. settings interface.c 3096

And this is a more entertaining example. There is a practice in which the pointer is reset to zero after release. Sometimes even special macros are written for this. A wonderful practice on the one hand: this way you can protect yourself from re-freeing memory. But here the order of expressions was confused and a free pointer comes to free (which is what the static analyzer notices).

ETOOLS_API int __stdcall ogg_enc(....) {
  format = open_audio_file(in, &enc_opts);
  if (!format) {
    fclose(in);
    return 0;
  };
  out = fopen(out_fn, "wb");
  if (out == NULL) {
    fclose(out);
    return 0;
  }    
}

But the problem applies not only to memory management, but also to resource management. You can, for example, forget to close the file, as in the fragment above. And the keyword in both cases is RAII. The same concept is behind smart pointers. Combined with move-semantics, RAII eliminates many memory leak errors. And the code written in this style allows you to more clearly determine the ownership of the resource.

As a small example, I’ll give a wrapper over FILE using the unique_ptr features :

auto deleter = [](FILE* f) {fclose(f);};
std::unique_ptr p(fopen("1.txt", "w"),
                                           deleter);

But for working with files, most likely you will want to have a more functional wrapper (and with a more understandable syntax). It's time to remember that in C ++ 17 they will add an API for working with file systems - std :: filesystem . But if this solution does not suit you and you want to use fread / fwrite instead of i / o-threads, then you can be inspired by unique_ptr and write your File , optimized for your needs and at the same time convenient, readable and safe.

What is the result?

Modern C ++ has brought in a lot of tools to help you write code more securely. Many constructions for compile-time calculations and checks have appeared. You can switch to a more convenient model for managing memory and resources.

But no programming technique or paradigm can save you from mistakes completely. So in C ++ along with new functionality, new errors unique to it are added. Therefore, you cannot completely rely on one thing: only a combination of high-quality code, code review and good tools can save you many hours and energy drinks that can be invested in something more useful.

Speaking of tools. I suggest trying PVS-Studio: we recently started developing a version for Linux and you can try it in practice: it supports any build system and makes it easy to test a project by simply compiling it. And for Windows developers, we have a convenient plug-in for Visual Studio, which you can try in the trial version.

If you want to share this article with an English-speaking audience, then please use the link to the translation: Pavel Belikov. How to avoid bugs using modern C ++ .

Have you read the article and have a question?

Often our articles are asked the same questions. We collected the answers here: Answers to questions from readers of articles about PVS-Studio, version 2015 . Please see the list.

Tags:

How to avoid mistakes using modern C ++ / PVS-Studio Blog