45247 - warning shows bug (uninitialized variable) in genname.c

Status: VERIFIED FIXED

(NSS :: Libraries, defect, P3)

Description

[Kathleen :Brade]

Scanning the list of warning on the Macintosh optimized build on tinderbox, I 
noticed this problem:
Variable 'nick' is not initialized before being used 
I'm not really sure what line this is coming from (without doing more probing).  
Please let me know if you need to know more precisely or can't reproduce.

My guess is:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/certdb/
genname.c&mark=1324#1313

Comment 1

[Wan-Teh Chang]

Reassigned to myself.

Comment 2

[Wan-Teh Chang]

In lib/certdb/genname.c, function CERT_GetNickName, 'nick'
is passed to SEC_ASN1DecodeItem without being initialized.

I don't know how to fix this.  Nelson, could you take a
look at this?

Comment 3

[Robert Relyea]

nick should not be declared as a pointer. nick-> references should change to 
nick. references. the function should take &nick in. This must always certainly 
cause problems after importing a CA!

bob

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

So, this code is obviously broken and doesn't work properly
on any platform.  This begs the question: is this code actually
used anywhere??

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

There were MANY bugs in the function CERT_GetNickName().
The loop that looks for duplicate nicknames in the DB leaked
the buffers containing the previous values of "nickname",
AND failed to decrement the reference count on the certificates
that it found. (I call this "leaking reference counts").

My proposed changes to that function are so extensive that,
instead of attaching a "patch", I'm going to attach my new
version of this function for y'all to review.  

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Proposed new implementation of CERT_GetNickName

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Another bug in this code was that after testing the variable 
"nicknameArena" for NULL, it then used cert->arena instead of
nicknameArena in the PORT_ArenaStrdup() call.  The new code
checks both variables for NULL before relying on cert->arena.
I believe the new code functions the same as the old, except
for the leaks and the potential null pointer dereference.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Fixed by rev 1.2 to nss/lib/certdb/genname.c
Thanks to Bob Relyea and Terry Hayes for reviewing this.

Comment 9

[Kathleen :Brade]

verifying that this is fixed on current branch/tag