Closed Bug 228624 Opened 21 years ago Closed 21 years ago

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


(NSS :: Libraries, defect, P1)



(Not tracked)



(Reporter: wtc, Assigned: wtc)



(2 files, 1 obsolete file)

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.
Attached patch Proposed patch (obsolete) — Splinter Review
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.
Priority: -- → P1
Target Milestone: --- → 3.9
Attachment #137507 - Flags: superreview?(rrelyea0264)
Attachment #137507 - Flags: review?(jpierre)
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);
Attachment #137507 - Flags: review?(jpierre) → review+
Comment on attachment 137507 [details] [diff] [review]
Proposed patch

Thank you for the code review and suggested change,

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.
Attachment #137507 - Attachment is obsolete: true
Attachment #137507 - Flags: superreview?(rrelyea0264)
Attachment #137616 - Flags: superreview?(rrelyea0264)
Attachment #137616 - Flags: review?(jpierre)
Attachment #137616 - Flags: review?(jpierre) → review+
Attachment #137616 - Flags: superreview?(rrelyea0264) → superreview+
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
Checking in pki/tdcache.c;
/cvsroot/mozilla/security/nss/lib/pki/tdcache.c,v  <--	tdcache.c
new revision: 1.37; previous revision: 1.36

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).
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?
Attachment #137670 - Flags: superreview?(bugz)
Attachment #137670 - Flags: review?(rrelyea0264)
Attachment #137670 - Flags: review?(rrelyea0264) → review+
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
Closed: 21 years ago
Resolution: --- → FIXED
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
Attachment #137670 - Flags: superreview?(bugz) → superreview+
You need to log in before you can comment on or make changes to this bug.