340218 - Coverity 910, memory leaks in CRMF code

Status: VERIFIED FIXED

(NSS :: Libraries, defect, P2)

Description

[Nelson Bolyard (seldom reads bugmail)]

There is at least one leak here, maybe more.
It is possible that this code has already been changed/fixed in the CVS
respository, and coverity is still scanning the older unfixed version.
Alexei, since you've recently done a lot of work on this code, perhaps
you know best about that.

In crmf_copy_cert_request(), if poolp is NULL, and  newReq is succesfully 
allocated, but then the call to SECITEM_CopyItem fails (returns 
rv != SECSuccess), then we go do loser.  At loser we call  
   CRMF_DestroyCertRequest(newReq);
which (surprisingly!) does NOT free newReq!  
So, the result is that crmf_copy_cert_request leaks newReq.  

It doesn't seem right to me that CRMF_DestroyCertRequest returns without
freeing newReq (in the no-pool case).  
I wonder how many other pieces of CRMF code are leaking because they assume 
(wrongly) that CRMF_DestroyCertRequest actually frees its argument. 
I'm inlined to think that the fix is for CRMF_DestroyCertRequest to free
the argument, rather than changing its callers to do so.  But deciding if
that's the right change to make, or not, involves examining every caller of 
CRMF_DestroyCertRequest.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

CID 575 & 910

Comment 2

[Julien Pierre]

Retargetting all P2s to 3.11.3 .

Comment 3

[Alexei Volkov]

I didn't find any leaks associated with potential leak in crmf_copy_cert_request(). We always allocating requests on arenas. The question that matters for CRMF_DestroyCertRequest is this arena solely belongs to a request, or some other data was allocated on it. Initiated with non-NULL value pool pointer of a request is the flag that shows that the request solely owns an arena.

Now, most of the CertRequests created with CreateCertRequest which, unlike crmf_copy_cert_request, creates CertRequests on arena. No problem to destruct such requests, since we only need to destruct request's arena. 
crmf_copy_cert_request() function of the other hand, uses caller arena to allocate a request. It is up to  crmf_copy_cert_request caller discretion to assign an arena value to a request pool.

We will have a problem if a fix will be made to CRMF_DestroyCertRequest to free request structure depending on pool value, since we will be freeing a memory that is a block in parent arena.

I think the best place to fix leak in this situation is  in crmf_copy_cert_request() after the call of CRMF_DestroyCertRequest(newReq) function.

Comment 4

[Alexei Volkov]

Attachment: proposed fix (checked in)

Proposed fix base on findings explained in comment #3 to bug 340218#c3

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

The current situation with CRMFCertRequest structures is a total mess.
Sometimes they're "stand alone" with their own arena.  
Sometimes they're part of the arena of another object.
Sometimes they may not have any arena at all.
Code cannot programattically tell, by examination of the contents,
which of those cases applies to any one CRMFCertRequest structure.

The patch proposed above fixes the immediate leak, but leaves the major 
problem in place.  I really want to see this all cleaned up.  

Here is a straw-man proposal:
- Change the code so that an arena is ALWAYS required.  
  CRMFCertRequest structures CANNOT exist without an arenapool.
- Ensure there is an arenapool pointer in the structure
- Add a flag to the structure that tells whether this pool belongs 
  exclusively to this structure, or belongs to another (parent) object.
- Have the CRMFCertRequest destroy function destroy the pool unless it
  belongs to antoher object.  

The issue I see with that proposal is that it changes public structures.
AFAIK, PSM is the only code in the world that uses it.  (This code is not
even in any of our shared libs, is it?)  So I don't think backwards binary
compatibility is really an issue.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 226875 [details] [diff] [review]
proposed fix (checked in)

r=nelson
I think this function still has lots of problems, but this will be one less.

Comment 7

[Alexei Volkov]

Integrating attachment 226875 [details] [diff] [review]:
/cvsroot/mozilla/security/nss/lib/crmf/crmfget.c,v  <--  crmfget.c
new revision: 1.3; previous revision: 1.2

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Unsetting target milestone in unresolved bugs whose targets have passed.

Comment 9

[timeless]

misterssl: can we split comment 5 into another bug?

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

timeless: yes, sure, go ahead.  
However, since the crmf/cmmf code is not in any shared lib, 
I think it will be prioritized very low, about p7.

Comment 11

[timeless]

anything to get a bug closed :)

comment 5 filed as bug 550834