875577 - add a CERT_DupCertList that works on CERTCertList

Status: RESOLVED WONTFIX

(NSS :: Libraries, defect)

Description

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

We have CERT_DupCertificate that takes a CERTCertificate, CERT_DupDistNames that takes a CERTDistNames, and CERT_DupGeneralNameList that takes a CERTGeneralNameList. And then we have CERT_DupCertList that takes a CERTCertificateList. This is a problem because a) it is inconsistent and confusing and b) it means we have no similar function that works on CERTCertList. It may be that the actual problem is that we have both CERTCertificateList and CERTCertList, but as far as I can tell, one is vectory-based and the other is linked-list-based, so perhaps there's value in having both. Anyway, my proposal is to add a function CERT_DupCertificateList that operates on CERTCertificateList and modifying CERT_DupCertList to operate on CERTCertList.

Comment 1

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: patch

Wan-Teh, would you be able to review this?
(NB: I'm sure there are some NSS conventions I'm not aware of, so I apologize in advance for that.)
Thanks!

Comment 2

[Wan-Teh Chang]

Comment on attachment 753550 [details] [diff] [review]
patch

Review of attachment 753550 [details] [diff] [review]:
-----------------------------------------------------------------

David: thank you for the patch. I believe the patch is correct.
I suggest some changes.

I assume you need a function to duplicate a CERTCertList?
Or are you suggesting the new function just for API symmetry?

Note: another difference between CERTCertificateList and
CERTCertList is that one contains DER-encoded certificate
bytes and the other contains decoded CERTCertificate objects.

::: lib/certdb/cert.h
@@ -1068,5 @@
>  extern CERTCertificateList *
>  CERT_CertListFromCert(CERTCertificate *cert);
>  
>  extern CERTCertificateList *
> -CERT_DupCertList(const CERTCertificateList * oldList);

NSS maintains backward compatibility, so we can't change the
prototype of a public function like this.

It is unfortunate that CERT_DupCertList actually operates on a
CERTCertificateList rather than a CERTCertList.

We will need to add a new function. I guess we can name it
CERT_DupCERTCertList?

::: lib/certdb/certdb.c
@@ +2541,5 @@
>  
> +CERTCertList *
> +CERT_DupCertList(const CERTCertList *oldList)
> +{
> +    CERTCertListNode *head;

Nit: Please name this variable |node|.

@@ +2546,5 @@
> +    CERTCertList *newList;
> +    CERTCertificate *cert;
> +
> +    newList = CERT_NewCertList();
> +    if ( newList == NULL) {

Nit: If you add a space after the opening parenthesis, you should
also add a space before the closing parenthesis. Please fix
this problem throughout this function.

@@ +2560,5 @@
> +        CERT_AddCertToListTail(newList, cert);
> +        head = CERT_LIST_NEXT(head);
> +    }
> +
> +    return newList;

Nit: add a blank line after this return statement.

Comment 3

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Thank you for the review. It's looking like it would be helpful to have a function to copy a CERTCertList for bug 846489, depending on how bug 731485 turns out (if it uses CERTCertList, really).

Comment 4

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Doesn't seem like this is necessary at this point.