Software Code Quality Of Apache Analyzed

fruey writes "Following Reasoning's February analysis of the Linux TCP/IP stack (putting it ahead of many commercial implementations for it's low error density), they recently pitted Apache 2.1 source code against commercial web server offerings, although they don't say which. Apparently, Apache is close, but no cigar..."

Comments

Open Source versus Closed

[ElectronOfAtom]

The difference is that now that someone has found 31 errors in the open source Apache software, they will be fixed fairly quickly whereas closed source software will have to have the company do a cost-benefit analysis, put together a team to do the fixes, probably charge to put out patches or minor upgrades (assuming the product is Microsoft's IIS ;b)...

Apache 2.1 does not yet exist

[David McBride]

Umm, Apache 2.1 hasn't been released yet. Current latest stable is 2.0.46.

I can only assume that they're looking through the current DEVELOPMENT codebase -- finding a higher ``defect density'' in such a development codebase compared with commercial offerings is not exactly unexpected.

They're also some automated code inspection product; the press release doesn't go into details as to the severity of the defects found or the testing methodology.

It'll be necessary to read through the full report before drawing any sound conclusions.

Re:Apache 2.1 does not yet exist

[David McBride]

The above link wants your email address. Bah.

The direct URLs for the reports are:
Defect Report
Metric Report

Re:Code defects appear to be a small part of the e

[phre4k]

Prette lame when we are talking server software where apache has the lead. (apache 63% vs IIS 25% netcraft.com)

/Esben

Re:No cigar, my ass.

[HowlinMad]

FYI

5100 != 58,944

58,944 is the number from the article.

FACT: Reading is Good

[Cancel]

That's not what they're saying at all. In fact, Reasoning concluded that there was no statistically significant difference in 'defect density' between Apache and the unnamed commercial product.

"In our February study that compared the defect density of the Linux TCP/IP stack to the average defect density of commercially developed TCP/IP stacks, we concluded that Open Source had a significantly lower defect density compared to commercial equivalents," said Bill Payne, President & CEO of Reasoning. "We received numerous inquiries about that study and took seriously requests for us to examine defect density rates in a less mature Open Source application and compare it with the commercial equivalent. Taking advantage of our database of automated software code inspection projects, we were able to do exactly that, and found the difference in defect density between the two was not significant." (emphasis mine)

Re:Defect?

[richie2000]

From the report:
NULL Pointer Dereference (Expression dereferences a NULL pointer) 29 instances
Uninitialized Variable (Variable is not initialized prior to use) 2 instances

They also list the files and code snippets where the errors were found.

In addition, the comparison is made against an industry average of commercial code they have tested this way, NOT against other webservers.

Defect Details

[Eustace Tilley]

Interested persons can download the full defect report free of charge.

Some things I found interesting:

1. Apache 2.1 (dev) is a mere 76,208 LOC.
2. No memory leaks detected
3. 29 NULL pointer dereferences
4. 2 Uninitialized variables
5. No bounds errors, no bad deallocs
6. otherchild.c had a rate of 7 NULL pointer dereferences per 1000 KSLC
7. 
   
   One of the explanations (given by Reasoning) for a NULL pointer dereference is "can occur in low memory conditions," which I think means the original allocator did not check for malloc failure.
   
   So you can get a sense of what a defect looks like, here is #21. The orignal uses bold and fonts improve readability, but I don't know how to reproduce that in slashcode:
   DEFECT CLASS: Null Pointer Dereference
   
   DEFECT ID 21
   
   LOCATION: httpd-2.1/srclib/apr/misc/unix/otherchild.c : 137
   
   DESCRIPTION The local pointer variable cur, declared on line 126, and assigned on line 128, may
   be NULL where it is dereferenced on line 137.
   PRECONDITIONS The conditional expression (cur) on line 129 evaluates to false.

   CODE FRAGMENT
   124 APR_DECLARE(void) apr_proc_other_child_unregister(void *data)
   125 {
   126 apr_other_child_rec_t *cur;
   127
   128 cur = other_children;
   129 while (cur) {
   130 if (cur->data == data) {
   131 break;
   132 }
   133 cur = cur->next;
   134 }
   135
   136 /* segfault if this function called with invalid parm */
   137 apr_pool_cleanup_kill(cur->p, cur->data, other_child_cleanup);
   138 other_child_cleanup(data);
   139 }

sorry, but thats pure BS...

[BigBadDude]

One of the explanations (given by Reasoning) for a NULL pointer dereference is "can occur in low memory conditions," which I think means the original allocator did not check for malloc failure.


appache got its own malloc() that kills the child (and closes connection) if it fails to allocate enough bytes.

Here are the links to the defect reports

[arrogance]

Defect Report

Metric Report

They make you fill out a form that asks for your email and then do an opt out checkbox at the bottom of the form (you have to check it to NOT get spam from them). The site's a bit slashdotted right now though.

Re:So if they found them...

[Jeremy Erwin]

If you download the defect report (available from here*, it will explain exactly where the bugs are.
For instance, the first bug is

DEFECT CLASS: Null Pointer Dereference DEFECT ID 1
LOCATION: httpd-2.1/modules/aaa/mod_auth_basic.c :291
DESCRIPTION The local pointer variable current_provider, declared on line 235, and assigned on line 257, may be NULL where it is dereferenced on line 291.
PRECONDITIONS The conditional expression (res) on line 253 evaluates to false AND
The conditional expression (!current_provider) on line 264 evaluates to true AND
The conditional expression (!provider || !provider->check_password) on line 268
evaluates to false AND
The conditional expression (auth_result != AUTH_USER_NOT_FOUND) on line
282 evaluates to false AND
The conditional expression (!conf->providers) on line 287 evaluates to false.

Each bug report is followed by the snippet of source code containing the defect.

The metric report simply reports the statistics. For instance, the most bug ridden file is otherchild.c. The most common bug class is "dereferencing a NULL pointer".

If the Apache developers simply want to fix the bugs, they can use the Defect Report. If they want conduct a brutal purge of their contributors, they can use the Metric report.

*Yes, Reasoning wants an email address. They will mail you a URL (a rather simple one at that) to access the reports.

Re:So if they found them...

[Skjellifetti]

None of that bug report is at all useful if there is no logical way for all of those preconditions they listed to actually be met.

Well, Yes and No. The problem is that there may be no logical way that the pointer may be NULL today. But tomorrow, a new coder will add something that modifies the preconditions and suddenly that pointer can indeed be NULL. Even where you are sure that a condition is impossible, it is usually a good idea to check for NULL in order to avoid future errors.

And for those who haven't seen this trick before, a nice habit to get into is to write your checks like so:

if (NULL == myPointer) { ... }

This lets the compiler catch errors where you meant '==' rather than just '='. As in

/* Do we really mean this? */
if (myPointer = NULL) { ... }

BINGO

[Anonymous Coward]

In almost every case they listed the pathway was via a failed malloc.

Apache has it's own malloc that kills the connection (and the child) if it fails.

That code can never be reached. Their test is invalid.

Re:Code defects appear to be a small part of the e

[johnnyb]

Actually, I've found that fixing bugs in large projects is about the same whether or not you are familiar with the project, provided that the author was no smoking crack at the time he wrote it.

For example, I managed to code, test, and patch a "fix" for PostgreSQL this weekend in under 2 hours, having never seen the code before.

The "fix" wasn't a bug, per se, i't just that the output of pg_dump wasn't optimal in my usage for dumping the schema for CVS revision control. I added two flags, -m -M, which molded the output to my liking.

If you haven't seen your code in two months, you and an outsider have about the same chance at finding and detecting bugs/misfeatures.

Re:So if they found them...

[conway]

Turning on all warnings in gcc (-Wall) catches this, and many other common errors.
(In effect it does a lint-like check on the source.)

Re:Microsoft C++ catches this. Doesn't gcc?

[Rasta Prefect]

This lets the compiler catch errors where you meant '==' rather than just '='.
MY compiler (Microsoft C++) does catch this

if (myPointer = NULL) { ... }
and issues a warning. Doesn't gcc?

Yes, it does. So does every other C compiler I've ever used (quite a few). I suspect the original poster may be the sort who ignores warnings....

The first "defect" is provably not a defect at all

[Anonymous Coward]

Looking at their first "bug", a little manual inspection shows that it's in the "can't happen" category, even without knowing about hidden information. The code looks like this:

current_provider = conf->providers;
do {
{some safe code}
if (!conf->providers) {
break;
}
current_provider = current_provider->next;
} while (current_provider);

and they identify the second-to-last line as the "possible NULL pointer reference". Note that the "break" before that line will be taken if the pointer is NULL, so it can't happen. In fact, the static analysis could have determined this if it were a little better at propagating values.

First conclusion: subtract at least one "bug" from the 31 defects in Apache. This lowers the rate to 0.51, the same as the "average commercial code" number they quote. Yahoo!

Second conclusion: their static analysis must identify a lot of false positives, if the very first one in the list is one (I would look at more, but I should really get back to work...)