854729 - Add const to many certificate-related NSS functions

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement, P2)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Add const to many certificate-related NSS functions

+++ This bug was initially created as a clone of Bug #853812 +++

This patch adds "const" to many parameters of NSS functions related to certificates, to make it clear that those functions are pure, at least with respect to the certificate passed in. The distinction between pure and non-pure functions in NSS is not obvious and these const declarations help a lot in understanding things. Note that I use the "const SECItem*" construct often to mean "pointer to a buffer that won't be modified" even though the compiler will not enforce that since SECItem.data is non-const.

The patch was r+d in bug but Ryan asked me to split it off into its own bug.

Comment 1

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

https://hg.mozilla.org/projects/nss/rev/9639e5f328c9

I made all the changes that Ryan suggested in bug 853812, except I did not make any whitespace-only changes to lines I didn't otherwise change, and I didn't attempt to fix any indention style issues. I removed the "const_cast" comments.

I left the "#include <prtypes.h>" I added to hasht.h because hasht.h uses PRBool, and if hasht.h is the first file you include, then the build will fail. I didn't think this was worth splitting into another bug.

Comment 2

[Wan-Teh Chang]

Attachment: Make all CERT_GetXXXName functions take a const CERTName * input

Brian's patch made CERT_GetCommonName take a const CERTName *
input.  There is code that assumes all of these CERT_GetXXXName
functions have the same function prototype and calls them via
a function pointer with that prototype.  So we need to change
them all to be consistent.

Comment 3

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 730837 [details] [diff] [review]
Make all CERT_GetXXXName functions take a const CERTName * input

I had added the compiler warning for const/non-const mismatches as a way to detect such possible problems. What code is calling these functions through a pointer? Perhaps there is a different warning generated for that case that needs to be marked as "treat as an error."

Comment 4

[Wan-Teh Chang]

(In reply to Brian Smith (:bsmith) from comment #3)
>
> I had added the compiler warning for const/non-const mismatches as a way to
> detect such possible problems. What code is calling these functions through
> a pointer?

Brian: I see my comment was confusing. The code I
referred to is not in NSS but rather in Chromium:

https://chromiumcodereview.appspot.com/10928107/diff/35/net/base/x509_util_nss.cc

Search for "CERTGetNameFunc" in that file.

Comment 5

[Wan-Teh Chang]

Comment on attachment 730837 [details] [diff] [review]
Make all CERT_GetXXXName functions take a const CERTName * input

https://hg.mozilla.org/projects/nss/rev/e13e4c2a5799

Comment 6

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Make the parameter to CERT_GetOCSPAuthorityInfoAccessLocation const

Comment 7

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 745008 [details] [diff] [review]
Make the parameter to CERT_GetOCSPAuthorityInfoAccessLocation const

Never mind, this is already part of an r+d patch.

Comment 8

[Wan-Teh Chang]

Attachment: Add const to CERT_GetNextNameConstraint and CERT_GetPrevNameConstraint

It is a little strange that these two functions will take const pointer inputs
but return non-const pointers. But we can't change the return type of these
functions.

These functions don't need to modify their inputs, so their inputs can be
declared as const pointers. This allows us to remove a const cast added in
a patch of this bug report.

Comment 9

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 746587 [details] [diff] [review]
Add const to CERT_GetNextNameConstraint and CERT_GetPrevNameConstraint

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

Wan-Teh, I think it doesn't make sense to return a non-const pointer for a const input like this. Making the result const, if that is correct, won't break the ABI; it would only break source compatibility in a trivially way. If it isn't correct to return a const result (i.e. callers will actually modify the resulting object), then this patch seems like it would encourage dangerous practice. Please let me know if I am misunderstanding the issue.

Comment 10

[Wan-Teh Chang]

Comment on attachment 746587 [details] [diff] [review]
Add const to CERT_GetNextNameConstraint and CERT_GetPrevNameConstraint

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

::: lib/certdb/genname.c
@@ +270,5 @@
>      return (CERTGeneralName *) (((char *) prev) - offsetof(CERTGeneralName, l));
>  }
>  
>  CERTNameConstraint *
> +CERT_GetNextNameConstraint(const CERTNameConstraint *current)

Brian: I was indeed trying to avoid breaking source compatibility.
Changing these two functions to return a const pointer won't break
the ABI.

Maybe the solution is to add "Const" versions of these two functions.