The Ultimate Question of Programming, Refactoring, and Everything. Part 1
Author: Andrey Karpov
Part 2 read here: https://medium.com/@Coder_HarryLee/the-ultimate-question-of-programming-refactoring-and-everything-part-2-824b1fad5e40
Full PDF version: https://yadi.sk/i/zKHIOS84r87nk
Table of contents
Preface
1. Don’t do the compiler’s job
2. Larger than 0 does not mean 1
3. Copy once, check twice
4. Beware of the ?: operator and enclose it in parentheses
5. Use available tools to analyze your code
6. Check all the fragments where a pointer is explicitly cast to integer types
7. Do not call the alloca() function inside loops
8. Remember that an exception in the destructor is dangerous.
9. Use the ‘\0’ literal for the terminal null character
10. Avoid using multiple small #ifdef blocks
11. Don’t try to squeeze as many operations as possible in one line
12. When using Copy-Paste, be especially careful with the last lines
13. Table-style formatting
14. A good compiler and coding style aren’t always enough
15. Start using enum class in your code, if possible
16. “Look what I can do!” — Unacceptable in programming
17. Use dedicated functions to clear private data
18. The knowledge you have, working with one language isn’t always applicable to another language
19. How to properly call one constructor from another
20. The End-of-file (EOF) check may not be enough
21. Check that the end-of-file character is reached correctly (EOF)
Yes, you’ve guessed correctly — the answer is “42”. In this article you will find 42 recommendations about coding in C++ that can help a programmer avoid a lot of errors, save time and effort. The author is Andrey Karpov — technical director of “Program Verification Systems”, a team of developers, working on PVS-Studio static code analyzer. Having checked a large number of open source projects, we have seen a large variety of ways to shoot yourself in the foot; there is definitely much to share with the readers. Every recommendation is given with a practical example, which proves the currentness of this question. These tips are intended for C/C++ programmers, but usually they are universal, and may be of interest for developers using other languages.
Preface
About the author. My name is Andrey Karpov. The scope of my interests − the C/C++ language and the promotion of code analysis methodology. I have been Microsoft MVP in Visual C++ for 5 years. The main aim of my articles and work in general — is to make the code of programs safer and more secure. I’ll be really glad if these recommendations help you write better code, and avoid typical errors. Those who write code standards for companies may also find some helpful information here.
A little bit of history. Not so long ago I created a resource, where I shared useful tips and tricks about programming in C++. But this resource didn’t get the expected number of subscribers, so I don’t see the point in giving a link to it here. It will be on the web for some time, but eventually, it will be deleted. Still, these tips are worth keeping. That’s why I’ve updated them, added several more and combined them in a single text. Enjoy reading!
1. Don’t do the compiler’s job
Consider the code fragment, taken from MySQL project. The code contains an error that PVS-Studio analyzer diagnoses in the following way: V525 The code containing the collection of similar blocks. Check items ‘0’, ‘1’, ‘2’, ‘3’, ‘4’, ‘1’, ‘6’ in lines 680, 682, 684, 689, 691, 693, 695.
static int rr_cmp(uchar *a,uchar *b)
{
if (a[0] != b[0])
return (int) a[0] - (int) b[0];
if (a[1] != b[1])
return (int) a[1] - (int) b[1];
if (a[2] != b[2])
return (int) a[2] - (int) b[2];
if (a[3] != b[3])
return (int) a[3] - (int) b[3];
if (a[4] != b[4])
return (int) a[4] - (int) b[4];
if (a[5] != b[5])
return (int) a[1] - (int) b[5]; <<<<====
if (a[6] != b[6])
return (int) a[6] - (int) b[6];
return (int) a[7] - (int) b[7];
}
Explanation
This is a classic error, related to copying fragments of code (Copy-Paste). Apparently, the programmer copied the block of code”if (a[1] != b[1]) (int) a[1] — (int) b[1];”. Then he started changing the indices and forgot to replace “1” with “5”. This resulted in the comparison function occasionally returning an incorrect value; this issue is going to be difficult to notice. And it’s really hard to detect since all the tests had not revealed it before we scanned MySQL with PVS-Studio.
Correct code
if (a[5] != b[5])
return (int) a[5] - (int) b[5];
Recommendation
Although the code is neat and easy-to-read, it didn’t prevent the developers from overlooking the error. You can’t stay focused when reading code like this because all you see is just similar looking blocks, and it’s hard to concentrate the whole time.
These similar blocks are most likely a result of the programmer’s desire to optimize the code as much as possible. He “unrolled the loop” manually. I don’t think it was a good idea in this case.
Firstly, I doubt that the programmer has really achieved anything with it. Modern compilers are pretty smart, and are very good at automatic loop unrolling if it can help improve program performance.
Secondly, the bug appeared in the code because of this attempt to optimize the code. If you write a simpler loop, there will be less chance of making a mistake.
I’d recommend rewriting this function in the following way:
static int rr_cmp(uchar *a,uchar *b)
{
for (size_t i = 0; i < 7; ++i)
{
if (a[i] != b[i])
return a[i] - b[i];
}
return a[7] - b[7];
}
Advantages:
- The function is easier to read and comprehend.
- You are much less likely to make a mistake writing it.
I am quite sure that this function will work no slower than its longer version.
So, my advice would be — write simple and understandable code. As a rule, simple code is usually correct code. Don’t try to do the compiler’s job — unroll loops, for example. The compiler will most definitely do it well without your help. Doing such fine manual optimization work would only make sense in some particularly critical code fragments, and only after the profiler has already estimated those fragments as problematic (slow).
2. Larger than 0 does not mean 1
The following code fragment is taken from CoreCLR project. The code has an error that PVS-Studio analyzer diagnoses in the following way: V698 Expression ‘memcmp(….) == -1’ is incorrect. This function can return not only the value ‘-1’, but any negative value. Consider using ‘memcmp(….) < 0’ instead.
bool operator( )(const GUID& _Key1, const GUID& _Key2) const
{ return memcmp(&_Key1, &_Key2, sizeof(GUID)) == -1; }
Explanation
Let’s have a look at the description of memcmp() function:
int memcmp ( const void * ptr1, const void * ptr2, size_t num );
Compares the first num bytes of the block of memory pointed by ptr1 to the first num bytes pointed by ptr2, returning zero if they all match, or a value different from zero representing which is greater, if they do not.
Return value:
- < 0 — the first byte that does not match in both memory blocks has a lower value in ptr1 than in ptr2 (if evaluated as unsigned char values).
- == 0 — the contents of both memory blocks are equal.
- > 0 — the first byte that does not match in both memory blocks has a greater value in ptr1 than in ptr2 (if evaluated as unsigned char values).
Note that if blocks aren’t the same, then the function returns values greater than or less than zero. Greater or less. This is important! You cannot compare the results of such functions as memcmp(),strcmp(), strncmp(), and so on with the constants 1 and -1.
Interestingly, the wrong code, where the result is compared with the 1/ -1 can work as the programmer expects for many years. But this is sheer luck, nothing more. The behavior of the function can unexpectedly change. For example, you may change the compiler, or the developers will optimizememcmp() in a new way, so your code will cease working.
Correct code
bool operator( )(const GUID& _Key1, const GUID& _Key2) const
{ return memcmp(&_Key1, &_Key2, sizeof(GUID)) < 0; }
Recommendation
Don’t rely on the way the function works now. If the documentation says that a function can return values less than or greater than 0, it does mean it. It means that the function can return -10, 2, or 1024. The fact that you always see it return -1, 0, or 1 doesn’t prove anything.
By the way, the fact that the function can return such numbers as 1024, indicates, that the result ofmemcmp() execution cannot be stored in the variable of char type. This is one more wide-spread error, whose consequences can be really serious. Such a mistake was the root of a serious vulnerability in MySQL/MariaDB in versions earlier than 5.1.61, 5.2.11, 5.3.5, 5.5.22. The thing is, that when a user connects to MySQL/MariaDB, the code evaluates a token (SHA from the password and hash) that is then compared with the expected value of memcmp() function. But on some platforms the return value can go beyond the range [-128..127] As a result, in 1 out of 256 cases the procedure of comparing hash with an expected value always returns true, regardless of the hash. Therefore, a simple command on bash gives a hacker root access to the volatile MySQL server, even if the person doesn’t know the password. The reason for this was the following code in the file ‘sql/password.c’:
typedef char my_bool;
...
my_bool check(...) {
return memcmp(...);
}
A more detailed description of this issue can be found here: Security vulnerability in MySQL/MariaDB.
3. Copy once, check twice
The fragment is taken from Audacity project. The error is detected by the following PVS-Studio diagnostic: V501 There are identical sub-expressions to the left and to the right of the ‘-’ operator.
sampleCount VoiceKey::OnBackward (....) {
...
int atrend = sgn(buffer[samplesleft - 2]-
buffer[samplesleft - 1]);
int ztrend = sgn(buffer[samplesleft - WindowSizeInt-2]-
buffer[samplesleft - WindowSizeInt-2]);
...
}Explanation
The “buffer[samplesleft — WindowSizeInt-2]” expression is subtracted from itself. This error appeared because of copying a code fragment (Copy-Paste): the programmer copied a code string but forgot to replace 2 with 1.
This a really banal error, but still it is a mistake. Errors like this are a harsh reality for programmers, and that’s why there will speak about them several times here. I am declaring war on them.
Correct code
int ztrend = sgn(buffer[samplesleft - WindowSizeInt-2]-
buffer[samplesleft - WindowSizeInt-1]);
Recommendation
Be very careful when duplicating code fragments.
It wouldn’t make sense to recommend rejecting the copy-paste method altogether. It’s too convenient, and too useful to get rid of such an editor functionality.
Instead, just be careful, and don’t hurry — forewarned is forearmed.
Remember that copying code may cause many errors. Here, take a look at some examples of bugsdetected with the V501 diagnostic. Half of these errors are caused by using Copy-Paste.
If you copy the code and then edit it — check what you’ve got! Don’t be lazy!
We’ll talk more about Copy-Paste later. The problem actually goes deeper than it may seem, and I won’t let you forget about it.
4. Beware of the ?: operator and enclose it in parentheses
Fragment taken from the Haiku project (inheritor of BeOS). The error is detected by the following PVS-Studio diagnostic: V502 Perhaps the ‘?:’ operator works in a different way than it was expected. The ‘?:’ operator has a lower priority than the ‘-’ operator.
bool IsVisible(bool ancestorsVisible) const
{
int16 showLevel = BView::Private(view).ShowLevel();
return (showLevel - (ancestorsVisible) ? 0 : 1) <= 0;
}
Explanation
Let’s check the C/C++ operation precedence. The ternary operator ?: has a very low precedence, lower than that of operations /, +, <, etc; it is also lower than the precedence of the minus operator. As a result, the program doesn’t work in the way the programmer expected.
The programmer thinks that the operations will execute in the following order:
(showLevel - (ancestorsVisible ? 0 : 1) ) <= 0
But it will actually be like this:
((showLevel - ancestorsVisible) ? 0 : 1) <= 0
The error is made in very simple code. This illustrates how hazardous the ?: operator is. It’s very easy to make a mistake when using it; the ternary operator in more complex conditions is pure damage to the code. It’s not only that you are very likely to make and miss a mistake; such expressions are also very difficult to read.
Really, beware of the ?: operator. I’ve seen a lot of bugs where this operator was used.
Correct code
return showLevel - (ancestorsVisible ? 0 : 1) <= 0;
Recommendation
In previous articles we’ve already discussed the problem of a ternary operator, but since then I’ve become even more paranoid. The example given above shows how easy it is to make an error, even in a short and simple expression, that’s why I’ll modify my previous tips.
I don’t suggest rejecting the ?: operator completely. It may be useful, and even necessary sometimes. Nevertheless, please do not overuse it, and if you have decided to use it, here is my recommendation:
ALWAYS enclose the ternary operator in parentheses.
Suppose you have an expression:
A = B ? 10 : 20;
Then you should write it like this:
A = (B ? 10 : 20);
Yes, the parentheses are excessive here…
But, it will protect your code later when you or your colleagues add an X variable to 10 or 20 while doing code refactoring:
A = X + (B ? 10 : 20);
Without the parentheses, you could forget that the ?: operator has low precedence, and accidentally break the program.
Of course, you can write “X+” inside the parentheses, but it will still lead to the same error, although it is additional protection that shouldn’t be rejected.
5. Use available tools to analyze your code
The fragment is taken from LibreOffice project. The error is detected by the following PVS-Studio diagnostic: V718 The ‘CreateThread’ function should not be called from ‘DllMain’ function.
BOOL WINAPI DllMain( HINSTANCE hinstDLL,
DWORD fdwReason, LPVOID lpvReserved )
{
....
CreateThread( NULL, 0, ParentMonitorThreadProc,
(LPVOID)dwParentProcessId, 0, &dwThreadId );
....
}
Explanation
I used to have a side job as a freelancer long time ago. Once I was given a task I failed to accomplish. The task itself was formulated incorrectly, but I didn’t realise that at the time. Moreover, it seemed clear and simple at first.
Under a certain condition in the DllMain I had to do some actions, using Windows API functions; I don’t remember which actions exactly, but it wasn’t anything difficult.
So I spent loads of time on that, but the code just wouldn’t work. More than that, when I made a new standard application, it worked; but it didn’t when I tried it in the DllMain function. Some magic, isn’t it? I didn’t manage to figure out the root of the problem at the time.
It’s only now that I work on PVS-Studio development, so many years later, that I have suddenly realized the reason behind that old failure. In the DllMain function, you can perform only a very limited set of actions. The thing is that some DLL may be not loaded yet, and you cannot call functions from them.
Now we have a diagnostic to warn programmers when dangerous operations are detected in DllMainfunctions. So it was this, which was the case with that old task I was working on.
Details
More details about the usage of DllMain can be found on the MSDN site in this article: Dynamic-Link Library Best Practices. I’ll give some abstracts from it here:
DllMain is called while the loader-lock is held. Therefore, significant restrictions are imposed on the functions that can be called within DllMain. As such, DllMain is designed to perform minimal initialization tasks, by using a small subset of the Microsoft Windows API. You cannot call any function inDllMainwhich directly, or indirectly, tries to acquire the loader lock. Otherwise, you will introduce the possibility that your application deadlocks or crashes. An error in a DllMain implementation can jeopardize the entire process and all of its threads.
The ideal DllMain would be just an empty stub. However, given the complexity of many applications, this is generally too restrictive. A good rule of thumb for DllMain is to postpone the initialization as for as long as possible. Slower initialization increases how robust the application is, because this initialization is not performed while the loader lock is held. Also, slower initialization enables you to safely use much more of the Windows API.
Some initialization tasks cannot be postponed. For example, a DLL that depends on a configuration file will fail to load if the file is malformed or contains garbage. For this type of initialization, the DLLs should attempt to perform the action, and in the case of a failure, exit immediately rather than waste resources by doing some other work.
You should never perform the following tasks from within DllMain:
- Call LoadLibrary or LoadLibraryEx (either directly or indirectly). This can cause a deadlock or a crash.
- Call GetStringTypeA, GetStringTypeEx, or GetStringTypeW (either directly or indirectly). This can cause a deadlock or a crash.
- Synchronize with other threads. This can cause a deadlock.
- Acquire a synchronization object that is owned by code that is waiting to acquire the loader lock. This can cause a deadlock.
- Initialize COM threads by using CoInitializeEx Under certain conditions, this function can callLoadLibraryEx.
- Call the registry functions. These functions are implemented in Advapi32.dll. If Advapi32.dll is not initialized before your DLL, the DLL can access uninitialized memory and cause the process to crash.
- Call CreateProcess. Creating a process can load another DLL.
- Call ExitThread. Exiting a thread during DLL detach can cause the loader lock to be acquired again, causing a deadlock or a crash.
- Call CreateThread. Creating a thread can work if you do not synchronize with other threads, but it is risky.
- Create a named pipe or other named object (Windows 2000 only). In Windows 2000, named objects are provided by the Terminal Services DLL. If this DLL is not initialized, calls to the DLL can cause the process to crash.
- Use the memory management function from the dynamic C Run-Time (CRT). If the CRT DLL is not initialized, calls to these functions can cause the process to crash.
- Call functions in User32.dll or Gdi32.dll. Some functions load another DLL, which may not be initialized.
- Use managed code.
Correct code
The code fragment from the LibreOffice project cited above may or may not work — it all a matter of chance.
It’s not easy to fix an error like this. You need refactor your code in order to make the DllMain function as simple, and short, as possible.
Recommendation
It’s hard to give recommendations. You can’t know everything; everyone may encounter a mysterious error like this. A formal recommendation would sound like this: you should carefully read all the documentation for every program entity you work with. But you surely understand that one can’t foresee every possible issue. You’d only spend all your time reading documentation then, have no time for programming. And even having read N pages, you couldn’t be sure you haven’t missed some article that could warn you against some trouble.
I wish I could give you somewhat more practical tips, but there is unfortunately only one thing I can think of: use static analyzers. No, it doesn’t guarantee you will have zero bugs. Had there been an analyzer all those years ago, which could have told me that I couldn’t call the Foo function in DllMain, I would have saved a lot of time and even more nerves: I really was angry, and going crazy, because of not being able to solve the task.
6. Check all the fragments where a pointer is explicitly cast to integer types
The fragment is taken from IPP Samples project. The error is detected by the following PVS-Studio diagnostic: V205 Explicit conversion of pointer type to 32-bit integer type: (unsigned long)(img)
void write_output_image(...., const Ipp32f *img,
...., const Ipp32s iStep) {
...
img = (Ipp32f*)((unsigned long)(img) + iStep);
...
}
Note. Some may say that this code isn’t the best example for several reasons. We are not concerned about why a programmer would need to move along a data buffer in such a strange way. What matters to us is the fact that the pointer is explicitly cast to the “unsigned long” type. And only this. I chose this example purely because it is brief.
Explanation
A programmer wants to shift a pointer at a certain number of bytes. This code will execute correctly in Win32 mode because the pointer size is the same as that of the long type. But if we compile a 64-bit version of the program, the pointer will become 64-bit, and casting it to long will cause the loss of the higher bits.
Note. Linux uses a different data model. In 64-bit Linux programs, the ‘long’ type is 64-bit too, but it’s still a bad idea to use ‘long’ to store pointers there. First, such code tends to get into Windows applications quite often, where it becomes incorrect. Second, there are special types whose very names suggest that they can store pointers — for example, intptr_t. Using such types makes the program clearer.
In the example above, we can see a classic error which occurs in 64-bit programs. It should be said right off that there are lots of other errors, too, awaiting programmers in their way of 64-bit software development. But it is the writing of a pointer into a 32-bit integer variable that’s the most widespread and insidious issue.
This error can be illustrated in the following way:
Figure 1. A) 32-bit program. B) 64-bit pointer refers to an object that is located in the lower addresses. C) 64-bit pointer is damaged.
Speaking about its insidiousness, this error is sometimes very difficult to notice. The program just “almost works”. Errors causing the loss of the most significant bits in pointers may only show up in a few hours of intense use of the program. First, the memory is allocated in the lower memory addresses, that’s why all the objects and arrays are stored in the first 4 GB of memory. Everything works fine.
As the program keeps running, the memory gets fragmented, and even if the program doesn’t use much of it, new objects may be created outside those first 4 GB. This is where the troubles start. It’s extremely difficult to purposely reproduce such issues.
Correct code
You can use such types as size_t, INT_PTR, DWORD_PTR, intrptr_t, etc. to store pointers.
img = (Ipp32f*)((uintptr_t)(img) + iStep);
Actually, we can do it without any explicit casting. It is not mentioned anywhere that the formatting is different from the standard one, that’s why there is no magic in using __declspec(align( # )) and so on. So, the pointers are shifted by the number of bytes that is divisible by Ipp32f; otherwise we will have undefined behavior (see EXP36-C)
So, we can write it like this:
img += iStep / sizeof(*img);
Recommendation
Use special types to store pointers — forget about int and long. The most universal types for this purpose are intptr_t and uintptr_t. In Visual C++, the following types are available: INT_PTR, UINT_PTR,LONG_PTR, ULONG_PTR,DWORD_PTR. Their very names indicate that you can safely store pointers in them.
A pointer can fit into the types size_t and ptrdiff_t too, but I still wouldn’t recommend using them for that, for they are originally intended for storing sizes and indices.
You cannot store a pointer to a member function of the class in uintptr_t.Member functions are slightly different from standard functions. Except for the pointer itself, they keep hidden value of this that points to the object class. However, it does not matter — in the 32-bit program, you can not assign such a pointer to unsigned int. Such pointers are always handled in a special way, that’s why there aren’t many problems in 64-bit programs. At least I haven’t seen such errors.
If you are going to compile your program into a 64-bit version, first, you need to review and fix all the code fragments where pointers are cast into 32-bit integer types. Reminder — there will be more troublesome fragments in the program, but you should start with the pointers.
For those who are creating or planning to create 64-bit applications, I suggest studying the following resource: Lessons on development of 64-bit C/C++ applications.
7. Do not call the alloca() function inside loops
This bug was found in Pixie project. The error is detected by the following PVS-Studio diagnostic: V505 The ‘alloca’ function is used inside the loop. This can quickly overflow stack.
inline void triangulatePolygon(....) {
...
for (i=1;i<nloops;i++) {
...
do {
...
do {
...
CTriVertex *snVertex =
(CTriVertex *) alloca(2*sizeof(CTriVertex));
...
} while(dVertex != loops[0]);
...
} while(sVertex != loops[i]);
...
}
...
}Explanation
The alloca(size_t) function allocates memory by using the stack. Memory allocated by alloca() is freed when leaving the function.
There’s not much stack memory usually allocated for programs. When you create a project in Visual C++, you may see that the default setting is just 1 megabyte for the stack memory size, this is why thealloca() function can very quickly use up all the available stack memory if used inside a loop.
In the example above, there are 3 nested loops at once. Therefore, triangulating a large polygon will cause a stack overflow.
It is also unsafe to use such macros as A2W in loops as they also contain a call of the alloca() function.
As we have already said, by default, Windows-programs use a stack of 1 Megabyte. This value can be changed; in the project settings find and change the parameters ‘Stack Reserve Size’, and ‘Stack Commit Size’. Details: “/STACK (Stack Allocations)”. However, we should understand that making the stack size bigger isn’t the solution to the problem — you just postpone the moment when the program stack will overflow.
Recommendation
Do not call the alloca() function inside loops. If you have a loop and need to allocate a temporary buffer, use one of the following 3 methods to do so:
- Allocate memory in advance, and then use one buffer for all the operations. If you need buffers of different sizes every time, allocate memory for the biggest one. If that’s impossible (you don’t know exactly how much memory it will require), use method 2.
- Make the loop body a separate function. In this case, the buffer will be created and destroyed right off at each iteration. If that’s difficult too, there’s only method N3 left.
- Replace alloca() with the malloc() function or new operator, or use a class such as std::vector. Take into account that memory allocation will take more time in this case. In the case of using malloc/newyou will have to think about freeing it. On the other hand, you won’t get a stack overflow when demonstrating the program on large data to the customer.
8. Remember that an exception in the destructor is dangerous
This issue was found in LibreOffice project. The error is detected by the following PVS-Studio diagnostic: V509 The ‘dynamic_cast<T&>’ operator should be located inside the try..catch block, as it could potentially generate an exception. Raising exception inside the destructor is illegal.
virtual ~LazyFieldmarkDeleter()
{
dynamic_cast<Fieldmark&>
(*m_pFieldmark.get()).ReleaseDoc(m_pDoc);
}
Explanation
When an exception is thrown in a program, the stack begins to unroll, and objects get destroyed by calling their destructors. If the destructor of an object being destroyed during stack unrolling throws another exception which leaves the destructor, the C++ library will immediately terminate the program by calling the terminate() function. What follows from this is the rule that destructors should never let exceptions out. An exception thrown inside a destructor must be handled inside the same destructor.
The code cited above is rather dangerous. The dynamic_cast operator will generate a std::bad_castexception if it fails to cast an object reference to the required type.
Likewise, any other construct that can throw an exception is dangerous. For example, it’s not safe to use the new operator to allocate memory in the destructor. If it fails, it will throw a std::bad_alloc exception.
Correct code:
The code can be fixed using the dynamic_cast not with a reference, but with the pointer. In this case, if it’s impossible to convert the type of the object, it won’t generate an exception, but will return nullptr.
virtual ~LazyFieldmarkDeleter()
{
auto p = dynamic_cast<Fieldmark*>m_pFieldmark.get();
if (p)
p->ReleaseDoc(m_pDoc);
}
Recommendation
Make your destructors as simple as possible. Destructors aren’t meant for memory allocation and file reading.
Of course, it’s not always possible to make destructors simple, but I believe we should try to reach that. Besides that, a destructor being complex is generally a sign of a poor class design, and ill-conceived solutions.
The more code you have in your destructor, the harder it is to provide for all possible issues. It makes it harder to tell which code fragment can or cannot throw an exception.
If there is some chance that an exception may occur, a good solution is usually to suppress it by using the catch(…):
virtual ~LazyFieldmarkDeleter()
{
try
{
dynamic_cast<Fieldmark&>
(*m_pFieldmark.get()).ReleaseDoc(m_pDoc);
}
catch (...)
{
assert(false);
}
}
True, using it may conceal some error in the destructor, but it may also help the application to run more stably in general.
I’m not insisting on configuring destructors to never throw exceptions — it all depends on the particular situation. Sometimes it’s rather useful to generate an exception in the destructor. I have seen that in specialized classes, but these were rare cases. These classes are designed in such a way that the objects generate an exception upon the destruction, but if it is a usual class like “own string”,”dot”, “brush” “triangle”, “document” and so on, in these cases the exceptions shouldn’t be thrown from the destructor.
Just remember that double exception on end cause a program termination, so it’s up to you to decide if you want this to happen in your project or not.
9. Use the ‘\0’ literal for the terminal null character
The fragment is taken from Notepad++ project. The error is detected by the following PVS-Studio diagnostic: The error text: V528 It is odd that pointer to ‘char’ type is compared with the ‘\0’ value. Probably meant: *headerM != ‘\0’.
TCHAR headerM[headerSize] = TEXT("");
...
size_t Printer::doPrint(bool justDoIt)
{
...
if (headerM != '\0')
...
}Explanation
Thanks to this code’s author, using the ‘\0’ literal to denote the terminal nullcharacter, we can easily spot and fix the error. The author did a good job, but not really.
Imagine this code were written in the following way:
if (headerM != 0)
The array address is verified against 0. The comparison doesn’t make sense as it always true. What’s that — an error or just a redundant check? It’s hard to say, especially if it is someone else’s code or code written a long time ago.
But since the programmer used the ‘\0’ literal in this code, we can assume that the programmer wanted to check the value of one character. Besides, we know that comparing the headerM pointer with NULLdoesn’t make sense. All of that taken into account, we figure that the programmer wanted to find out if the string is empty or not but made a mistake when writing the check. To fix the code, we need to add a pointer dereferencing operation.
Correct code
TCHAR headerM[headerSize] = TEXT("");
...
size_t Printer::doPrint(bool justDoIt)
{
...
if (*headerM != _T('\0'))
...
}Recommendation
The number 0 may denote NULL, false, the null character ‘\0’, or simply the value 0. So please don’t be lazy — avoid using 0 for shorter notations in every single case. It only makes the code less comprehensible, and errors harder to find.
Use the following notations:
- 0 — for integer zero;
- nullptr — for null pointers in C++;
- NULL — for null pointers in C;
- ‘\0’, L’\0', _T(‘\0’) — for the terminal null;
- 0.0, 0.0f — for zero in expressions with floating-point types;
- false, FALSE — for the value ‘false’.
Sticking to this rule will make your code clearer, and make it easier for you and other programmers to spot bugs during code reviews.
10. Avoid using multiple small #ifdef blocks
The fragment is taken from CoreCLR project. The error is detected by the following PVS-Studio diagnostic: V522 Dereferencing of the null pointer ‘hp’ might take place.
heap_segment* gc_heap::get_segment_for_loh (size_t size
#ifdef MULTIPLE_HEAPS
, gc_heap* hp
#endif //MULTIPLE_HEAPS
)
{
#ifndef MULTIPLE_HEAPS
gc_heap* hp = 0;
#endif //MULTIPLE_HEAPS
heap_segment* res = hp->get_segment (size, TRUE);
if (res != 0)
{
#ifdef MULTIPLE_HEAPS
heap_segment_heap (res) = hp;
#endif //MULTIPLE_HEAPS
....
}
Explanation
I believe that #ifdef/#endif constructs are evil — an unavoidable evil, unfortunately. They are necessary and we have to use them. So I won’t urge you to stop using #ifdef, there’s no point in that. But I do want to ask you to be careful to not “overuse” it.
I guess many of you have seen code literally stuffed with #ifdefs. It’s especially painful to deal with code where #ifdef is repeated every ten lines, or even more often. Such code is usually system-dependent, and you can’t do without using #ifdef in it. That doesn’t make you any happier, though.
See how difficult it is to read the code sample above! And it is code reading which programmers have to do as their basic activity. Yes, I do mean it. We spend much more time reviewing and studying existing code than writing new one. That’s why code which is hard to read reduces our efficiency so much, and leaves more chance for new errors to sneak in.
Getting back to our code fragment, the error is found in the null pointer dereferencing operation, and occurs when the MULTIPLE_HEAPS macro is not declared. To make it easier for you, let’s expand the macros:
heap_segment* gc_heap::get_segment_for_loh (size_t size)
{
gc_heap* hp = 0;
heap_segment* res = hp->get_segment (size, TRUE);
....
The programmer declared the hp variable, initialized it to NULL, and dereferenced it right off. If MULTIPLE_HEAPS hasn’t been defined, we’ll get into trouble.
Correct code
This error is still living in CoreCLR (12.04.2016) despite a colleague of mine having reported it in the article “25 Suspicious Code Fragments in CoreCLR”, so I’m not sure how best to fix this error.
As I see it, since (hp == nullptr), then the ‘res’ variable should be initialized to some other value, too — but I don’t know what value exactly. So we’ll have to do without the fix this time.
Recommendations
Eliminate small #ifdef/#endif blocks from your code — they make it really hard to read and understand! Code with “woods” of #ifdefs is harder to maintain and more prone to mistakes.
There is no recommendation to suit every possible case — it all depends on the particular situation. Anyway, just remember that #ifdef is a source of trouble, so you must always strive to keep your code as clear as possible.
Tip N1. Try refusing #ifdef.
#ifdef can be sometimes replaced with constants and the usual if operator. Compare the following 2 code fragments: A variant with macros:
#define DO 1
#ifdef DO
static void foo1()
{
zzz();
}
#endif //DO
void F()
{
#ifdef DO
foo1();
#endif // DO
foo2();
}
This code is hard to read; you don’t even feel like doing it. Bet you’ve skipped it, haven’t you? Now compare it to the following:
const bool DO = true;
static void foo1()
{
if (!DO)
return;
zzz();
}
void F()
{
foo1();
foo2();
}
It’s much easier to read now. Some may argue the code has become less efficient since there is now a function call and a check in it. But I don’t agree with that. First, modern compilers are pretty smart and you are very likely to get the same code without any extra checks and function calls in the release version. Second, the potential performance losses are too small to be bothered about. Neat and clear code is more important.
Tip N2. Make your #ifdef blocks larger.
If I were to write the get_segment_for_loh() function, I wouldn’t use a number of #ifdefs there; I’d make two versions of the function instead. True, there’d be a bit more text then, but the functions would be easier to read, and edit too.
Again, some may argue that it’s duplicated code, and since they have lots of lengthy functions with #ifdefin each, having two versions of each function may cause them to forget about one of the versions when fixing something in the other.
Hey, wait! And why are your functions lengthy? Single out the general logic into separate auxiliary functions — then both of your function versions will become shorter, ensuring that you will easily spot any differences between them.
I know this tip is not a cure-all. But do think about it.
Tip N3. Consider using templates — they might help.
Tip N4. Take your time and think it over before using #ifdef. Maybe you can do without it? Or maybe you can do with fewer #ifdefs, and keep this “evil” in one place?
11. Don’t try to squeeze as many operations as possible in one line
The fragment is taken from Godot Engine project. The error is detected by the following PVS-Studio diagnostic: V567 Undefined behavior. The ‘t’ variable is modified while being used twice between sequence points.
static real_t out(real_t t, real_t b, real_t c, real_t d)
{
return c * ((t = t / d - 1) * t * t + 1) + b;
}
Explanation
Sometimes, you can come across code fragments where the authors try to squeeze as much logic as possible into a small volume of code, by means of complex constructs. This practice hardly helps the compiler, but it does make the code harder to read and understand for other programmers (or even the authors themselves). Moreover, the risk of making mistakes in such code is much higher, too.
It is in such fragments, where programmers try to put lots of code in just a few lines, that errors related to undefined behavior are generally found. They usually have to do with writing in and reading from one and the same variable within one sequence point. For a better understanding of the issue, we need to discuss in more detail the notions of “undefined behavior” and “sequence point”.
Undefined behavior is the property of some programming languages to issue a result that depends on the compiler implementation or switches of optimization. Some cases of undefined behavior (including the one being discussed here) are closely related to the notion of a “sequence point”.
A sequence point defines any point in a computer program’s execution at which it is guaranteed that all side effects of previous evaluations will have been performed, and no side effects from subsequent evaluations have yet been revealed. In C/C++ programming languages there are following sequence points:
- sequence points for operators “&&”, “||”, “,”. When not overloaded, these operators guarantee left-to-right execution order;
- sequence point for ternary operator “?:”;
- sequence point at the end of each full expression (usually marked with ‘;’);
- sequence point in place of the function call, but after evaluating the arguments;
- sequence point when returning from the function.
Note. The new C++ standard has discarded the notion of a “sequence point”, but we’ll be using the above given explanation to let those of you unfamiliar with the subject, grasp the general idea easier and faster. This explanation is simpler than the new one, and is sufficient for us to understand why one shouldn’t squeeze lots of operations into one “pile”.
In the example we have started with, there is none of the above mentioned sequence points, while the ‘=’ operator, as well as the parentheses, can’t be treated as such. Therefore, we cannot know which value of the t variable will be used when evaluating the return value.
In other words, this expression is one single sequence point, so it is unknown in what order the t variable will be accessed. For instance, the “t * t” subexpression may be evaluated before or after writing into the “ t = t / d — 1” variable.
Correct code
static real_t out(real_t t, real_t b, real_t c, real_t d)
{
t = t / d - 1;
return c * (t * t * t + 1) + b;
}
Recommendation
It obviously wasn’t a good idea to try to fit the whole expression in one line. Besides it being difficult to read, it also made it easier for an error to sneak in.
Having fixed the defect and split the expression into two parts, we have solved 2 issues at once — made the code more readable, and gotten rid of undefined behavior by adding a sequence point.
The code discussed above is not the only example, of course. Here’s another:
*(mem+addr++) =
(opcode >= BENCHOPCODES) ? 0x00 : ((addr >> 4)+1) << 4;
Just as in the previous case, the error in this code has been caused by unreasonably complicated code. The programmer’s attempt to increment theaddr variable within one expression has led to undefined behavior as it is unknown which value the addr variable will have in the right part of the expression — the original or the incremented one.
The best solution to this problem is the same as before — do not complicate matters without reason; arrange operations in several expressions instead of putting them all in one:
*(mem+addr) = (opcode >= BENCHOPCODES) ? 0x00 : ((addr >> 4)+1) << 4;
addr++;
There is a simple yet useful conclusion to draw from all of this — do not try to fit a set of operations in as few lines if possible. It may be more preferable to split the code into several fragments, thus making it more comprehensible, and reducing the chance errors occuring.
Next time you’re about to write complex constructs, pause for a while and think what using them will cost you, and if you are ready to pay that price.
12. When using Copy-Paste, be especially careful with the last lines
This bug was found in Source SDK library. The error is detected by the following PVS-Studio diagnostic: V525 The code containing the collection of similar blocks. Check items ‘SetX’, ‘SetY’, ‘SetZ’, ‘SetZ’.
inline void SetX( float val );
inline void SetY( float val );
inline void SetZ( float val );
inline void SetW( float val );
inline void Init( float ix=0, float iy=0,
float iz=0, float iw = 0 )
{
SetX( ix );
SetY( iy );
SetZ( iz );
SetZ( iw );
}
Explanation
I’m 100% sure this code was written with the help of Copy-Paste. One of the first lines was copied several times, with certain letters changed in its duplicates. At the very end, this technique failed the programmer: his attention weakened, and he forgot to change letter ‘Z’ to ‘W’ in the last line.
In this example, we are not concerned about the fact of a programmer making a mistake; what matters is that it was made at the end of a sequence of monotonous actions.
I do recommend reading the article “The Last Line Effect”. Due to public interest a scientific version of it also got published.
Put briefly, when copying code fragments through the Copy-Paste method, it is highly probable that you will make a mistake at the very end of the sequence of copied lines. It’s not my guess, it’s statistical data.
Correct code
{
SetX( ix );
SetY( iy );
SetZ( iz );
SetW( iw );
}Recommendation
I hope you have already read the article I’ve mentioned above. So, once again, we are dealing with the following phenomenon. When writing similarly looking code blocks, programmers copy and paste code fragments with slight changes. While doing so, they tend to forget to change certain words or characters, and it most often happens at the end of a sequence of monotonous actions because their attention weakens.
To reduce the number of such mistakes, here are a few tips for you:
- Arrange your similar looking code blocks in “tables”: it should make mistakes more prominent. We will discuss the “table” code layout in the next section. Perhaps in this case the table layout wasn’t of much help, but still it’s a very useful thing in programming.
- Be very careful and attentive when using Copy-Paste. Stay focused, and double-check the code you have written — especially the last few lines.
- You have now learned about the last line effect; try to keep this in mind, and tell your colleagues about it. The very fact of you knowing how such errors occur, should help you avoid them.
- Share the link to the “The Last Line Effect” article with your colleagues.
13. Table-style formatting
Fragment taken from the ReactOS project (open-source operating system compatible with Windows). The error is detected by the following PVS-Studio diagnostic: V560 A part of conditional expression is always true: 10035L.
void adns__querysend_tcp(adns_query qu, struct timeval now) {
...
if (!(errno == EAGAIN || EWOULDBLOCK ||
errno == EINTR || errno == ENOSPC ||
errno == ENOBUFS || errno == ENOMEM)) {
...
}Explanation
The code sample given above is small and you can easily spot the error in it. But when dealing with real-life code, bugs are often very hard to notice. When reading code like that, you tend to unconsciously skip blocks of similar comparisons and go on to the next fragment.
The reason why it happens has to do with the fact that conditions are poorly formatted and you don’t feel like paying too much attention to them because it requires certain effort, and we assume that since the checks are similar, there are hardly any mistakes in the condition and everything should be fine.
One of the ways out is formatting the code as a table.
If you felt too lazy to search for an error in the code above, I’ll tell you: “errno ==” is missing in one of the checks. It results in the condition always being true as the EWOULDBLOCK is not equal to zero.
Correct code
if (!(errno == EAGAIN || errno == EWOULDBLOCK ||
errno == EINTR || errno == ENOSPC ||
errno == ENOBUFS || errno == ENOMEM)) {
Recommendation
For a start, here’s a version of this code formatted in the simplest “table” style. I don’t like it actually.
if (!(errno == EAGAIN || EWOULDBLOCK ||
errno == EINTR || errno == ENOSPC ||
errno == ENOBUFS || errno == ENOMEM)) {
It’s better now, but not quite.
There are two reasons why I don’t like this layout. First, the error is still not much visible; second, you have to insert too many spaces to align the code.
That’s why we need to make two improvements in this formatting style. The first one is we need to use no more than one comparison per line: it makes errors easy to notice. For example:
a == 1 &&
b == 2 &&
c &&
d == 3 &&
The second improvement is to write operators &&, ||, etc., in a more rational way, i.e. on the left instead of on the right.
See how tedious it is to align code by means of spaces:
x == a &&
y == bbbbb &&
z == cccccccccc &&
Writing operators on the left makes it much faster and easier:
x == a
&& y == bbbbb
&& z == cccccccccc
The code looks a bit odd, but you’ll get used to it very soon.
Let’s combine these two improvements to write our code sample in the new style:
if (!( errno == EAGAIN
|| EWOULDBLOCK
|| errno == EINTR
|| errno == ENOSPC
|| errno == ENOBUFS
|| errno == ENOMEM)) {
Yes, it’s longer now — yet the error has become clearly seen, too.
I agree that it looks strange, but nevertheless I do recommend this technique. I’ve been using it myself for half a year now and enjoy it very much, so I’m confident about this recommendation.
I don’t find it a problem at all that the code has become longer. I’d even write it in a way like this:
const bool error = errno == EAGAIN
|| errno == EWOULDBLOCK
|| errno == EINTR
|| errno == ENOSPC
|| errno == ENOBUFS
|| errno == ENOMEM;
if (!error) {
Feel disappointed with the code being too lengthy and cluttered? I agree. So let’s make it a function!
static bool IsInterestingError(int errno)
{
return errno == EAGAIN
|| errno == EWOULDBLOCK
|| errno == EINTR
|| errno == ENOSPC
|| errno == ENOBUFS
|| errno == ENOMEM;
}
....
if (!IsInterestingError(errno)) {
You may think that I’m dramatizing things, being too much of a perfectionist. But I assure you that errors are very common in complex expressions, and I wouldn’t ever bring them up weren’t they ‘so frequent. They are everywhere. And they are very difficult to notice.
Here’s another example from WinDjView project:
inline bool IsValidChar(int c)
{
return c == 0x9 || 0xA || c == 0xD ||
c >= 0x20 && c <= 0xD7FF ||
c >= 0xE000 && c <= 0xFFFD ||
c >= 0x10000 && c <= 0x10FFFF;
}
The function consists of just a few lines, but it still has an error. The function always returns true. The reason, in the long run, has to do with poor formatting and programmers maintaining the code for many years being unwilling to read it carefully.
Let’s refactor this code in the “table” style, I’d also add some parentheses:
inline bool IsValidChar(int c)
{
return
c == 0x9
|| 0xA
|| c == 0xD
|| (c >= 0x20 && c <= 0xD7FF)
|| (c >= 0xE000 && c <= 0xFFFD)
|| (c >= 0x10000 && c <= 0x10FFFF);
}
You don’t have to format your code exactly the way I suggest. The aim of this post is to draw your attention to typos in “chaotically” written code. By arranging it in the “table” style, you can avoid lots of silly typos, and that’s already great. So I hope this post will help you.
Note
Being completely honest, I have to warn you that “table” formatting may sometimes cause harm. Check this example:
inline
void elxLuminocity(const PixelRGBi& iPixel,
LuminanceCell< PixelRGBi >& oCell)
{
oCell._luminance = 2220*iPixel._red +
7067*iPixel._blue +
0713*iPixel._green;
oCell._pixel = iPixel;
}
It’s taken from the eLynx SDK project. The programmer wanted to align the code, so he added 0 before the value 713. Unfortunately, he forgot that 0 being the first digit in a number means that this number is octal.
An array of strings
I hope that the idea about the table formatting of the code is clear, but I feel like giving couple more examples. Let’s have a look at one more case. By bringing it here, I am saying that the table formatting should be used not only with conditions, but also with other various constructions of a language.
The fragment is taken from Asterisk project. The error is detected by the following PVS-Studio diagnostic: V653 A suspicious string consisting of two parts is used for array initialization. It is possible that a comma is missing. Consider inspecting this literal: “KW_INCLUDES” “KW_JUMP”.
static char *token_equivs1[] =
{
....
"KW_IF",
"KW_IGNOREPAT",
"KW_INCLUDES"
"KW_JUMP",
"KW_MACRO",
"KW_PATTERN",
....
};
There is a typo here — one comma is forgotten. As a result two strings that have completely different meaning are combined in one, i.e. we actually have:
....
"KW_INCLUDESKW_JUMP",
....
The error could be avoided if the programmer used the table formatting. Then, if the comma is omitted, it will be easy to spot.
static char *token_equivs1[] =
{
....
"KW_IF" ,
"KW_IGNOREPAT" ,
"KW_INCLUDES" ,
"KW_JUMP" ,
"KW_MACRO" ,
"KW_PATTERN" ,
....
};
Just like last time, pay attention, that if we put the delimiter to the right (a comma in this case), you have to add a lot of spaces, which is inconvenient. It is especially inconvenient if there is a new long line/phrase: we will have to reformat the entire table.
That’s why I would again recommend formatting the table in the following way:
static char *token_equivs1[] =
{
....
, "KW_IF"
, "KW_IGNOREPAT"
, "KW_INCLUDES"
, "KW_JUMP"
, "KW_MACRO"
, "KW_PATTERN"
....
};
Now it’s very easy to spot a missing comma and there is no need to use a lot of spaces — the code is beautiful and intuitive. Perhaps this way of formatting may seem unusual, but you quickly get used to it — try it yourself.
Finally, here is my short motto. As a rule, beautiful code is usually correct code.
14. A good compiler and coding style aren’t always enough
We have already spoken about good styles of coding, but this time we’ll have a look at an anti-example. It’s not enough to write good code: there can be various errors and a good programming style isn’t always a cure-all.
The fragment is taken from PostgreSQL. The error is detected by the following PVS-Studio diagnostic: V575 The ‘memcmp’ function processes ‘0’ elements. Inspect the third argument.
Cppcheck analyzer can also detect such errors. It issues a warning: Invalid memcmp() argument nr 3. A non-boolean value is required.
Datum pg_stat_get_activity(PG_FUNCTION_ARGS)
{
....
if (memcmp(&(beentry->st_clientaddr), &zero_clientaddr,
sizeof(zero_clientaddr) == 0))
....
}
Explanation
A closing parenthesis is put in a wrong place. It’s just a typo, but unfortunately it completely alters the meaning of the code.
The sizeof(zero_clientaddr) == 0 expression always evaluates to ‘false’ as the size of any object is always larger than 0. The false value turns to 0, which results in the memcmp() function comparing 0 bytes. Having done so, the function assumes that the arrays are equal and returns 0. It means that the condition in this code sample can be reduced to if (false).
Correct code
if (memcmp(&(beentry->st_clientaddr), &zero_clientaddr,
sizeof(zero_clientaddr)) == 0)
Recommendation
It’s just the case when I can’t suggest any safe coding technique to avoid typos. The only thing I can think of is “Yoda conditions”, when constants are written to the left of the comparison operator:
if (0 == memcmp(&(beentry->st_clientaddr), &zero_clientaddr,
sizeof(zero_clientaddr)))
But I won’t recommend this style. I don’t like and don’t use it for two reasons:
First, it makes conditions less readable. I don’t know how to put it exactly, but it’s not without reason that this style is called after Yoda.
Second, they don’t help anyway if we deal with parentheses put in a wrong place. There are lots of ways you can make a mistake. Here’s an example of code where using the Yoda conditions didn’t prevent the incorrect arrangement of parentheses:
if (0 == LoadStringW(hDllInstance, IDS_UNKNOWN_ERROR,
UnknownError,
sizeof(UnknownError) / sizeof(UnknownError[0] -
20)))
This fragment is taken from the ReactOS project. The error is difficult to notice, so let me point it out for you: sizeof(UnknownError[0] — 20).
So Yoda conditions are useless here.
We could invent some artificial style to ensure that every closing parenthesis stands under the opening one. But it will make the code too bulky and ugly, and no one will be willing to write it that way.
So, again, there is no coding style I could recommend to avoid writing closing parentheses in wrong places.
And here’s where the compiler should come in handy and warn us about such a strange construct, shouldn’t it? Well, it should but it doesn’t. I run Visual Studio 2015, specify the /Wall switch… and don’t get any warning. But we can’t blame the compiler for that, it has enough work to do as it is.
The most important conclusion for us to draw from today’s post is that good coding style and compiler (and I do like the compiler in VS2015) do not always make it. I sometimes hear statements like, “You only need to set the compiler warnings at the highest level and use good style, and everything’s going to be OK” No, it’s not like that. I don’t mean to say some programmers are bad at coding; it’s just that every programmer makes mistakes. Everyone, no exceptions. Many of your typos are going to sneak past the compiler and good coding style.
So the combo of good style + compiler warnings is important but not sufficient. That’s why we need to use a variety of bug search methods. There’s no silver bullet; the high quality of code can be only achieved through a combination of several techniques.
The error we are discussing here can be found by means of the following methods:
- code review;
- unit-tests;
- manual testing;
- static code analysis;
- etc.
I suppose you have already guessed that I am personally interested in the static code analysis methodology most of all. By the way, it is most appropriate for solving this particular issue because it can detect errors at the earliest stage, i.e. right after the code has been written.
Indeed, this error can be easily found by such tools as Cppcheck or PVS-Studio.
Conclusion. Some people don’t get it that having skill isn’t enough to avoid mistakes. Everyone makes them — it’s inevitable. Even super-guru make silly typos every now and then. And since it’s inevitable, it doesn’t make sense blaming programmers, bad compilers, or bad style. It’s just not going to help. Instead, we should use a combination of various software quality improving techniques.
15. Start using enum class in your code, if possible
All the examples of this error I have are large. I’ve picked one of the smallest, but it’s still quite lengthy. Sorry for that.
This bug was found in Source SDK library. The error is detected by the following PVS-Studio diagnostic: V556 The values of different enum types are compared: Reason == PUNTED_BY_CANNON.
enum PhysGunPickup_t
{
PICKED_UP_BY_CANNON,
PUNTED_BY_CANNON,
PICKED_UP_BY_PLAYER,
};
enum PhysGunDrop_t
{
DROPPED_BY_PLAYER,
THROWN_BY_PLAYER,
DROPPED_BY_CANNON,
LAUNCHED_BY_CANNON,
};
void CBreakableProp::OnPhysGunDrop(...., PhysGunDrop_t Reason)
{
....
if( Reason == PUNTED_BY_CANNON )
{
PlayPuntSound();
}
....
}
Explanation
The Reason variable is an enumeration of the PhysGunDrop_t type. This variable is compared to the named constant PUNTED_BY_CANNON belonging to another enumeration, this comparison being obviously a logical error.
This bug pattern is quite widespread. I came across it even in such projects as Clang, TortoiseGit, and Linux Kernel.
The reason why it is so frequent is that enumerations are not type safe in the standard C++; you may get easily confused about what should be compared with what.
Correct code
I don’t know for sure what the correct version of this code should look like. My guess is that PUNTED_BY_CANNON should be replaced with DROPPED_BY_CANNON or LAUNCHED_BY_CANNON. Let it be LAUNCHED_BY_CANNON.
if( Reason == LAUNCHED_BY_CANNON )
{
PlayPuntSound();
}
Recommendation
Consider yourself lucky if you write in C++; I recommend that you start using enum class right now and the compiler won’t let you compare values, that refer to different enumerations. You won’t be comparing pounds with inches anymore.
There are certain innovations in C++ I don’t have much confidence in. Take, for instance, the autokeyword. I believe it may be harmful when used too often. Here’s how I see it: programmers spend more time reading the code rather than writing it, so we must ensure that the program text is easy-to-read. In the C language, variables are declared in the beginning of the function, so when you edit the code in the middle or at the end of it, it’s not always easy to figure what some Alice variable actually means. That’s why there exists a variety of variable naming notations. For instance, there is a prefix notation, wherepfAlice may stand for a “pointer to float”.
In C++, you can declare variables whenever you need, and it is considered a good style. Using prefixes and suffixes in variable names is no longer popular. And here the auto keyword emerges, resulting in programmers starting to use multiple mysterious constructs of the “auto Alice = Foo();” kind again. Alice, who the fuck is Alice?!
Sorry for digressing from our subject. I wanted to show you that some of the new features may do both good and bad. But it’s not the case with enum class: I do believe it does only good.
When using enum class, you must explicitly specify to which enumeration a named constant belongs to. It protects the code from new errors. That is, the code will look like this:
enum class PhysGunDrop_t
{
DROPPED_BY_PLAYER,
THROWN_BY_PLAYER,
DROPPED_BY_CANNON,
LAUNCHED_BY_CANNON,
};
void CBreakableProp::OnPhysGunDrop(...., PhysGunDrop_t Reason)
{
....
if( Reason == PhysGunDrop_t::LAUNCHED_BY_CANNON )
{
PlayPuntSound();
}
....
}
True, fixing old code may involve certain difficulties. But I do urge you to start using enum class in new code right from this day on. Your project will only benefit from it.
I don’t see much point in introducing enum class. Here’s a few links for you to learn all the details about this new wonderful feature of the C++11 language:
- Wikipedia. C++11. Strongly typed enumerations.
- Cppreference. Enumeration declaration.
- StackOverflow. Why is enum class preferred over plain enum?
16. “Look what I can do!” — Unacceptable in programming
This section will be slightly similar to “Don’t try to squeeze as many operations as possible in one line”, but this time I want to focus on a different thing. Sometimes it feels like programmers are competing against somebody, trying to write the shortest code possible.
I am not speaking about complicated templates. This is a different topic for discussion, as it is very hard to draw a line between where these templates do harm, and where they do good. Now I am going to touch upon a simpler situation which is relevant for both C and C++ programmers. They tend to make the constructions more complicated, thinking, “I do it because I can”.
The fragment is taken from KDE4 project. The error is detected by the following PVS-Studio diagnostic: V593 Consider reviewing the expression of the ‘A = B == C’ kind. The expression is calculated as following: ‘A = (B == C)’.
void LDAPProtocol::del( const KUrl &_url, bool )
{
....
if ( (id = mOp.del( usrc.dn() ) == -1) ) {
LDAPErr();
return;
}
ret = mOp.waitForResult( id, -1 );
....
}
Explanation
After looking at this code, I always have questions such as: What was the point of doing it? Did you want to save a line? Did you want to show that you can combine several actions in one expression?
As a result we have a typical error pattern — using expressions of the if (A = Foo() == Error) kind.
The precedence of the comparison operation is higher than that of the assignment operation. That’s why the “mOp.del( usrc.dn() ) == -1” comparison is executed first, and only then the true (1) or false (0) value is assigned to the id variable.
If mOp.del() returns ‘-1’, the function will terminate; otherwise, it will keep running and the ‘id’ variable will be assigned an incorrect value. So it will always equal 0.
Correct code
I want to emphasize: adding extra parentheses is not a solution to the problem. Yes, the error can be eliminated. But it’s the wrong way.
There were additional parentheses in the code — have a closer look. It’s difficult to say what they were meant for; perhaps the programmer wanted to get rid of the compiler warnings. Perhaps he suspected that the operation priority may be not right, and wanted to fix this issue, but failed to do so. Anyway, those extra brackets don’t help.
There is a deeper problem here. If it is a possible not to make the code more complicated, don’t. It is better to write:
id = mOp.del(usrc.dn());
if ( id == -1 ) {
Recommendation
Don’t be so lazy as not to write an extra code line: complex expressions are hard to read, after all. Do the assignment first, and only then, the comparison. Thus you will make it easier for programmers who will be maintaining your code later, and also it will reduce the chances of making a mistake.
So my conclusion is — don’t try to show off.
This tip sounds trivial, but I hope it will help you. It’s always better to write clear and neat code, instead of in a “see how cool I am” style.
17. Use dedicated functions to clear private data
The fragment is taken from the Apache HTTP Server project. The error is detected by the following PVS-Studio diagnostic: V597 The compiler could delete the ‘memset’ function call, which is used to flush ‘x’ buffer. The RtlSecureZeroMemory() function should be used to erase the private data.
static void MD4Transform(
apr_uint32_t state[4], const unsigned char block[64])
{
apr_uint32_t a = state[0], b = state[1],
c = state[2], d = state[3],
x[APR_MD4_DIGESTSIZE];
....
/* Zeroize sensitive information. */
memset(x, 0, sizeof(x));
}
Explanation
In this code the programmer uses a call of the memset() function to erase private data. But it’s not the best way to do that because the data won’t actually be erased. To be more exact, whether or not they will be erased depends on the compiler, its settings, and the Moon phase.
Try to look at this code from the compiler’s viewpoint. It does its best to make your code work as fast as possible, so it carries out a number of optimizations. One of them is to remove the calls of functions which don’t affect the program’s behavior, and are therefore excessive from the viewpoint of the C/C++ language. This is exactly the case with the memset() function in the code sample above. True, this function changes the ‘x’ buffer, but this buffer is not used anywhere after that, which means the call of the memset() function can — and ought to — be deleted.
Important! What I’m telling you now is not a theoretical model of the compiler’s behavior — it’s a real-life one. In such cases, the compiler does remove the calls of the memset() function. You can do a few experiments to check it for yourself. For more details and examples on this issue, please see the following articles:
- Security, security! But do you test it?
- Safe Clearing of Private Data.
- V597. The compiler could delete the ‘memset’ function call, which is used to flush ‘Foo’ buffer. The RtlSecureZeroMemory() function should be used to erase the private data
- Zero and forget — caveats of zeroing memory in C (see also the discussionof this article).
- MSC06-C. Beware of compiler optimizations.
What makes this error with removed memset() calls especially tricky, is its being very hard to track. When working in the debugger, you will most likely be dealing with un-optimized code, with the function call still there. You can only find the error when studying the assembler listing, generated when building the optimized application version.
Some programmers believe that it has to do with a bug in the compiler, and that it has no right to throw away the calls of such an important function asmemset(). But this is not the case. This function is by no means more, or less, important than any other, so the compiler has full right to optimize the code where it is called. After all, such code may turn out to be excessive indeed.
Correct code
memset_s(x, sizeof(x), 0, sizeof(x));
or
RtlSecureZeroMemory(x, sizeof(x));
Recommendation
You should use special memory clearing functions that the compiler is not allowed to remove for its optimization purposes.
Visual Studio, for instance, offers the RtlSecureZeroMemory function; and starting with C11, you can use the memset_s function. If necessary, you can even create a safe function of your own — there are lots of examples on the Internet. Here is a couple of them.
Version No.1.
errno_t memset_s(void *v, rsize_t smax, int c, rsize_t n) {
if (v == NULL) return EINVAL;
if (smax > RSIZE_MAX) return EINVAL;
if (n > smax) return EINVAL;
volatile unsigned char *p = v;
while (smax-- && n--) {
*p++ = c;
}
return 0;
}Version No.2.
void secure_zero(void *s, size_t n)
{
volatile char *p = s;
while (n--) *p++ = 0;
}
Some programmers even go further, and implement functions to fill the array with pseudo-random values, these functions running at different times to ensure better protection from time-measuring attacks. You can find the implementations of such functions on the internet, too.
18. The knowledge you have, working with one language isn’t always applicable to another language
The fragment is taken from Putty project. Ineffective code is detected by the following PVS-Studio diagnostic: V814 Decreased performance. Calls to the ‘strlen’ function have being made multiple times when a condition for the loop’s continuation was calculated.
static void tell_str(FILE * stream, char *str)
{
unsigned int i;
for (i = 0; i < strlen(str); ++i)
tell_char(stream, str[i]);
}
Explanation
There’s no actual error here, but such code can be extremely inefficient when we deal with long strings, as the strlen() function is called in every loop iteration. So the error, if there is one here, is one of inefficiency.
As a rule, this kind of thing is typically found in code written by those that have previously worked with the Pascal language (or Delphi). In Pascal, the evaluation of the terminating condition of the loop is computed just once, thus this code is suitable and quite commonly used.
Let’s have a look at an example of code written in Pascal. The word called will be printed only once, because the pstrlen() is called only once.
program test;
var
i : integer;
str : string;
function pstrlen(str : string): integer;
begin
writeln('called');
strlen := Length(str);
end;
begin
str := 'a pascal string';
for i:= 1 to pstrlen(str) do
writeln(str[i]);
end.
Effective code:
static void tell_str(FILE * stream, char *str)
{
size_t i;
const size_t len = strlen(str);
for (i = 0; i < len; ++i)
tell_char(stream, str[i]);
}
Recommendation
Don’t forget that in C/C++, loop termination conditions are re-computed at the end of each and every iteration. Therefore it’s not a good idea to call inefficient slow functions as part of this evaluation, especially if you can compute it just the once, before the loop is entered.
In some cases the compiler might be able to optimize the code with strlen(). For instance, if the pointer always refers to the same string literal, but we shouldn’t rely on that in any way.
19. How to properly call one constructor from another
This issue was found in LibreOffice project. The error is detected by the following PVS-Studio diagnostic: V603 The object was created but it is not being used. If you wish to call constructor, ‘this->Guess::Guess(….)’ should be used.
Guess::Guess()
{
language_str = DEFAULT_LANGUAGE;
country_str = DEFAULT_COUNTRY;
encoding_str = DEFAULT_ENCODING;
}
Guess::Guess(const char * guess_str)
{
Guess();
....
}
Explanation
Good programmers hate writing duplicate code. And that’s great. But when dealing with constructors, many shoot themselves in the foot, trying to make their code short and neat.
You see, a constructor can’t simply be called like an ordinary function. If we write “A::A(int x) { A(); }”, it will lead to creating a temporary unnamed object of the A type, instead of calling a constructor without arguments.
This is exactly what happens in the code sample above: a temporary unnamed object Guess() is created and gets immediately destroyed, while the class member language_str and others remain uninitialized.
Correct code:
There used to be 3 ways to avoid duplicate code in constructors. Let’s see what they were.
The first way is to implement a separate initialization function, and call it from both constructors. I’ll spare you the examples — it should be obvious as it is.
That’s a fine, reliable, clear, and safe technique. However, some bad programmers want to make their code even shorter. So I have to mention two other methods.
They are pretty dangerous, and require you to have a good understanding of how they work, and what consequences you may have to face.
The second way:
Guess::Guess(const char * guess_str)
{
new (this) Guess();
....
}
Third way:
Guess::Guess(const char * guess_str)
{
this->Guess();
....
}
The second and the third variant are rather dangerous because the base classes are initialized twice. Such code can cause subtle bugs, and do more harm than good. Consider an example where such a constructor call is appropriate, where it’s not.
Here is a case where everything is fine:
class SomeClass
{
int x, y;
public:
SomeClass() { new (this) SomeClass(0,0); }
SomeClass(int xx, int yy) : x(xx), y(yy) {}
};
The code is safe and works well since the class only contains simple data types, and is not derived from other classes. A double constructor call won’t pose any danger.
And here’s another example where explicitly calling a constructor will cause an error:
class Base
{
public:
char *ptr;
std::vector vect;
Base() { ptr = new char[1000]; }
~Base() { delete [] ptr; }
};
class Derived : Base
{
Derived(Foo foo) { }
Derived(Bar bar) {
new (this) Derived(bar.foo);
}
Derived(Bar bar, int) {
this->Derived(bar.foo);
}
}
So we call the constructor using the expressions “new (this) Derived(bar.foo);” or “this->Derived(bar.foo)”.
The Base object is already created, and the fields are initialized. Calling the constructor once again will cause double initialization. As a result, a pointer to the newly allocated memory chunk will be written intoptr, which will result in a memory leak. As for double initialization of an object of the std::vectortype, the consequences of it are even harder to predict. One thing is clear: code like that is not permissible.
Do you need all that headache, after all? If you can’t utilize C++11’s features, then use method No. 1 (create an initialization function). An explicit constructor call may be only needed on very rare occasions.
Recommendation
And now we have a feature to help us with the constructors, at last!
C++11 allows constructors to call other peer constructors (known as delegation). This allows constructors to utilize another constructor’s behavior with a minimum of added code.
For example:
Guess::Guess(const char * guess_str) : Guess()
{
....
}
To learn more about delegating constructors, see the following links:
- Wikipedia. C++11. Object construction improvement.
- C++11 FAQ. Delegating constructors.
- MSDN. Uniform Initialization and Delegating Constructors.
20. The End-of-file (EOF) check may not be enough
The fragment is taken from SETI@home project. The error is detected by the following PVS-Studio diagnostic: V663 Infinite loop is possible. The ‘cin.eof()’ condition is insufficient to break from the loop. Consider adding the ‘cin.fail()’ function call to the conditional expression.
template <typename T>
std::istream &operator >>(std::istream &i, sqlblob<T> &b)
{
....
while (!i.eof())
{
i >> tmp;
buf+=(tmp+' ');
}
....
}
Explanation
The operation of reading data from a stream object is not as trivial as it may seem at first. When reading data from streams, programmers usually call theeof() method to check if the end of stream has been reached. This check, however, is not quite adequate as it is not sufficient and doesn’t allow you to find out if any data reading errors or stream integrity failures have occurred, which may cause certain issues.
Note. The information provided in this article concerns both input and output streams. To avoid repetition, we’ll only discuss one type of stream here.
This is exactly the mistake the programmer made in the code sample above: in the case of there being any data reading error, an infinite loop may occur as the eof() method will always return false. On top of that, incorrect data will be processed in the loop, as unknown values will be getting to the tmp variable.
To avoid issues like that, we need to use additional methods to check the stream status: bad(), fail().
Correct code
Let’s take advantage of the fact that the stream can implicitly cast to the booltype. The true value indicates that the value is read successfully. More details about the way this code works can be foundon StackOverflow.
template <typename T>
std::istream &operator >>(std::istream &i, sqlblob<T> &b)
{
....
while (i >> tmp)
{
buf+=(tmp+' ');
}
....
}
Recommendation
When reading data from a stream, don’t use the eof() method only; check for any failures, too.
Use the methods bad() and fail() to check the stream status. The first method is used to check stream integrity failures, while the second is for checking data reading errors.
However, it’s much more convenient to use bool() operator, as it is shown in the example of the correct code.
21. Check that the end-of-file character is reached correctly (EOF)
Let’s continue the topic of working with files. And again we’ll have a look at EOF. But this time we’ll speak about a bug of a completely different type. It usually reveals itself in localized versions of software.
The fragment is taken from Computational Network Toolkit. The error is detected by the following PVS-Studio diagnostic: V739 EOF should not be compared with a value of the ‘char’ type. The ‘c’ should be of the ‘int’ type.
string fgetstring(FILE* f)
{
string res;
for (;;)
{
char c = (char) fgetc(f);
if (c == EOF)
RuntimeError("error reading .... 0: %s", strerror(errno));
if (c == 0)
break;
res.push_back(c);
}
return res;
}
Explanation
Let’s look at the way EOF is declared:
#define EOF (-1)
As you can see, the EOF is nothing more than ‘-1 ‘ of int type. Fgetc() function returns a value of inttype. Namely, it can return a number from 0 to 255 or -1 (EOF). The values read are placed into a variable of char type. Because of this, a symbol with the 0xFF (255) value turns into -1, and then is handled in the same way as the end of file (EOF).
Users that use Extended ASCII Codes, may encounter an error when one of the symbols of their alphabet is handled incorrectly by the program.
For example in the Windows 1251 code page, the last letter of Russian alphabet has the 0xFF code, and so, is interpreted by the program as the end-of-file character.
Correct code
for (;;)
{
int c = fgetc(f);
if (c == EOF)
RuntimeError("error reading .... 0: %s", strerror(errno));
if (c == 0)
break;
res.push_back(static_cast<char>(c));
}
Recommendation
There is probably no particular recommendation here, but as we are speaking about EOF, I wanted to show an interesting variant of an error, that some people aren’t aware of.
Just remember, if the functions return the values of int type, don’t hasten to change it into char. Stop and check that everything is fine. By the way, we have already had a similar case discussing the functionmemcmp() in Chapter N2 — “Larger than 0 does not mean 1” (See the fragment about a vulnerability in MySQL)
