miscellaneous minor NSS bugs to be fixed in NSS 3.12.0

RESOLVED FIXED in 3.12

Status

NSS
Libraries
P3
normal
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: Nelson Bolyard (seldom reads bugmail), Assigned: Nelson Bolyard (seldom reads bugmail))

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 2 obsolete attachments)

Created attachment 303425 [details] [diff] [review]
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.
Attachment #303425 - Flags: review?(wtc)
Created attachment 303426 [details] [diff] [review]
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.
Attachment #303426 - Flags: review?(julien.pierre.boogz)
(Assignee)

Updated

9 years ago
Priority: -- → P3

Comment 2

9 years ago
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.
Attachment #303426 - Flags: review?(julien.pierre.boogz) → review-

Comment 3

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

r=wtc.
Attachment #303425 - Flags: review?(wtc) → review+

Comment 4

9 years ago
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

9 years ago
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

9 years ago
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

9 years ago
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.
Attachment #303426 - Flags: review- → review+
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.
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.
Status: NEW → ASSIGNED
(Assignee)

Updated

9 years ago
Attachment #303425 - Attachment description: remove #ifdef DEBUG from sslimpl.h → remove #ifdef DEBUG from sslimpl.h (checked in)
Created attachment 303559 [details] [diff] [review]
patch to nssinit.c, to return success, not failure - as checked in

This is the patch I committed
Attachment #303426 - Attachment is obsolete: true

Comment 11

9 years ago
Created attachment 305116 [details] [diff] [review]
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.
Attachment #305116 - Flags: review?(nelson)
Comment on attachment 305116 [details] [diff] [review]
Eliminates a compiler warning on SSL_AtomicIncrementLong

ok
Attachment #305116 - Flags: review?(nelson) → review+

Comment 13

9 years ago
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

9 years ago
Created attachment 305655 [details] [diff] [review]
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).
Attachment #305655 - Flags: review?(nelson)
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.
Attachment #305655 - Flags: review?(nelson) → review+

Comment 16

9 years ago
Created attachment 308340 [details] [diff] [review]
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
Attachment #305655 - Attachment is obsolete: true
(Assignee)

Updated

9 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
(Assignee)

Updated

9 years ago
Summary: miscellaneous minor NSS bugs → miscellaneous minor NSS bugs to be fixed in NSS 3.12.0
You need to log in before you can comment on or make changes to this bug.