How to reduce the likelihood of errors at the stage of writing code. Note N4
This is the fourth article where I want to share useful observations about error patterns and how to deal with them. This time I will touch on a topic such as handling rare and emergency situations in programs. Considering many programs, I came to the conclusion that the error handling code in C / C ++ programs is one of the most unreliable places.
What causes such defects? The program, instead of displaying the message “file X was not found,” crashes and forces the user to guess what he is doing wrong. The program for working with the database displays an unintelligible message, instead of reporting that one of the fields is incorrectly filled. Let's try to fight with this kind of errors that annoy our users.
Introduction
Initial information for readers who are not familiar with my previous notes. They can be found here:
As always, I will not be abstract, but I'll start with examples. This time, the examples will be taken from the Firefox source code. I will try to demonstrate that even in a high-quality and well-known application with error-handling code, this is not the best way. Defects were found by me using the PVS-Studio 4.50 analyzer.
Error examples
Example N1. Incomplete check of the integrity of the table
int AffixMgr :: parse_convtable (..., const char * keyword)
{
...
if (strncmp (piece, keyword, sizeof (keyword))! = 0) {
HUNSPELL_WARNING (stderr,
"error: line% d: table is corrupt \ n",
af-> getlinenum ());
delete * rl;
* rl = NULL;
return 1;
}
...
} PVS-Studio Diagnostics: V579 for strncmp It is possibly a mistake. Inspect the third argument. affixmgr.cpp 3708
')
An attempt was made here to check the integrity of the table. Unfortunately, this check may or may not work. To calculate the length of a keyword, use the sizeof () operator, which is naturally incorrect. As a result, the efficiency of the code depends on a happy set of circumstances (the length of the keyword, the size of the 'keyword' pointer in the current data model ).
Example 2. Idle check when reading a file
int PatchFile :: LoadSourceFile (FILE * ofile)
{
...
size_t c = fread (rb, 1, r, ofile);
if (c <0) {
LOG (("LoadSourceFile:"
"error reading destination file:" LOG_S "\ n",
mFile));
return READ_ERROR;
}
...
} Diagnostics of PVS-Studio: V547 Expression 'c <0' is always false. Unsigned type value is never <0. updater.cpp 1179
This is an example when the error handling code is written according to the principle “to be”. The programmer did not even think about what he actually wrote, and how it would work. Check is incorrect. The fread () function returns the number of bytes read using an unsigned type. Prototype function:
size_t fread (
void * buffer,
size_t size,
size_t count,
FILE * stream
); Naturally, to store the result, the variable 'c' is used, which has type size_t. As a result, the test result (c <0) is always false.
This is a good example. At first glance, it seems that there is some kind of verification, but in practice it turns out that it is completely useless.
A similar error can be seen in other places:
V547 Expression 'c <0' is always false. Unsigned type value is never <0. updater.cpp 2373
V547 Expression 'c <0' is always false. Unsigned type value is never <0. bspatch.cpp 107
Example 3. Checking a pointer to NULL after using it
nsresult
nsFrameSelection :: MoveCaret (...)
{
...
mShell-> FlushPendingNotifications (Flush_Layout);
if (! mShell) {
return NS_OK;
}
...
} PVS-Studio Diagnostics: V595 The mShell 'pointer has been verified against nullptr. Check lines: 1107, 1109. nsselection.cpp 1107
If the pointer is zero, then we must process this special case and return NS_OK from the function. It is embarrassing that the mShell pointer is already used up to this point.
Most likely, this code works, since the mShell pointer is always unequal to NULL. I give an example to show that one can make a mistake even in very simple checks. There is a check, but there is no sense from it.
Example 4. Checking a pointer to NULL after using it
CompileStatus
mjit :: Compiler :: performCompilation (JITScript ** jitp)
{
...
JaegerSpew (JSpew_Scripts,
"successfully compiled (code \"% p \ ") (size \"% u \ ") \ n",
(* jitp) -> code.m__code.executableAddress (),
unsigned ((* jitp) -> code.m_size));
if (! * jitp)
return Compile_Abort;
...
} PVS-Studio Diagnostics: V595 The '* jitp' pointer was used before it was verified against nullptr. Check lines: 547, 549. compiler.cpp 547
By the way, using the pointer before the check is a common mistake. This is another example on this topic.
Example 5. Incomplete Input Validation
PRBool
nsStyleAnimation :: AddWeighted (...)
{
...
if (unit [0] == eCSSUnit_Null || unit [1] == eCSSUnit_Null ||
unit [0] == eCSSUnit_Null || unit [0] == eCSSUnit_URL) {
return PR_FALSE;
}
...
} PVS-Studio Diagnostics: V501 There are identical sub-expressions 'unit [0] == eCSSUnit_Null' ||| operator. nsstyleanimation.cpp 1767
It seems there are 2 typos here. It’s hard to say exactly how the code should look here, but probably they wanted to write this:
if (unit [0] == eCSSUnit_Null || unit [1] == eCSSUnit_Null ||
unit [0] == eCSSUnit_URL || unit [1] == eCSSUnit_URL) { Due to typos, the function may start processing incorrect input values.
Example 6. Incomplete Input Validation
nsresult PresShell :: SetResolution (float aXResolution, float aYResolution)
{
if (! (aXResolution> 0.0 && aXResolution> 0.0)) {
return NS_ERROR_ILLEGAL_VALUE;
}
...
} PVS-Studio diagnostics: V501 operator: aXResolution> 0.0 && aXResolution> 0.0 nspresshell.cpp 5114
And here is another example of unsuccessful verification of input parameters. This time, due to a typo, the value of the aYResolution argument is not checked.
Example 7. Untranslated pointer
nsresult
SVGNumberList :: SetValueFromString (const nsAString & aValue)
{
...
const char * token = str.get ();
if (token == '\ 0') {
return NS_ERROR_DOM_SYNTAX_ERR; // nothing between commas
}
...
} Diagnostics of PVS-Studio: V528 It is odd that the pointer to the 'char' type is compared with the '\ 0' value. Probably meant: * token == '\ 0'. svgnumberlist.cpp 96
Verifying that there is nothing between the commas does not work. To find out if the string is empty or not, you can compare the first character with '\ 0'. But here it is not the first character that is compared with zero, but the pointer. This pointer is always unequal to zero. The correct check should look like this: (* token == '\ 0').
Example 8. Incorrect type for index storage
PRBool
nsIEProfileMigrator :: TestForIE7 ()
{
...
PRUint32 index = ieVersion.FindChar ('.', 0);
if (index <0)
return PR_FALSE;
...
} Diagnostics of PVS-Studio: V547 Expression 'index <0' is always false. Unsigned type value is never <0. nsieprofilemigrator.cpp 622
The function will not return PR_FALSE if there is no dot in the line and continues to work with incorrect data. The error is that the unsigned data type is selected for the 'index' variable. Check (index <0) does not make sense.
Example 9. Forming the wrong error message
cairo_status_t
_cairo_win32_print_gdi_error (const char * context)
{
...
fwprintf (stderr, L "% s:% S", context, (wchar_t *) lpMsgBuf);
...
} Diagnostics of PVS-Studio: V576 Incorrect format. Consider checking the fwprintf function. Wchar_t type symbols is expected. cairo-win32-surface.c 129
Even if the error is successfully detected, it still needs to be able to properly handle. And since no one tests error handlers either, there you can see a lot of interesting things.
The _cairo_win32_print_gdi_error () function will print the abracadabra. As a third argument, the fwprintf () function expects a pointer to a unicode string, and instead receives a string in the format 'const char *'.
Example 10. Error writing dump
bool ExceptionHandler :: WriteMinidumpForChild (...)
{
...
DWORD last_suspend_cnt = -1;
...
// this thread may have already died
// the handle is a non-fatal error
if (NULL! = child_thread_handle) {
if (0 <= (last_suspend_cnt =
SuspendThread (child_thread_handle))) {
...
} Diagnostics of PVS-Studio: V547 Expression is always true. Unsigned type value is always> = 0. Exception_handler.cc 846
This is another example in the error handler. Here, the result returned by the SuspendThread function is incorrectly processed. The variable last_suspend_cnt is of type DWORD, which means it will always be greater than or equal to 0.
About other bugs in Firefox
I will make a small digression and tell you about the results of checking Firefox in general. The project is qualitative, and PVS-Studio revealed few errors. However, since it is large, there are a lot of errors in quantitative terms. Unfortunately, I was not able to fully examine the report issued by the PVS-Studio tool. The fact is that for Firefox there is no project file for Visual Studio. The project was checked by the console version of PVS-Studio, called from the make-file. By opening a report in Visual Studio, you can view all diagnostic messages. But since there is no project, Visual Studio does not suggest where any variables are declared, does not allow to go to the place where the macros are defined, and so on. As a result, the analysis of an unknown project is extremely time-consuming, and I was able to study only part of the messages.
Mistakes are diverse. For example, there is a way out of the array:
class nsBaseStatis: public nsStatis {
public:
...
PRUint32 mLWordLen [10];
...
nsBaseStatis :: nsBaseStatis (...)
{
...
for (PRUint32 i = 0; i <20; i ++)
mLWordLen [i] = 0;
...
}
...
}; Diagnostics of PVS-Studio: V557 Array overrun is possible. The value of 'i' index could reach 19. detectcharset.cpp 89
Although this and similar errors are interesting, they are not related to the topic of this article. Therefore, if interested, you can look at some other errors in this file: mozilla-test.txt .
Let's return to errors in error handlers.
I decided to cite not a couple, but 10 examples to convince you of the urgency of the problems of defects in error handlers. Of course, error handlers are not the most critical and important parts of the program. But after all, programmers write them, which means they hope to improve the behavior of the program with their help. Unfortunately, as my observations show, very often checks and error handlers do not work. Look, it was enough for me one, albeit a large project, to show a lot of errors of this type.
What to do with it and what recommendations can be given?
First recommendation
It must be admitted that you can make a mistake even in a simple check. This is the most difficult and important. It is precisely because error handlers are considered to be simple code fragments, there are so many typos and other defects. Error handlers are not tested and not tested. They do not write tests.
Of course, writing tests for error handlers is very difficult and often not economically feasible. But if the programmer at least knows about the danger, this is already a lot. Aware, it means armed. With error handlers, you can make this analogy.
According to statistics, climbers often fall at the end of the climb. This happens not because of fatigue, but because the person thinks that he has very little left. He relaxes, loses attentiveness and, as a result, makes mistakes more often. Something similar happens to the programmer when writing code. He spends a lot of effort and attention on the algorithm, and writes various checks without focusing, as he is sure that he cannot make a mistake in them.
So now you are warned. And I am sure that this is already very good and useful.
If you say that such stupid mistakes are made only by students and inexperienced programmers, then you are wrong. Typos are easy to do. I offer this small note on this topic: “ The second myth - professional developers do not make stupid mistakes ”. I can confirm this with many examples from various projects. But I think that it’s quite enough to think about it.
Second recommendation
Dump saving mechanisms, logging functions and other similar auxiliary mechanisms deserve to be made unit tests for them.
The non-working mechanism for saving a dump is not only useless, it only creates the appearance that in case of trouble it can be used. If a user sends a corrupted dump-file, then it will not only not help, but it can only be misleading and more time will be spent searching for errors than if the dump-file were completely absent.
The recommendation looks simple and obvious. But do many of those reading this note have unit tests to test the WriteMyDump class?
Third recommendation
Use static code analyzers. The possibility of finding defects in error handlers is one of the strengths of the static analysis methodology. Static analysis covers all branches of a code, regardless of the frequency of their use in a running application. He can identify errors that manifest themselves extremely rarely.
In other words, with static analysis, the code coverage is 100%. To achieve such code coverage with the help of other types of testing is almost impossible. Code coverage for unit tests and regression testing is usually less than 80%. The remaining 20% is very difficult to test. These 20% include the majority of error handlers and rare situations.
Fourth recommendation
You can try to use the methodology for making faults . The point is that a number of functions from time to time begin to return various error codes, and the program should correctly process them. For example, you can write your own malloc () function, which from time to time will return NULL, even if the memory is still there. This will let you know how the program will behave when the memory really ends. Similarly, you can do with functions such as fopen (), CoCreateInstance (), CreateDC (), and so on.
There are special programs that allow you to automate this process and not to write your own functions, which sometimes lead to failure. Unfortunately, I did not work with such systems, so I find it difficult to tell about them in more detail.
Conclusion
Defects in error handlers are common. Unfortunately, I am not sure that the recommendations given in the article are sufficient to avoid them. But I hope that you are interested in this problem, and you can think of ways to reduce the number of shortcomings in your programs. Also, I and other readers will be grateful if you share your ideas and methodologies that will allow you to avoid the errors of the considered type.
Source: https://habr.com/ru/post/134571/
All Articles