245420 - cert reference leaks in CMMF code

Status: RESOLVED WONTFIX

(NSS :: Libraries, defect, P3)

Description

[Nelson Bolyard (seldom reads bugmail)]

The crmftest program is continuing to reveal bugs in the CRMF/CMMF library
and in the ASN.1 encoder.  

When the CMMF library is used to encode a CMMF response message to a CRMF
cert request, the result leaks numerous cert references.  

patch forthcoming.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v1

Fix leaked cert references in CMMF_DestroyCertRepContent.
Avoid double-frees by NULLing out pointers after they've been freed.
Comment on lack of modularity in code 
   One object's destructor destroys contents of another object :(

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 149889 [details] [diff] [review]
patch v1

Julien, please review.	Thanks.  
I plan to leave this bug open until the entire "CMMF Stuff" portion of the
crmftest program completes without any reference leaks.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 149889 [details] [diff] [review]
patch v1

I'm removing the review request from this patch.  
See bug 245429 for background on this action.

I think the object being destroyed here is completely bogus.
It should be outputting exact copies of the already-encoded certs,
not re-encoding those certs from their decoded components!  

It should contain an array of SECItems containing DER-encoded
Certs,	not an array of CERTCertificate structures.
So the real fix to this bug is probably a complete rewrite of
the code that implements the object from which the message is
constructed.  Such a rewrite would obviate this patch completely.

I will either obsolete this patch, or request review of it, 
when I am closer to a complete solution to the CMMF request
encoding problems.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 149889 [details] [diff] [review]
patch v1

Julien, please review.

The real solution to this bug is to fix bug 245941, but that is much more work
than I want to do right now, since there is no urgent call for that to be
fixed.	In the meantime, this patch, together with the patch to bug 245943 are
sufficient to prevent crashes and leaks in the CMMF code.  So, I want to go
ahead and check in these fixes now, and perhaps fix bug 245941 at a later time.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 149889 [details] [diff] [review]
patch v1

Wan-Teh, please review

Comment 6

[Wan-Teh Chang]

Comment on attachment 149889 [details] [diff] [review]
patch v1

r=wtc.

The current code only works if inCertRepContent->poolp
is not NULL, so the original code's wrapping everything
inside a "inCertRepContent->poolp != NULL" test is correct.

If inCertRepContent->poolp is NULL, we would need to free
some other fields using PORT_Free.  But we never create
inCertRepContent with a NULL inCertRepContent->poolp.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Thanks for the review.  I have checked this patch in.  

/cvsroot/mozilla/security/nss/lib/crmf/respcmn.c,v  <--  respcmn.c
new revision: 1.9; previous revision: 1.8

I am marking this bug fixed.  Bug 245941 will continue to remind us 
of that remaining unfixed issue in this code.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

I am reopening this bug, because I found another cert ref leak in 
the crmf library.  patch forthcoming.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch part 2 - another leak

This patch plugs another leak, very similar to the one plugged in the previous
patch.

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch part 2, v2

Don't fail to free the certs just because the pool pointer is null.

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 153371 [details] [diff] [review]
patch part 2, v2

Julien, please review.	I suggest comparing this patch to the previous one in
this bug, which is now checked in.  It fixes a similar bug in a different place
in the code.

Comment 12

[Julien Pierre]

Comment on attachment 153371 [details] [diff] [review]
patch part 2, v2

Nelson,

The only thing I can tell this patch changes is for the case where
inKeyRecRep->poolp is NULL where the certs don't get freed. Is that the case
you are fixing, ? The rest of the patch looks to me like rewriting with
diferent intermediate variables and indenting, but no behavior change. Am I
missing something ?

My comments on this patch are :

1) following on the example of the first patch, you should NULL the
CERTCertificate* after calling CERT_DestroyCertificate .

2) if inKeyRecRep->poolp is NULL, then I think there is a leak of the
CMMFKeyRecRepContent structure. You probably need to free inKeyRecRep with
PORT_Free in an else statement

Comment 13

[Julien Pierre]

Comment on attachment 153371 [details] [diff] [review]
patch part 2, v2

just marking this denied as part of request cleanup . see previous comments in
the bug.

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

Julien, in answer to your question in comment 12, yes, there is another
change here.  I removed the following test:
       if (!inKeyRecRep->isDecoded) {
So that the actions that were formerly conditional on that flag now
happen whether that flag is set or not.  This fixes a leak.

As for the case where inKeyRecRep->poolp is NULL, IIRC, this is 
intended to deal with a case where this structure appears inside of
another outer structure, which has the pool.  In that case, the pool
pointer in the inner structure is null to prevent the pool from being
freed until the outer structure is destroyed.  The inner structure isn't
leaked, but rather it isn't freed until later.  So please reconsider
your r- of this patch.

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 153371 [details] [diff] [review]
patch part 2, v2

Julien, I am asking you to re-review this patch in light of the comments above,
and the comments in bug 245941.  

This patch is no less correct than the code without the patch.	

All the work on CMMF came to a complete stop because of the r- on this patch.

Comment 16

[Nelson Bolyard (seldom reads bugmail)]

This work is primarily of interest to mozilla, not to servers.
I won't be able to get to it before 3.12.
Bob, Wan-Teh, any interesting in working on this for mozilla before then?

Comment 17

[Nelson Bolyard (seldom reads bugmail)]

Kai, I want to be sure you're aware of this bug.  
How important is this bug to you?
Is FF/TB known to leak when it uses CRMF? 
Is FF/TB known to have shutdown problems after doing cert enrollment with CRMF?

Comment 18

[Kai Engert [:KaiE:]]

> Is FF/TB known to have shutdown problems after doing cert enrollment with CRMF?

I'm not aware of a way to use TB to do cert enrollment using CRMF.

FF does not offer a "switch profile" mechanism, therefore the shutdown cleanup requirements are not as strict as with SeaMonkey.

I just tested a SeaMonkey build that has initial support for doing ECC CRMF key generation. I tested both RSA and ECC key generation. After producing such requests, SeaMonkey is still able to switch to another profile. It seems the cert leaks you have noticed do not stop NSS from shutting down?

Comment 19

[Nelson Bolyard (seldom reads bugmail)]

Alexei, you might be interested in finishing this.

Comment 20

[Nelson Bolyard (seldom reads bugmail)]

Maybe this is dead code.  Nobody wants the fix for this leak.
Mayme this bug should be WONTFIX.  For now, I'll give it to nobody.

Comment 21

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 153371 [details] [diff] [review]
patch part 2, v2

This patch no longer applies to the trunk.  
The function was significantly changed by a checkin for Bug 308887.

Comment 22

[Nelson Bolyard (seldom reads bugmail)]

I'm not going to keep this bug around.  
There's no agreement that this is even a problem.