228624 - PK11_ListCertsInSlot does not return the correct nicknames of the cert instances.

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Wan-Teh Chang]

PK11_ListCertsInSlot does not return the correct
nicknames of the cert instances.

It returns cert->nickname as the nickname, which
is not always right.

In bug 72291 Julien fixed a similar problem for
PK11_ListCerts.  My fix for this bug will be based
on Julien's work.

Comment 1

[Wan-Teh Chang]

Attachment: Proposed patch

This patch fixes two problems.

1. PK11_ListCertsInSlot should return the correct nickname
for each cert instance.  The instance nicknames are available
in the Stan layer, so we just need to reach into the Stan
layer to get them.

2. When adding a cert to a Stan trust domain's cert cache,
if we get a collision, we should merge all the instances
of the cert before destroying the duplicate cert; otherwise
the instances in the duplicate cert will be lost.

Note that this patch is very complicated because I tried
to clean up properly after an out-of-memory error.  Suggestions
on simplifying the cleanup code are welcome.

Comment 2

[Julien Pierre]

Comment on attachment 137507 [details] [diff] [review]
Proposed patch

The patch looks good.

This is a suggestion to eliminate duplication in the cleanup code :

+	/* merge the instances of the cert */
+	instances = nssPKIObject_GetInstances(&cert->object);
+	if (instances == NULL) {
+	    nssCertificate_Destroy(rvCert);
+	    goto loser;
+	}
+	for (ci = instances, i = 0; *ci; ci++, i++) {
+	    nssCryptokiObject *instance = nssCryptokiObject_Clone(*ci);
+	    if (instance) {
+		 if (nssPKIObject_AddInstance(&rvCert->object, instance)
+							== SECSuccess) {
+		     continue;
+		 }
+		 nssCryptokiObject_Destroy(instance);
+	     }
+	     remove_object_instances(&rvCert->object, instances, i);
+	     nssCryptokiObjectArray_Destroy(instances);
+	     nssCertificate_Destroy(rvCert);
+	     goto loser;
+	 }
+	nssCryptokiObjectArray_Destroy(instances);

Comment 3

[Wan-Teh Chang]

Comment on attachment 137507 [details] [diff] [review]
Proposed patch

Thank you for the code review and suggested change,
Julien.

While I tried to implement a variation of your suggested
change, I found a bug in this patch.

In lib/pki/tdcache.c, function add_cert_to_cache, if we
fail to merge the cert instances, we can't goto loser;
otherwise we would unlock td->cache->lock twice.

I will attach a better patch next.

Comment 4

[Wan-Teh Chang]

Attachment: Proposed patch v2

Comment 5

[Wan-Teh Chang]

Comment on attachment 137616 [details] [diff] [review]
Proposed patch v2

I checked in this patch.

Checking in pk11wrap/pk11cert.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11cert.c,v  <--  pk11cert.c
new revision: 1.128; previous revision: 1.127
done
Checking in pki/tdcache.c;
/cvsroot/mozilla/security/nss/lib/pki/tdcache.c,v  <--	tdcache.c
new revision: 1.37; previous revision: 1.36
done

There is a problem at a higher level that eventually
causes the cache collision that my patch for tdcache.c
handles.  So one could argue that we could just fix
that higher-level problem.  But Bob suggested that
my code to merge cert instances on a cache collision
is still useful.  So I'm going to track that
higher-level problem in a separate bug (bug 228881).

Comment 6

[Wan-Teh Chang]

Attachment: Supplemental patch #1

It seems that we need to call STAN_ForceCERTCertificateUpdate
whenever we add instances to a NSSCertificate.	This patch
does that.

Ian, is this correct?  Is STAN_ForceCERTCertificateUpdate
harmless if we did not add any new instance to the NSSCertificate?

Comment 7

[Wan-Teh Chang]

Supplemental patch checked in.

Checking in tdcache.c;
/cvsroot/mozilla/security/nss/lib/pki/tdcache.c,v  <--  tdcache.c
new revision: 1.38; previous revision: 1.37
done

Comment 8

[Ian McGreer]

Comment on attachment 137670 [details] [diff] [review]
Supplemental patch #1

It is correct to update the CERTCertificate there.

In general, the idea of STAN_ForceCERTCertificateUpdate was that it would be a
"refresh" function, safe to call at any time, to update the fields of the
CERTCertificate.