Closed
Bug 436430
Opened 16 years ago
Closed 16 years ago
Make NSS public headers compilable with NO_NSPR_10_SUPPORT defined
Categories
(NSS :: Libraries, defect, P2)
NSS
Libraries
Tracking
(Not tracked)
RESOLVED
FIXED
3.12.1
People
(Reporter: wtc, Assigned: wtc)
References
Details
Attachments
(10 files, 4 obsolete files)
207 bytes,
text/plain
|
Details | |
211 bytes,
text/plain
|
Details | |
72.46 KB,
patch
|
Details | Diff | Splinter Review | |
32.81 KB,
patch
|
Details | Diff | Splinter Review | |
77.59 KB,
patch
|
Details | Diff | Splinter Review | |
4.86 KB,
patch
|
nelson
:
review+
|
Details | Diff | Splinter Review |
422.03 KB,
patch
|
Details | Diff | Splinter Review | |
1.52 KB,
patch
|
nelson
:
review+
|
Details | Diff | Splinter Review |
1.47 KB,
patch
|
nelson
:
review+
|
Details | Diff | Splinter Review |
2.09 KB,
patch
|
nelson
:
review+
|
Details | Diff | Splinter Review |
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•16 years ago
|
||
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". (?)
Assignee | ||
Comment 2•16 years ago
|
||
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•16 years ago
|
||
I found these old scripts, and am storing them here for posterity.
Comment 4•16 years ago
|
||
Comment 5•16 years ago
|
||
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
Attachment #323332 -
Flags: review?(wtc)
Comment 6•16 years ago
|
||
What versions of NSS need these changes? trunk only? 3.11.x too?
Comment 7•16 years ago
|
||
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.
Attachment #323338 -
Flags: review?(wtc)
Comment 8•16 years ago
|
||
In the previous comment, I wrote lib/zlib when I meant lib/jar. Sorry for the confusion.
Comment 9•16 years ago
|
||
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.
Attachment #323340 -
Flags: review?(wtc)
Assignee | ||
Comment 10•16 years ago
|
||
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•16 years ago
|
||
(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•16 years ago
|
||
(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•16 years ago
|
||
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.
Attachment #323340 -
Attachment is obsolete: true
Attachment #324174 -
Flags: review?(wtc)
Attachment #323340 -
Flags: review?(wtc)
Comment 14•16 years ago
|
||
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.
Updated•16 years ago
|
Attachment #324176 -
Flags: review?(wtc)
Assignee | ||
Comment 15•16 years ago
|
||
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.
Attachment #324176 -
Attachment is obsolete: true
Attachment #325059 -
Flags: review?(nelson)
Attachment #324176 -
Flags: review?(wtc)
Comment 17•16 years ago
|
||
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•16 years ago
|
||
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.
Attachment #325059 -
Flags: review?(rrelyea)
Assignee | ||
Comment 19•16 years ago
|
||
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•16 years ago
|
||
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.
Assignee | ||
Comment 21•16 years ago
|
||
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.
Attachment #325059 -
Attachment is obsolete: true
Attachment #325059 -
Flags: review?(rrelyea)
Attachment #325059 -
Flags: review?(nelson)
Assignee | ||
Comment 22•16 years ago
|
||
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).
Attachment #325098 -
Flags: review?(nelson)
Assignee | ||
Comment 23•16 years ago
|
||
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.
Attachment #323338 -
Attachment is obsolete: true
Attachment #323338 -
Flags: review?(wtc)
Comment 24•16 years ago
|
||
Comment on attachment 325098 [details] [diff] [review] Test program nonspr10.c (checked in) r=nelson, thanks.
Attachment #325098 -
Flags: review?(nelson) → review+
Assignee | ||
Comment 25•16 years ago
|
||
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.
Attachment #325098 -
Attachment description: Test program nonspr10.c → Test program nonspr10.c (checked in)
Assignee | ||
Updated•16 years ago
|
Status: NEW → ASSIGNED
Target Milestone: --- → 3.12.1
Comment 26•16 years ago
|
||
I suggest we create new bugs for the additional work, and declare victory for this one.
Updated•16 years ago
|
Priority: -- → P2
Version: unspecified → trunk
Comment 27•16 years ago
|
||
We export nssckbi in a developement package for PKCS #11 developers. It would be worth having those headers adjusted as well. bob
Assignee | ||
Comment 28•16 years ago
|
||
cert_VerifyCertChainPkix incorrectly declares its |time| parameter as PRUint64.
Attachment #325346 -
Flags: review?(nelson)
Assignee | ||
Updated•16 years ago
|
Attachment #325346 -
Attachment is patch: true
Assignee | ||
Comment 29•16 years ago
|
||
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
Attachment #325348 -
Flags: review?(nelson)
Assignee | ||
Comment 30•16 years ago
|
||
This patch changes "UNIX" time to NSPR time in the comments in secder.h. These comments must have come from pre-NSPR libsec time.
Attachment #325350 -
Flags: review?(nelson)
Updated•16 years ago
|
Attachment #325346 -
Flags: review?(nelson) → review+
Updated•16 years ago
|
Attachment #325348 -
Flags: review?(nelson) → review+
Updated•16 years ago
|
Attachment #325350 -
Flags: review?(nelson) → review+
Assignee | ||
Comment 31•16 years ago
|
||
Comment on attachment 325346 [details] [diff] [review] PRUint64 => PRTime in cert_VerifyCertChainPkix (checked in) Checked in on the NSS trunk (NSS 3.12.1).
Attachment #325346 -
Attachment description: PRUint64 => PRTime in cert_VerifyCertChainPkix → PRUint64 => PRTime in cert_VerifyCertChainPkix (checked in)
Assignee | ||
Comment 32•16 years ago
|
||
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).
Attachment #325348 -
Attachment description: Do not declare CERT_CreateNicknameCertList and CERT_CreateEmailAddrCertList → Do not declare CERT_CreateNicknameCertList and CERT_CreateEmailAddrCertList (checked in)
Assignee | ||
Comment 33•16 years ago
|
||
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).
Attachment #325350 -
Attachment description: "UNIX" time => NSPR time in the secder.h comments → "UNIX" time => NSPR time in the secder.h comments (checked in)
Assignee | ||
Comment 34•16 years ago
|
||
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.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Updated•16 years ago
|
Attachment #323332 -
Flags: review?(wtc)
Updated•16 years ago
|
Attachment #324174 -
Flags: review?(wtc)
You need to log in
before you can comment on or make changes to this bug.
Description
•