417641 - miscellaneous minor NSS bugs to be fixed in NSS 3.12.0

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P3)

Description

[Nelson Bolyard (seldom reads bugmail)]

Attachment: remove #ifdef DEBUG from sslimpl.h (checked in)

I'm going to attach several small patches for various bugs to this bug.

#1) I have been burned MANY times by building libSSL code for DEBUG, only to 
find that it doesn't build for optimized because it's missing header files.
The problem is caused because sslimpl.h #includes many NSPR header files 
when DEBUG is defined that it does NOT include when DEBUG is NOT defined.
So, I propose to just remove the #ifdef DEBUG, so that those #includes 
will happen on DEBUG and non-DEBUG builds, alike.  Then I won't get burned
by missing #includes in optimized builds any more.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Attachment: return Success, not failure, when it succeeds

This code path does all the steps correctly without error, and then returns
SECFailure.  I guess it's a dead code path, but it's ridiculous.

Comment 2

[Julien Pierre]

Comment on attachment 303426 [details] [diff] [review]
return Success, not failure, when it succeeds

I don't think this is OK because numFuncs doesn't get incremented.

Comment 3

[Wan-Teh Chang]

Comment on attachment 303425 [details] [diff] [review]
remove #ifdef DEBUG  from sslimpl.h (checked in)

r=wtc.

Comment 4

[Julien Pierre]

Actually numFuncs doesn't need to be incremented. It's a dead and invalid code path because :

 nss_GetShutdownEntry(NULL, NULL);

should never return i>0 .

That's because it's not possible to add a NULL function into the array.
I would suggest changing the code to :

PORT_Assert(0 == nss_GetShutdownEntry(NULL, NULL))

and removing the if statement immediately after.

Comment 5

[Wan-Teh Chang]

Comment on attachment 303426 [details] [diff] [review]
return Success, not failure, when it succeeds

I think this patch is correct.  When we unregister a shutdown handler,
we leave an empty slot in the array.  The nss_GetShutdownEntry(NULL, NULL)
call finds an empty slot.

But all the i > 0 tests should be changed to i >= 0 because 0 is a legal
slot index.

Comment 6

[Julien Pierre]

Wan-Teh,

I'm still not sure about this. What's numFuncs supposed to hold ? I thought it would be the actual number of entries in use. But I see that it's never decremented upon unregister - actually never decremented at all.

The code could use some comments in the list definition to state what numFuncs and maxFuncs mean. maxFuncs seems to be the number of entries allocated.

It would be a lot clearer if numFuncs was decremented at unregister time, and the removed entry swapped with the last entry in the array. This was there would never be a hole in the array, and numFuncs would hold the actual number of functions in the list.

Comment 7

[Julien Pierre]

Comment on attachment 303426 [details] [diff] [review]
return Success, not failure, when it succeeds

Wan-Teh is right, this is  a correct patch. He is lso correct that  all the i>0 tests from nss_GetShutdownEntry are wrong and need to be changed to >= 0.

I would request also that a comment be added to 
NSSShutdownListStr
to explain what maxFuncs and numFuncs are.

It seems numFuncs is the peak number of entries in use. Maybe peakFuncs would be more appropriate name than numFuncs.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 303426 [details] [diff] [review]
return Success, not failure, when it succeeds

Checking in nssinit.c; new revision: 1.88; previous revision: 1.87

My checkin for bug 397486 added yet another function to the list of 
registered shutdown functions.  It passed my tests on Windows, but
it has turned tinderbox orange on at least some systems.  
I really have no idea why it is failing on those systems.
I have committed this fix, in hopes that it will help, but I really 
don't expect it to help.  I'm going to dinner.  If the tree is still
orange when I return, I will back out all the changes for that bug.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

I made a patch that fixes the i > 0 problem, and changes the variable
names as Julien suggested, and committed it, in hopes that it will fix
the problems.  I think it will.   I think the i > 0 problem is the cause.
But as I said in comment 8, if the tree doesn't go green, I'll back out 
all the relevant changes I made today.  Off to dinner now.

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch to nssinit.c, to return success, not failure - as checked in

This is the patch I committed

Comment 11

[Wan-Teh Chang]

Attachment: Eliminates a compiler warning on SSL_AtomicIncrementLong

gcc 4.2.1 warns:
ssl3con.c: In function 'SSL_AtomicIncrementLong':
ssl3con.c:538: warning: value computed is not used

This patch eliminates that warning.  The use of if statement
also makes the code easier to understand by lesser programmers.

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 305116 [details] [diff] [review]
Eliminates a compiler warning on SSL_AtomicIncrementLong

ok

Comment 13

[Wan-Teh Chang]

Comment on attachment 305116 [details] [diff] [review]
Eliminates a compiler warning on SSL_AtomicIncrementLong

I checked in the SSL_AtomicIncrementLong patch on the NSS trunk
for NSS 3.12.

Checking in ssl3con.c;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3con.c,v  <--  ssl3con.c
new revision: 1.109; previous revision: 1.108
done

Comment 14

[Wan-Teh Chang]

Attachment: Fix compiler warnings in lib/ssl/derive.c

'extractable' is an unused variable.

'suite' is a PRUint16, so compare it with an integer (0) rather
than a pointer (NULL).

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 305655 [details] [diff] [review]
Fix compiler warnings in lib/ssl/derive.c

It may be worthwhile to add a small comment explaining that zero is used 
as a list terminator because cipher suite zero is SSL_NULL_WITH_NULL_NULL 
and the SSL3 & TLS specs forbid negotiating that cipher suite number.

Comment 16

[Wan-Teh Chang]

Attachment: Fix compiler warnings in lib/ssl/derive.c (as checked in)

Re: Nelson's comment 15: I don't understand why the 'ciphersuites'
list needs a terminator.  The 'nsuites' argument tells us the length
of the list.  Also, I could interpret that I should skip 0 and keep
going rather than stopping at 0, like this:

     /* determine which KEAs to test */
-    for (i=0; i < nsuites && (suite = *ciphersuites++) != NULL; i++) {
+    for (i=0; i < nsuites; i++) {
+        if ((suite = *ciphersuites++) == 0)
+            continue;  /* skip and keep going */

I checked in the patch with the comment Nelson suggested
on the NSS trunk for NSS 3.12.

Checking in derive.c;
/cvsroot/mozilla/security/nss/lib/ssl/derive.c,v  <--  derive.c
new revision: 1.9; previous revision: 1.8
done