469944 - when built with Microsoft compilers, serious NSS errors are ignored!

Status: RESOLVED FIXED

(NSS :: Build, defect, P2)

Description

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Patch that greatly improves this situation

By default, MSVC ignores serious errors, such as calls to functions 
with the wrong numbers of arguments, or calls to undeclared functions.
Other compilers treat these as fatal errors.  MSVC merely warns.

This has caused some problems.  If NSS calls a function that is undeclared
on Windows only, we just don't find out about it. 

I have a patch that really helps with this.  It promotes certain warnings
to being compile-stopping errors.  With it, I found several problems in 
NSS, for which I will also attach patches to this bug.  

Wan-Teh, please review this small patch.  Thanks.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 353361 [details] [diff] [review]
Patch that greatly improves this situation

Credit where it's due.  
I found most of the contents of this patch on this web page:

http://www.codeproject.com/KB/debug/pragma_tips.aspx?df=100&forumid=3525&exp=0&select=144539&tid=144539

I will ask the author for permission to include this in NSS.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch part 2 - fix calls to undeclared isatty function in crlutil

With the first patch above in place, the build of crlutil fails because 
a file generated by lex calls isatty which is not declared.  The solution
is to include <io.h>.  I coded a change to the sed script that postprocesses
the output of lex, but I don't know if it is correct.  (The version of sed
I used doesn't grok this format.)

Alexei, please review.

Comment 3

[Wan-Teh Chang]

Comment on attachment 353361 [details] [diff] [review]
Patch that greatly improves this situation

r=wtc.

You can manually look up the official messages for these
warnings from MSDN, for example,
  http://msdn.microsoft.com/en-us/library/8kyye6db(VS.71).aspx
instead of
  +#pragma warning(error : 4541) // RTTI train wreck
This way you won't need to copy the code from the Code Project
article.  The messages from the Code Project article may be
more fun though.

Comment 4

[Wan-Teh Chang]

Comment on attachment 353365 [details] [diff] [review]
patch part 2 - fix calls to undeclared isatty function in crlutil

> #ifndef _WIN32
> #include <unistd.h>
>+#else
>+#include <io.h>
> #endif

It's better to change this to

  #ifdef _WIN32
  #include <io.h>
  #else
  #include <unistd.h>
  #endif

Comment 5

[Wan-Teh Chang]

Comment on attachment 353361 [details] [diff] [review]
Patch that greatly improves this situation

One side effect is that this patch will also affect third-party
code that includes any public NSS header when they recompile
their code against NSS 3.12.3.  So we should document this
in the NSS 3.12.3 release notes, and Kai should make sure
to do a verification build on Windows before pushing
NSS 3.12.3 to mozilla-central or some other Mozilla branch.

Comment 6

[Wan-Teh Chang]

Comment on attachment 353361 [details] [diff] [review]
Patch that greatly improves this situation

To avoid interfering with other projects' build systems, we
should do this on our own compiler command line in coreconf.
For example, the flag
  -we4541
is equivalent to the pragma
  #pragma warning(error : 4541) // RTTI train wreck

http://msdn.microsoft.com/en-us/library/thxezb7y(VS.71).aspx

The challenge of this approach is to figure out the MSVC
version in makefiles.  It's doable, but we don't seem to have
the code.  (I wrote the code once, but lost it.)

Another approach is to move this to a new header, and use
the -FI compiler flag to include it.

http://msdn.microsoft.com/en-us/library/8c5ztk84(VS.71).aspx

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 353361 [details] [diff] [review]
Patch that greatly improves this situation

Wan-Teh, thanks for pointing out -we.  I had looked for a command line
approach to this problem and had not found it, but that's it.  

-FI sounds exactly like what Doug Turner needs for his "shunt".
I'll write him about that separately.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Do the different versions of the MSVC compilers use different warning 
numbers for the same warnings?  I'm really asking, do we need to check
compiler version numbers?   

The version number check I put into my first patch was frankly just a guess.
I suspect it is sufficient to simply check that it is an MSVC compiler.
But I really don't know.  I will make a patch that adds -we options to 
OS_CFLAGS on Windows when not using gcc, without considering compiler version.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v2 - change warnings to errors on command line

This alternative approach found many more problems than the first one.
Wan-teh, please review.

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch part 3 - stop casting function pointers to data pointers in ckfw

MSVC reports a warning each time a function pointer is cast to a data pointer.
ckfw does that a LOT.
Now that we're causing many more of MSVC's warnings to be compile errors,
ckfw doesn't compile.  
This patch fixes it.  
Bob, please review.

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch part 3 - alternative, remove unnecessary casting of NULL from ckfw (checked in)

This patch is an alternative to the version named "part 3" above.
It goes beyond what was required to make MSVC happy, and removes LOTS of
unnecessary casting of NULL in comparisons.  IMO, this makes the code 
MUCH more readable.  

Bob, please review.  This was all done with two global search and replace 
commands, so it should not be necessary to review each and every change, 
because they weren't produced by hand.

Comment 12

[Wan-Teh Chang]

Comment on attachment 353489 [details] [diff] [review]
patch v2 - change warnings to errors on command line

Hi Nelson, please add this to coreconf/WIN32.mk instead.
All of these files include coreconf/WIN32.mk.

It'd be nice to add a comment describing what those
warnings are, essentially what you have in the previous
patch.

Comment 13

[Robert Relyea]

Comment on attachment 353495 [details] [diff] [review]
patch part 3 - stop casting function pointers to data pointers in ckfw

r+ The new code is easier to read anyway.

bob

Comment 14

[Robert Relyea]

Comment on attachment 353495 [details] [diff] [review]
patch part 3 - stop casting function pointers to data pointers in ckfw

removing my r+ because I prefer your other option better.

Comment 15

[Robert Relyea]

Comment on attachment 353501 [details] [diff] [review]
patch part 3 - alternative, remove unnecessary casting of NULL from ckfw (checked in)

r+ rrelyea

This is the better options, I like it more. (see I can speak english if I put my mind to it:).

Comment 16

[Wan-Teh Chang]

Comment on attachment 353501 [details] [diff] [review]
patch part 3 - alternative, remove unnecessary casting of NULL from ckfw (checked in)

It'd be nice to reduce the whitespace.  Two examples:

>-  if ((NSSCKFWCryptoOperation *)NULL == fwOperation) {
>+  if (! fwOperation) {

Delete the space between ! and the pointer variable.

>-  if ((NSSCKFWCryptoOperation *)NULL != fwOperation) {
>+  if (  fwOperation) {

Delete the spaces after the opening parenthesis.

I also don't like the unnecessary casts of NULL in the
CKFW code.  (They're also present in NSPR's plstr.h
functions.)  But I think a patch of this magnitude is
more appropriate for NSS 3.13 because it adds a lot of
noise to the diffs between NSS 3.12.2 and NSS 3.12.3,
and a lot of people read the diffs, to evaluate the risk
of upgrading or to track down a regression.

Comment 17

[Alexei Volkov]

Comment on attachment 353365 [details] [diff] [review]
patch part 2 - fix calls to undeclared isatty function in crlutil

Lets change it as suggested by Wan-teh.

Comment 18

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 353501 [details] [diff] [review]
patch part 3 - alternative, remove unnecessary casting of NULL from ckfw (checked in)

I made the whitespace changes that Wan-Teh requested, and checked this in.
Checking in ckfw/crypto.c;    new revision: 1.4;  previous revision: 1.3
Checking in ckfw/find.c;      new revision: 1.9;  previous revision: 1.8
Checking in ckfw/hash.c;      new revision: 1.4;  previous revision: 1.3
Checking in ckfw/instance.c;  new revision: 1.12; previous revision: 1.11
Checking in ckfw/mechanism.c; new revision: 1.6;  previous revision: 1.5
Checking in ckfw/mutex.c;     new revision: 1.9;  previous revision: 1.8
Checking in ckfw/object.c;    new revision: 1.16; previous revision: 1.15
Checking in ckfw/session.c;   new revision: 1.13; previous revision: 1.12
Checking in ckfw/sessobj.c;   new revision: 1.14; previous revision: 1.13
Checking in ckfw/slot.c;      new revision: 1.7;  previous revision: 1.6
Checking in ckfw/token.c;     new revision: 1.13; previous revision: 1.12
Checking in ckfw/wrap.c;      new revision: 1.18; previous revision: 1.17

Comment 19

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v3 - change warnings to errors on command line

I believe this is the change that wtc suggested.
Wan-Teh, please review this tiny patch.

Comment 20

[Wan-Teh Chang]

Comment on attachment 361230 [details] [diff] [review]
patch v3 - change warnings to errors on command line

It's better to add the new code between line
120 and line 121:

    118         DEFINES    += -DDEBUG -D_DEBUG -UNDEBUG -DDEBUG_$(USERNAME)
    119     endif
    120 else # !NS_USE_GCC
    121     ifdef BUILD_OPT
    122         OS_CFLAGS  += -MD

The section you chose deals with the assembler
(AS).  So all these compiler warning flags would
be out of place there.

It'd be nice to add a comment describing what
those warnings are, essentially what you have
in attachment 353361 [details] [diff] [review].

Comment 21

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch as committed

This is the patch I checked in.
Checking in WIN32.mk; new revision: 1.27; previous revision: 1.26

Comment 22

[Wan-Teh Chang]

Attachment: Add two more warnings about const arguments

Warning 4028 means a function parameter is const in the function
declaration but not in the definition.  This is a function prototype
mismatch between declaration and definition.

Warning 4090 means we're trying to pass a const pointer to a function
that takes a non-const pointer.

Both of these should be compilation errors.

I hope there is a better way than keep adding new -we<xxxx> flags.

Comment 23

[Nelson Bolyard (seldom reads bugmail)]

reopening.  
Review requests for patches attached to resolved bugs do not show up in my
searches, and tend to be ignored for long periods.

Comment 24

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 362420 [details] [diff] [review]
Add two more warnings about const arguments

Several comments about this patch:

A) I agree that pointer assignments that implicitly cast away the const 
attribute of the object SHOULD be compilation errors on ALL platforms.
It should be necessary to *explicitly* cast away const to perform such 
assignments, and we should never do that.  

I thought that most compilers on most platforms already did make such 
pointer assignments an error, but to my surprise, I find that they 
evidently do not.  When I applied this patch and tried to build NSS, 
I ran into MANY errors on Windows.  NSS makes this mistake in many 
places, yet it builds without errors on all supported platforms today.  
This patch makes the Windows MSVC compiler more pedantic about this 
than any of our other compilers, evidently.  

B) A great ongoing source of frustration for NSS developers has been
that one compiles the code successfully on one platform, then commits
it, only to find that it does not compile on another platform.  To 
eliminate that frustration, it is desirable that all platforms have 
the same rules, all declaring errors for the same problems.  That is 
not entirely possible, but IMO, we should strive for it as much as 
possible.  My reason for filing and working on this bug was to try 
to make MSVC approximately as pedantic as other platforms, to reduce
my own frustration from building code on Windows that does not build
on other platforms.

Of course, some platforms will always be more pedantic than others.  
But I am reluctant to change the Windows platform behavior to make it 
more pedantic than the other platforms, because most NSS developers do 
NOT develop on Windows as their primary platform.  If Windows becomes
significantly more pedantic than other platforms, many developers 
will commit code that then breaks on Windows, which will compound 
frustrations.  

So, I think two things must be done before this patch can be committed.
1) All the places in NSS that stop compiling because of it need to be
fixed, and 
2) If possible, a similar change should be made to the builds with 
other compilers to make them also treat this as an error, so that 
the pedantry of all (or most) compilers is increased similarly.

I would give this patch an r+ once those other things are achieved.  
I am now working on fixing the const problems revealed by this patch,
but there are many and changing them will be a big patch.

Comment 25

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Supplemental Patch - v1 - fix const errors in NSS (checked in)

With the combination of this patch and Wan-Teh's previous one, NSS builds OK.
This wasn't as bad as I expected it to be, in part because NSS already 
explicitly cast's away const in many places.  

Note: I added one place where we explicitly cast away const.  
PKCS#11 never uses "const" in its function declarations, but some NSS 
functions that call them declare the passed values as const.  I don't know
if C_CreateObject ever modifies its input template array or not.  If not,
then this cast away is fine, but if so, then it is wrong for 
PK11_CreateNewObject to declare its input argument as pointer to const.

Comment 26

[Wan-Teh Chang]

Comment on attachment 362429 [details] [diff] [review]
Supplemental Patch - v1 - fix const errors in NSS (checked in)

r=wtc.  Sorry that I didn't test my patch on the entire
NSS.  I only tested it in the lib/freebl directory.

Note: I didn't realize all compilers merely warn about
assigning a const pointer to a non-const pointer.  So
I am fine with NOT adding -we4090.  If we add -we4090,
won't that make MSVC more strict than GCC and Sun Studio?
-we4028 should be safe because the declaration and definition
of a function should have the same prototype.

Either way, all of the changes in this patch are good.
I am a little worried about the following change in
lib/pk11wrap/pk11obj.c:

>+	crv = PK11_GETTAB(slot)->C_CreateObject(rwsession, 
>+	      /* cast away const :-( */         (CK_ATTRIBUTE_PTR)theTemplate,
>+						count, objectID);

Since 'slot' can be an arbitrary third-party PKCS #11 library,
are you sure it's safe to assume that it won't modify theTemplate?
(I can't imagine why any PKCS #11 library would need to modify
the Template.)

Should we ask the PKCS #11 working group to add more 'const'
to the PKCS #11 API in the upcoming v2.30?

Nit: in lib/libpkix/pkix_pl_nss/pki/pkix_pl_ocspresponse.c,
please indent by 8 spaces and avoid tabs (the local convention
in libpkix).

Comment 27

[Wan-Teh Chang]

Nelson, I didn't see your comment 25 before I wrote comment 26.
Sorry.  I agree that removing the 'const' from PK11_CreateNewObject
is another option.

Comment 28

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 362429 [details] [diff] [review]
Supplemental Patch - v1 - fix const errors in NSS (checked in)

Checked in on trunk
cmd/lib/secutil.c;      new revision: 1.93; previous revision: 1.92
cmd/lib/secutil.h;      new revision: 1.31; previous revision: 1.30
pkix_pl_ocspresponse.c; new revision: 1.16; previous revision: 1.15
pkix_pl_common.c;       new revision: 1.6;  previous revision: 1.5
pkix_pl_common.h;       new revision: 1.7;  previous revision: 1.6
lib/pk11wrap/pk11obj.c; new revision: 1.21; previous revision: 1.20

Comment 29

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 362420 [details] [diff] [review]
Add two more warnings about const arguments

Wan-Teh, if you can devise a patch to make gcc also treat 
these issues as errors, so that msvc and gcc would agree,
I'd be happy with such a patch, I believe.

Comment 30

[Wan-Teh Chang]

Attachment: Add the warning about function prototype mismatch between declaration and definition

Nelson, let's just add -we4028 for now.  I verified
that a function prototype mismatch between declaration
and definition is a compilation error for GCC.

Passing a const pointer to a function that takes a
non-const pointer is also just a compiler warning
for GCC.  So I removed -we4090 from this patch.

Comment 31

[Wan-Teh Chang]

Attachment: Add the warning about function prototype mismatch between declaration and definition (correct patch; checked in)

I attached the wrong patch.  This is the correct one.

Comment 32

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 362534 [details] [diff] [review]
Add the warning about function prototype mismatch between declaration and definition (correct patch; checked in)

r=nelson

Comment 33

[Wan-Teh Chang]

Comment on attachment 362534 [details] [diff] [review]
Add the warning about function prototype mismatch between declaration and definition (correct patch; checked in)

I checked in this patch on the NSS trunk (NSS 3.12.3).

Checking in WIN32.mk;
/cvsroot/mozilla/security/coreconf/WIN32.mk,v  <--  WIN32.mk
new revision: 1.29; previous revision: 1.28
done

Comment 34

[Nelson Bolyard (seldom reads bugmail)]

I would like to nominate two more warnings for promotion to errors on 
Windows.  They are:

C4024: <function> : different types for formal and actual parameter <n>
C4047: 'function' : <type 1> differs in levels of indirection from <type 2>

Comment 35

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Promote one more warning to error, suppress deprecation warnings (checked in)

Recently I wrote a patch that compared an error code to the name of the
function that returns error codes, not to the value returned by the 
function, e.g. 
   if (PORT_GetError == SEC_ERROR_something)
instead of
   if (PORT_GetError() == SEC_ERROR_something)

It got past review.  My compiler only generated a warning about it, 
and that warning was lost is a DELUGE of worthless warnings about calls
to "deprecated" (but still fully supported) libc functions.  

I don't want this to happen again.  So, I've written this patch that will
a) promote that warning code to be a full error, and 
b) suppress all those darn worthless deprecation warnings about libc.

I've checked that NSS builds OK with this patch.  There's not a big rush
on this, but I'd like to get it in while the pain and embarrassment is 
still fresh in my mind.

Comment 36

[Nelson Bolyard (seldom reads bugmail)]

Thanks for the review.
Checking in WIN32.mk; new revision: 1.31; previous revision: 1.30

Comment 37

[Wan-Teh Chang]

Attachment: Document what waring 4047 is

I've checked this trivial patch in.

Checking in WIN32.mk;
/cvsroot/mozilla/security/coreconf/WIN32.mk,v  <--  WIN32.mk
new revision: 1.33; previous revision: 1.32
done