103736 - NSS uses |char *| when it should use |const char*|

Status: NEW

(NSS :: Libraries, defect, P3)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

NSS APIs use |char*| for all strings, including those that the function does not
modify (and where the client of NSS should be able to depend on that const-ness
in the future).  This is not compatible with the way some of its clients, such
as PSM2, use strings, or with the way other libraries (e.g., libc, nspr) use
|char*| and |const char*|.  PSM uses the Mozilla string classes, which often use
|const char*|.  The combination of the correct use of |const| in the Mozilla
string classes and the lack of |const| in the NSS APIs requires at least casts,
which are ugly and should normally indicate unsafe operations that need to be
checked carefully.  Worse, it leads to frequent programmer error such as
unnecessary copying (from a |const char*| into an specially-allocated and owned
|char*| just to call a function) or to leaks when the caller expects |char*| to
mean that NSS will take ownership of the string buffer (e.g., bug 103011).

Thus, I think NSS APIs should use |const char*| for strings that NSS will not
modify so that NSS can be compatible with the string conventions used by its
callers.

Comment 1

[Wan-Teh Chang]

I agree with you fully.

In practice, adding 'const' where appropriate is an
expensive operation because of the const propogation
effect.  It is difficult for us to fix all the public
NSS functions to use 'const' correctly.  Therefore,
I request that you open new bug reports for specific
NSS functions that you know should be fixed.

Within the NSS group, I will ensure that new functions
use 'const' correctly. 

Comment 2

[Kai Engert [:KaiE:]]

Is it possible to find out (besides from reading the NSS code), whether a NSS
function will not modify its parameters, although it's a non-const?

Currently we are casting const away in PSM code, something Mozilla
superreviewers do not like.

Just for discussion, what do you think we should prefer?

Should we just continue to live with the risk of passing in pointers to const
memory, that might be modified by NSS, hoping we will not write wrong code?

Or, to be on the safe but slower side, should we change the coding policy of
PSM, and allocate temporary copies whenever we need to pass a string to NSS?

Comment 3

[Robert Relyea]

We should continue to do the casts. NSS does not modify string values anywhere
except maybe some private interfaces. Most of the data structures in NSS are
opaque (or supposed to be) so their 'const' status should be irrelevent. (making
copies of these objects usually means incrementing a reference count anyway).

As we work out the Stan API we can be more strict about use of const in NSS.

bob

Comment 4

[Julien Pierre]

Wan-Teh,

The "const propagation" effect you mentioned causes even new functions to have
incorrect non-const arguments when they should be const, unless ugly casts are
used. I don't think we should cast everywhere in the implementation, rather we
should fix all the prototypes. Going from non-const arguments to const arguments
will not break any client program, as you can always pass const or non-const
data as a const argument. We should probably look at all the header files and do
it in one shot rather than work one API at a time. It would be a very large
patch but one that would be worth it for all the C++ programmers out there,
myself included. If I had the cycles now I would volunteer to make that large patch.

Comment 5

[Christopher Hoess (gone)]

REMIND is deprecated per bug 35839.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

We sure have a lot of euphamisms for "WONTFIX", including "resolved/REMIND", 
"target:Future".  But this isn't P1 by any definition of P1.