436430 - Make NSS public headers compilable with NO_NSPR_10_SUPPORT defined

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Wan-Teh Chang]

NSPR headers define the int8, int16, int32, int64, ...
types that are also defined by some other headers.  One
way to avoid the typedef conflict is to define NO_NSPR_10_SUPPORT
before including NSPR headers.  The NO_NSPR_10_SUPPORT macro
will also turn off some macros such as PRArenaPool and
PRHashTable.  (Those symbols have the PL prefix in the
current NSPR.)

But NSS public headers still use the int64 type and
the PRArenaPool macro, so an NSS-based application
cannot compile the NSS public headers with NO_NSPR_10_SUPPORT
defined.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

This ought to be EASY to fix.  I think my old "gsr" script (global search
and replace) that replaces symbols in a list a files would do this nicely
in a few minutes.  

I wonder if any NSS developers still have a copy of that script, or its
companion script, named "gsrword".  (?)

Comment 2

[Wan-Teh Chang]

Nelson, I don't have your "gsr" script.

Here are my findings on this bug.

The following NSPR 1.0 compatibility stuff is used by
NSS public headers:

  #define PRArenaPool PLArenaPool
  typedef PRInt64 int64;
  typedef PRUint16 uint16;

Note: the use of int64 in NSS public headers should be
replaced by PRTime instead of PRInt64.

The following NSPR 1.0 compatibility stuff is used by
the rest of NSS:

  typedef PRUint32 uint32;
  #define BITS_PER_BYTE PR_BITS_PER_BYTE
  typedef PRUint8 uint8;
  typedef PRUintn uint;
  #define PR_PUBLIC_API PR_IMPLEMENT
  typedef PRInt16 int16;
  typedef PRInt32 int32;
  #define PR_ARENA_ALLOCATE PL_ARENA_ALLOCATE
  #define PR_ARENA_RELEASE PL_ARENA_RELEASE

The following NSPR 1.0 compatibility stuff is used by
mozilla/dbm:

  typedef PRUint8 uint8;
  typedef PRUint16 uint16;
  typedef PRInt32 int32;
  typedef PRUint32 uint32;
  typedef PRUintn uint;

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Global Search and Replace (gsr) shell script

I found these old scripts, and am storing them here for posterity.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Global Search and Replace a word (gsrword) shell script

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch for dbm

I made this patch using the gsrword script attached above.
It changes the symbols suggested for mozilla/dbm in comment 2 above.

I don't know if the list of symbols given in comment 2 was exhaustive or not.
If so, then this patch should suffice to resolve this bug for that directory.
Wan-Teh, please review.

The commands I used to make the patch were approximately these:

cd mozilla/dbm
DBM_FILES=$(grep -l 'int[0-9]' */*.[ch])
gsrword.sh uint8 PRUint8 $DBM_FILES
gsrword.sh uint16 PRUint16 $DBM_FILES
gsrword.sh uint32 PRUint32 $DBM_FILES
gsrword.sh uint PRUintn $DBM_FILES
gsrword.sh int32 PRInt32 $DBM_FILES
cvs diff -pu5 > /tmp/436430a.txt

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

What versions of NSS need these changes?
trunk only?
3.11.x too?

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Monster patch: change files in security/nss per comment 2

This patch may be a candidate for the biggest monster patch of all time for 
NSS.  It is the output of cvs diff after running the following script:

gsrword.sh BITS_PER_BYTE PR_BITS_PER_BYTE      $(cat NSS_FILES)
gsrword.sh PRArenaPool PLArenaPool             $(cat NSS_FILES)
gsrword.sh PR_ARENA_ALLOCATE PL_ARENA_ALLOCATE $(cat NSS_FILES)
gsrword.sh PR_ARENA_RELEASE PL_ARENA_RELEASE   $(cat NSS_FILES)

gsrword.sh int16 PRInt16   $(cat NSS_FILES)
gsrword.sh int32 PRInt32   $(cat NSS_FILES)
gsrword.sh int64 PRTime    $(cat NSS_FILES)
gsrword.sh uint16 PRUint16 $(cat NSS_FILES)
gsrword.sh uint32 PRUint32 $(cat NSS_FILES)
gsrword.sh uint8 PRUint8   $(cat NSS_FILES)
gsrword.sh uint PRUintn    $(cat NSS_FILES)

This patch is missing the changes to nss/lib/zlib which must be regenerated.
nss/lib/zlib was the only part of NSS that used PR_PUBLIC_API, but it 
didn't use NSPR's definition of that macro.  Rather, it used its own
definition, so changing PR_PUBLIC_API to PR_IMPLEMENT in those sources 
doesn't have the desired effect.  I will produce a separate patch for 
nss/lib/zlib shortly.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

In the previous comment, I wrote lib/zlib when I meant lib/jar.  
Sorry for the confusion.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch for nss/lib/jar

This patch makes the changes to lib/jar/*.[ch] that were suggested in
comment 2, except for the change to PR_PUBLIC_API.  Instead, it gets rid
of the PR_PUBLIC_API macro entirely.

Comment 10

[Wan-Teh Chang]

Nelson, thanks a lot for creating these patches.  I
believe the patches are correct.  We just need to
manually fix the indentation in a few places.

Are we sure we want the monster patch in NSS 3.12.1?
It will obscure the important fixes in NSS 3.12.1
when someone reads the diffs between 3.12 and 3.12.1.

Also, mozilla/dbm is not worth fixing.

Since lib/jar is only used by signtool and modutil,
and nobody compiles them with -DMOZILLA_CLIENT, I
think we should just delete the #ifdef MOZILLA_CLIENT
code in lib/jar.

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

(In reply to comment #10)

> Are we sure we want the monster patch in NSS 3.12.1?
> It will obscure the important fixes in NSS 3.12.1
> when someone reads the diffs between 3.12 and 3.12.1.

Maybe we only need to fix the public header files.

All the patches to all the individual files in the "monster patch" are
mutually independent.  You can apply any subset of them you like without
worry about the rest.  So, maybe we should just make a new patch that is 
a pure subset of the monster patch, patching only the public header files.
What do you think of that proposal?  Would you like me to do that?

> Also, mozilla/dbm is not worth fixing.

Well, I think that if any of the NSS .c files is worth fixing, then dbm
is just as worthy as the rest.  We're not going to see a cessation of the
use of cert8.db files any time soon.  But perhaps fixing the .c files 
and the private header files is simply not worth doing.

> Since lib/jar is only used by signtool and modutil,
> and nobody compiles them with -DMOZILLA_CLIENT, I
> think we should just delete the #ifdef MOZILLA_CLIENT
> code in lib/jar.

Agreed.  I'd like to see lib/jar cleaned up, whether or not any of the 
other .c files in NSS is also cleaned up.

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

(In reply to comment #10)
> Since lib/jar is only used by signtool and modutil,
> and nobody compiles them with -DMOZILLA_CLIENT, I
> think we should just delete the #ifdef MOZILLA_CLIENT
> code in lib/jar.

To my surprise, and horror, I see that lib/jar *is* being built with 
-DMOZILLA_CLIENT=1  

That is defined in manifest.mn.  :(

lib/jar is such a pant load. :(

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch for lib/jar, v2

This bigger patch for lib/jar does more cleanup, gets rid of macros
MOZILLA_CLIENT, MOZILLA_CLIENT_OLD and the redefined PR_PUBLIC_API macro.

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Monster patch subset: only header files

This subset of the above Monster patch includes only the header files.
I did not attempt to separate public headers from private. 
The public headers are a subset of this patch.  

We could further reduce this patch by removing changes to the header 
files for freebl, libpkix, softoken/legacydb, and files whose names end 
with i.h, it.h or ti.h (I think).  But my preference would be to fix as 
many of the sources files as we are willing to tolerate.  That's why I 
haven't marked the original "Monster patch" obsolete.  I'm willing to 
accept that patch.

Comment 15

[Wan-Teh Chang]

Attachment: Monster patch subset: only public header files

I took an even smaller subset: only the public header files.
In this subset, I removed only one space to fix indentation.

I also added a test program to verify that the public headers
can be compiled with -DNO_NSPR_10_SUPPORT.  Nelson, you just
need to review this new test program nonspr10.c.

Comment 17

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 325059 [details] [diff] [review]
Monster patch subset: only public header files

Wan-Teh, the #if 0 sections of the test program must be removed, 
either by removing the entire sections, or by removing just the 
#if and #endif.  

AFAIK, none of the nssckbi headers are part of the public NSS API.
But let's ask Bob for his opinion on that.

I'm confident that none of the lib/jar headers are part of the public
NSS API.  

So I think both of those entire #if 0 sections should go.  

I would add to the list of public headers:
nss/cmd/lib/SECerrs.h
nss/cmd/lib/SSLerrs.h
nss/cmd/lib/NSPRerrs.h 

You might incorporate them into the test program with code like this,
added just before main:

typedef struct tuple_str {
    PRErrorCode  errNum;
    const char * errString;
} tuple_str;

#define ER2(a,b)   {a, b},
#define ER3(a,b,c) {a, c},

const tuple_str errStrings[] = {
#include "SSLerrs.h"
#include "SECerrs.h"
#include "NSPRerrs.h"
};

Comment 18

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 325059 [details] [diff] [review]
Monster patch subset: only public header files

Bob, please comment on the questions/issues in the preceding comment.

Comment 19

[Wan-Teh Chang]

Nelson, I'm only addressing public NSS headers.  They
are exported to mozilla/dist/public/nss in a NSS build
tree, and are listed in
http://lxr.mozilla.org/security/source/security/nss/pkg/solaris/SUNWtlsd/prototype

SECerrs.h, SSLerrs.h, and NSPRerrs.h are not public NSS
headers.  They are only used by the NSS command-line tools.

It is fine if NSS itself requires the NSPR 1.0 compatibility
typedefs and macros.  My goal is to not let that requirement
propagate to NSS-based apps via the NSS headers they include.

Comment 20

[Nelson Bolyard (seldom reads bugmail)]

Wan-Teh, at present, these 3 extra header files are THE one and only way for
applications to get the NSS error strings, since the NSS error strings are
not available yet in any shared libs.  But I guess they're not relevant to 
the objectives of this particular bug.

Comment 21

[Wan-Teh Chang]

Attachment: Monster patch subset: only public header files (checked in)

I checked in the public header changes on the NSS trunk (NSS 3.12.1).

I'm going to attach the test program (not checked in) as a separate
patch next.

Comment 22

[Wan-Teh Chang]

Attachment: Test program nonspr10.c (checked in)

I had to change cmd/manifest.mn so that cmd/tests
gets built.

In cmd/tests/manifest.mn, set NO_MD_RELEASE to 1
to exclude the test programs in that directory from
our binary distribution.

I removed the three jar headers, all the nssck*.h
headers except nssckbi.h (which defines the version
numbers of our built-in roots module), and
utilrename.h (which our clients should not include
directly).  I also replaced pkcs11n.h by pkcs11t.h
(our clients should not include pkcs11n.h directly).

Comment 23

[Wan-Teh Chang]

Attachment: Monster patch (the rest): change files in security/nss per comment 2

Here is the rest of Nelson's monster patch for security/nss.
It may be better to regenerate this patch when we're ready
to check it in.

Comment 24

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 325098 [details] [diff] [review]
Test program nonspr10.c (checked in)

r=nelson, thanks.

Comment 25

[Wan-Teh Chang]

Comment on attachment 325098 [details] [diff] [review]
Test program nonspr10.c (checked in)

I added the cmd/tests/nonspr10.c test program on the NSS trunk
(NSS 3.12.1).

This concludes the work that must be done for this bug.  I'd
like to also fix nss/lib/jar and some other minor issues I found
while reviewing the code.  If I don't do this in two months,
we can mark this bug fixed.

Comment 26

[Nelson Bolyard (seldom reads bugmail)]

I suggest we create new bugs for the additional work, and declare victory 
for this one.

Comment 27

[Robert Relyea]

We export nssckbi in a developement package for PKCS #11 developers. It would be worth having those headers adjusted as well.

bob

Comment 28

[Wan-Teh Chang]

Attachment: PRUint64 => PRTime in cert_VerifyCertChainPkix (checked in)

cert_VerifyCertChainPkix incorrectly declares its |time| parameter
as PRUint64.

Comment 29

[Wan-Teh Chang]

Attachment: Do not declare CERT_CreateNicknameCertList and CERT_CreateEmailAddrCertList (checked in)

CERT_CreateNicknameCertList and CERT_CreateEmailAddrCertList are
declared but not implemented.  These LXR queries verify that:
http://lxr.mozilla.org/security/search?string=CERT_CreateNicknameCertList
http://lxr.mozilla.org/security/search?string=CERT_CreateEmailAddrCertList

Comment 30

[Wan-Teh Chang]

Attachment: "UNIX" time => NSPR time in the secder.h comments (checked in)

This patch changes
    "UNIX" time
to
    NSPR time
in the comments in secder.h.  These comments must have come
from pre-NSPR libsec time.

Comment 31

[Wan-Teh Chang]

Comment on attachment 325346 [details] [diff] [review]
PRUint64 => PRTime in cert_VerifyCertChainPkix (checked in)

Checked in on the NSS trunk (NSS 3.12.1).

Comment 32

[Wan-Teh Chang]

Comment on attachment 325348 [details] [diff] [review]
Do not declare CERT_CreateNicknameCertList and CERT_CreateEmailAddrCertList (checked in)

Checked in on the NSS trunk (NSS 3.12.1).

Comment 33

[Wan-Teh Chang]

Comment on attachment 325350 [details] [diff] [review]
"UNIX" time => NSPR time in the secder.h comments (checked in)

Checked in on the NSS trunk (NSS 3.12.1).

Comment 34

[Wan-Teh Chang]

Marked the bug fixed.

I opened bug 439556 for the cleanup of lib/jar.

I am fine with not fixing mozilla/dbm and the rest of NSS.