Move the declaration for CERT_CompareNameSpace from cert.h to genname.h

because it is a private function.  Change the interface for this
function so that it returns a SECStatus, unambiguously indicating the
success or failure of the name constraints test.  The function no
longer takes a list of cert subject names, instead, it takes a list
of cert pointers, and optionally outputs one of those pointers when
an error occurs.  This eliminates a cert reference leak.

security/nss/lib/certdb/cert.h

@@ -1170,13 +1170,6 @@ CERT_GetCertificateNames(CERTCertificate *cert, PRArenaPool *arena);
 int
 CERT_GetNamesLength(CERTGeneralName *names);
 
-CERTCertificate *
-CERT_CompareNameSpace(CERTCertificate  *cert,
-		      CERTGeneralName  *namesList,
-		      SECItem          *namesListIndex,
-		      PRArenaPool      *arena,
-		      CERTCertDBHandle *handle);
-
 SECStatus 
 CERT_EncodeSubjectKeyID(PRArenaPool *arena, char *value, int len, SECItem *encodedValue);
 

security/nss/lib/certdb/genname.c

@@ -1392,12 +1392,12 @@ cert_CompareNameWithConstraints(CERTGeneralName     *name,
 ** If some name is not acceptable, returns a pointer to the cert that
 ** contained that name.
 */
-CERTCertificate *
+SECStatus
 CERT_CompareNameSpace(CERTCertificate  *cert,
 		      CERTGeneralName  *namesList,
- 		      SECItem          *namesListIndex,
+ 		      CERTCertificate **certsList,
  		      PRArenaPool      *arena,
- 		      CERTCertDBHandle *handle)
+ 		      CERTCertificate **pBadCert)
 {
     SECStatus            rv;
     SECItem              constraintsExtension;
@@ -1410,53 +1410,57 @@ CERT_CompareNameSpace(CERTCertificate  *cert,
     rv = CERT_FindCertExtension(cert, SEC_OID_X509_NAME_CONSTRAINTS, 
                                 &constraintsExtension);
     if (rv != SECSuccess) {
-	return (PORT_GetError() == SEC_ERROR_EXTENSION_NOT_FOUND)
-	       ? NULL  /* success, space is unconstrained. */
-	       : CERT_DupCertificate(cert); /* failure, some other error */
+	if (PORT_GetError() == SEC_ERROR_EXTENSION_NOT_FOUND) {
+	    rv = SECSuccess;
+	} else {
+	    count = -1;
+	}
+	goto done;
     }
     /* TODO: mark arena */
     constraints = cert_DecodeNameConstraints(arena, &constraintsExtension);
     currentName = namesList;
-    if (constraints == NULL)  /* decode failed */
-	goto loser;
+    if (constraints == NULL) { /* decode failed */
+	rv = SECFailure;
+    	count = -1;
+	goto done;
+    } 
     do {
  	if (constraints->excluded != NULL) {
  	    rv = CERT_GetNameConstraintByType(constraints->excluded, 
 	                                      currentName->type, 
  					      &matchingConstraints, arena);
- 	    if (rv != SECSuccess)
- 		goto loser;
- 	    if (matchingConstraints != NULL) {
+ 	    if (rv == SECSuccess && matchingConstraints != NULL) {
  		rv = cert_CompareNameWithConstraints(currentName, 
 		                                     matchingConstraints,
  						     PR_TRUE);
- 		if (rv != SECSuccess) 
- 		    goto loser;
  	    }
+	    if (rv != SECSuccess) 
+		break;
  	}
  	if (constraints->permited != NULL) {
  	    rv = CERT_GetNameConstraintByType(constraints->permited, 
 	                                      currentName->type, 
  					      &matchingConstraints, arena);
-            if (rv != SECSuccess) 
-		goto loser;
- 	    if (matchingConstraints != NULL) {
+            if (rv == SECSuccess && matchingConstraints != NULL) {
  		rv = cert_CompareNameWithConstraints(currentName, 
 		                                     matchingConstraints,
  						     PR_FALSE);
- 		if (rv != SECSuccess) 
- 		    goto loser;
  	    }
+	    if (rv != SECSuccess) 
+		break;
  	}
  	currentName = cert_get_next_general_name(currentName);
  	count ++;
     } while (currentName != namesList);
+done:
+    if (rv != SECSuccess) {
+	badCert = (count >= 0) ? certsList[count] : cert;
+    }
+    if (pBadCert)
+	*pBadCert = badCert;
     /* TODO: release back to mark */
-    return NULL;
-loser:
-    /* TODO: release back to mark */
-    badCert = CERT_FindCertByName (handle, &namesListIndex[count]);
-    return badCert;
+    return rv;
 }
 
 /* Search the cert for an X509_SUBJECT_ALT_NAME extension.

security/nss/lib/certdb/genname.h

@@ -132,7 +132,16 @@ CERT_DupGeneralNameList(CERTGeneralNameList *list);
 /* returns the length of a CERTGeneralName */
 int
 CERT_GetNamesLength(CERTGeneralName *names);
+
 /************************************************************************/
+
+SECStatus
+CERT_CompareNameSpace(CERTCertificate  *cert,
+		      CERTGeneralName  *namesList,
+ 		      CERTCertificate **certsList,
+ 		      PRArenaPool      *arena,
+ 		      CERTCertificate **pBadCert);
+
 SEC_END_PROTOS
 
 #endif

security/nss/lib/certhigh/certvfy.c

@@ -665,8 +665,8 @@ cert_VerifyCertChain(CERTCertDBHandle *handle, CERTCertificate *cert,
     PRArenaPool *arena = NULL;
     CERTGeneralName *namesList = NULL;
     CERTGeneralName *subjectNameList = NULL;
-    SECItem *namesIndex = NULL;
-    int namesIndexLen = 10;
+    CERTCertificate **certsList      = NULL;
+    int certsListLen = 16;
     int namesCount = 0;
 
     cbd_FortezzaType last_type = cbd_None;
@@ -738,30 +738,31 @@ cert_VerifyCertChain(CERTCertDBHandle *handle, CERTCertificate *cert,
 	goto loser;
     }
 
-    namesIndex = (SECItem *) PORT_ZAlloc(sizeof(SECItem) * namesIndexLen);
-    if (namesIndex == NULL) {
+    certsList = PORT_ZNewArray(CERTCertificate *, certsListLen);
+    if (certsList == NULL)
 	goto loser;
-    }
 
     for ( count = 0; count < CERT_MAX_CERT_CHAIN; count++ ) {
 	int subjectNameListLen;
 	int i;
 
-	/* Construct a list of names for the current and all previous certifcates 
-	   to be verified against the name constraints extension of the issuer
-	   certificate. */
-	subjectNameList = CERT_GetCertificateNames(subjectCert, arena);
+	/* Construct a list of names for the current and all previous 
+	 * certifcates to be verified against the name constraints extension 
+	 * of the issuer certificate. 
+	 */
+	subjectNameList    = CERT_GetCertificateNames(subjectCert, arena);
 	subjectNameListLen = CERT_GetNamesLength(subjectNameList);
-	for (i = 0; i < subjectNameListLen; i++) {
-	    if (namesIndexLen <= namesCount + i) {
-		namesIndexLen = namesIndexLen * 2;
-		namesIndex = (SECItem *) PORT_Realloc(namesIndex, namesIndexLen * 
-						       sizeof(SECItem));
-		if (namesIndex == NULL) {
-		    goto loser;
-		}
+	if (certsListLen <= namesCount + subjectNameListLen) {
+	    certsListLen = (namesCount + subjectNameListLen) * 2;
+	    certsList = 
+	        (CERTCertificate **)PORT_Realloc(certsList, 
+		                     certsListLen * sizeof(CERTCertificate *));
+	    if (certsList == NULL) {
+		goto loser;
 	    }
-	    rv = SECITEM_CopyItem(arena, &(namesIndex[namesCount + i]), &(subjectCert->derSubject));
+	}
+	for (i = 0; i < subjectNameListLen; i++) {
+	    certsList[namesCount + i] = subjectCert;
 	}
 	namesCount += subjectNameListLen;
 	namesList = cert_CombineNamesLists(namesList, subjectNameList);
@@ -946,12 +947,12 @@ cert_VerifyCertChain(CERTCertDBHandle *handle, CERTCertificate *cert,
 	    PORT_SetError(SEC_ERROR_INADEQUATE_KEY_USAGE);
 	    LOG_ERROR_OR_EXIT(log,issuerCert,count+1,requiredCAKeyUsage);
 	}
-	/* make sure that the entire chain is within the name space of the current issuer
-	 * certificate.
-	 */
-
-	badCert = CERT_CompareNameSpace(issuerCert, namesList, namesIndex, arena, handle);
-	if (badCert != NULL) {
+	/* make sure that the entire chain is within the name space of the 
+	** current issuer certificate.
+	*/
+	rv = CERT_CompareNameSpace(issuerCert, namesList, certsList, 
+	                           arena, &badCert);
+	if (rv != SECSuccess || badCert != NULL) {
 	    PORT_SetError(SEC_ERROR_CERT_NOT_IN_NAME_SPACE);
             LOG_ERROR_OR_EXIT(log, badCert, count + 1, 0);
 	    goto loser;
@@ -975,8 +976,8 @@ cert_VerifyCertChain(CERTCertDBHandle *handle, CERTCertificate *cert,
 loser:
     rv = SECFailure;
 done:
-    if (namesIndex != NULL) {
-	PORT_Free(namesIndex);
+    if (certsList != NULL) {
+	PORT_Free(certsList);
     }
     if ( issuerCert ) {
 	CERT_DestroyCertificate(issuerCert);