496997 - NSPR-free freebl needs to hang onto library references.

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Robert Relyea]

The NSPR-free freebl automatically detects whether NSPR is loaded or not, and if it is it uses the loaded NSPR. The code automatically detects if NSPR is loaded on the fly. This allows an application which uses freebl directly to load another service which loads NSS on the fly. The assumption was that as long as NSS is loaded, it would maintain it's references to NSPR. Currently the code assumes that NSPR would then not be unloaded while freebl is loaded.

There is a problem, however. One application in particular loads and unloads NSS multiple times (indirectly through loading and unloading it's module). This application also calls libgcrypt which loads and uses freebl in the NSPR-free mode. On the first NSS load and use, freebl switches to using NSPR rather than it's internal stubs. The application then unloads NSS. Since freebl does not have a reference to NSPR, NSPR is also unloaded. When it loaded again, it may not be in the same place it was initially loaded to.

Freebl should not free it's reference to NSPR once NSPR is loaded. This will result in the apparent leak of 2 library handles (NSPR and libnssutil3), but these are one-time-per-process only leaks and will guarrentee NSPR would not be unloaded once freebl finds it.

This is a linux only issue since linux is the only platform that supports nspr-free freebl.

Comment 1

[Robert Relyea]

Attachment: Don't free the library if we successfully load NSPR.

Comment 2

[Wan-Teh Chang]

Comment on attachment 382205 [details] [diff] [review]
Don't free the library if we successfully load NSPR.

r=wtc.

Perhaps you can work harder to free the NSPR and
nssutil library references at the right time.

Comment 3

[Robert Relyea]

bobs-laptop(74) cvs commit stubs.c
Checking in stubs.c;
/cvsroot/mozilla/security/nss/lib/freebl/stubs.c,v  <--  stubs.c
new revision: 1.5; previous revision: 1.4
done

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Bob, I agree with this bug's summary, that 
"NSPR-free freebl needs to hang onto library references."  
But that's not what this patch does.  This patch LEAKS those references, 
rather than "hanging onto" them.  I can already imagine the criticisms we'll
receive over that.  I suspect we can do better.

I suggest that, instead of storing those shared library handles in automatic 
variables, whose contents are lost when the function returns, they should be 
stored in static variables.  The give freebl a _fini routine which gets called 
if/when freebl gets unloaded.  It would free up those library handles.  
That way, when all the libraries that depend on nspr are unloaded, nspr can 
also be unloaded.  Isn't that more desirable?

Comment 5

[Robert Relyea]

There isn't a platform independent way to set up those routines for unloading, and on linux (the only platform using the code). The OS automatically recognizes that these load calls were made from this shared library and closes them when unloading this library.

I coded the original they way I did precisely because there was no way to later safely release the libraries. I was basically depending on whoever loaded the libraries in the first place.

bob

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Bob, 

IMO, we don't need a platform-independent way, since this only runs on Linux.

> [Linux] automatically recognizes that these load calls were made from 
> this shared library and closes them when unloading this library.

Really?  That can't be safe!  Consider a dynamically shared library (call it 
libX) with a dlopen-like function.  The application loads libX and calls its
dlopen-like function to load libY.  libX returns the handle from dlopen to
the caller (the application), which then unloads libX.  As I understand what
you wrote, Linux would then unload libY also.  I'm astonished it would do that.

Comment 7

[Robert Relyea]

grumble... I was going by the empirical information collected by someone else (I asked them to try grabbing references to these libraries in their module to hold them in memory even after the module was unloaded).

On more reflection, I agree with you it was surprising so I decided to investigate myself. In fact linux does not unload libraries that were loaded by an unloaded shared library. I must assume the more likely case that the person I asked to try the empirical test either had a problem implementing the test, or the code which was supposed to grab the references did not do so.

Since this is a linux only, _fini should work. The only issue is the potential race condition if two callers try to initialize the the NSPR stubs at the same time and each get references to the NSPR and/or NSSUtil. The result of this race is exactly the same as not coding _fini at all, so that seems like a reasonable compromize.

bob

Comment 8

[Robert Relyea]

Attachment: close file descripters on shared library unload.

1. This patch is on top of the already checked in patch.

2. use of _fini and _init are deprecated in Lunix (they still exist, but are used as part of the OS management. Using them create multiple defines unless you adjust your link line). The new way is to create functions with the ((destructor)) ((constructor) attribute.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 382628 [details] [diff] [review]
close file descripters on shared library unload.

I'm on the fence about whether to give this r+ or r-.
Here are 3 points.

>+void * FREEBLnsprGlobalLib = NULL;
>+void * FREEBLnssutilGlobalLib = NULL;

1. The above two variables should be static, IMO.

>      void *nspr = NULL; 
>      void *nssutil = NULL; 
>  
>      /* NSPR should be first */
>      if (!ptr_PR_DestroyLock) {

2. I think the above test should probably be changed to
       if (!FREEBLnsprGlobalLib) {
   and likewise, the test in the next section below (now shown here)
   should be changed to
       if (!FREEBLnssutilGlobalLib) {
   Do you agree? 

3, I notice that, in this function, if the load and initialization of nspr
succeeds, and the utility library loads, but the initialization of the util 
library fails, we unload the utility library but not nspr.  Now, the good 
news is that this patch will ensure that NSPR ultimately gets unloaded, 
but I'm thinking that this function should be all-or-nothing.  It should 
load both libs, or load no libs.

Comment 10

[Robert Relyea]

> 1. The above two variables should be static, IMO.

Agreed.

> 2. I think the above test should probably be changed to

I thought I had some reason for user the ptr, but I can't remember it now. It probably was the lack of another global, so I'm ok with your suggestion.

> 3, I notice that, in this function, if the load and initialization of nspr
> succeeds, and the utility library loads, but the initialization of the util 
> library fails, we unload the utility library but not nspr.

We don't want to unload NSPR at this point because we've successfully loaded the pointers for the NSPR stubs. They will actually start being used, so we don't want to unload nspr.


I'll provide a new patch with parts 1 & 2 fixed.

bob

Comment 11

[Robert Relyea]

Attachment: Address nelson's comments.

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 382836 [details] [diff] [review]
Address nelson's comments.

r=nelson.  Thanks, Bob.

Comment 13

[Robert Relyea]

Why was this reopened? I'm closing fixed, but if it needs to be reopened please do so and include a comment on why.

bob

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

Bob, it was reopened back at comment 4, and then you didn't re-close it
when you committed your fix after comment 12.  You never gave this bug 
a final "checked in and closed" comment.  I agree that it's fixed now.