Last Comment Bug 426886 - Use "const" char* in PK11_ImportCertForKey
: Use "const" char* in PK11_ImportCertForKey
Status: RESOLVED FIXED
:
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: trunk
: x86 Linux
: -- normal (vote)
: 3.12.1
Assigned To: nobody
:
Mentors:
Depends on:
Blocks: 426830
  Show dependency treegraph
 
Reported: 2008-04-03 14:55 PDT by Kai Engert (:kaie)
Modified: 2008-05-29 10:25 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch v1 (4.87 KB, patch)
2008-04-03 15:31 PDT, Kai Engert (:kaie)
wtc: review+
Details | Diff | Review
Patch v2 (6.32 KB, patch)
2008-04-03 19:28 PDT, Kai Engert (:kaie)
wtc: review+
julien.pierre: superreview+
Details | Diff | Review

Description Kai Engert (:kaie) 2008-04-03 14:55:43 PDT
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) 2008-04-03 15:31:40 PDT
Created attachment 313462 [details] [diff] [review]
Patch v1
Comment 2 Nelson Bolyard (seldom reads bugmail) 2008-04-03 17:27:08 PDT
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) 2008-04-03 19:27:23 PDT
(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) 2008-04-03 19:28:42 PDT
Created attachment 313519 [details] [diff] [review]
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) 2008-04-03 19:44:12 PDT
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) 2008-04-03 20:17:23 PDT
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 2008-04-04 08:12:00 PDT
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) 2008-04-04 15:38:48 PDT
Thanks for the review.

I plan to postpone the check in until after RC1.
Comment 9 Julien Pierre 2008-04-04 18:57:23 PDT
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 2008-04-07 18:05:17 PDT
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) 2008-05-29 10:25:13 PDT
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

Note You need to log in before you can comment on or make changes to this bug.