511312 - NSS fails to load softoken, looking for sqlite3.dll

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Julien Pierre]

One of our customers has seen an issue very similar to bug 494107, but this time on the Windows platform.

The application is a plug-in DLL to a web server running as a service. The plug-in fails to initialize NSS (NSS_NoDB_Init) when moving from NSS 3.11.9 to NSS 3.12.3 .

The directory of the plug-in or NSS (which are the same) is not in the Windows system PATH that applies to the service.

After debugging, I determined that the failure that is seen with 3.12 is actually that nss3.dll fails to dynamically load softokn3.dll . If I add sqlite3.dll in the system PATH, then it can be succesfully loaded.

Unfortunately, Windows has nothing like an rpath to solve this problem like we did on Unix platforms.

But there is an option to LoadLibraryEx called LOAD_WITH_ALTERED_SEARCH_PATH . The option allows direct-linked dependencies to be found by Windows even if they are not in the PATH. We should use it when loading softokn3.so . This requires a change in NSPR because PR_LoadLibraryWithFlags only uses LoadLibrary, and has no optional flag to do this. I will open the NSPR RFE as a dependency.

What puzzled me the most in this situation is that NSS was able to load at all even in the previous version of the web server. The web server plug-in should not have been able to find its own nss3.dll direct dependency since it wasn't in the PATH. The only explanation was that the web server itself was using LoadLibraryEx with this new LOAD_WITH_ALTERED_SEARCH_PATH option when loading its plug-ins.

Comment 1

[Julien Pierre]

In the last paragraph, I meant it was surprising that the previous version of the web server plug-in, which used NSS 3.11.9, was able to load .

Comment 2

[Wan-Teh Chang]

This is a duplicate of bug 454508.  The options were discussed in
that bug.  A workaround is for the web server plug-in to load
both nss3.dll and softokn3.dll.  This is the workaround used by
the Chromium browser when it imports saved passwords from Firefox.
See NSSDecryptor::Init() in
http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/importer/nss_decryptor_win.cc?revision=21251&view=markup&pathrev=21251

Comment 3

[Julien Pierre]

Wan-Teh,

Thanks for that. I didn't remember that bug. The test case in it was quite different. There were several distinct sqlite3.dll .

In the case of our application, there is only a single sqlite3.dll on the system. And all the NSPR/NSS DLLs are stored in a single location, the same location as the web server plug-in DLL. But it is not the location of the web server executable, which is actually from a different vendor.

I gather that you mean that the web server plug-in should link directly with both nss3.dll and softokn3.dll ? That may work to resolve this particular problem, but I'm afraid it may create more in the future. We have options in NSS_Initialize that specifically deal with loading like NSS_INIT_PK11RELOAD . I think architecturally this app change would not be a very good one.

Regarding https://bugzilla.mozilla.org/show_bug.cgi?id=454508#c5, 
The workaround 1 - having the app dynamically load softokn3.dll with LOAD_WITH_ALTERED_SEARCH_PATH is an option also that accomplishes the same thing. But it's additional code for the application that should probably not required.

The workaround 2 - using SetDllDirectory - will not work, since we need to support Windows 2000, and we don't want to change properties of a process in which the plug-in is only a guest.

I think the right way to fix this would be to make the change in NSPR as I described in bug 511315 to add a flag to PR_LoadLibraryWithFlags . Then, genload.c should be changed to use that flag when trying to load softoken.
In theory, this change shouldn't affect the FIPS validation. Except that we have only one copy of genload.c, and it is used twice, once within the softoken PKCS#11 module, and one above. Even though we only need to make this change for the loader above the PKCS#11 line, a change to this source file affects both above and below the PKCS#11 line.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

I agree that this is a duplicate of bug 454508.
Maybe we should reopen that bug.

Comment 5

[Wan-Teh Chang]

Yes, I meant that the web server plug-in should link directly
with the import libraries nss3.lib and softokn3.lib as a workaround.
This doesn't require additional code for the web server plug-in;
it is a build change.

Please ignore the two workarounds I described in bug 454508 comment 5
for your web server plug-in.  Those two workarounds are for
applications that load NSS DLLs using LoadLibrary or LoadLibraryEx.

Comment 6

[Julien Pierre]

Wan-Teh,

Re: comment (,
The web server plug-in already links with the nss3.lib import library.
I think we may have taken the softokn3.lib import library out of our distribution on purpose since we didn't want any apps to link against softoken and some apps had made that mistake in the past. If so, the app could still create an import library on the fly during the build and link against it.
But I think have the app link against softokn3 should be a temporary workaround and not a long-term one.

Nelson,
Re: comment 4, we can either reopen bug 454508 or continue the discussion here.

Comment 7

[Julien Pierre]

Attachment: Fix that affects FIPS. Use new PR_LD_ALTERED_SEARCH_PATH flag on PR_LoadLibraryWithFlags call

This fix depends on the enhancement to NSPR in bug 511315 .

Comment 8

[Wan-Teh Chang]

Comment on attachment 395507 [details] [diff] [review]
Fix that affects FIPS. Use new PR_LD_ALTERED_SEARCH_PATH flag on PR_LoadLibraryWithFlags call

For backward compatibility, we may need to try both the
default and the altered search paths.

We may want to consider this fix for NSS 3.13 only.
Right now NSS 3.12.x requires NSPR 4.7.x only.  With
this patch, NSS will require NSPR 4.8.1.  NSPR 4.8
dropped Windows 9x support.  It could be problematic
for old apps to upgrade to NSPR 4.8.1 if they have
to support Windows 9x (even if just nominally).

Comment 9

[Julien Pierre]

Wan-Teh,

What apps still have requirements to support Win9x ? I think we will need this fix in NSS 3.12.x for Sun.

Comment 10

[Julien Pierre]

We could maintain source and binary compatibility with NSPR 4.7 by surrounding my patch with
#ifdef PR_LD_ALTERED_SEARCH_PATH .
This would allow the source to compile against either NSPR 4.7.x or 4.8.x, thus preserving compatibility with Win9x, if it is really needed .

Comment 11

[Wan-Teh Chang]

Ah, you're right.  The #ifdef will allow the patch to work with
either NSPR 4.7.x or 4.8.x.

I'd like to see how you plan to address the backward compatibility
issue (try both the default and alternate search paths).  We only
need to do that on Windows.  This may affect how we define
PR_LD_ALT_SEARCH_PATH in NSPR (whether it should be inside
#ifdef WIN32).  We don't want to do something like:

  dlh = PR_LoadLibraryWithFlags(libSpec, PR_LD_NOW | PR_LD_LOCAL |
                                PR_LD_ALT_SEARCH_PATH);
  if (!dlh)
      dlh = PR_LoadLibraryWithFlags(libSpec, PR_LD_NOW | PR_LD_LOCAL);

because the second call will be redundant on non-Windows platforms.

Comment 12

[Julien Pierre]

Wan-Teh,

I was only planning on having one PR_LoadLibraryWithFlags call for loading softoken, with PR_LD_ALT_SEARCH_PATH set. Are there cases that this would break ?

I think this would require a Windows app that doesn't have the PATH set, and has softoken dependencies (eg. sqlite3.dll) stored in a different location than softokn3.dll, and that sqlite3.dll would not be in the PATH, but would be in the executable's directory. Does such an app exist ?

Comment 13

[Wan-Teh Chang]

Do we use those functions in lib/freebl/genload.c to load
other PKCS #11 modules?  I just wanted you to think whether
this will break any apps, and suggested a conservative
approach to guarantee backward compatibility.  Perhaps your
patch is fine.  I just don't have time to think this through.

Comment 14

[Julien Pierre]

Wan-Teh,

Re: comment 13,
No, we don't use the code in genload.c to load other PKCS#11 modules. It's only used to load softoken, or dependencies of softoken.

See http://mxr.mozilla.org/security/source/security/nss/lib/pk11wrap/pk11load.c#270 . There are two branches - one for mod->internal starting at line 274, and the else case at line 306 . The else case uses PR_LoadLibrary, not PR_LoadLibrary with flags.

Thus, attachment 395507 [details] [diff] [review] only affects the loading of softoken. One would have to be in the specific situation I described in comment 12 for the application to break . I can tell you that situation is never true for Sun applications, but I can't be 100% sure about other apps. If we are truly worried about this case, then we probably would want to further enhance NSPR to have specific flags to try both the normal and alternate search path, and perhaps select the order of those search paths as well. That seems a bit overkill, unless we know there is a more than theoretical possibility of breakage.

Comment 15

[Julien Pierre]

An alternative to attachment 395507 [details] [diff] [review] is to make a copy of genload.c in the pk11wrap (perhaps call it genload2.c) and change pk11load.c to include that copy rather than freebl/genload.c . The fix can then be made to genload2.c only. The advantage of this approach is that FIPS-validated code would be untouched.

Or perhaps this genload code should be moved to nssutil3, since there seems to be a need for it in several places. Another place is in libpkix. See bug 511576 .

Comment 16

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 395507 [details] [diff] [review]
Fix that affects FIPS. Use new PR_LD_ALTERED_SEARCH_PATH flag on PR_LoadLibraryWithFlags call

If I'm not mistaken, we've decided (or at least agreed in principle)
to implement one of the alternative changes proposed in comment 15.
This will obviate any changes to FIPS validated code.

Comment 17

[Julien Pierre]

Nelson,
Re: comment 16, that's correct. The goal is to move genload.c to nssutil3. I am working on it now.

Comment 18

[Julien Pierre]

Attachment: Fix outside of FIPS boundary

This fix makes a new copy of genload.c in lib/util for 3.12.5 which uses the new PR_LD_ALT_SEARCH_PATH flag in PR_LoadLibraryWithFlags . It exports UTIL_LoadLibrary . It also changes the code in lib/pk11wrap to use that copy .

I will add a separate patch to softoken for SOFTOKEN_3_13_BRANCH to make use of UTIL_LoadLibrary as well.

Comment 19

[Wan-Teh Chang]

Comment on attachment 396605 [details] [diff] [review]
Fix outside of FIPS boundary

I only reviewed the new header util/genload.h.

Nit: I don't know what the "gen" in "genload.c" means.
It'd be nice to pick a more informative file name.

>+#include "nspr.h"

It should be enough to include "prlink.h".  "nspr.h"
includes all NSPR headers.

>+PRLibrary *
>+UTIL_LoadLibrary(const char* NameOfExistingSharedLib,
>+                 PRFuncPtr FuncInExistingSharedLib,
>+                 const char *nameToLoad);

Nit: The parameter names are too long.

Parameter names should not be capitalized.  After
working on NSS and NSPR for so many years, you should
have figured out our naming convention :-)

I believe this function falls back to loading the
named library from the shared library search path.
That isn't documented.

You should document that on Windows it uses the
"alternate" search path for DLLs, and how it deals
with symbolic links on Unix.

Comment 20

[Julien Pierre]

Wan-Teh,

Re: comment 19,

I'm not sure what "gen" means either, I am guessing "generic" ? I did not make up this name - my goal was merely to move the code to nssutil.

If you look at the existing copy of genload.c at http://mxr.mozilla.org/security/source/security/nss/lib/freebl/genload.c#155, you will see that the parameter names were already quite long. I merely changed "This" to "Existing", as I thought "This" was no longer correct since the C code is no longer #included .

Regarding the fallback and links, yes, the behavior should be documented in the header file.

Do you think the rest of the code is correct ? I was going to ask you for a 2nd review, but you already replied before I got a chance.

Comment 21

[Wan-Teh Chang]

Attachment: Diffs between lib/freebl/genload.c and lib/util/genload.c (to help code review)

Comment 22

[Wan-Teh Chang]

Comment on attachment 396605 [details] [diff] [review]
Fix outside of FIPS boundary

1. pk11wrap/pk11load.c

I suggest the following variable names:

  nss_default_name => my_shlib_name (because pk11warp is part of libnss3.so)
  softoken_default_name => softoken_shlib_name

because I don't know what "default" means here.
We don't use any non-default names for those two
shared libraries in this file.

In softoken_LoadDSO, we don't need the local variable
softoken_name.  Just use softoken_shlib_name directly.

>+  if (!softoken_name) {
>     PR_SetError(PR_LOAD_LIBRARY_ERROR, 0);
>     return PR_FAILURE;
>   }

This can be removed.

2. File names for util/genload.{h,c}

I suggest secload.{h,c} or nssload.{h,c}.

3. util/genload.c

>+#include <nspr.h>
>+#include <secport.h>

We should use "" instead of <> for these two headers.
<> means system headers, but NSPR and NSS aren't system
libraries on most platforms.

Nit: nspr.h includes all NSPR headers.  If it is enough
to include just one or two NSPR headers (e.g., "prlink.h"),
do that.  Otherwise, it's fine to include "nspr.h".

A comment in this file still contains "softoken".
That should be removed.

>+            libSpec.type = PR_LibSpec_Pathname;
>+            libSpec.value.pathname = fullName;
>+            dlh = PR_LoadLibraryWithFlags(libSpec, PR_LD_NOW | PR_LD_LOCAL |
>+                                          PR_LD_ALT_SEARCH_PATH );

Since we specify the library with a full pathname here,
we should not need the PR_LD_ALT_SEARCH_PATH flag, right?
(I believe it's harmless to specify that flag though.)

>+    /* Get the pathname for NameOfExistingSharedLib, i.e. /usr/lib/libnss3.so

Nit: i.e. => e.g.

>+     * So, we require the address of a function in the existing library,
>+     * provided by the caller.

This sentence used to be

       * But we can just get the address of this function !

[Note: To understand the following point, you need to read
the entire paragraph in that block comment.]

This is actually an important point.  The only reliable way
to get the address of a function in a library is for that
library itself to return the address of a static function.
So it is best to require that UTIL_LoadLibrary should only
load the target library relative to itself.  This way we
can just pass the address of a static function in the file
that contains the UTIL_LoadLibrary call.

4. util/genload.h

>+#include "nspr.h"

Including "prlink.h" should be enough.

UTIL_LoadLibrary should be documented more completely.
I already mentioned this issue yesterday.

I have two nits about the function name
- We should not add yet another prefix for NSS functions.
  We have too many function name prefixes already.
  Let's use SEC_ or NSS_.  I know you already added
  UTIL_SetForkState in NSS 3.12.3, but in that same release
  Nelson used the NSS_ prefix for NSS_GetAlgorithmPolicy
  and NSS_SetAlgorithmPolicy.
- The function's name should suggest the important feature
  of this function, that it simulate $ORIGIN.  So I suggest
  naming this function XXX_LoadLibraryFromOrigin.

Comment 23

[Julien Pierre]

Wan-Teh,

Re: comment 22,
Thanks for your review. Given the number of recommended changes, I assume this is an r-. Please mark it as such.

I will attach a new patch incorporating your feedback after I get review from Nelson.

The only issues I have with your review is about genload.c .

3a. Actually, the native LOAD_WITH_ALTERED_SEARCH_PATH flag, and the corresponding PR_LD_ALT_SEARCH_PATH flag, only has an effect if the name of the library to be loaded specifies an absolute path. Please see the doc at http://msdn.microsoft.com/en-us/library/ms684179(VS.85).aspx . Microsoft says about this flag :

"If this value is used and lpFileName specifies an absolute path, the system uses the alternate file search strategy discussed in the Remarks section to find associated executable modules that the specified module causes to be loaded. If this value is used and lpFileName specifies a relative path, the behavior is undefined.

If this value is not used, or if lpFileName does not specify a path, the system uses the standard search strategy discussed in the Remarks section to find associated executable modules that the specified module causes to be loaded."

We actually want to use this PR_LD_ALT_SEARCH_PATH together with the absolute pathname in order to triggered the alternate search strategy. It's this alternate strategy that allows the library's dependencies to be found by Windows from the absolute path of the library to be loaded.

In the second PR_LoadLibraryWithFlags, when we fall back to a simple file name, we could drop the PR_LD_ALT_SEARCH_PATH flag. But the Microsoft documentation says the flag will actually be ignored anyway unless it's an absolute path - ie. the default load strategy will be used.

3b. About the comments, I thought  :
"But we can just get the address of this function !" was no longer appropriate, since UTIL_LoadLibrary allows the caller to load a library that's not necessarily in the same directory as libnssutil3.so which contains the code.
What exactly do you want to see in the comment that is currently missing in my patch ? 

I would also like to note that our application product that reported the problem has tested my patch and reported success with it - they no longer need to set the PATH for NSS to initialize.

Comment 24

[Wan-Teh Chang]

Comment on attachment 396605 [details] [diff] [review]
Fix outside of FIPS boundary

Thanks for explaining the LOAD_WITH_ALTERED_SEARCH_PATH
flag to me.  Could you remove the PR_LD_ALT_SEARCH_PATH
flag from the second PR_LoadLibraryWithFlags call to make
it clear that the standard search strategy will be used?

I'm sorry that I wasn't clear what I wanted the comment
to say.  Here is what you have:

+    /* Get the pathname for NameOfExistingSharedLib, i.e. /usr/lib/libnss3.so
+     * PR_GetLibraryFilePathname works with either the base library name or a
+     * function pointer, depending on the platform. We can't query an exported
+     * symbol such as NSC_GetFunctionList, because on some platforms we can't
+     * find symbols in loaded implicit dependencies.
+     * So, we require the address of a function in the existing library,
+     * provided by the caller.
+     */

How about this?  Please edit it as you see fit.

+    /* Get the pathname for NameOfExistingSharedLib, i.e. /usr/lib/libnss3.so
+     * PR_GetLibraryFilePathname works with either the base library name or a
+     * function pointer, depending on the platform.
+     * We require the address of a function in the "reference library",
+     * provided by the caller.  To avoid getting the address of the stub/thunk
+     * of an exported function by accident, use the address of a static function
+     * rather than an exported function.
+     */

Re: function prefix and header file name: I think PORT_
is also a good prefix for the new function, and we can
just declare it in the existing secport.h header.

Comment 25

[Nelson Bolyard (seldom reads bugmail)]

In comment 21, Julien quotes from a MS web page which says:

> "If this value [LOAD_WITH_ALTERED_SEARCH_PATH] is used and lpFileName 
> specifies a relative path, the behavior is undefined.

So, I sincerely hope that, on Windows, when the PR_LD_ALTERED_SEARCH_PATH flag 
is set, PR_LoadLibraryWithFlags will check for a relative path name and return
an error, rather than allowing "undefined behavior" to occur!!

I would also hope that any/all callers of PR_LoadLibraryWithFlags that pass 
the PR_LD_ALTERED_SEARCH_PATH flag would do their utmost to avoid passing 
relative path names.

Comment 26

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 396605 [details] [diff] [review]
Fix outside of FIPS boundary

I completely agree with Wan-Teh that we should use SEC_ or NSS_ (or maybe 
PORT_) but not UTIL_ as a prefix for NSS's util shared lib.

I went looking for the origin of that comment 
> But we can just get the address of this function !
in part because I thought I wrote it, and in part to try to refresh my 
memory of why I decided to do it that way.  I found it back in 
Attachment 195088 [details] [diff] for bug 303508.  Along the way of digging that up, I was 
reminded of the huge number of times we've had to revise and revise our 
dynamic loader code due to unforeseen problems.  We've had problems because 
we moved the dlopen call from one shared library to another (because we 
started using NSPR instead of calling dlopen).  We've had problems that 
appeared only in setuid root programs.  Problems have occurred when the 
library name to be loaded had a specific path name form, or used forward 
slashes instead of back slashes as path separators (bug 319658), etc.  
We've recently seen problems with inability to find libraries on all platforms.

One thing has become abundantly clear from all this pain.  That is that we do 
not have nearly adequate regression tests for dynamic loading.  Consequently, 
I dread making any unnecessary changes, and making any changes that are bigger than necessary to this code.

My original proposal was to make a copy of genload.c in pk11wrap and alter it 
to use the new loader flag, but make no other change to it.  I am very confident that there would be no surprises to that approach.  

This patch moves that code into libnssutil.  While that seems logical and efficient, factoring code out of several places into one common place, and potentially reducing maintenance cost of duplicated code, we see that it 
makes the function more complicated (it has 3 arguments instead of one, and 
one of those three is difficult to explain properly and get right).  
I'm just afraid this is going to lead to yet another round of releases, all 
of which attempt to fix yet another bug in dynamic loading.  I think that 
would really hurt our credibility, terribly, and worse, I'm not sure we'll
be around to keep fixing it much longer.  So, let's take the lowest risk
approach we can find at this time.  If we're still here in 90 days, then 
we can attempt to be more grandiose, and move it to libnssutil in 3.13.

Comment 27

[Nelson Bolyard (seldom reads bugmail)]

I see there is one implied point that I failed to spell out.  Today, without 
any patch, genload.c is #included and thus effectively compiled into pk11wrap,
part of libnss3.  My proposal to simply copy genload.c into pk11wrap would 
have resulted in the function being compiled in the same place, in the same shared library, as before.  Only the source would have moved.  There would
be no issues associated from the code moving to a different shared library
that could possibly be in a different directory.

Comment 28

[Julien Pierre]

Nelson,

Re: comment 26,

One big advantage of moving the code to nssutil and having only one instantiation of the code (as opposed to 2 today - one in libsoftoken, and one in libnss) is that any other loading problem could be fixed in nssutil in the future, which is defined outside of FIPS. As I mentioned before, I also want to change softoken to use the util copy of the loader at the next opportunity we have, though I didn't include the patch for that yet.

I agree that we have inadequate testing, and I filed 3 RFEs in bugzilla last week about new QA tests. See bug 512017 , 512018 and 512019 .

If you are really that concerned about the new function having 3 arguments instead of 1, I could keep bring it back down to 1 which would keep the code for the nssutil copy nearly identical except for naming. The reason I decided not to do that, and to add the extra 2 arguments was in case we ever needed this loading strategy to work in situations where not all the NSS libraries are in the same location. As long as we don't have that need, the 2 extra arguments are not strictly necessary. I made the change only to future-proof the API when making it public, not because of any immediate requirement. I certainly wouldn't call this change grandiose, it is just a minor change. And I believe it can be adequately documented, and the patch introduces no regression.

Whether we are still here in 90 days or not is not a pervasive argument. The code will be open-source just like always, and whoever still needs NSS at that date will be able to enhance or fix it in anyway they like. If they don't like the fix, they can also go back to the previous version if they wish. I don't think we should prevent new code from going in unless there is something actually wrong with it. Otherwise we might as well declare all of NSS frozen and quit now.

Comment 29

[Nelson Bolyard (seldom reads bugmail)]

Julien,
For NSS 3.12.5, we will take the lowest risk strategy.
Moving code to libutil gives us no greater benefit than moving it to
pk11wrap, until we can move it out of softoken/freebl, which won't 
happen before 3.13.  Considering that some of us will very likely be
job hunting soon, having to explain recent regressions in our product 
might be counterproductive.

Comment 30

[Wan-Teh Chang]

Comment on attachment 396605 [details] [diff] [review]
Fix outside of FIPS boundary

Julien,

I forgot to suggest that you use #ifdef PR_LD_ALT_SEARCH_PATH
to allow people to compile NSS 3.12.5 with older versions of
NSPR.  You can add a reminder comment that the ifdef can be
removed when NSPR 4.8.1 or later is widely used.

Nelson,

As long as you use PR_LoadLibraryWithFlags, it is fine to
move this code to a different shared library.  What matters
is the shared library that calls dlopen().  When we first
added the libfreebl shared libraries (for Solaris and HP-UX
only), I wrote custom loading code to call dlopen directly
in libfreebl.a to get the desired property ($ORIGIN relative
to libsoftokn3.so, which is linked with libfreebl.a).  Then
we discovered $ORIGIN doesn't work in setuid program, so we
simulated it using PR_GetLibraryFilePathname, and we also
switched to PR_LoadLibraryWithFlags.

Comment 31

[Julien Pierre]

Nelson,

Re: comment 29,

Bob already agreed last week in mozilla-nssdev that the approach of putting the loader code in nssutil was the way to go. So does Wan-Teh. If you think the patch will cause a regression, now would be a most excellent time to point out how technically it will do so.

If we had branched 3.12, I might agree with taking the minimalist patch approach, but we have not. As it is, there is only a single copy of the util code being maintained on the trunk, that has to work for both 3.12 and 3.13 softoken . The minimalist patch approach is OK for softoken 3.12, but not for 3.13 . Unless we branch the util code now, I need to come up with a util patch that worked for both 3.12 and 3.13 versions of softoken.

As for benefits, moving the loader function to util also helps resolve bug 511576, which doesn't have to wait until 3.13 to be fixed. Otherwise, we would have to make yet a third instance of the loader code in libpkix. That would be really ugly, not to mention very temporary, and it would have to be undone later to use libutil. That would be unnecessary busy work.

Moving the loader code to util allows NSS 3.12.5 to have 2 copies of the loader code, one in softoken and one in util, the later of which would be called from 2 places, pk11wrap and libpkix. When switching to softoken 3.13, it would also use the util copy, bringing down the number of loader code in memory to a single one. This is really the most painless way to go forward, and it avoids the need for branching, as well as the need for any immediate change to softoken 3.12.4, which would affect FIPS.

Comment 32

[Julien Pierre]

Attachment: Updated patch for trunk, outside of FIPS boundary. Integrates Wan-Teh's feedback.

Comment 33

[Julien Pierre]

Attachment: Patch for softoken and freebl for SOFTOKEN_3_13_BRANCH

I found that there were already 2 different copies of the loader code in use in softoken : one in freebl/genload.c, included from freebl/loader.c, and a second one in softoken/lgglue.c, used to load nssdbm3 . sigh.
This patch gets rid of both copies, and switches over to using the new PORT_LoadLibraryFromOrigin from nssutil3 (see previous attachment).

Comment 34

[Julien Pierre]

Comment on attachment 397402 [details] [diff] [review]
Patch for softoken and freebl for SOFTOKEN_3_13_BRANCH

Oops. Parts of the patches overlapped. I will reattach one for softoken/freebl only.

Comment 35

[Julien Pierre]

Attachment: Patch for softoken and freebl. For SOFTOKEN_3_13_BRANCH .

Comment 36

[Nelson Bolyard (seldom reads bugmail)]

Julien,  In comment 31, you seem to be arguing that, unless we know of 
a reason why this will regression, we must allow the change.  That is 
precisely the opposite of risk averse thinking.  

Recently you changed uint32 to PRUint32 in many places, using a similar 
argument.  We did not know, at the time, that on some 32-bit platforms, 
uint32 was a long, and hence uint32* was a long*, which many compilers 
treat as incompatible with an int*, even when long and int have the same 
size.  So, the change introduced a source level incompatibility, because 
some code passed long* arguments to functions whose declaration was changed
to PRUint32* which is int*.  That change is now the cause of much unhappiness towards NSS from the Mozilla people. 

Right now, it is important for the future of the NSS team for us to be risk 
averse.  If we're around to produce a 3.13 later, then that will be a good 
time to consider the additional risk.  A bug-fix release to immediately 
follow 3.12.4 is not the time to take risk.

Comment 37

[Julien Pierre]

Nelson,

I don't see the relevance of your example in comment 36 about other code. No existing public function prototypes are being changed.
If you want to argue about risk, please identify specific risk in the code that has been attached. Just because it isn't the smallest possible patch for 3.12.5 doesn't mean it is incorrect. And the smallest patch is certainly not the best patch for 3.13.

Comment 38

[Wan-Teh Chang]

Attachment: Diffs between lib/freebl/genload.c and lib/util/secload.c (to help code review)

Comment 39

[Wan-Teh Chang]

Comment on attachment 397392 [details] [diff] [review]
Updated patch for trunk, outside of FIPS boundary. Integrates Wan-Teh's feedback.

r=wtc.  Note: please get Nelson's approval for checkin.
Even though I know this stuff very well and think this
patch is safe, I'm just a module peer now.

1. I suggest that secload.c be renamed portload.c because
the new function uses the PORT_ prefix.

2. In util/secload.c

>+            dlh = PR_LoadLibraryWithFlags(libSpec, PR_LD_NOW | PR_LD_LOCAL
>+#ifdef PR_LD_ALT_SEARCH_PATH
>+            /* allow library's dependencies to be found in the same directory
>+             * on Windows even if PATH is not set. Requires NSPR 4.8.1 . */
>+                                          | PR_LD_ALT_SEARCH_PATH 
>+#endif
>+                                          );

I suggest that you do the ifdef in a differet way so that it
doesn't "intersect" an expression.

              PRIntn flags = PR_LD_NOW | PR_LD_LOCAL;
#ifdef PR_LD_ALT_SEARCH_PATH
              flags |= PR_LD_ALT_SEARCH_PATH;
#endif
              dlh = PR_LoadLibraryWithFlags(libSpec, flags);

Declare the 'flags' variable at the beginning of a block
to make pre-C99 compilers happy.

Is everyone happy with the LoadLibraryFromOrigin name?
It assumes that you know about the $ORIGIN linker keyword.
Another good name would be LoadLibraryInSameDirectory.

The block comment before PORT_LoadLibraryFromOrigin is
already in the header file.  It doesn't need to be repeated
here.

>+     * We require the address of a function in the "reference library",
>+     * provided by the caller. To avoid getting the address of the stub/thunk
>+     * of an exported function by accident, use the address of a static
>+     * function rather than an exported function.

This comment should be moved to the header.

3. In util/secport.h

>+PRLibrary *
>+PORT_LoadLibraryFromOrigin(const char* existingShLibName,
>+                 PRFuncPtr staticShLibFunc,
>+                 const char *newShLibName);

Important: please make newShLibName the first parameter
because the library to load is the most important parameter.

Please update the indentation of the second and third parameters.

Please make the same changes to the function definition in
util/secload.c.

Comment 40

[Nelson Bolyard (seldom reads bugmail)]

Taking.  Sun must have a solution for 3.12.5, though not necessarily the
patch currently attached.

Comment 41

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 397392 [details] [diff] [review]
Updated patch for trunk, outside of FIPS boundary. Integrates Wan-Teh's feedback.

I still have very strong misgivings about this patch, but I just don't have time to mess with it any more.  So, I'll take a chance with it.  If this means that shared library loading in 3.12.x remains broken for yet another release, I probably won't be the one to fix it.

Comment 42

[Nelson Bolyard (seldom reads bugmail)]

Checking in pk11wrap/pk11load.c; new revision: 1.28; previous revision: 1.27
Checking in util/manifest.mn;    new revision: 1.21; previous revision: 1.20
Checking in util/nssutil.def;    new revision: 1.11; previous revision: 1.10
Checking in util/secload.c;      initial revision: 1.1
Checking in util/secport.h;      new revision: 1.23; previous revision: 1.22

Alexei, Before he left, Julien found other places in NSS, outside of the FIPS
boundary, where NSS was loading other shared libraries.  He believed that 
all of those places needed to be converted to call this new function 
PORT_LoadLibraryFromOrigin in libutil.  I believe he's right that all our 
loaders in libNSS should be consolidated.  I seem to recall that he said he
found one in libPKIX.  Please look for it and look at what it would take to 
convert it to use this new function.

Comment 43

[Nelson Bolyard (seldom reads bugmail)]

Fixed in NSS 3.12.5