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.

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 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 .

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 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

Never mind my comment, the current code is correct. bob

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.

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.

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