Closed
Bug 438870
Opened 16 years ago
Closed 16 years ago
Free Freebl hashing code of dependencies on NSPR and libUtil
Categories
(NSS :: Libraries, defect, P1)
Tracking
(Not tracked)
RESOLVED
FIXED
3.12.3
People
(Reporter: rrelyea, Assigned: rrelyea)
References
Details
(Whiteboard: FIPS)
Attachments
(4 files, 7 obsolete files)
2.82 KB,
patch
|
Details | Diff | Splinter Review | |
1.94 KB,
patch
|
nelson
:
review+
wtc
:
review+
|
Details | Diff | Splinter Review |
45.95 KB,
patch
|
nelson
:
review+
wtc
:
review+
|
Details | Diff | Splinter Review |
6.61 KB,
patch
|
Details | Diff | Splinter Review |
Certain Linux system libraries would like to provide hashing through the NSS FIPS validated implementations, but they are sensitive to proliferation of library dependencies. These libraries would like to access the hash algorithms directly from softoken, but would not want to pick up a dependency on NSPR (or even the rest of NSS).
Assignee | ||
Comment 1•16 years ago
|
||
My proposal would be to include a compile time option to softoken and freebl which allows them to stub out calls they make to libnspr4, libplds4, libplc4, and libnssutil3. This compile time option would be Linux only. The necessary subset to do hashing through softoken would be implemented as stubs (Linux only). On linux, these functions would use the real versions of libnssutil and libnspr4 if those libraries are already linked into the application (which would be the case if libnss3 is included in the applications), so standard NSS applications would continue to use nspr and nssutil. In addition, if the application is trying to open softoken with a database specified, then softoken will attempt to load these libraries if they aren't already loaded. While I have a patch that implements the above, I'll delay posting it until the discussion of the outlines of the proposal are complete. bob
Comment 2•16 years ago
|
||
If this is only for hashing, and hashing uses no keys, is there any chance they could use blapi rather than PKCS11/softoken?
Comment 3•16 years ago
|
||
I have a crazy proposal. If money is no object, we should create a hash-only mini crypto module. This module won't have any CSPs. The extra work required to meet FIPS requirements is the power-up self-test of the hash algorithms and the software integrity test, which will require that the mini hash module include HMAC for internal use. This module only needs to be validated at Level 1 on one (x86) or two (x86 and x86-64) Linux platforms. Hopefully that'll make the validation much cheaper. Then, the softoken can use the mini hash module as a submodule.
Assignee | ||
Comment 4•16 years ago
|
||
wtc: Your proposal is not unlike kai's proposal, except it adds fips validation. It would still require the other FIPS stuff, however: POST and audit, both of which is in softoken right now. Nelson, it's primarily the FIPS requirements that hold back direct to blapi, but even in that case we need to stub out blapi's use of NSPR and nssutil. bob
Comment 5•16 years ago
|
||
FIPS Level 1 doesn't have audit requirements. We don't need to validate such a trivial crypto module at Level 2. Also, I believe the auditable events all have to do with CSPs, so the audit requirements may not apply to the mini hash module anyway. We can easily move the hash algorithms' power-up self-tests from the softoken to the mini hash module. The software integrity test would require duplicating the PR_GetLibraryFilePathname code.
Assignee | ||
Comment 6•16 years ago
|
||
This patch consists of 4 basic types of changes: 1) build changes. 2) conditional includes a header for renaming the nspr/nssutil functions. 3) sdb changes to dynamically load sqlite3. 4) stub implementation (stubs.c) The general plan is to rename those functions to the stubs. The stubs call the real function if it's been loaded, otherwise it uses it's stub implementation (sometimes NULL if the function was not needed). at C_Initiailize time, we try to initialize the stubs to use the 'real' (actual NSPR/nssutil) implementation. If we find one of the libraries is not already, and we detect we are trying to load the database, try to explicitly load that shared library. bob
Comment 7•16 years ago
|
||
I like Wan-Teh's idea. Sun would much prefer that we all (Sun & red Hat) build one common set of binaries, and build and test (and FIPS validate) the same things. Having Red Hat (or any major user of NSS) produce a build of NSS that is essentially completely custom (Franken NSS, stub softoken) means that the testing that is one on one set of builds does not increase our confidence much in other builds.
Assignee | ||
Comment 8•16 years ago
|
||
You understand that we still need the freebl portion of this patch, since freebl itself needs to be NSPR-free. WTC are you talking a separate shared library (on top of freebl) for hashing? bob
Comment 9•16 years ago
|
||
Yes, I'm talking a separate shared library for hashing. If the Linux system libraries don't need to use the hash algorithms in FIPS mode, then this mini hash module doesn't need to do power-up self-tests and software integrity test. The mini hash module would only receive the algorithm validation certificates.
Comment 10•16 years ago
|
||
I think Wan-Teh is talking about a separate stand-alone PKCS#11 module for hashing only. I think it could have the necessary hash functions linked in directly and would not need a separate freebl shared lib. If, as Wan-Teh suggests, the apps that want this module don't need FIPS compliance, then I'm not sure there's ANY FIPS validation cost for it at all.
Comment 11•16 years ago
|
||
A hashing library in the form of a pkcs#11 module wouldn't help us at all, because it would still require all of the NSPR/NSS infrastructure in order to load and use it. I think the idea is to implement the basic hashing only using ANSI C.
Comment 12•16 years ago
|
||
This hash mini module should be a plain shared library. It doesn't need to be a PKCS #11 module. We still need to submit this shared library for algorithm validation at least. It is much cheaper than a full crypto module validation.
Assignee | ||
Comment 13•16 years ago
|
||
We need the FIPS validation. It's the only reason for replacing the existing hashing. If it wasn't for fips, nelson's suggestion of direct to freebl would be exactly what we would use. bob
Assignee | ||
Comment 14•16 years ago
|
||
I think a separate PKCS #11 module for hashing would not be a good idea. We still need to do hashing in our regular token or SSL macing will fail since we can't move hash states from one token into another. (my objection is based on the assumption that you mean both modules would live in NSS at the same time, and NSS would use the hashing PKCS #11 module for hashing). Or are you saying a separate PKCS #11 module that's not really used by NSS only by applications doing hashing?
Comment 15•16 years ago
|
||
Bob, I wasn thinking that the apps that need only hashing would use the hashing-only module, and the ones that need more would use softoken. Isn't authentication to the module required for all FIPS modules? Or is it only required for modules with CSPs? If you just want a shared library that has certified implementations of the hash algorithms, and it doesn't require any API that has "sessions" that are accessed through login (IOW, no need whatsoever for PKCS#11) then you can do that without any changes to the source code in freebl at all (except perhaps to the makefile, and you could even have an alternative makefile). You just need to link with a .a library that supplies the tiny subset of NSPR that freebl uses. In that case, the freebl API seems entirely adequate. On the other hand, if you do require login access control, then I think you want PKCS#11. Kai says you don't want PKCS#11, which tells me that you don't have any login access control requirements. Then why is blapi not a sufficient API?
Comment 16•16 years ago
|
||
Stray character in last comment. Should read: I *was* thinking that...
Comment 17•16 years ago
|
||
Let's consider the sha1sum and sha256sum commands. Can they use the softoken in FIPS mode? The user would need to enter a password, and we need to run all the power-up self-tests, including expensive RSA and DSA, just to run a SHA-1 or SHA-256 operation. So my crazy proposal is a practical way for them to use a hash mini module in FIPS mode, because the hash mini module only needs to be Level 1 (no operator authentication required), and it only needs to perform four SHA-x power-up self-tests and one HMAC software integrity test. The hash mini module is just a plain shared library like freebl. It is not a PKCS #11 module. Softoken will also use it for hashing. NSS (lib/pk11wrap) doesn't manage the hash mini module. To NSS, the hash mini module is an internal component of the softoken.
Comment 18•16 years ago
|
||
Bob, what is the objection of these Linux programs to picking up an "NSPR dependency" ? Do they not want to have NSPR loaded in their process at all ? Or do they not want to program to it directly ? Today they can already use the PKCS#11 API without using NSPR directly. As for the hash mini-module, do we want that to be a dumbed-down version of hashing functions ? Eg. one that doesn't have platform optimizations, like freebl does ? Or do we want it to be the real optimized thing, which would be called by softoken ? If so, that would still require dynamic loading. softoken would need to dynamically load the "right" hashing mini-module; and then the client programs would need to know about the multiple too. This exposes significant implementation details, that I think are desireable to keep private. I would suggest that if there is a hash mini-module, that we do not change the current architecture of the softoken - ie. that softoken would not call into it. The hash mini-module could be built additionally on Linux only, with the rest of the OS, and for the right target platform, and without a need for runtime loading.
Comment 19•16 years ago
|
||
(In reply to comment #18) > Bob, what is the objection of these Linux programs to picking up an "NSPR > dependency" ? Do they not want to have NSPR loaded in their process at all ? Julien, we are talking about the core C library, which must have as little external dependencies as possible (ideally we're being asked to keep the current state) > As for the hash mini-module, do we want that to be a dumbed-down version of > hashing functions ? Approach (A) Have only a single hashing implementation for all platforms, in order to avoid duplicating code. Move all current NSS hashing code to libhash, include platform specific optimizations (meaning, libhash is the only place where we implement hashing). Define a new external interface for hashing that uses ANSI-C types, avoiding all NSPR types. Change NSS to call into libhash for all hashing needs. Declare libhash an integral part of NSS. Julien, you said, if we were to change softoken/freebl to call into the new libhash, it would be required to teach applications about this new dependency. But I suspect that's not necessary, because of our recent change to remove the link-time dependency from nss to softoken? Therefore I'm not sure about your argument regarding dynamic loading. Softoken already uses dynamic loading to load freebl, therefore it should be ok to have freebl load libhash, too? Approach (B) Similar to A, but: - NSS keeps all its internal hashing code - NSS on Linux calls into libhash - NSS on other platforms uses its internal hashing Approach (C) libhash as a Linux-only fork of today's NSS hashing NSS never calls into libhash I would prefer (A).
Comment 20•16 years ago
|
||
Kai, Some of your proposed approaches say "for Linux". Are they for all flavors of Linux? or only for Red Hat? I would want to avoid having separate build configurations for separate flavors of Linux. The problem with having NSS use libhash (approach A) is that it makes libhash part of the body of code that must be FIPS validated, on every platform in which softoken uses libhash and is FIPS validated. That's not feasible in the general case, because not all OSes have libhash, and not all of those that do are willing to replace their libhash with NSS's.
Comment 21•16 years ago
|
||
(In reply to comment #20) > Kai, Some of your proposed approaches say "for Linux". > Are they for all flavors of Linux? Yes, if we should pick anything Linux specific, then I think we should strive for treating all flavors of Linux identical. The only reason I have elaborated on possible approaches was Julien's mentioning of Linux-only solutions. I'm not sure what the right thing to do is, just having the general thought that it's preferable to avoid platform specific solutions as much as possible. > The problem with having NSS use libhash (approach A) is that it makes > libhash part of the body of code that must be FIPS validated, on every > platform in which softoken uses libhash and is FIPS validated. good point Yes, the original thought was to have a separate Linux-only libhash. Using libhash on all platforms is a newer idea. NSS hashing code could be moved over to libhash (transforming it into plain C). NSS could thereby drop its internal hashing code. The implementation of existing hashing API of freebl/softoken would simply forward calls to libhash. Yes, using that approach would require us to validate libhash to the same extent as the remainder of freebl/softoken. But at the same time we'd no longer require to FIPS validate the freebl/softoken hashing API? > That's not feasible in the general case, because not all OSes have > libhash, and not all of those that do are willing to replace their > libhash with NSS's. Well, as of today, there is no libhash (yet). Moving all of NSS hashing code into a non-NSPR ANSI-C-only libhash on all platforms would be approach A. In that scenario the separation into libhash would be a core NSS change, and all platforms would get it.
Comment 22•16 years ago
|
||
Kai, Re: comment 19, Re: Approach (A) We already don't have the same hashing implementation for all platforms. For example, we have a specially optimized SHA-1 assembly source for AMD64 on Solaris. Hashing is currently part of a dynamically loadable freebl library, loaded by softoken. While at the moment, there is only one source code for hashing per platform - ie. all the libsoftoken*.so on one given platforms are built from the same source code relative to each other - this may not remain the case, as we may want to add more optimizations in the future, based for example on different CPU architectures (eg. Intel vs AMD, SSE2, 64-bit Niagara vs Ultrasparc III/IV, etc). I don't think we should lose track of that. SHA 256, 384, 512 might be optimized natively in the future too. I think having the same hashing implementation for all platforms is not desirable. Even though they are built from the same source code, the various libfreebl are built differently. For example, on the Sparc 32 bits platform, the same SHA-1 source code is compiled 3 times, with different compiler flags, and linked into 3 different libfreebl shared libraries. So, we would have to have 3 different libhash. Or maybe 4 of them : one proxy libhash, loading one of the other 3 libhash libraries with the actual implementations. This sounds like an awful lot of work. As far as my comment to teach applications, it was regarding non-NSS applications. Since there will be multiple libhash binaries to choose from per architecture, there still needs to be a loader somewhere. Either you build an intermediate libhash and put the loader in it, or you put the loader in the application itself. I would strongly object to the later. If softoken loads libhash, it will have to load multiple implementations of them, just like it loads multiple implementations of libfreebl today . Approach (B) I would prefer that approach. No consumers on any other platform is asking for any hashing functions that don't link to NSPR. Currently, on each of the two Linux variants we support (x86 and x64), there is only one freebl binary. So, you only have to load a single libhash. If you ever want to have hashing optimizations on Linux that are specific to certain x86 or x64 CPUs, would you have to create multiple libhash ? I know Solaris marks binaries based on their instruction set, so you cannot load binaries that contain instructions incompatible with the local CPU. I don't remember if Linux does the same thing. If it does, the shared lib flags can probably be hacked, but I wouldn't recommend it. Approach (C) I don't think we should fork anything. So, I vote for (B).
Comment 23•16 years ago
|
||
If Red Hat's policy allows us to build libfreebl3.so with unresolved symbols, here is a variant of Bob's proposal that doesn't require changes to the lib/freebl source files. 1. Create a new .so for the NSPR and lib/util function stubs. Let's call it libnsprutilstubs.so for now. 2. Do not link libfreebl3.so with -lnssutil3 -lplc4 -lplds4 -lnspr4. To allow unresolved symbols, we need to remove the -Wl,-z,defs linker flag, too. The customers interested in a NSPR-free libfreebl3.so would link with -lnsprutilstubs instead of -lnssutil3 -lplc4 -lplds4 -lnspr4.
Comment 24•16 years ago
|
||
Replace PL_strlen by strlen in lib/freebl. This eliminates the direct dependency on libplc4.so and libplds4.so. For Mac OS X, we need the -dylib_file linker flag to help the direct dependency libnssutil3.dylib find its dependencies libplc4.dylib and libplds4.dylib.
Attachment #335131 -
Attachment is obsolete: true
Attachment #336110 -
Flags: review?(rrelyea)
Comment 25•16 years ago
|
||
Do we no longer care about WinCE?
Comment 26•16 years ago
|
||
Mozilla's WinCE port uses a "shunt" library, which provides the missing libc functions. lib/freebl is already using strlen before my patch. freebl3.dll right now imports more than 15 libc functions from msvcrt, so strlen is not the only one.
Comment 27•16 years ago
|
||
OK, the "shunt" library is evidently an addition since I last worked on the WinCE port of NSS. I wasn't thinking of strlen itself, particularly, but of the fact that WinCE's supply of libc functions that all Unix programmers take for granted is incomplete, at best. This effort to rid NSS of as much of NSPR as possible is surely going to unintentionally lead to calls to functions that are non-existent on some platforms.
Assignee | ||
Updated•16 years ago
|
Attachment #336110 -
Flags: review?(rrelyea) → review+
Assignee | ||
Comment 28•16 years ago
|
||
Comment on attachment 336110 [details] [diff] [review] Don't use PL_strlen in lib/freebl Nelson raised an objection in the comment. I want to make sure he feels the objection has been adequately addressed before we check this in.
Attachment #336110 -
Flags: superreview?(nelson)
Comment 29•16 years ago
|
||
Comment on attachment 336110 [details] [diff] [review] Don't use PL_strlen in lib/freebl If it is the case that all support platforms, including WinCE now support all the libc functions for which replacements are found in libplc4, and those libc functions work correctly on all those supported platforms, then I have no objection to ridding NSS of its dependence on libplc4. If this patch did only that, I would be happy with it. But this patch also rids freebl of libplds4, and it appears to me to be incomplete, at best. > -L$(NSPR_LIB_DIR) \ >- -lplc4 \ >- -lplds4 \ > -lnspr4 \ > $(NULL) > else # ! NS_USE_GCC > EXTRA_SHARED_LIBS += \ > $(DIST)/lib/nssutil3.lib \ >- $(NSPR_LIB_DIR)/$(NSPR31_LIB_PREFIX)plc4.lib \ >- $(NSPR_LIB_DIR)/$(NSPR31_LIB_PREFIX)plds4.lib \ > $(NSPR_LIB_DIR)/$(NSPR31_LIB_PREFIX)nspr4.lib \ > $(NULL) > endif # NS_USE_GCC >@@ -109,11 +105,13 @@ > -L$(DIST)/lib \ > -lnssutil3 \ > -L$(NSPR_LIB_DIR) \ >- -lplc4 \ >- -lplds4 \ It causes freebl to be linked without plds4. plds4 implements PLArenaPools. PLArenaPools are not in libc. This patch does not appear to offer any replacement for PLArenaPools. If you link freebl without plds4, you have no PLArenaPool support. Freebl depends heavily on PLArenaPools. http://mxr.mozilla.org/security/search?string=arena&find=freebl How will freebl work in the absence of code for PL_ArenaPools? Is there a missing part of this patch that supplies support for PLArenaPools, or that attempts to rid freebl of its dependence on PLArenaPools?
Updated•16 years ago
|
Attachment #336110 -
Flags: superreview?(nelson) → superreview-
Comment 30•16 years ago
|
||
Comment on attachment 336110 [details] [diff] [review] Don't use PL_strlen in lib/freebl In comment 29, I forgot to set the SR flag. If you can show me how freebl will work without any code to implement PLArenaPools, then I may reconsider my review of this patch.
Comment 31•16 years ago
|
||
Comment on attachment 336110 [details] [diff] [review] Don't use PL_strlen in lib/freebl >@@ -90,15 +90,11 @@ > -L$(DIST)/lib \ > -lnssutil3 \ > -L$(NSPR_LIB_DIR) \ >- -lplc4 \ >- -lplds4 \ > -lnspr4 \ > $(NULL) freebl uses PORT_NewArena, which is defined in libnssutil3. freebl doesn't call PL_InitArenaPool. Similarly for the other PLArenaPool functions. freebl uses the PORT_ArenaXXX and PORT_FreeArena functions defined in libnssutil3.
Comment 32•16 years ago
|
||
Comment on attachment 336110 [details] [diff] [review] Don't use PL_strlen in lib/freebl Wan-Teh, You're right that freebl's dependence on libplds4 is indirect, going through libutil, so freebl need not link directly with libplds4. So SR+ for this patch. However, I suspect this means that the "NSPR-Free" freebl will also be the "libUtil-free" freebl. This means that some replacement for libutil will probably be statically linked into this new libhash. So, to avoid having two implementations of the hash algorithms, we're going to have two implementations of libutil instead. :(
Attachment #336110 -
Flags: superreview- → superreview+
Comment 33•16 years ago
|
||
Yes, with the introduction of libnssutil3.so in NSS 3.12, "NSPR-free" freebl is really "NSPR and libnssutil3-free" freebl.
Comment 34•16 years ago
|
||
So, what is the plan? static linking of arenapool code into libhash?
Comment 35•16 years ago
|
||
Bob, Wan-Teh, A crucial component of the success of this project can be summarized as: "No Surpises". When the first patch to implement this is attached to this bug, it should contain only code that we're all expecting. There should be no surprises in it. That means that the proposed changes need to be clearly communicated up front, rather than after the big patch is written.
Comment 36•16 years ago
|
||
er, make that "So Surprises".
Summary: NSPR-free version of softoken (for hashing only). → NSPR-free and libutil-free version of softoken (for hashing only)
Assignee | ||
Comment 37•16 years ago
|
||
re: comment 32. yes, util functions are on the list of functions that are stubbed out. Most of them are stubbed out with 'failure' functions. In the 'patch for discussion' you will see stubs for NSPR, PLDS, PLC, and UTIL. the combination of making freebl the interface (rather than softoken) and Wan-Teh's patch reduces the list to only NSPR and UTIL, with several of those symbols going away as well. bob
Comment 38•16 years ago
|
||
I checked in the patch on the NSS trunk (NSS 3.12.2). I use PORT_Strlen instead of strlen and added 'const' to the typecasts on 'src' to make the code look similar to SHA1_Hash, SHA256_Hash, etc. Checking in config.mk; /cvsroot/mozilla/security/nss/lib/freebl/config.mk,v <-- config.mk new revision: 1.20; previous revision: 1.19 done Checking in md2.c; /cvsroot/mozilla/security/nss/lib/freebl/md2.c,v <-- md2.c new revision: 1.5; previous revision: 1.4 done Checking in md5.c; /cvsroot/mozilla/security/nss/lib/freebl/md5.c,v <-- md5.c new revision: 1.12; previous revision: 1.11 done
Attachment #336110 -
Attachment is obsolete: true
Assignee | ||
Updated•16 years ago
|
Priority: -- → P1
Whiteboard: FIPS
Assignee | ||
Comment 39•16 years ago
|
||
This patch implements the document passed around for the last month. I've incorporated some changes from comments by wtc. Some notes about the implementation. 1. Most of the files touched in this patch represent the stub header to map the real functions to their stub counterparts. Only the freebl.so files are mapped, the loader still requires NSPR. 2. The stubs are initialized whenever any applications calls to get the FreeBL vector. At this point, since the loader requires the requisite shared libraries, no further calls to the init function is needed. 3.The rest of the patch is made up of makefile changes and new files. 3a. stubs.h - maps the NSPR and Util functions to the stub version. 3b. stubs.c - implements the needed stub functions. those functions which are not needed simply return errors if NSPR is not loaded. With the changes already made, the list of functions that need to be stubbed out has dropped considerably. In addition, with only the need to support hashing, the number of stubs that actually need implementation has dropped as well. 3c. nsslowhash.h - the public header file to include the new hashing interface. 3d. nsslowhash.c - implementation of the new hash interface. This includes poweron-self tests for FIPS. (it may also require some audit stuff which is not in this patch). The integrity check is accomplished by calling the freebl BLAPI_VerifySelf() function. 3e. freebl_hash.def is an alternate export .def file which includes the hashing interface. bob
Attachment #325003 -
Attachment is obsolete: true
Assignee | ||
Updated•16 years ago
|
Attachment #345191 -
Flags: superreview?(wtc)
Attachment #345191 -
Flags: review?(nelson)
Comment 40•16 years ago
|
||
Comment on attachment 345191 [details] [diff] [review] freebl patch Preliminary comments: 1. I am very surprised to see that many "stub" functions simply fail if NSPR is not is use. Allocate functions return NULL, free functions leak. 2. I am even more surprised to see that, when NSPR is not in used, none of the functions sets any error codes (except to the extent that underlying libc functions do so). There's not a single assignment to errno in the whole body of code. Basically, the definition of this new API doesn't include any error codes at all. Code that fails without explaining why will quickly be hated by everyone. It will certainly not bring respect to NSS in Linux. I certainly don't want to have to answer questions like: Why did this function fail? This NSPR-free API MUST be defined to explicitly deal with (report) errors. At a minimum, every function that fails because NSPR is not in use needs to assign a meaningful value to errno.
Assignee | ||
Comment 41•16 years ago
|
||
re 1. This was intentional. Those functions where were not needed to do hashing should not be implemented. I have implementations for them, but including them would only introduce extra occasions. As for the Free functions, they are freeing objects that can only be obtained by the aformentioned allocate functions, all of which fail. re 2. That's a valid concern, which I have not problem with addressing.
Comment 42•16 years ago
|
||
Bob, The functions that always fail when NSPR is not in use should probably all assert and/or abort. If they are getting called, or ever do get called, when NSPR is not in use, that should not go unnoticed.
Assignee | ||
Comment 43•16 years ago
|
||
I have not problem with that. bob
Updated•16 years ago
|
Attachment #345191 -
Flags: review?(nelson) → review-
Comment 44•16 years ago
|
||
Comment on attachment 345191 [details] [diff] [review] freebl patch Additional comments, in random order (sorry) First block of comments: All about stubs.c 1. Organization (this part is just a wish, not a must) stubs.c seems to have 3 main parts: a. definitions and declarations of pointers. b. stub function implementations c. code to initialize all the pointers in part a. I wish the stuff in part c was up close to part a, before part b. Similarly, inside part a, we see these subparts in this order: p) definition of STUB_DECLARE which defines a pointer (or symbol) q) definitions of many STUB_SAFE macros that use that pointer/symbol r) definitino of STUB_FETCH_FUNCTION which initializes the pointer. here again, I wish part r was immediately after part p 2. Some functions look like they should be macros. freebl_getLibrary looks like it could be simply: #define freebl_getLibrary(libName) dlopen(libName, RTLD_LAZY|RTLD_NOLOAD) freebl_releaseLibrary looks like it could be #define freebl_releaseLibrary(lib) if (lib) dlclose(lib) or if you prefer #define freebl_releaseLibrary(lib) do {if (lib) dlclose(lib);} while (0) 3) function freebl_InitStubs belongs inline in FREEBL_InitStubs Unless I'm overlooking something, I see no benefit to having FREEBL_InitStubs be a function that only calls freebl_InitStubs. I think you could and should just move the body of freebl_InitStubs into the #ifdef FREEBL_NO_WEAK block in FREEBL_InitStubs, but I'm willing to be convinced otherwise if there's some reason. 4) leak in freebl_InitStubs(). The very last call to freebl_releaseLibrary(nssutil) needs to also call freebl_releaseLibrary(nspr) or else that reference to the nspr libs will be leaked. 5) at line 39, the definition #define WEAK __attribute__((weak)) seems like it obviously belongs down after the #else at line 78. IOW, that define should not be defined if FREEBL_NO_WEAK is defined. Any attempt to use the WEAK macro should result in a build failure if FREEBL_NO_WEAK is defined, I think. Second block: comments about stubs.h (numbers starting with 11) 11) When SOFTOKEN_NO_WEAK is defined, then UTIL_IS_LOADED() is defined as a test to see if a pointer is non-NULL. That pointer is declared, but not defined anywhere. I see no code anywhere that would ever define it (allocate space for it) or make it be non-NULL. 12) I see two alternative definitions of the macro UTIL_IS_LOADED, but no invocations of it. Maybe lines 49-55 are all just dead? 13) I definitely need to read up about this new weak attribute feature. It seems like it is definitely not the "weak symbol" linker feature that I first learned to use in SVR4 in 1990. Third block, freebl_hash.def, 20) Considering that this new lib didn't (and won't) exist in 3.11.x, I don't know why this file contains a section defining symbols for 3.11. 21) Is this shared lib ALSO going to export a symbol named FREEBL_GetVector? doesn't that create a conflict with the symbol by that name in freebl if and when libsoftoken or libfreebl is linked with this new libfreebl_hash? I must be missing part of the big picture here. 22) The new section of symbols named NSSRAWHASH_3.12 should probably be NSS 3.12.3 or 3.12.5 or something, since this wasn't in 3.12.0 I have one last piece left to review, which is nslowhash.c. I will get to it this week.
Assignee | ||
Comment 45•16 years ago
|
||
> 2. Some functions look like they should be macros. > #define freebl_getLibrary(libName) dlopen(libName, RTLD_LAZY|RTLD_NOLOAD) > #define freebl_releaseLibrary(lib) if (lib) dlclose(lib) > or > #define freebl_releaseLibrary(lib) do {if (lib) dlclose(lib);} while (0) that should be easy enough to add.. > 3) function freebl_InitStubs belongs inline in FREEBL_InitStubs Earlier incarnations of FREEBL_InitStubs did more. At that time it was easier to read if there where common activies in that function between the Weak and explicit symbol use. It's probably OK to make this change. > 4) leak in freebl_InitStubs(). Oh, duh... I was thinking I had to hang onto those handles or the library would unload, but I've opened the library LAZY/NO_LOAD so I can free those handles without unloading the library. > 5) at line 39, the definition > #define WEAK __attribute__((weak)) It was in use even in the 'NO_WEAK' case for the locking code. That code turned out not to be necessary as we never lock when doing hashing. Anyway there are some cases where it may still be used. > 11 & 12 UTIL_IS_LOADED(). I believe that macro is no longer necessary and can be deleted. > 13) It's a text 'weak' symbol. If there is no supplier at load time, then the text value is 'NULL'. Using it gives a different semantic then the LAZY/NOLOAD and symbol lookup option. Using weak symbols as the advantage of making the loader do all the work, the disadvantage is is the symbol isn't available at load time, it will never be available. > 20)/ 21) For Redhat this *is* freebl. It's not freebl_hash or any other name. the 3_11 symbol needs to match the existing symbol or the dependencies won't work correctly (and it needs to export the FREEBL_GetVector. I think you are confused with a different proposal that got rejected months ago. > 22 Seems reasonable, what version are we on now>
Comment 46•16 years ago
|
||
(In reply to comment #45) > > 4) leak in freebl_InitStubs(). > > Oh, duh... I was thinking I had to hang onto those handles or the library would > unload, but I've opened the library LAZY/NO_LOAD so I can free those handles > without unloading the library. Well, correct me if I'm wrong, but if the shared library was already loaded before this function was called, then dlclosing these handles will not unload it. But if this function actually loaded the shared lib into memory, then it is appropriate to unload it in this function upon failure, no? > > 5) at line 39, the definition > > #define WEAK __attribute__((weak)) > > It was in use even in the 'NO_WEAK' case for the locking code. That code turned > out not to be necessary as we never lock when doing hashing. Anyway there are > some cases where it may still be used. Please point out those cases. I didn't find any uses of WEAK except in the non-NO_WEAK case. But maybe I overlooked some macro expansion. Actually, given that this patch unconditionally defines the NO_WEAK symbol, is there any need for the ifdef'ed code? Do we really want to commit dead code in the initial checkin? > > 20)/ 21) > > For Redhat this *is* freebl. It's not freebl_hash or any other name. the 3_11 > symbol needs to match the existing symbol or the dependencies won't work > correctly (and it needs to export the FREEBL_GetVector. I see. So, this is an alternate .def file to the one used on other platforms? I seem to recall that there are platforms (though I don't remember which ones) on which the name given before the '{' is meaningful to the platform linker or loader. Is it a break of binary compatibility to change that name now? > I think you are confused with a different proposal that got rejected months > ago. No doubt. :)
Comment 47•16 years ago
|
||
I'm changing the bug summary, because this really is a change to freebl, not softoken.
Summary: NSPR-free and libutil-free version of softoken (for hashing only) → Free Freebl hashing code of dependencies on NSPR and libUtil
Comment 48•16 years ago
|
||
Comment on attachment 345191 [details] [diff] [review] freebl patch Here are my final comments on this patch. These comments are about nsslowhash.[ch]. These comments complete my review of this patch. 1. Naming conventions. (subtitle: We only get one chance to make a good first impression.) As you know, the NSS naming convention for public functions and type names For private names, we have strayed far from that convention at times, but for public names, I believe we have followed that convention pretty carefully. The convention for public symbols has always been to combine a prefix with a suffix. The prefix is always ALLCAPS with no underscores. The Suffix is MixedCase with no underscores and starts with a capital. For function names the prefix and suffix are combined with an intervening _ character, e.g. PREFIX_Suffix, and for type names, they are merely concatenated, e.g. PREFIXSuffix. I would like to see this new public interface continue to follow those same conventions. To that end, I would propose that the PREFIX be changed from NSS_LOWHASH to NSSLOWHASH or even simply NSSHASH. (I don't think we'll confuse the old HASH prefix with the new NSSHASH prefix.) And I would propose that *all* suffixes be converted to MixedCase without any underscores. So I propose these changes: NSS_LOWHASH_CONTEXT => NSSHASHContext NSS_LOW_INIT_CONTEXT => NSSHASHInitContext NSS_LOWHASH_Begin => NSSHASH_Begin (and similarly for all the NSS_LOWHASH functions) NSS_LOW_Init => NSSHASH_Init or NSSLOW_Init NSS_LOW_Shutdown => NSSHASH_Shutdown or NSSLOW_Shutdown I'd like to see all the new symbols use the same prefix, rather than having two new prefixes, NSSHASH (or NSSLOWHASH) and NSSLOW. 2. Comments on NSS_LOW_INIT_CONTEXT. 2a: apparently immutable, but not "const". It appears to me that the value placed into this context is set when the structure is allocated, but is never changed after that. Should we ensure that by declaring the Init function to return a "const NSS_LOW_INIT_CONTEXT *" instead of a "NSS_LOW_INIT_CONTEXT *" ? 2b: allocated, but evidently immutable and not really context dependent. I wonder: is there any reason this structure couldn't be static, rather than allocated, and have the Init function simply return the address of the one and only static copy? Would multiple copies in the same process have different values for it? Could we have two such static const structures, one containing PR_TRUE and one containing PR_FALSE, and return the address of one or the other? 2c: unused. Besides being created and destroyed, the only other function that is apparently intended to use it is NSS_LOWHASH_NewContext. New function NSS_LOWHASH_NewContext() takes two arguments, the first of which is NSS_LOW_INIT_CONTEXT *initContext. But it never uses that argument. So, really, what purpose does that argument serve? Do we need it? Is it a placeholder, in case we ever think up a good need for it?
Comment 49•16 years ago
|
||
I found the answers to some of my questions in an email Bob sent me. > a. I added a handle returned from NSS_LOWHASH_Init. This handle can be > used to manage more than one initialization call. Currently it's nothing > more than a malloc and free and serves no existing purpose. It's mainly a > 'future proof' function in case per init state information is needed in > the future. I can remove it with no loss to the current API. We ought to take steps to ensure that people MUST use this new handle correctly, or else get rid of it. As long as we ignore its value altogether, coders will do things like pass NULL. Then later, if we try to make a use of it, and begin to return an error if it's not a valid handle, they will complain that we've broken their working code and are not binary compatible. We've been burned by this sort of thing before, more than once. So, we must either (a) make sure they use it correctly from day one, or (b) get rid of it. As long as the handle is opaque, I think we could initially return the address of a static value, and have the "destroy" function check that it's the right address and panic if not. That still gives us the freedom to later change it to be allocated, etc. > b. The API used unsigned int as a length not a PRUint32. I did this for > the following reasons: 1) the length size should be platform specific > rather than a fixed 32bit value (it really should be a Unix size_t, I'm not convinced of that. The size limits of the data that the hash and cipher algorithms can take are (in most cases) defined by the algorithm itself. Also note that NSS makes much internal use of SECItem, whose length is an unsigned int. If you really think that an algorithm should be able to handle more than 4GB of data in one gulp, then change the size declaration of a PRUint64, not to a size_t. But check first to make sure that all the algorithms can handle a length longer than 32-bite.
Comment 50•16 years ago
|
||
Nelson, I don't understand your objection to the use of size_t for the size of a memory buffer. But I don't care about this issue strongly enough to engage in a debate. I agree with your suggestion that the new function names should follow our naming convention XXX_Foo. They should not be named XXX_YYY_Foo, which is the naming convention of "Stan" code and only used in lib/smime.
Assignee | ||
Comment 51•16 years ago
|
||
> 1. Naming conventions. I don't have a problem with NSSLOWHASH in all those places. and NSSLOW for the init/shutdown. I originally had the same prefix everywhere in the design, but changed it in response to wtc's comments. I think I prefer NSSLOW_Init, since, in theory, if we ever have to expand to more than hashing, it won't look like an historical dreg. > 2. Comments on NSS_LOW_INIT_CONTEXT. Answered in the email... as found in your comment 49. I don't have a problem with returning a constant value and verifying that that value has been properly passed in to us. >> b. The API used unsigned int > I'm not convinced of that. As I said in the email, I don't have a problem changing it. I went with it mostly for consistency with other NSS definitions for the same function. My big concern was the potential size differences between PRInt32 and int on various platforms (since almost all the other API's, both internal and external, in NSS use int). Anyway I was hoping for that feedback at the design stage (which is why I put the full protoypes out in the design). Still I'm not against changing them at this stage. bob
Assignee | ||
Comment 52•16 years ago
|
||
>> Oh, duh... I was thinking > Well, correct me if I'm wrong No, you are right. the "Oh, duh" was on my part, not yours. (that is I agree with your assessement that this needs to be fixed). >> It was in use even in the 'NO_WEAK' case for the locking code > Please point out those cases. It was an earlier version of the code. I initially had it as part of the ifdef and moved it out. That code is now gone, but it seems that it's previous existence speaks to the fact that is is 'useful' or 'permissible' to use WEAK symbols other than in the case of the define. > I seem to recall that there are platforms (though I don't remember which ones) > on which the name given before the '{' is meaningful to the platform linker > or loader. Is it a break of binary compatibility to change that name now? Yes, Linux is one. Solaris is possibly (probably) another:). That is why I didn't change the name. All the old symbols have the old names. New symbols have new names. The format of these names, though, are just convention. Packagers read these names and build dependency lists for packages to install. This is how you get a package with a new NSS binary which use new APIs to pull the new NSS package when it gets installed.
Comment 53•16 years ago
|
||
Comment on attachment 345191 [details] [diff] [review] freebl patch Bob, I will wait until you have addressed Nelson's review comments and review the next version of this patch. In comment 49, Nelson made a good point that NSS uses SECItem a lot to point to memory buffers. So it's a good idea for this new freebl API to specify a memory buffer with 'unsigned char *' and 'unsigned int'. I still like size_t, so that 64-bit platforms can "be all they can be". But CAPI and CNG also use unsigned 32-bit integer for the buffer length: http://msdn.microsoft.com/en-us/library/aa380202(VS.85).aspx http://msdn.microsoft.com/en-us/library/aa375468(VS.85).aspx So using 'unsigned int' is fine. >+void NSS_LOWHASH_Update(NSS_LOWHASH_CONTEXT *context, >+ const unsigned char *buf, >+ int len); >+void NSS_LOWHASH_End(NSS_LOWHASH_CONTEXT *context, >+ unsigned char *buf, >+ unsigned int *ret, int len); The 'len' parameter of these two functions should be 'unsigned int'.
Attachment #345191 -
Flags: superreview?(wtc)
Assignee | ||
Comment 54•16 years ago
|
||
So my question is, should it be unsigned int or PRUint32? I'm quite sure given the our current public and internal APIs that it should not be size_t now. I think that's the only question that needs to be nailed down for me to complete a new patch, unless nelson has any issues with comment 52. bob
Comment 55•16 years ago
|
||
I would use 'unsigned int' to match SECItem and the current freebl functions.
Assignee | ||
Comment 56•16 years ago
|
||
Updated based on review comments.
Attachment #345191 -
Attachment is obsolete: true
Assignee | ||
Comment 57•16 years ago
|
||
One of the comments was to force asserts in unimplemented functions. This identified one case where we were calling these functions in the test cases. This patch adjusts that problem (blapitest was failing since it didn't load libnssutil3.so).
Assignee | ||
Updated•16 years ago
|
Attachment #348225 -
Flags: review?(nelson)
Assignee | ||
Updated•16 years ago
|
Attachment #348226 -
Flags: review?(nelson)
Comment 58•16 years ago
|
||
Comment on attachment 348226 [details] [diff] [review] Build update. (checked in) Whoa! This means that 3.12.0 was still linking the cmds with static libutil, right? Ouch! Good catch! r+
Attachment #348226 -
Flags: review?(nelson) → review+
Comment 59•16 years ago
|
||
Comment on attachment 348225 [details] [diff] [review] Updated patch from comments Rats! I SO wanted to give this patch r+. It's 99.8% of the way. But I found correctness issues in the new FREEBL_InitStubs. I think this patch is good to go, except for these issues. >+extern SECStatus >+FREEBL_InitStubs() >+{ >+ SECStatus rv = SECSuccess; >+#ifdef FREEBL_NO_WEAK >+ void *nspr = NULL; >+ void *nssutil = NULL; >+ >+ /* NSPR should be first */ >+ if (!ptr_PR_DestroyLock) { >+ void *nspr = freebl_getLibrary(nsprLibName); That second definition of a variable named nspr means that the outer definition will always be NULL. I think you meant to code simply: + nspr = freebl_getLibrary(nsprLibName); >+ if (!nspr) { >+ return SECFailure; >+ } >+ rv = freebl_InitNSPR(nspr); >+ freebl_releaseLibrary(nspr); I'm pretty sure you don't want to release nspr here. In the previous patch, that release was down inside the following if. Is that where it belongs? >+ if (rv != SECSuccess) { >+ return rv; >+ } >+ } >+ /* now load NSSUTIL */ >+ if (!ptr_SECITEM_ZfreeItem_Util) { >+ nssutil= freebl_getLibrary(nssutilLibName); >+ if (!nssutil) { >+ freebl_releaseLibrary(nspr); Because of the problems mentioned above, the value of nspr here will either be NULL, or will be the value that has previously already been passed to freebl_releaseLibrary, making this a double free. >+ return SECFailure; >+ } >+ rv = freebl_InitNSSUtil(nssutil); >+ freebl_releaseLibrary(nssutil); >+ if (rv != SECSuccess) { >+ return rv; >+ } >+ } >+#endif >+ >+ return rv; >+}
Attachment #348225 -
Flags: review?(nelson) → review-
Assignee | ||
Comment 60•16 years ago
|
||
Attachment #348225 -
Attachment is obsolete: true
Attachment #348594 -
Flags: review?(nelson)
Comment 61•16 years ago
|
||
Comment on attachment 348226 [details] [diff] [review] Build update. (checked in) r=wtc. Bob, Nelson, are you sure that USE_STATIC_LIBS = 1 should not apply to libnssutil?
Attachment #348226 -
Flags: review+
Comment 62•16 years ago
|
||
Comment on attachment 348594 [details] [diff] [review] Fix reference leak/double free issue. (checked in) Bob, I'm still reviewing this patch. I will give you my comments on a hard copy. Here is a quick comment: in lib/freebl/config.mk, you added OS_PTHREAD= But there is no make variable by that name. I believe you meant USE_PTHREADS= I'm not sure if it works though. You should use the "ldd libfreebl3.so" command to verify that libfreebl3.so doesn't depend on libpthread.so.
Comment 63•16 years ago
|
||
Comment on attachment 348226 [details] [diff] [review] Build update. (checked in) (In reply to comment #61) > are you sure that USE_STATIC_LIBS = 1 should not apply to libnssutil? Thank you for asking that question, Wan-Teh. When I first reviewed that patch, I did not review enough context in the file to notice that these changes are in the part for USE_STATIC_LIBS. Dunno how I missed that. But now that I see that it is part of the USE_STATIC_LIBS case, I am *NOT* sure that libnssutil should be an exception for USE_STATIC_LIBS. I am inclined to undo my r+ and think about it some more.
Attachment #348226 -
Flags: review+ → review?
Comment 64•16 years ago
|
||
Comment on attachment 348594 [details] [diff] [review] Fix reference leak/double free issue. (checked in) (In reply to comment #60) > Created an attachment (id=348594) [details] > Fix reference leak/double free issue. Bob, this new patch does much more than what your comment said. It changes a different set of files! Please explain all the differences between this patch and the previous one.
Assignee | ||
Comment 65•16 years ago
|
||
When you used interdiff, make sure you selected 'Update patch from comments' not 'Build update'
Assignee | ||
Comment 66•16 years ago
|
||
re libnssutil3.so.... So it's already required, since libfreebl3.so is dynamically loaded. (which is where I was running into the problem). bob (NOTE: BTW, it's almost to the point where USE_STATIC_LIBS can be reduced to the freebl test programs)
Comment 67•16 years ago
|
||
Please ignore my comment 64. I wrote: > It changes a different set of files! No, it doesn't. But the bugzilla interdiff tool says it does. I downloaded both patches and compared them side by side to see the real diffs. I will write a better review comment shortly.
Comment 68•16 years ago
|
||
USE_STATIC_LIBS is also required for the various database test and analysis programs, e.g. dbck, which I still use and maintain, even though we don't build them regularly.
Assignee | ||
Comment 69•16 years ago
|
||
re comment 62. No, thre is an OS_THREADS variable which puts the pthread library on the link line. USE_PTHREADS did not seem to take it off. bob
Assignee | ||
Comment 70•16 years ago
|
||
The dbtest stuff still loads freebl, and thus libutil. BTW if we still use dbck, we need to make sure it's built by default. It currently does not build on NSS 3.12. bob
Comment 71•16 years ago
|
||
Comment on attachment 348594 [details] [diff] [review] Fix reference leak/double free issue. (checked in) Bob, ignoring the error handling code, in the latest patch, the new function FREEBL_InitStubs now does this: >+ /* NSPR should be first */ >+ if (!ptr_PR_DestroyLock) { >+ nspr = freebl_getLibrary(nsprLibName); >+ rv = freebl_InitNSPR(nspr); >+ freebl_releaseLibrary(nspr); >+ } >+ /* now load NSSUTIL */ >+ if (!ptr_SECITEM_ZfreeItem_Util) { >+ nssutil= freebl_getLibrary(nssutilLibName); >+ rv = freebl_InitNSSUtil(nssutil); >+ freebl_releaseLibrary(nssutil); >+ } That is: it loads each library, gets the symbol addresses, and then immediately unloads the library. Is that what you intended?
Assignee | ||
Comment 72•16 years ago
|
||
Yes, The load library only gets a reference to an already loaded library. (RT_LAZY|RT_NOLOAD). So we simply get a reference, lookup the addresses and release that reference. bob
Updated•16 years ago
|
Attachment #348594 -
Flags: review?(nelson) → review+
Comment 73•16 years ago
|
||
Comment on attachment 348594 [details] [diff] [review] Fix reference leak/double free issue. (checked in) OK, so it's not your intent to actually ever load either shared lib in this function, but only to get handles to loaded libs. In that case, remove this comment: >+ * if force is set, explicitly load the libraries if they are not already >+ * loaded. If we could not use the real libraries, return failure. Also, since you never need to have more than one handle at any time, I suggest you move the definitions of the nspr and nssutil pointers down into the blocks that use them. i.e. delete the two definitions that exist now at function scope, and change the lines that make the two calls to freebl_getLibrary to define those variables there. then r=nelson >+extern SECStatus >+FREEBL_InitStubs() >+{ >+ SECStatus rv = SECSuccess; >+#ifdef FREEBL_NO_WEAK >+ void *nspr = NULL; >+ void *nssutil = NULL; >+ >+ /* NSPR should be first */ >+ if (!ptr_PR_DestroyLock) { >+ nspr = freebl_getLibrary(nsprLibName); >+ if (!nspr) { >+ return SECFailure; >+ } >+ rv = freebl_InitNSPR(nspr); >+ freebl_releaseLibrary(nspr); >+ if (rv != SECSuccess) { >+ return rv; >+ } >+ } >+ /* now load NSSUTIL */ >+ if (!ptr_SECITEM_ZfreeItem_Util) { >+ nssutil= freebl_getLibrary(nssutilLibName); >+ if (!nssutil) { >+ return SECFailure; >+ } >+ rv = freebl_InitNSSUtil(nssutil); >+ freebl_releaseLibrary(nssutil); >+ if (rv != SECSuccess) { >+ return rv; >+ } >+ } >+#endif >+ >+ return rv; >+} >Index: stubs.h >=================================================================== >RCS file: stubs.h >diff -N stubs.h >--- /dev/null 1 Jan 1970 00:00:00 -0000 >+++ stubs.h 17 Nov 2008 18:47:32 -0000 >@@ -0,0 +1,51 @@ >+/* >+ * Allow freebl and softoken to be loaded without util or NSPR. >+ * >+ * These symbols are overridden once real NSPR, and libutil are attached. >+ */ >+ >+#ifndef _STUBS_H >+#define _STUBS_H_ 1 >+ >+#ifdef _LIBUTIL_H_ >+/* must be included before util */ >+/*#error stubs.h included too late */ >+#define MP_DIGITES(x) "stubs included too late" >+#endif >+ >+/* hide libutil rename */ >+#define _LIBUTIL_H_ 1 >+ >+#define PORT_Alloc PORT_Alloc_stub >+#define PORT_ArenaZAlloc PORT_ArenaZAlloc_stub >+#define PORT_Free PORT_Free_stub >+#define PORT_FreeArena PORT_FreeArena_stub >+#define PORT_GetError PORT_GetError_stub >+#define PORT_NewArena PORT_NewArena_stub >+#define PORT_SetError PORT_SetError_stub >+#define PORT_ZAlloc PORT_ZAlloc_stub >+#define PORT_ZFree PORT_ZFree_stub >+ >+#define SECITEM_AllocItem SECITEM_AllocItem_stub >+#define SECITEM_CompareItem SECITEM_CompareItem_stub >+#define SECITEM_CopyItem SECITEM_CopyItem_stub >+#define SECITEM_FreeItem SECITEM_FreeItem_stub >+#define SECITEM_ZfreeItem SECITEM_ZfreeItem_stub >+ >+#define PR_Assert PR_Assert_stub >+#define PR_CallOnce PR_CallOnce_stub >+#define PR_Close PR_Close_stub >+#define PR_DestroyLock PR_DestroyLock_stub >+#define PR_Free PR_Free_stub >+#define PR_GetLibraryFilePathname PR_GetLibraryFilePathname_stub >+#define PR_Lock PR_Lock_stub >+#define PR_NewLock PR_NewLock_stub >+#define PR_Open PR_Open_stub >+#define PR_Read PR_Read_stub >+#define PR_Seek PR_Seek_stub >+#define PR_Sleep PR_Sleep_stub >+#define PR_Unlock PR_Unlock_stub >+ >+extern int FREEBL_InitStubs(void); >+ >+#endif >Index: sysrand.c >=================================================================== >RCS file: /cvsroot/mozilla/security/nss/lib/freebl/sysrand.c,v >retrieving revision 1.4 >diff -u -p -r1.4 sysrand.c >--- sysrand.c 22 Aug 2008 01:33:02 -0000 1.4 >+++ sysrand.c 17 Nov 2008 18:47:32 -0000 >@@ -34,6 +34,10 @@ > * > * ***** END LICENSE BLOCK ***** */ > >+#ifdef FREEBL_NO_DEPEND >+#include "stubs.h" >+#endif >+ > #include "seccomon.h" > #if defined(XP_UNIX) || defined(XP_BEOS) > #include "unix_rand.c" >Index: tlsprfalg.c >=================================================================== >RCS file: /cvsroot/mozilla/security/nss/lib/freebl/tlsprfalg.c,v >retrieving revision 1.5 >diff -u -p -r1.5 tlsprfalg.c >--- tlsprfalg.c 5 Nov 2005 01:00:14 -0000 1.5 >+++ tlsprfalg.c 17 Nov 2008 18:47:32 -0000 >@@ -37,10 +37,15 @@ > * ***** END LICENSE BLOCK ***** */ > /* $Id: tlsprfalg.c,v 1.5 2005/11/05 01:00:14 wtchang%redhat.com Exp $ */ > >+#ifdef FREEBL_NO_DEPEND >+#include "stubs.h" >+#endif >+ > #include "sechash.h" > #include "alghmac.h" > #include "blapi.h" > >+ > #define PHASH_STATE_MAX_LEN SHA1_LENGTH > > /* TLS P_hash function */
Updated•16 years ago
|
Attachment #348226 -
Flags: review? → review+
Comment 74•16 years ago
|
||
Comment on attachment 348226 [details] [diff] [review] Build update. (checked in) I'm going to reinstate my r+ for this patch. If this is a problem for dbck, I'll just have to deal with it, if and when I finally get around to getting dbck to build with NSS 3.12.
Comment 75•16 years ago
|
||
Comment on attachment 348594 [details] [diff] [review] Fix reference leak/double free issue. (checked in) r=wtc. I wrote my review comments on a hard copy of the patch and gave it to Bob. I suggest that Bob check this patch in first and address my review comments in a follow-up patch.
Attachment #348594 -
Flags: review+
Comment 76•16 years ago
|
||
(In reply to comment #69) > re comment 62. No, thre is an OS_THREADS variable which puts the pthread > library on the link line. USE_PTHREADS did not seem to take it off. I see. OS_THREAD is an internal variable of coreconf. Unsetting OS_THREAD is fine for now. Ideally we should add a new make variable to turn off pthread dependency, or just eliminate the pthread dependency from the shared libraries on Linux. (I remember that's the direction you're going.) So it's fine to compile with -D_REENTRANT but not link with -lpthread on Linux?
Assignee | ||
Comment 77•16 years ago
|
||
Checking in on carpool 3... Checking in Makefile; /cvsroot/mozilla/security/nss/lib/freebl/Makefile,v <-- Makefile new revision: 1.94; previous revision: 1.93 done Checking in aeskeywrap.c; /cvsroot/mozilla/security/nss/lib/freebl/aeskeywrap.c,v <-- aeskeywrap.c new revision: 1.5; previous revision: 1.4 done Checking in alg2268.c; /cvsroot/mozilla/security/nss/lib/freebl/alg2268.c,v <-- alg2268.c new revision: 1.8; previous revision: 1.7 done Checking in alghmac.c; /cvsroot/mozilla/security/nss/lib/freebl/alghmac.c,v <-- alghmac.c new revision: 1.7; previous revision: 1.6 done Checking in arcfive.c; /cvsroot/mozilla/security/nss/lib/freebl/arcfive.c,v <-- arcfive.c new revision: 1.6; previous revision: 1.5 done Checking in arcfour.c; /cvsroot/mozilla/security/nss/lib/freebl/arcfour.c,v <-- arcfour.c new revision: 1.18; previous revision: 1.17 done Checking in camellia.c; /cvsroot/mozilla/security/nss/lib/freebl/camellia.c,v <-- camellia.c new revision: 1.2; previous revision: 1.1 done Checking in config.mk; /cvsroot/mozilla/security/nss/lib/freebl/config.mk,v <-- config.mk new revision: 1.21; previous revision: 1.20 done Checking in desblapi.c; /cvsroot/mozilla/security/nss/lib/freebl/desblapi.c,v <-- desblapi.c new revision: 1.7; previous revision: 1.6 done Checking in dh.c; /cvsroot/mozilla/security/nss/lib/freebl/dh.c,v <-- dh.c new revision: 1.8; previous revision: 1.7 done Checking in dsa.c; /cvsroot/mozilla/security/nss/lib/freebl/dsa.c,v <-- dsa.c new revision: 1.19; previous revision: 1.18 done Checking in ec.c; /cvsroot/mozilla/security/nss/lib/freebl/ec.c,v <-- ec.c new revision: 1.20; previous revision: 1.19 done RCS file: /cvsroot/mozilla/security/nss/lib/freebl/freebl_hash.def,v done Checking in freebl_hash.def; /cvsroot/mozilla/security/nss/lib/freebl/freebl_hash.def,v <-- freebl_hash.def initial revision: 1.1 done Checking in ldvector.c; /cvsroot/mozilla/security/nss/lib/freebl/ldvector.c,v <-- ldvector.c new revision: 1.18; previous revision: 1.17 done Checking in manifest.mn; /cvsroot/mozilla/security/nss/lib/freebl/manifest.mn,v <-- manifest.mn new revision: 1.53; previous revision: 1.52 done Checking in md2.c; /cvsroot/mozilla/security/nss/lib/freebl/md2.c,v <-- md2.c new revision: 1.6; previous revision: 1.5 done Checking in md5.c; /cvsroot/mozilla/security/nss/lib/freebl/md5.c,v <-- md5.c new revision: 1.13; previous revision: 1.12 done RCS file: /cvsroot/mozilla/security/nss/lib/freebl/nsslowhash.c,v done Checking in nsslowhash.c; /cvsroot/mozilla/security/nss/lib/freebl/nsslowhash.c,v <-- nsslowhash.c initial revision: 1.1 done RCS file: /cvsroot/mozilla/security/nss/lib/freebl/nsslowhash.h,v done Checking in nsslowhash.h; /cvsroot/mozilla/security/nss/lib/freebl/nsslowhash.h,v <-- nsslowhash.h initial revision: 1.1 done Checking in pqg.c; /cvsroot/mozilla/security/nss/lib/freebl/pqg.c,v <-- pqg.c new revision: 1.16; previous revision: 1.15 done Checking in prng_fips1861.c; /cvsroot/mozilla/security/nss/lib/freebl/prng_fips1861.c,v <-- prng_fips1861.c new revision: 1.28; previous revision: 1.27 done Checking in rawhash.c; /cvsroot/mozilla/security/nss/lib/freebl/rawhash.c,v <-- rawhash.c new revision: 1.5; previous revision: 1.4 done Checking in rijndael.c; /cvsroot/mozilla/security/nss/lib/freebl/rijndael.c,v <-- rijndael.c new revision: 1.21; previous revision: 1.20 done Checking in rsa.c; /cvsroot/mozilla/security/nss/lib/freebl/rsa.c,v <-- rsa.c new revision: 1.38; previous revision: 1.37 done Checking in sha512.c; /cvsroot/mozilla/security/nss/lib/freebl/sha512.c,v <-- sha512.c new revision: 1.12; previous revision: 1.11 done Checking in sha_fast.c; /cvsroot/mozilla/security/nss/lib/freebl/sha_fast.c,v <-- sha_fast.c new revision: 1.13; previous revision: 1.12 done Checking in shvfy.c; /cvsroot/mozilla/security/nss/lib/freebl/shvfy.c,v <-- shvfy.c new revision: 1.11; previous revision: 1.10 done RCS file: /cvsroot/mozilla/security/nss/lib/freebl/stubs.c,v done Checking in stubs.c; /cvsroot/mozilla/security/nss/lib/freebl/stubs.c,v <-- stubs.c initial revision: 1.1 done RCS file: /cvsroot/mozilla/security/nss/lib/freebl/stubs.h,v done Checking in stubs.h; /cvsroot/mozilla/security/nss/lib/freebl/stubs.h,v <-- stubs.h initial revision: 1.1 done Checking in sysrand.c; /cvsroot/mozilla/security/nss/lib/freebl/sysrand.c,v <-- sysrand.c new revision: 1.5; previous revision: 1.4 done Checking in tlsprfalg.c; /cvsroot/mozilla/security/nss/lib/freebl/tlsprfalg.c,v <-- tlsprfalg.c new revision: 1.6; previous revision: 1.5 done bobs-laptop(64) cvs commit cmd/platlibs.mk lib/util/nssutil.def Checking in cmd/platlibs.mk; /cvsroot/mozilla/security/nss/cmd/platlibs.mk,v <-- platlibs.mk new revision: 1.58; previous revision: 1.57 done Checking in lib/util/nssutil.def; /cvsroot/mozilla/security/nss/lib/util/nssutil.def,v <-- nssutil.def new revision: 1.7; previous revision: 1.6 done
Comment 78•16 years ago
|
||
Bob, Are all the previously r+ patches checked in now? Is there more work to be done for this bug? If all previously r+ patches are checked in, and there is no more work to be done for this bug, please resolve it as fixed. If all previously r+ patches are checked in, but there remains work to be done, please change the names of the checked in patches to include the words "checked in", and add a comment explaining what else needs to be done before this bug will be resolved. Thanks.
Target Milestone: 3.12.1 → 3.12.3
Assignee | ||
Updated•16 years ago
|
Attachment #348226 -
Attachment description: Build update. → Build update. (checked in)
Assignee | ||
Updated•16 years ago
|
Attachment #348594 -
Attachment description: Fix reference leak/double free issue. → Fix reference leak/double free issue. (checked in)
Assignee | ||
Comment 79•16 years ago
|
||
Yes, the checkin logs were already attached to the bug. I was waiting for the tinderbox cycle before I closed the bug....
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Comment 80•16 years ago
|
||
Bob, why don't you set FREEBL_NO_DEPEND=1 by default on Linux?
Comment 81•16 years ago
|
||
This patch implements most of my suggested changes.
Attachment #349110 -
Flags: review?(rrelyea)
Assignee | ||
Comment 82•16 years ago
|
||
I considered it a platform option. My plan was to set it in the rpm. I see no reason for the option outside of redhat's system NSS build. (Mozilla, for instance, doesn't need it). bob
Comment 83•16 years ago
|
||
While testing this patch under all build configurations (FREEBL_NO_DEPEND, FREEBL_NO_WEAK), I found that PR_Unlock's stub should return PRStatus instead of void.
Attachment #349110 -
Attachment is obsolete: true
Attachment #349449 -
Flags: review?(rrelyea)
Attachment #349110 -
Flags: review?(rrelyea)
Comment 84•16 years ago
|
||
(In reply to comment #82) > I considered it a platform option. My plan was to set it in the rpm. I see no > reason for the option outside of redhat's system NSS build. (Mozilla, for > instance, doesn't need it). If FREEBL_NO_DEPEND will be set in the Linux build that will be FIPS validated, most applications, including Mozilla, will need to set FREEBL_NO_DEPEND so that they can claim FIPS conformance. If so, we should just set FREEBL_NO_DEPEND on Linux in coreconf or in lib/freebl/Makefile.
Assignee | ||
Comment 85•16 years ago
|
||
Comment on attachment 349449 [details] [diff] [review] Supplemental patch v2 r- 1. The weak symbol... I've left it outside the ifdef's on purpose, see comments. (Note, this is just a preference, If this were the only issue, I would have given the patch an r+). 2. The real reason for r-, ldvector.c and stubs.h. The test in stubs.h are there to make sure we don't include the stubs after it's impossible for them to take effect. In the case of ldvector, you don't actually want the stubs to take effect, as ldvector will not link with the stubs.c file. In short, ldvector should not include stubs.h (this is why I didn't do this initially). Moving the declaration to a different .h file would be OK. The rest of the patch is fine. If you want check those in (including #1) consider this comment as an implicit r+. bob
Attachment #349449 -
Flags: review?(rrelyea) → review-
Comment 86•16 years ago
|
||
I checked in this patch on the NSS trunk (NSS 3.12.3) with Bob's implicit r+. I didn't move the definition of the WEAK macro into the ifdef, and I omitted the changes to ldvector.c and stubs.h. I now understand why ldvector.c should not include stubs.h. But I still think my proposed changes to stubs.h are correct. I verified that utilrename.h and stubs.h can be included in either order with my changes to stubs.h because we do #define PORT_Alloc_Util PORT_Alloc_stub instead of #define PORT_Alloc PORT_Alloc_stub
Attachment #349449 -
Attachment is obsolete: true
You need to log in
before you can comment on or make changes to this bug.
Description
•