Last Comment Bug 418546 - reference leak in CERT_PKIXVerifyCert
: reference leak in CERT_PKIXVerifyCert
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: trunk
: All All
: P1 normal (vote)
: 3.12
Assigned To: Robert Relyea
Depends on:
  Show dependency treegraph
Reported: 2008-02-19 18:36 PST by Julien Pierre
Modified: 2008-03-17 19:12 PDT (History)
2 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---

Fix memory leaks in pkix glue code. (6.59 KB, patch)
2008-03-06 10:15 PST, Robert Relyea
alvolkov.bgs: review-
Details | Diff | Splinter Review
Updated patch taking account the review comments (3.82 KB, patch)
2008-03-07 15:05 PST, Robert Relyea
alvolkov.bgs: review+
nelson: superreview-
Details | Diff | Splinter Review
Use correct failure test (3.86 KB, patch)
2008-03-14 09:35 PDT, Robert Relyea
no flags Details | Diff | Splinter Review
Patch for checkin... (3.87 KB, patch)
2008-03-14 09:36 PDT, Robert Relyea
nelson: review+
Details | Diff | Splinter Review

Description Julien Pierre 2008-02-19 18:36:52 PST
I get a leak on every successful cert verification when using vfychain with PKIX :

[jp96085@monstre]/net/monstre/export/home/julien/nss/tip/mozilla/dist/SunOS5.10_i86pc_DBG.OBJ/bin 154 % ./vfychain -p -d . -u 4 ee.der l1ca.der
Chain is good!
Assertion failure: secmod_PrivateModuleCount == 0, at pk11util.c:120
Abort (core dumped)
Comment 1 Julien Pierre 2008-02-19 18:38:20 PST
In this specific case, I was testing certs from bug 399152 . I added root I and root II to the DB as trusted . And I was verifying the EE and adding the intermediate on the command line.
Comment 2 Julien Pierre 2008-02-19 18:55:55 PST
Looks like the leak is specific to CERT_PKIXVerifyCert. When using libpkix through CERT_VerifyCertificate and the environment variable, I don't get the reference leak assertion. So, this is a leak in CERT_PKIXVerifyCert.

[jp96085@monstre]/net/monstre/export/home/julien/nss/tip/mozilla/dist/SunOS5.10_i86pc_DBG.OBJ/bin 185 % setenv NSS_ENABLE_PKIX_VERIFY 1
[jp96085@monstre]/net/monstre/export/home/julien/nss/tip/mozilla/dist/SunOS5.10_i86pc_DBG.OBJ/bin 186 % ./vfychain -d . -u 4 ee.der
Chain is good!
[jp96085@monstre]/net/monstre/export/home/julien/nss/tip/mozilla/dist/SunOS5.10_i86pc_DBG.OBJ/bin 187 %

And there is no leak while going through cert_VerifyCertChainPkix .

So, I'm reassigning this bug to Bob.
Comment 3 Robert Relyea 2008-03-06 10:15:17 PST
Created attachment 307745 [details] [diff] [review]
Fix memory leaks in pkix glue code.
Comment 4 Alexei Volkov 2008-03-06 12:00:28 PST
Comment on attachment 307745 [details] [diff] [review]
Fix memory leaks in pkix glue code.

1. >+    if (error && error != PKIX_ALLOC_ERROR()) {

Unnecessary check for PKIX_ALLOC_ERROR. It is safe to call decref without doing it.
2. In function cert_PKIXMakeOIDList error now get decrefed twice.

3. >@@ -1444,11 +1461,17 @@ cert_pkixSetParam(PKIX_ProcessingParams 
Please don't do changes in cert_pkixSetParam. They will collide with patch

4.     error = PKIX_ProcessingParams_Create(anchors, &procParams, plContext);
>     if (error != NULL) {              /* need pkix->nss error map */
>@@ -1677,13 +1724,21 @@ SECStatus CERT_PKIXVerifyCert(
>     certSelector = cert_GetTargetCertConstraints(cert, plContext);
>+    if (certSelector != NULL) {
>+        PORT_SetError(SEC_ERROR_IO);    /* need pkix->nss error map */
>+        goto cleanup;
>+    }
We have the map. But need to have PKIX_Error object to be able to convert. The best is to set error at cert_GetTargetCertConstraints function level.
Comment 5 Robert Relyea 2008-03-07 15:05:07 PST
Created attachment 308047 [details] [diff] [review]
Updated patch taking account the review comments

Note: interdiff won't work on this patch. That is because the original file was very far out of date and the original patch conflicted with lots of changes (as alexi pointed out in his review).

the new patch is small enough to be reviewed on it's own merits, so a diff between it and the original is not really useful anyway.

Comment 6 Nelson Bolyard (seldom reads bugmail) 2008-03-07 17:23:05 PST
Comment on attachment 308047 [details] [diff] [review]
Updated patch taking account the review comments

>@@ -1703,13 +1732,19 @@ SECStatus CERT_PKIXVerifyCert(
>     certSelector = cert_GetTargetCertConstraints(cert, plContext);
>+    if (certSelector != NULL) {

That test seems exactly backwards.  certSelector is not an error
object, it's the object you want to use in the next call below.
I'm pretty sure you don't want to pass NULL in the next call.

>+        goto cleanup;
>+    }
>     error = PKIX_ProcessingParams_SetTargetCertConstraints
>         (procParams, certSelector, plContext);
>     if (error != NULL) {
>         goto cleanup;
>     }
Comment 7 Robert Relyea 2008-03-14 09:35:07 PDT
Created attachment 309445 [details] [diff] [review]
Use correct failure test

This patch interdiffs with patch 308047 if for ease in a quick review
Comment 8 Robert Relyea 2008-03-14 09:36:29 PDT
Created attachment 309446 [details] [diff] [review]
Patch for checkin...

This patch is the same as 309445 except it was made against the current trunk. (cvs updated)
Comment 9 Nelson Bolyard (seldom reads bugmail) 2008-03-14 10:45:08 PDT
Comment on attachment 309446 [details] [diff] [review]
Patch for checkin...

All throughout cert_pkixSetParam we see code in error paths that 
sets an error code that may not accurately reflect the underlying
error.  Here are two examples of many:

>             policyOIDList = cert_PKIXMakeOIDList(param->value.array.oids,
>                                 param->value.arraySize,plContext);
>+	    if (policyOIDList == NULL) {
>+		r = SECFailure;

>                 error = PKIX_PL_Date_Create_UTCTime(NULL, &date, plContext);
>+                if (error != NULL) {
>+                    errCode = SEC_ERROR_INVALID_TIME;

The problem is that the underlying cause may be out of memory, and reporting 
that the input arguments were wrong, when they weren't, disserves the caller.
But it's beyond the scope of this patch to correct that throughout this function.
So, r=nelson
Comment 10 Robert Relyea 2008-03-14 11:07:39 PDT
Checking in certvfypkix.c;
/cvsroot/mozilla/security/nss/lib/certhigh/certvfypkix.c,v  <--  certvfypkix.c
new revision: 1.16; previous revision: 1.15
Comment 11 Julien Pierre 2008-03-17 19:12:28 PDT
I verified the fix.

Note You need to log in before you can comment on or make changes to this bug.