Last Comment Bug 438870 - Free Freebl hashing code of dependencies on NSPR and libUtil
: Free Freebl hashing code of dependencies on NSPR and libUtil
Status: RESOLVED FIXED
FIPS
:
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: 3.12
: x86 Linux
: P1 normal (vote)
: 3.12.3
Assigned To: Robert Relyea
:
Mentors:
Depends on: 497251
Blocks: FIPS2008
  Show dependency treegraph
 
Reported: 2008-06-12 10:25 PDT by Robert Relyea
Modified: 2009-08-10 16:40 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch for discussion (90.87 KB, patch)
2008-06-13 11:51 PDT, Robert Relyea
no flags Details | Diff | Review
Build a stubs .so. Link libfreebl3.so with unresolved symbols. (956 bytes, patch)
2008-08-22 16:54 PDT, Wan-Teh Chang
no flags Details | Diff | Review
Don't use PL_strlen in lib/freebl (2.25 KB, patch)
2008-08-29 14:23 PDT, Wan-Teh Chang
rrelyea: review+
nelson: superreview+
Details | Diff | Review
Don't use PL_strlen in lib/freebl (as checked in) (2.82 KB, patch)
2008-10-03 12:42 PDT, Wan-Teh Chang
no flags Details | Diff | Review
freebl patch (46.39 KB, patch)
2008-10-28 16:25 PDT, Robert Relyea
nelson: review-
Details | Diff | Review
Updated patch from comments (46.00 KB, patch)
2008-11-14 11:33 PST, Robert Relyea
nelson: review-
Details | Diff | Review
Build update. (checked in) (1.94 KB, patch)
2008-11-14 11:35 PST, Robert Relyea
nelson: review+
wtc: review+
Details | Diff | Review
Fix reference leak/double free issue. (checked in) (45.95 KB, patch)
2008-11-17 10:50 PST, Robert Relyea
nelson: review+
wtc: review+
Details | Diff | Review
Supplemental patch (6.34 KB, patch)
2008-11-19 19:22 PST, Wan-Teh Chang
no flags Details | Diff | Review
Supplemental patch v2 (10.69 KB, patch)
2008-11-21 10:28 PST, Wan-Teh Chang
rrelyea: review-
Details | Diff | Review
Supplemental patch v3 (checked in) (6.61 KB, patch)
2008-11-27 07:40 PST, Wan-Teh Chang
no flags Details | Diff | Review

Description Robert Relyea 2008-06-12 10:25:10 PDT
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).
Comment 1 Robert Relyea 2008-06-12 10:32:49 PDT
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 Nelson Bolyard (seldom reads bugmail) 2008-06-12 11:01:48 PDT
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 Wan-Teh Chang 2008-06-13 11:16:34 PDT
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.

Comment 4 Robert Relyea 2008-06-13 11:33:28 PDT
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 Wan-Teh Chang 2008-06-13 11:46:34 PDT
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.
Comment 6 Robert Relyea 2008-06-13 11:51:44 PDT
Created attachment 325003 [details] [diff] [review]
patch for discussion

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 Nelson Bolyard (seldom reads bugmail) 2008-06-13 12:30:18 PDT
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.  
Comment 8 Robert Relyea 2008-06-13 14:05:52 PDT
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 Wan-Teh Chang 2008-06-13 14:24:13 PDT
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 Nelson Bolyard (seldom reads bugmail) 2008-06-13 14:40:36 PDT
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 Kai Engert (:kaie) 2008-06-13 14:51:22 PDT
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 Wan-Teh Chang 2008-06-13 16:28:31 PDT
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.
Comment 13 Robert Relyea 2008-06-13 17:22:54 PDT
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
Comment 14 Robert Relyea 2008-06-13 17:27:10 PDT
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 Nelson Bolyard (seldom reads bugmail) 2008-06-13 17:44:14 PDT
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 Nelson Bolyard (seldom reads bugmail) 2008-06-13 17:51:01 PDT
Stray character in last comment.  Should read: I *was* thinking that...
Comment 17 Wan-Teh Chang 2008-06-13 18:12:01 PDT
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 Julien Pierre 2008-06-23 20:00:15 PDT
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 Kai Engert (:kaie) 2008-06-24 05:57:07 PDT
(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 Nelson Bolyard (seldom reads bugmail) 2008-06-24 09:28:32 PDT
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 Kai Engert (:kaie) 2008-06-24 10:28:26 PDT
(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 Julien Pierre 2008-06-24 18:17:44 PDT
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 Wan-Teh Chang 2008-08-22 16:54:36 PDT
Created attachment 335131 [details] [diff] [review]
Build a stubs .so.  Link libfreebl3.so with unresolved symbols.

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 Wan-Teh Chang 2008-08-29 14:23:38 PDT
Created attachment 336110 [details] [diff] [review]
Don't use PL_strlen in lib/freebl

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.
Comment 25 Nelson Bolyard (seldom reads bugmail) 2008-08-29 14:46:20 PDT
Do we no longer care about WinCE?
Comment 26 Wan-Teh Chang 2008-08-29 16:06:04 PDT
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 Nelson Bolyard (seldom reads bugmail) 2008-08-29 16:22:01 PDT
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.
Comment 28 Robert Relyea 2008-09-30 10:38:17 PDT
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.
Comment 29 Nelson Bolyard (seldom reads bugmail) 2008-09-30 16:41:38 PDT
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?
Comment 30 Nelson Bolyard (seldom reads bugmail) 2008-09-30 16:45:21 PDT
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 Wan-Teh Chang 2008-09-30 18:31:01 PDT
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 Nelson Bolyard (seldom reads bugmail) 2008-10-01 07:34:27 PDT
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.  :(
Comment 33 Wan-Teh Chang 2008-10-01 08:51:15 PDT
Yes, with the introduction of libnssutil3.so in NSS 3.12,
"NSPR-free" freebl is really "NSPR and libnssutil3-free"
freebl.
Comment 34 Nelson Bolyard (seldom reads bugmail) 2008-10-01 09:34:24 PDT
So, what is the plan?  static linking of arenapool code into libhash?
Comment 35 Nelson Bolyard (seldom reads bugmail) 2008-10-01 09:38:45 PDT
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 Nelson Bolyard (seldom reads bugmail) 2008-10-01 10:34:25 PDT
er, make that "So Surprises".
Comment 37 Robert Relyea 2008-10-01 11:46:49 PDT
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 Wan-Teh Chang 2008-10-03 12:42:51 PDT
Created attachment 341658 [details] [diff] [review]
Don't use PL_strlen in lib/freebl (as checked in)

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
Comment 39 Robert Relyea 2008-10-28 16:25:58 PDT
Created attachment 345191 [details] [diff] [review]
freebl patch

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
Comment 40 Nelson Bolyard (seldom reads bugmail) 2008-10-30 22:40:51 PDT
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.
Comment 41 Robert Relyea 2008-10-31 10:18:31 PDT
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 Nelson Bolyard (seldom reads bugmail) 2008-10-31 12:31:43 PDT
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.
Comment 43 Robert Relyea 2008-10-31 13:41:10 PDT
I have not problem with that.

bob
Comment 44 Nelson Bolyard (seldom reads bugmail) 2008-11-06 02:21:59 PST
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.
Comment 45 Robert Relyea 2008-11-06 10:27:19 PST
> 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 Nelson Bolyard (seldom reads bugmail) 2008-11-06 11:09:56 PST
(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 Nelson Bolyard (seldom reads bugmail) 2008-11-06 11:13:48 PST
I'm changing the bug summary, because this really is a change to freebl,
not softoken.
Comment 48 Nelson Bolyard (seldom reads bugmail) 2008-11-06 11:51:15 PST
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 Nelson Bolyard (seldom reads bugmail) 2008-11-06 13:22:00 PST
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 Wan-Teh Chang 2008-11-06 13:49:07 PST
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.
Comment 51 Robert Relyea 2008-11-06 14:23:26 PST
> 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
Comment 52 Robert Relyea 2008-11-06 14:33:01 PST
>> 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 Wan-Teh Chang 2008-11-07 11:35:59 PST
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'.
Comment 54 Robert Relyea 2008-11-07 11:59:39 PST
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 Wan-Teh Chang 2008-11-07 12:15:31 PST
I would use 'unsigned int' to match SECItem and the current freebl
functions.
Comment 56 Robert Relyea 2008-11-14 11:33:32 PST
Created attachment 348225 [details] [diff] [review]
Updated patch from comments

Updated based on review comments.
Comment 57 Robert Relyea 2008-11-14 11:35:44 PST
Created attachment 348226 [details] [diff] [review]
Build update. (checked in)

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).
Comment 58 Nelson Bolyard (seldom reads bugmail) 2008-11-16 21:27:31 PST
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+
Comment 59 Nelson Bolyard (seldom reads bugmail) 2008-11-16 21:46:44 PST
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;
>+}
Comment 60 Robert Relyea 2008-11-17 10:50:37 PST
Created attachment 348594 [details] [diff] [review]
Fix reference leak/double free issue. (checked in)
Comment 61 Wan-Teh Chang 2008-11-17 11:12:19 PST
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?
Comment 62 Wan-Teh Chang 2008-11-17 11:19:22 PST
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 Nelson Bolyard (seldom reads bugmail) 2008-11-17 12:53:26 PST
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.
Comment 64 Nelson Bolyard (seldom reads bugmail) 2008-11-17 12:57:03 PST
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.
Comment 65 Robert Relyea 2008-11-17 13:05:53 PST
When you used interdiff, make sure you selected 'Update patch from comments' not 'Build update'
Comment 66 Robert Relyea 2008-11-17 13:08:27 PST
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 Nelson Bolyard (seldom reads bugmail) 2008-11-17 13:09:34 PST
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 Nelson Bolyard (seldom reads bugmail) 2008-11-17 13:10:49 PST
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.
Comment 69 Robert Relyea 2008-11-17 13:11:26 PST
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
Comment 70 Robert Relyea 2008-11-17 13:13:45 PST
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 Nelson Bolyard (seldom reads bugmail) 2008-11-17 13:36:31 PST
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?
Comment 72 Robert Relyea 2008-11-17 14:23:08 PST
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
Comment 73 Nelson Bolyard (seldom reads bugmail) 2008-11-17 14:48:06 PST
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 */
Comment 74 Nelson Bolyard (seldom reads bugmail) 2008-11-17 16:37:14 PST
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 Wan-Teh Chang 2008-11-17 19:36:39 PST
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.
Comment 76 Wan-Teh Chang 2008-11-17 19:40:45 PST
(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?
Comment 77 Robert Relyea 2008-11-18 11:49:29 PST
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 Nelson Bolyard (seldom reads bugmail) 2008-11-19 12:54:41 PST
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.
Comment 79 Robert Relyea 2008-11-19 14:11:50 PST
Yes, the checkin logs were already attached to the bug. I was waiting for the tinderbox cycle before I closed the bug....
Comment 80 Wan-Teh Chang 2008-11-19 19:10:10 PST
Bob, why don't you set FREEBL_NO_DEPEND=1 by default on Linux?
Comment 81 Wan-Teh Chang 2008-11-19 19:22:20 PST
Created attachment 349110 [details] [diff] [review]
Supplemental patch

This patch implements most of my suggested changes.
Comment 82 Robert Relyea 2008-11-20 09:51:38 PST
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 Wan-Teh Chang 2008-11-21 10:28:34 PST
Created attachment 349449 [details] [diff] [review]
Supplemental patch v2

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.
Comment 84 Wan-Teh Chang 2008-11-24 15:15:41 PST
(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.
Comment 85 Robert Relyea 2008-11-26 11:15:48 PST
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
Comment 86 Wan-Teh Chang 2008-11-27 07:40:39 PST
Created attachment 350337 [details] [diff] [review]
Supplemental patch v3 (checked in)

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

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