208038 - crashes in genname.c after memory allocation failures

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Nelson Bolyard (seldom reads bugmail)]

This bug is a continuation of bug 204555, which has been marked fixed. 

Even after the patches for bug 204555 are applied, there remain numerous 
places in nss/lib/certdb/genname.c that allocate memory, or call other 
functions that allocate memory, and do not check that the allocation 
succeeded before attempting to use the resultant pointers.  

I will attach a patch shortly.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Attachment: eliminate null ptr derefs and other cleanup

This patch does the following:
1. Adds numerous tests for null pointers, or for SECFailure returned by
functions that do allocations, and jumps to loser to takes other appropriate
action.
2. Uses the PORT_ZNew and PORT_ArenaZNew macros rather than deplicating their
expansions.
3. Reduces code duplicated in many cases in large switch statements.
4. Adds comments suggesting the use of arena marks and releases in various 
functions.
5. Add comments and reduce warnings in a function that returns pointers of four
different types as void *s.
6. Uses #if 0 to remove several functions that are not exported from NSS shared
libs and are not used within NSS, and which contain numerous places that
potentially use NULL pointers.	Alternatively, these functions could simply be
replaced with a comment explaining why they were eliminated.

This patch depends on the patches of bug 204555 having already been applied.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Marking P1 for 3.9, but I'd like this to go into Moz 1.4 if possible.

Comment 3

[Wan-Teh Chang]

Comment on attachment 124784 [details] [diff] [review]
eliminate null ptr derefs and other cleanup

This patch is larger than necessary.  Some of the cleanups
are a matter of style and are not necessary.  For example:

1. Removing the curly braces around an "if" body containing
a single statement.  Some people use the curly braces to
prevent potential mistakes if the code is modified in the
future (see bug 203379 comment 18 for a recent example of
this mistake).

2. Removing the explicit cast of void * (the return value of
allocation functions) to XXX *.  This cast is not necessary
in C but is required in C++.  Some people use it in C so that
the code can be compiled by a C++ compiler.

Other comments:

You added some comments "mark arena", "unmark arena", and
"release arena back to mark" that do not match the code.
These comments should be either removed or marked with
something like "XXX", "TODO", or "FIXME".

There are some formatting or indentation changes that may
be worthwhile for the tip but obscure the "meat" of this
patch.	You may want to prepare a smaller version of this
patch without those changes for the NSS 3.8 branch and
Mozilla 1.4.

You added a few PORT_Assert(0) statements.  I didn't look at
them closely.  I just wanted to make sure that those
assertions detect internal NSS errors (i.e., bugs), not
invalid input.	Invalid input should not result in an assertion
failure unless the function has no way to return a failure
status.

Comment 4

[Wan-Teh Chang]

In the previous comment, I wrote:
> 2. Removing the explicit cast of void * (the return value of
> allocation functions) to XXX *.  This cast is not necessary
> in C but is required in C++.  Some people use it in C so that
> the code can be compiled by a C++ compiler.

Sorry, these changes are correct.  I didn't see the accompanying
changes to use the PORT_ZNew macro.  My mistake.

These changes belong to the category that is worthwhile for the
tip but obscures the meat of this patch. Same suggestion.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Attachment: v2, bare minimum patch, detect allocation failures

This patch is the bare minimum component of the previous patch.  
This patch is suitable for the branches.  
The rest of the large cleanup in the first patch will go to the trunk.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Fixed in NSS 3.9