We’ve checked Apache HTTP Server with PVS-Studio and as we expected, we found errors. The errors are few, which we also expected.
Other developers also come across this situation while testing PVS-Studio on their projects. Unfortunately, the first conclusion you would like to draw on seeing just a few errors is that a tool like this is useless to you. I have just invented a good analogy to show why this is not so.
Imagine the following situation. We have a work by Leo Tolstoy, for instance, “War and Peace“. We want to test the mechanism of spelling check integrated into Microsoft Word on this book. We launch analysis and find just a few real errors and a lot of false reports. Besides, Leo Tolstoy was apt to write long sentences. Microsoft Word, being unable to understand the text, will underline a lot of fragments with green supposing that there are punctuation mistakes. We are looking at this all and turning our nose up at it. We don’t like that Word considers many names incorrect. We don’t like the punctuation analysis. We conclude that spelling check in Word is an absolutely useless thing.
Let’s find out what is wrong about this way of studying the tool.
We forget that people have spent a lot of time and effort working on errors in the book. Leo Tolstoy rewrote his drafts and eliminated mistakes. The editor worked turning the text into the book. Something was fixed in the next editions.
Of course, the program will not find all the mistakes in the book. People can do it better. But the program does it much faster and therefore reduces the waste of human effort. The true advantage of this tool lies in its regular use: you have just written a paragraph, noticed an underlined mistake and fixed it instantly. Now, while rereading the text, a person can concentrate on the contents and subtle mistakes instead of fixing trivial misprints.
Remarks on the issue of false reports sound unconvincing too. You only need to add an unknown word to the dictionary once and it will no longer bother the text’s author. For the rest of the obviously false warnings you can click the right mouse button and select “ignore”.
I think it’s unreasonable to argue on the usefulness of text checking mechanisms embedded in such programs as Word, Thunderbird and TortoiseSVN. We use them and don’t worry that they find few mistakes in the book “War and Peace “.
There is a similar situation with static analysis of software source code. It’s meaningless to check a project like Apache HTTP Server and try to estimate a probable profit to be drawn from the analysis. The code of Apache HTTP Server is old from the viewpoint of programming. As a project, Apache HTTP Server has existed for more than 15 years. The code’s quality is confirmed by a huge amount of projects based on it. Of course, a lot of errors were fixed due to the wide use of the code by plenty of programmers and a long development time.
It’s logical to suppose that many of the errors found earlier could have been found much easier if static analysis had been used. The analogy with Word is complete in this case. Regular search of errors is much more useful than a one-time search.
As in the Word editor, false reports in PVS-Studio can be easily eliminated: you just have to click on a false message and hide it (a special comment will be automatically added into the corresponding code fragment).
Let’s make the final comparison by the parameter of error detection speed. The Word editor can underline mistakes at once. The PVS-Studio analyzer does it after compiling the files. This can’t be done earlier because the syntax is too complicated to analyze incomplete code. It’s enough, however. We may consider PVS-Studio as a tool to get more compiler-generated warnings. By the way, PVS-Studio can do it in various Visual Studio versions: 2005, 2008, 2010. So you are welcome to try the incremental analysis.
All in all, I find the analogy with Word absolute. If somebody disagrees, we can continue the discussion in comments or via e-mail (karpov [@] viva64.com).
I have formulated the main idea I wanted to share with you. However, I would disappoint the readers if I didn’t write some examples of errors found in Apache HTTP Server. So, let’s speak a bit on the errors detected.
An example of unnecessary sizeof().
PSECURITY_ATTRIBUTES GetNullACL(void)
{
PSECURITY_ATTRIBUTES sa;
sa = (PSECURITY_ATTRIBUTES) LocalAlloc(LPTR,
sizeof(SECURITY_ATTRIBUTES));
sa->nLength = sizeof(sizeof(SECURITY_ATTRIBUTES));
...
}
PVS-Studio’s diagnostic message is the following: V568 It’s odd that the argument of sizeof() operator is the ‘sizeof (SECURITY_ATTRIBUTES)’ expression. libhttpd util_win32.c 115
The size of the SECURITY_ATTRIBUTES structure is set incorrectly. As a result, correct handling of this structure is impossible.
However, I suspect that most errors I will cite here are located in code that gets control very rarely. That’s why you shouldn’t worry too much. The next sample is similar. The error is in the function responsible for error handling.
apr_size_t ap_regerror(int errcode, const ap_regex_t *preg,
char *errbuf, apr_size_t errbuf_size)
{
...
apr_snprintf(errbuf, sizeof errbuf,
"%s%s%-6d", message, addmessage, (int)preg->re_erroffset);
...
}
PVS-Studio’s diagnostic message is the following: V579 The apr_snprintf function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the second argument. libhttpd util_pcre.c 85
It means that it is the pointer size that is passed into a function instead of the buffer size. As a result, the error message is formulated incorrectly. Actually the message’s length will take 3 or 7 characters.
There are also classic Copy-Paste errors.
static int magic_rsl_to_request(request_rec *r)
{
...
if (state == rsl_subtype || state == rsl_encoding ||
state == rsl_encoding) {
...
}
PVS-Studio’s diagnostic message is the following: V501 There are identical sub-expressions ‘state == rsl_encoding’ to the left and to the right of the ‘||’ operator. mod_mime_magic mod_mime_magic.c 787
The comparison really must look this way:
if (state == rsl_subtype || state == rsl_separator ||
state == rsl_encoding) {
There are errors that will occur only in Windows operating system.
typedef UINT_PTR SOCKET;
static unsigned int __stdcall win9x_accept(void * dummy)
{
SOCKET csd;
...
do {
clen = sizeof(sa_client);
csd = accept(nsd, (struct sockaddr *) &sa_client, &clen);
} while (csd < 0 && APR_STATUS_IS_EINTR(apr_get_netos_error()));
...
}
PVS-Studio’s diagnostic message is the following: V547 Expression ‘csd < 0’ is always false. Unsigned type value is never < 0. libhttpd child.c 404
The accept function in the Visual Studio header files returns a value that has the unsigned SOCKET type. That’s why the check ‘csd < 0’ is invalid since its result is always false. The returned values must be explicitly compared to different constants, for instance, SOCKET_ERROR.
There is a sort of errors that can be referred to potential vulnerabilities. I don’t know if it is possible, at least in theory, to use them for attack. Still, I want to give a couple of such samples here.
typedef size_t apr_size_t;
APU_DECLARE(apr_status_t) apr_memcache_getp(...)
{
...
apr_size_t len = 0;
...
len = atoi(length);
...
if (len < 0) {
...
}
else {
...
}
}
PVS-Studio’s diagnostic message is the following: V547 Expression ‘len < 0’ is always false. Unsigned type value is never < 0. aprutil apr_memcache.c 814
The “len < 0” condition in this code is always false since the ‘len’ variable gas an unsigned type. Correspondingly, if we send a string with a negative number to input, a failure is likely to occur.
Apache also has much code where various buffers with data are being cleared. Most likely, it is determined by the safety purposes. But clearing itself often fails because of the following error:
#define MEMSET_BZERO(p,l) memset((p), 0, (l))
void apr__SHA256_Final(sha2_byte digest[], SHA256_CTX* context) {
...
MEMSET_BZERO(context, sizeof(context));
...
}
PVS-Studio’s diagnostic message is the following: V512 A call of the ‘memset’ function will lead to underflow of the buffer ‘(context)’. apr sha2.c 560
Only the first bytes in the buffer are being cleared because the sizeof() operator returns the pointer size instead of the size of the data buffer. I’ve found at least 6 such errors.
The remaining errors are boring, so I won’t write about them. Thank you for your attention. Come and try PVS-Studio. We still give keys to developers of free open-source projects and those who feel a strong urge to try the full analyzer. Write to us.
