Closed Bug 875577 Opened 7 years ago Closed 4 years ago

add a CERT_DupCertList that works on CERTCertList

Categories

(NSS :: Libraries, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: keeler, Assigned: keeler)

References

Details

Attachments

(1 file)

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.
Attached patch patchSplinter Review
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!
Assignee: nobody → dkeeler
Status: NEW → ASSIGNED
Attachment #753550 - Flags: review?(wtc)
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.
Attachment #753550 - Flags: review?(wtc) → review-
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).
Depends on: 731485
Doesn't seem like this is necessary at this point.
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.