Apache Code Inspection Report (fwd)

[Marc Slemko <ma...@znep.com>]

(re. the poor quality cnet story that is just advertising, and the
corresponding slashdot post)

Yea... umh... some of their analysis is pretty poor.  I didn't forward
this earlier as I assumed someone more involved in httpd devlopment right
now would.

But yes, there are some issues they point out that should be fixed if they
are still present.  Fixing them certainly won't significantly change the
overall software quality though.

The attachments are at:

http://icarus.apache.org/~marc/reasoning/

---------- Forwarded message ----------
Date: Wed, 25 Jun 2003 10:17:56 -0700
From: Jeff Klagenberg <je...@reasoning.com>
To: lars@apache.org, thom@planetarytramp.net, marcs@znep.com,
     striker@apache.org, trawick@apache.org
Subject: Apache Code Inspection Report

Hello,

Recently Reasoning ran our inspection service on of the Apache httpd 2.1 for Linux code base (a version checked out on 1/31/03).  We did find a number of defects that should be brought to the team's attention, the reports generated are attached.  The total number of defects discovered was 31, the majority being Null Pointer Dereferences (NPDs) and a couple Unitialized Variable.  Some of these NPDs occurred during an out of memory condition, most of our clients choose to have these identified to provide a more graceful / informative exit.

The attachments are:
*	metrics-linux-apache-public.pdf    A report showing the distribution of defects within the application.  Customer's have found that files / classes with a higher density of defects our service detects have a higher risk of other defects as well.

*	defect-linux-apache-public.pdf     A report detailing each defect found in the source code, including the conditions under which the defect becomes an issue.

We do hope you find the reports as useful as our clients do.  I am curious to receive any feedback on how useful you feel these are.  Please contact me if you have any questions or comments.

Best Regards,
Jeff


Jeff Klagenberg
Director, Product Management
Reasoning Incorporated
+1 650 316 4374 office
+1 650 316 4430 fax
+1 650 430 3677 mobile
http://www.reasoning.com



 <<metrics-linux-apache-public.pdf>>  <<defect-linux-apache-public.pdf>>

Re: Apache Code Inspection Report (fwd)

[Dirk-Willem van Gulik <di...@webweaving.org>]

On Tue, 1 Jul 2003, Cliff Woolley wrote:

> don't use it.  We intentionally do not check for OOM conditions because,
> though we've had many heated debates about this, we've always arrived at
> the consensus that if you hit OOM, your box is hosed anyway and virtually
> any effort you make to correct it other than dying will just make things

Never tried this with 2.0; but with 1.3 I've regularly used
loging.conf/ulimits to manage footprint; and relied on getting a child
emit an early "Ouch!  Out of memory!" followed by its exit(1) bail to amke
sure things did not spin out of control and hose the box.

Dw

Re: Apache Code Inspection Report (fwd)

[Cliff Woolley <jw...@virginia.edu>]

On Tue, 1 Jul 2003, [ISO-8859-1] Andr� Malo wrote:

> ID 2 ... hmm, am I blind or don't we have an abort_fn registered in our
> pools? Why? That would solve a lot of problems, IMHO. If there's an
> abort_fn registered, all IDs referencing to this one are invalid.

There is such a feature in the pools code, but as far as I'm aware we
don't use it.  We intentionally do not check for OOM conditions because,
though we've had many heated debates about this, we've always arrived at
the consensus that if you hit OOM, your box is hosed anyway and virtually
any effort you make to correct it other than dying will just make things
worse.  Not to mention the fact that several platforms (including linux
iirc) have optimistic memory allocation, so even if you /are/ out of
memory, you probably won't know it, so the checks would be useless anyhow.

--Cliff

Re: Apache Code Inspection Report (fwd)

[André Malo <nd...@perlig.de>]

* Marc Slemko wrote:

> But yes, there are some issues they point out that should be fixed if they
> are still present.  Fixing them certainly won't significantly change the
> overall software quality though.

Well, let's have a look. My first analysis:

ID 1 seems to be invalid, the preconditions code path cannot happen.
(second and last precondition are in opposite of each other, because
current_provider == conf->providers in this case)

ID 2 ... hmm, am I blind or don't we have an abort_fn registered in our
pools? Why? That would solve a lot of problems, IMHO. If there's an
abort_fn registered, all IDs referencing to this one are invalid.

ID 3 is valid. We should bail out and return -1 if tag == NULL, I think.

ID 4: Not sure. What shall happen, if a bucket cannot be created?

ID 5: see ID 2

ID 6, 7, 8: see ID 4

ID 9: seems to be invalid. strchr(segstart, '\0') cannot return NULL on an
already valid C-string.

ID 10:  see ID 2

ID 11: see ID 4

ID 12: hmm, I don't seem to understand the critical loop... Could someone
explain it (why do we start with i=1?). An if(args) wrapper should solve it
anyway.

ID 13: we should leave it as is, because it shows us errors in the caller
functions. (would call it "by design").

ID 14: see ID 13

ID 15, 16, 17, 18: see ID 4

ID 19: ap_strchr_c(valid_string, '\0') cannot be NULL. see ID 9

ID 20: seems to be invalid?

ID 21: don't know, I'm not an APR guru ;-)

ID 22, 23: hmm. by design, IMHO, see ID 2

ID 24: see ID 4

ID 25: very valid!

ID 26: seems to be valid, needs an if (pTail) wrapper.

ID 27: see ID 22

ID 28: haha! Already fixed two days ago or so.

ID 29: should be fixed.

ID 30: errr, cannot happen, but we should bail out (assert?)

ID 31: correct, though I don't think we should double check n.

More opinions and reviews would be helpful, of course.

nd