353584 - possible double free in DestroyCertificate by calling sftk_freeCertDB(handle) twice

Status: RESOLVED INVALID

(NSS :: Libraries, defect)

Description

[Alexei Volkov]

sftk_freeCertDB(handle) called second time will try to free already dealocated memory, given that bug 353572 is fixed.

Comment 1

[Alexei Volkov]

Attachment: set handle to NULL after it got freed.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 239444 [details] [diff] [review]
set handle to NULL after it got freed.

Alexei, you and I both looked at this code today and came to the same
conclusion about it.  You filed this bug about that.

I now believe that this bug is invalid.  The code in DestroyCertificate
is correct, and will remain correct even when Julien's patch for 
bug 353572 is applied.  I now believe that his patch does not cause a
crash in this code path.  Worse, I think this patch actually would 
introduce bugs where none existed.  So, I'm giving it r-.  I'll explain 
more in the next comment to this bug.

Comment 3

[Julien Pierre]

OK, the objects are refcounted, so indeed this is not a bug.
I remember sitting in a marathon review with Bob Relyea to review this code, about 5 - 6 hours. I have an email with all my review feedback to him from 9/7/2005.
This seems like a long time ago somehow ;)
So, Nelson, all this code was already reviewed - not that I remember too much about it, mind you.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

function sftk_freeCertDB doesn't unconditionally do anything destructive
to the "handle" structure passed in to it.  The only act it performs 
unconditionally is to decrement a reference count in the handle.  
It decrements the ref count on the DB, and only if that count goes to zero
does it then close the DB and destroy the objects being held by the handle.

The so-called "double free" case posited for this bug requires that 
sftk_freeCertDB be called twice in DestroyCertificate, AND that the 
cert DB's ref count be equal to 1 on the FIRST call.  That cannot happen.
I will explain why.

When DestroyCertificate is called with a non-NULL cert, that cert already
contains a reference to the cert DB, so if the cert is actually destroyed
by DestroyCertificate, (cert's ref count goes to zero), then the cert DB's 
ref count needs to be decremented to show that this cert no longer holds 
that reference.  That is the purpose of the FIRST call to sftk_freeCertDB 
in DestroyCertificate.

If DestroyCertificate is called with the argument lockdb set to any non-zero
value, then DestroyCertificate acquires its own reference to the cert DB.
That is, it INCREMENTS the DB ref count by one near the beginning of the 
function when lockdb is non-zero.  Then at the end of the function, it 
decrements the ref count by one, releasing its own reference.  That is the 
purpose of the SECOND call to sftk_freeCertDB in DestroyCertificate.

Let's look at two cases of calls to DestroyCertificate, one case with 
lockdb false (zero) and one case with it true (non-zero).

If lockdb is zero then the second call to sftk_freeCertDB in 
DestroyCertificate will not be made, and so there is no chance of a 
"double-free" in that case.

If lockdb is non-zero, then DestroyCertificate will have incremented the
DB ref count prior to getting to the FIRST call to sftk_freeCertDB.  In 
that case, when the FIRST call to sftk_freeCertDB is performed in DestroyCertificate, the cert DB ref count will be a MINIMUM of 2, and 
sftk_freeCertDB will NOT close the cert DB and will not destroy any 
objects in the handle.  And when the first call to sftk_freeCertDB returns, 
the cert DB's ref count will be a minimum of 1.  The second call will 
decrement it yet again, and if it goes to zero, will destroy it.  

So, there is no "double-free" type of bug here.  

You can reopen this if you disagree.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

I think we might want to look at this code to understand how it could mislead 
two developers to independently form the same wrong conclusion about it.  

Some thoughts:

1. misleading name.  The name sftk_freeCertDB suggests that the function
performs an unconditionally destructive act like a call to free().  
The problem was characterized as a "double free", even though the function
isn't primarily about freeing anything allocated from the heap.  
I doubt that we want a long but descriptive name like
sftk_DecrementCertDBRefCountAndDestoyIfZero, but perhaps we need something 
better than "free".  The DB ref count is incremented by a function named nsslowcert_reference().  What verb is the opposite of "reference" or 
"AddReference"?  DeReference isn't it.  DropReference, maybe?

2. Unnatural nesting of Add/Drop reference functions.  DestroyCertificate 
begins with the cert holding a reference, then the function acquires a 
reference of its own.  But the two references are not dropped in opposite 
order of that.  Perhaps they should be.  Perhaps the code could be made more 
obviously correct if it was reordered to release the references in reverse
order of acquisition.  

3. Maybe we should add a comment to DestroyCertificate, between the first
and second calls of sftk_freeCertDB, that says 
/* There's no chance of handle being destroyed twice here.  See bug 353584. */

Comment 6

[Julien Pierre]

Nelson,

This wouldn't be the first function named this way that operates on refcounted objects in NSS . See PK11_ReferenceSlot / PK11_FreeSlot which work the same way as sftk_getCertDB / sftk_freeCertDB . Once you know the slot or the DB objects are refcounted, it makes sense.  Maybe Release is the word we want to use instead of free ?

Comment 7

[Wan-Teh Chang]

I'd just add two comments like these:

 	    nsslowcert_UnlockFreeList();
 	    if (handle) {
+               /* release the reference owned by the just freed 'cert' */
 	        sftk_freeCertDB(handle);
 	    }
		
 	    cert = NULL;
         }
 	if ( lockdb && handle ) {
 	    nsslowcert_UnlockDB(handle);
+           /* release the reference this function acquired above */
 	    sftk_freeCertDB(handle);
 	}
     }

Comment 8

[Robert Relyea]

I have not problem with the name 'release' for this function.

bob