426886 - Use "const" char* in PK11_ImportCertForKey

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Kai Engert (:KaiE:)]

It sucks that I get compiler errors when I try to pass in const char * to NSS functions.

We should change the API signature to const wherever possible.

Here is one example:

PK11SlotInfo *
PK11_ImportDERCertForKey(SECItem *derCert, char *nickname,void *wincx)

I propose to change the nickname to become "const" char*

Comment 1

[Kai Engert (:KaiE:)]

Attachment: Patch v1

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

If you change the types of these NSS functions' arguments to const,
aren't you simply moving the warnings from your code into NSS?

The functions you changed probably call other functions that also take
char * pointers that are not declared const.  So, you eliminate a warning
in your code, and create new warnings in NSS.  That sucks, too.
I object to doing that.  

If we're going to try to fix the const stuff, it has to be done all the way
through NSS.  

Comment 3

[Kai Engert (:KaiE:)]

(In reply to comment #2)
> If you change the types of these NSS functions' arguments to const,
> aren't you simply moving the warnings from your code into NSS?

Nelson, my code does not show warnings.
It stops with a compiler error, because it is forbidden to pass a "char*" into a "const char*" parameter.

When I compile NSS with this patch, I do not get any warnings about const mismatches.


> The functions you changed probably call other functions that also take
> char * pointers that are not declared const.  

Yes, I had originally complained about PK11_ImportCertForKey, but if you look at the patch you'll notice that I'm changing several places. This is because I have looked at the calls it makes to other functions, and the use of the nickname parameter is very limited.

I followed the code paths to the end (I hope).


> So, you eliminate a warning
> in your code, and create new warnings in NSS.  That sucks, too.
> I object to doing that.  

I don't see any warnings about const.


> If we're going to try to fix the const stuff, it has to be done all the way
> through NSS.  

I was hoping we could do it in steps.

Comment 4

[Kai Engert (:KaiE:)]

Attachment: Patch v2

Sorry, patch v1 was an earlier version of the patch, which did not have all changes to the header files.

Comment 5

[Kai Engert (:KaiE:)]

Ok, the compilers behave differently in C and C++ mode :-/

Given this snippet:

    #include "stdio.h"
    
    void func(char *p)
    {
      puts(p);
    }
    
    int main(int argc, char *argv[])
    {
      const char *x = "hello";
      func(x);
      return 0;
    }

$ LANG=C gcc ct1.c
ct1.c: In function 'main':
ct1.c:11: warning: passing argument 1 of 'func' discards qualifiers from pointer target type

$ LANG=C g++ ct1.c
ct1.c: In function 'int main(int, char**)':
ct1.c:11: error: invalid conversion from 'const char*' to 'char*'
ct1.c:11: error:   initializing argument 1 of 'void func(char*)'


The C compiler gives a warning.

The C++ compiler forbids the const mismatch with an error.

Comment 6

[Kai Engert (:KaiE:)]

I think the question we must answer is:

What happens if we change some variables/parameters to be const (as proposed by my patch), and pass those functions to code that uses a non-const parameter.

I think the previous comment demonstrates that we will get a warning for such attempts.

Furthermore, I have done two full builds of NSS, once without my patch, and once with my patch applied. The outputs were identical, no new warnings were introduced.

I think this proves that my patch is complete?


In order to verify the theory that my patch is complete, I performed a test using an incomplete, incorrect patch, which changes the signature of PK11_ImportCertForKey, only.
As expected I saw a new warning inside that function, for the call to PK11_ImportCert.

Comment 7

[Wan-Teh Chang]

Comment on attachment 313519 [details] [diff] [review]
Patch v2

r=wtc.

Nit: in pk11pub.h, you'll notice that rrelyea uses
an unusual method of using tabs to indent the continuation
lines.  I suggest you try to imitate his method when
modifying his code.

Comment 8

[Kai Engert (:KaiE:)]

Thanks for the review.

I plan to postpone the check in until after RC1.

Comment 9

[Julien Pierre]

Kai,

Re: comment 3,

Changing the function signature to take a const char* instead of a char* should be OK . It is not forbidden to pass a const char* variable to a function that takes a char* function unlike what you said . The reverse is forbidden, however. You can't pass a const char* vqriqble to a function that expects a char* . With a C compiler, this may be a warning, and with a C++ compiler, it is an error, as you noted.

As long as you don't get a warning or error compiling the modified NSS code when you change our function prototoype, non-const to const type changes will not affect any calling code, either internal or external.

Comment 10

[Julien Pierre]

There was some confusion on both my part and Kai :)

Kai wrote in comment 3 that "it is forbidden to pass a char* into a const char* parameter". That is not the case . Which is what I meant to correct in comment 9 .

But I also wrote erroneously that "it is not forbidden to pass a const char* variable to a function that takes a char*". The later is forbidden ;). But passing a char* variable into a function that takes const char* is OK.

Comment 11

[Kai Engert (:KaiE:)]

checked in, marking fixed.

Checking in dev/ckhelper.c;
/cvsroot/mozilla/security/nss/lib/dev/ckhelper.c,v  <--  ckhelper.c
new revision: 1.37; previous revision: 1.36
done
Checking in dev/dev.h;
/cvsroot/mozilla/security/nss/lib/dev/dev.h,v  <--  dev.h
new revision: 1.41; previous revision: 1.40
done
Checking in dev/devtoken.c;
/cvsroot/mozilla/security/nss/lib/dev/devtoken.c,v  <--  devtoken.c
new revision: 1.48; previous revision: 1.47
done
Checking in pk11wrap/pk11cert.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11cert.c,v  <--  pk11cert.c
new revision: 1.166; previous revision: 1.165
done
Checking in pk11wrap/pk11pub.h;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11pub.h,v  <--  pk11pub.h
new revision: 1.22; previous revision: 1.21
done