323079 - When libsoftoken loads the freebl library, we don't always want to resolve the symbolic link.

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Wan-Teh Chang]

In rev. 1.26 of mozilla/security/nss/lib/freebl/loader.c, we
added code to always resolve the symbolic link when libsoftoken
loads the freebl library and the pathname of libsoftoken is a
symbolic link.

When I tried to integrate NSS 3.11 with the Mozilla Firefox and
Thunderbird on Mac OS X, I found that this change breaks their
"developer" build, in which all the NSS libraries are symbolic
links pointing to the real files in various directories.  On
Mac OS X, they use a special symbol called @executable_path
(similar to Solaris linker's $ORIGIN) to locate dynamic shared
libraries, so they don't set the DYLD_LIBRARY_PATH environment
variable (the equivalent of LD_LIBRARY_PATH).  So this is a case
where we don't want to resolve the symbolic link when loading
the freebl library.

Comment 1

[Wan-Teh Chang]

Attachment: Proposed patch

First load the freebl library using libsoftoken's pathname
as is.  If the load fails and the pathname is a symbolic
link, resolve the symbolic link and try again.  Note the
ordering of the two attempts.

Here is a summary of the patch.  Please review it because
this bug blocks the integration of NSS 3.11 with Mozilla.

1. I changed bl_GetOriginalPathname function to be a Unix-only
function (or rather, it is only defined on platforms that have
symbolic links).

2. I changed bl_GetOriginalPathname to return NULL when the
'link' argument is not a symbolic link.  This change simplifies
the caller's code: it can detect if 'link' is a symbolic link
and resolve the link in one shot.

3. I moved some of the code in bl_LoadLibrary to the new function
bl_LoadFreeblLibInSoftokenDir because the code is common to the
two loading attempts.

Comment 2

[Julien Pierre]

Comment on attachment 208226 [details] [diff] [review]
Proposed patch

I don't agree that the changes simplify the code. There are now 3 different ways to load freebl instead of 2 .

The name of the function "bl_LoadFreeblLibInSoftokenDir" is confusing. It should just be called bl_LoadFreeblLibInPath . 

Also, the comments in that function about loading in the same path as softoken should be moved to the calling function, where the logic really is .

I think this Firefox/Thunderbird "developer build" is really at the edge of what we should support.

IMO, we should have a firm position that any and all products must install all the actual NSS shared libraries in the same physical location on the disk. They can use symlinks to point to them if they want, but the actual files should be all in the same place.

If they aren't willing to do that in their developer build, then as developers they should at least be willing to set the DYLD_LIBRARY_PATH in a script, just like we do when we run the QA in our own NSS developer build with links from mozilla/dist/OBJDIR . In fact, the PR_LoadLibraryWithFlags with an unqualified pathname was left in the code precisely to deal with this case .

Comment 3

[Robert Relyea]

Sigh, I never did understand way we didn't do the load this way to begin with. It's *MUCH* easier to understand, and I find the new code *MUCH* easier to read.

As for 'Firefox/Thunderbird "developer build" is really at the edge of
what we should support.', they are doing exactly the same thing NSS does, which makes them the second product that uses this method.

I'm ambivalent about the name of the function that strips the softoken name, Julien's name is probably better (since it really strips off the leaf name, not specificately the softoken name).

Julien, is there anything that actually breaks if we do this, or does this enable a large class set of applications?

(My earlier uneasiness with loading freebl dynamically has always been can we find it reliably. Anything we can do to increase the chances of finding it reliably should be used, IMHO).

bob

Comment 4

[Robert Relyea]

Comment on attachment 208226 [details] [diff] [review]
Proposed patch

r+ with the following comment.

please check the
if (iterations == 1 && retlen < 0)
test.

I believe you wanted ||.

bob

Comment 5

[Robert Relyea]

Never mind my comment, the current code is correct.

bob

Comment 6

[Julien Pierre]

Comment on attachment 208226 [details] [diff] [review]
Proposed patch

I believe the new code is correct. I read the patch too quickly and withdraw my objections about the function naming and the moving of the comments to the caller - they really do belong in the leaf.

Re: the edge issue, the patch does enable a new class of applications, but as you point out, the Firefox/Thunderbird is doing exactly what NSS does, and it should therefore be able to use the same solution we use, which is to set the LD_LIBRARY_PATH in our QA scripts. Nobody is upgrading the libraries separately from the executable product, so that surely should be an acceptable solution.

The loader code was written with the expectation that only 2 cases were supported, because the NSS distribution was the only product that didn't have all the files in the same place. Every product had to ship the libraries files all in the same place, and every product known to us back in the summer when the new loader was written did. I won't stop this patch from going in, but since we are choosing to support this case now, I just wonder what request will come next for the loader. IMO it helps to have a clear definition of the library requirements and shipping all the NSS libraries in the same place was supposed to be one of them.

Comment 7

[Wan-Teh Chang]

Here is some more information about the Mozilla Firefox and
Thunderbird "developer" builds.  I use Thunderbird as an example.

When the Thunderbird build completes, the build tree has the
following directory that contains the executable (thunderbird-bin)
and the dynamic shared libraries (lib*.dylib).  Note that
thunderbird-bin is not a symbolic link, and all the NSS libraries
are symbolic links pointing to symbolic links in dist/lib, which
point to files residing in different directories.

$ pwd
/Users/wtc/MyFirefoxBuild/mozilla/tbird-opt-static/dist/Thunderbird.app/Contents/MacOS
$ ls -l
...
lrwxrwxrwx     1 wtc  wtc        97 Jan 10 18:15 libfreebl3.chk -> /Users/wtc/MyFirefoxBuild/mozilla/tbird-opt-static/security/manager/../../dist/lib/libfreebl3.chk
lrwxrwxrwx     1 wtc  wtc        99 Jan 10 18:15 libfreebl3.dylib -> /Users/wtc/MyFirefoxBuild/mozilla/tbird-opt-static/security/manager/../../dist/lib/libfreebl3.dylib
...
lrwxrwxrwx     1 wtc  wtc        98 Jan 10 18:15 libsoftokn3.chk -> /Users/wtc/MyFirefoxBuild/mozilla/tbird-opt-static/security/manager/../../dist/lib/libsoftokn3.chk
lrwxrwxrwx     1 wtc  wtc       100 Jan 10 18:15 libsoftokn3.dylib -> /Users/wtc/MyFirefoxBuild/mozilla/tbird-opt-static/security/manager/../../dist/lib/libsoftokn3.dylib
lrwxrwxrwx     1 wtc  wtc        96 Jan 10 18:15 libssl3.dylib -> /Users/wtc/MyFirefoxBuild/mozilla/tbird-opt-static/security/manager/../../dist/lib/libssl3.dylib
...
-rwxr-xr-x     1 wtc  wtc  17616968 Jan 11 14:30 thunderbird-bin
...
$ ls -l /Users/wtc/MyFirefoxBuild/mozilla/tbird-opt-static/security/manager/../../dist/lib
...
lrwxr-xr-x    1 wtc  wtc     96 Jan 10 18:03 libfreebl3.chk -> /Users/wtc/MyFirefoxBuild/mozilla/tbird-opt-static/nss/freebl/Darwin_SINGLE_SHLIB/libfreebl3.chk
lrwxr-xr-x    1 wtc  wtc     98 Jan 10 18:01 libfreebl3.dylib -> /Users/wtc/MyFirefoxBuild/mozilla/tbird-opt-static/nss/freebl/Darwin_SINGLE_SHLIB/libfreebl3.dylib
...
lrwxr-xr-x    1 wtc  wtc     78 Jan 11 14:28 libsoftokn3.chk -> /Users/wtc/MyFirefoxBuild/mozilla/tbird-opt-static/nss/softokn/libsoftokn3.chk
lrwxr-xr-x    1 wtc  wtc     80 Jan 10 18:01 libsoftokn3.dylib -> /Users/wtc/MyFirefoxBuild/mozilla/tbird-opt-static/nss/softokn/libsoftokn3.dylib
...
lrwxr-xr-x    1 wtc  wtc     72 Jan 10 18:02 libssl3.dylib -> /Users/wtc/MyFirefoxBuild/mozilla/tbird-opt-static/nss/ssl/libssl3.dylib
...
$

The executable thunderbird-bin uses the pathnames
@executable_path/libssl3.dylib, @executable_path/libsoftokn3.dylib,
etc. to load the dynamic shared libraries it depends on.  The
keyword @executable_path expands to the directory in which the
executable resides.  My experiment showed that @executable_path
resolves symbolic links; since thunderbird-bin is not a symbolic
link, it makes no difference in this case.

The Mozilla build instructions at
http://developer.mozilla.org/en/docs/Build_and_Install#Running_Your_New_Build,
explain how to run the finished build (DeerPark is the codename
for Firefox):

  Mac OS X

  On macintosh, the build system produces an application
  bundle at objdir/dist/AppName.app, for example
  objdir/dist/DeerPark.app.

  ..

  You can run the application by either opening the application
  bundle via the Finder, or from the command line using

  $ objdir/dist/AppName[Debug].app/Contents/MacOS/appname

When you "open the application bundle via the Finder", the
Finder runs the executable program specified in the
"Info.plist" file of the application bundle.  The executable
program firefox-bin or thunderbird-bin is not a shell script,
and it relies on the @executable_path keyword to load the
dynamic shared libraries it depends on, as I explained above.

The patch for this bug is necessary to make that continue to
work.

I welcome suggestions for better function names and comments.
Thanks a lot for the quick reviews.

Comment 8

[Wan-Teh Chang]

I checked in the patch on the NSS trunk (NSS 3.12) and
the NSS_3_11_BRANCH (NSS 3.11.1).

Checking in loader.c;
/cvsroot/mozilla/security/nss/lib/freebl/loader.c,v  <--  loader.c
new revision: 1.27; previous revision: 1.26
done

Checking in loader.c;
/cvsroot/mozilla/security/nss/lib/freebl/loader.c,v  <--  loader.c
new revision: 1.26.2.1; previous revision: 1.26
done