Last Comment Bug 712579 - Load nssckbi from the location of nss3
: Load nssckbi from the location of nss3
Status: RESOLVED FIXED
[inbound][qa-]
:
Product: Core
Classification: Components
Component: Security: PSM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla12
Assigned To: Mike Hommey [:glandium]
:
:
Mentors:
Depends on:
Blocks: 486716 683127
  Show dependency treegraph
 
Reported: 2011-12-21 01:51 PST by Mike Hommey [:glandium]
Modified: 2012-02-01 13:29 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
[v1] Load nssckbi from the location of nss3 (2.86 KB, patch)
2011-12-27 23:17 PST, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review
Load nssckbi from the location of nss3. (3.67 KB, patch)
2011-12-29 08:32 PST, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review
Load nssckbi from the location of nss3 - for aurora (3.45 KB, patch)
2012-01-25 09:12 PST, Mike Hommey [:glandium]
mh+mozilla: review+
blassey.bugs: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Mike Hommey [:glandium] 2011-12-21 01:51:18 PST
Currently, PSM tries to load nssckbi from the application and GRE directories, before trying in the system directories. With the upcoming new linker for android in bug 683127, nssckbi will be in none of these locations. However, it will still be wherever nss3 is loaded from.

The PR_GetLibraryFilePathname function can allow us to get the location of the nss3 library, and try to load nssckbi from there.
Comment 1 Mike Hommey [:glandium] 2011-12-27 23:17:47 PST
Created attachment 584530 [details] [diff] [review]
[v1] Load nssckbi from the location of nss3

This works on linux, mac and windows:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mh@glandium.org-84d52bf7b258/
Note these builds have an additional patch that also removes the current search locations, to check that the new one is the one being hit.
On android, this new search location alone doesn't work, but searching in NS_XPCOM_CURRENT_PROCESS_DIR and NS_GRE_DIR already fails today, so in fact it already falls back to load without a path and getting what the custom linker already loaded. It will however work with the new linker (bug 683127).
Comment 2 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-12-28 15:36:19 PST
Is it possible for an addon or plugin to to load its own module also named nss3, such that PR_GetLibraryFilePathname could return the path to that library instead of the path to NSS's libnss3?

Is the new linker only to be used on Android? Or also on Linux?

Does this patch work correctly when the path to libnss3 contains non-ASCII characters?
Comment 3 Mike Hommey [:glandium] 2011-12-29 00:01:07 PST
(In reply to Brian Smith (:bsmith) from comment #2)
> Is it possible for an addon or plugin to to load its own module also named
> nss3, such that PR_GetLibraryFilePathname could return the path to that
> library instead of the path to NSS's libnss3?

On Linux, it is definitely not possible, because PR_GetLibraryFilePathname uses dladdr on a NSS function, which symbol is bound when libxul is loaded. On Mac, that would depend in what order libraries are returned by _dyld_get_image_name, and I guess the last loaded ones would be last, making the first NSS loaded the one being returned. On Windows, GetModuleHandleW is used, which documentation says that the behavior is undefined if there are several modules loaded with the same name. Not sure what that means in practice.

> Is the new linker only to be used on Android? Or also on Linux?

Only Android at first, but it can be used on Linux for easier debugging, and may be used in releases in the future.

> Does this patch work correctly when the path to libnss3 contains non-ASCII
> characters?

I must say I didn't test that, but that can surely be tested.

Note that if finding libnssckbi.so fails this way, we still fall back to loading from the GRE and APP directories, so even if the part using PR_GetLibraryFilePathname fails, it should still work.

Anyways, if there's concern over the behavior on Windows and Mac, we can #ifdef MOZ_LINKER and add the relevant define in Makefile.in. Would you prefer that?
Comment 4 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-12-29 00:33:12 PST
(In reply to Mike Hommey [:glandium] from comment #3)
> (In reply to Brian Smith (:bsmith) from comment #2)

> Anyways, if there's concern over the behavior on Windows and Mac, we can
> #ifdef MOZ_LINKER and add the relevant define in Makefile.in. Would you
> prefer that?

I think that would be better, since the behavior is, at best, unclear on those platforms. It would also be good to #error in this code if MOZ_LINKER is defined and the platform isn't Android or Linux, so that we know to revisit this code in case we ever implement MOZ_LINKER on other platforms.

What are the possible directories that libnss could be in on Android and on Linux when the new linker is used? Are these directories that only the current app (on Android) or current user (on Linux) can write to?
Comment 5 Mike Hommey [:glandium] 2011-12-29 00:50:24 PST
They are in no directory, they are in a zip.
Comment 6 Mike Hommey [:glandium] 2011-12-29 08:32:28 PST
Created attachment 584758 [details] [diff] [review]
Load nssckbi from the location of nss3.
Comment 7 Kai Engert (:kaie) 2012-01-10 11:22:20 PST
Comment on attachment 584758 [details] [diff] [review]
Load nssckbi from the location of nss3.

I think we don't need the additional changes in patch v2.
I like v1 better.
Marking v2 as obsolete.

The strategy used in patch v1 has been proposed in bug 486716.
We should use it on all platforms.
Comment 8 Kai Engert (:kaie) 2012-01-10 11:23:47 PST
(In reply to Brian Smith (:bsmith) from comment #2)
> Is it possible for an addon or plugin to to load its own module also named
> nss3, such that PR_GetLibraryFilePathname could return the path to that
> library instead of the path to NSS's libnss3?
> 
> Does this patch work correctly when the path to libnss3 contains non-ASCII
> characters?


We shouldn't have to worry about these theoretical issues.

According to Bob, this strategy is already used by NSS to find one of the other core crypto modules of NSS (freebl).
Comment 9 Kai Engert (:kaie) 2012-01-10 11:28:42 PST
Comment on attachment 584530 [details] [diff] [review]
[v1] Load nssckbi from the location of nss3

>+      if (possible_ckbi_locations[il] == nss_lib) {
>+        // Get the location of the nss3 library.
>+        char *library_name = PR_GetLibraryName(nsnull, nss_lib);


I would another non-null check.
However, in bug 486716, Bob proposes to use

  static const char *library_name = SHLIB_PREFIX"nss3."SHLIB_SUFFIX;  

If you like this approach, you could get rid of the function call and avoid having to add another non-null check.


>+        char *nss_path = PR_GetLibraryFilePathname(library_name, (PRFuncPtr) NSS_Initialize);
>+        PR_FreeLibraryName(library_name);


and yould avoid alloc/free, too.


r=kaie with this change.
Comment 11 Ed Morley [:emorley] 2012-01-11 18:04:05 PST
https://hg.mozilla.org/mozilla-central/rev/345863ffe8c8
Comment 12 Mike Hommey [:glandium] 2012-01-25 09:12:22 PST
Created attachment 591498 [details] [diff] [review]
Load nssckbi from the location of nss3 - for aurora

[Approval Request Comment]
This is a dependency of bug 683127 (bug 683127 doesn't work without this). It adds a location where PSM looks for nssckbi, which is where nss3 was loaded from. This fixes bug 486716. It has been on m-c for 2 weeks.
Comment 13 Mike Hommey [:glandium] 2012-01-26 23:57:09 PST
https://hg.mozilla.org/releases/mozilla-aurora/rev/06ee9e6ba425

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