430135 - Need improvement to pkix object leak testing

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Alexei Volkov]

Attachment: object leak test modifications.

It is weak on several points:
   * unable to test CERT_PKIXVerifyCert because of incorrect test start position.
   * stack that generated an error is not printed out
   * policy validation functions are not covered.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 316878 [details] [diff] [review]
object leak test modifications.

We conducted a review by phone today.  In that review, we concluded
that it should be impossible for the condition (!errorGenerated) to 
be false in the first two tests (shown below), that is, errorGenerated
MUST be 0/Null in those places, so we can remove those two tests, and 
instead insert an assertion immediately before the first call to 
cert_pkix_FindOutputParam shown below.

>@@ -2175,6 +2168,9 @@ do {
> 
>     oparam = cert_pkix_FindOutputParam(paramsOut, cert_po_trustAnchor);
>     if (oparam != NULL) {
>+#ifdef PKIX_OBJECT_LEAK_TEST
>+        if (!errorGenerated)
>+#endif /* PKIX_OBJECT_LEAK_TEST */
>         oparam->value.pointer.cert = 
>                 cert_NSSCertFromPKIXCert(trustAnchorCert,plContext);
>     }
>@@ -2187,6 +2183,9 @@ do {
> 
>     oparam = cert_pkix_FindOutputParam(paramsOut, cert_po_certList);
>     if (oparam != NULL) {
>+#ifdef PKIX_OBJECT_LEAK_TEST
>+        if (!errorGenerated)
>+#endif /* PKIX_OBJECT_LEAK_TEST */
>         error = cert_PkixToNssCertsChain(builtCertList,
>                                          &oparam->value.pointer.chain,
>                                          plContext);

In the third test of the condition, errorGenerated can be true. 
I propose that the patch insert a test of "if (!errorGenerated)" and
not change the line that follows it, similar to the above two patches.

>@@ -2199,7 +2198,11 @@ cleanup:
>     if (verifyNode) {
>         /* Return validation log only upon error. */
>         oparam = cert_pkix_FindOutputParam(paramsOut, cert_po_errorLog);
>+#ifdef PKIX_OBJECT_LEAK_TEST
>+        if (!errorGenerated && r && oparam != NULL) {
>+#else
>         if (r && oparam != NULL) {
>+#endif /* PKIX_OBJECT_LEAK_TEST */
>             PKIX_Error *tmpError =
>                 cert_GetLogFromVerifyNode(oparam->value.pointer.log,
>                                           verifyNode, plContext);

E.g. 

>+#ifdef PKIX_OBJECT_LEAK_TEST
>+        if (!errorGenerated)
>+#endif
>         if (r && oparam != NULL) {
>             PKIX_Error *tmpError =


r+ with those changes.

Comment 2

[Alexei Volkov]

patch is integrated. Closing the bug...

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Alexei,
This checkin caused one of the Tinderboxes to go orange. 
It has been orange ever since this checkin.  

Comment 4

[Alexei Volkov]

Last checkin didn't integrate three certs needed for one of the object leak test. Certs are now checked in and tbox is green again.