Bug 459297

Do we support flash without nspluginwrapper?

As an option yes.  The distro allows you to use firefox with native plugins if you uninstall nspluginwrapper.

https://bugzilla.mozilla.org/show_bug.cgi?id=427715
https://bugzilla.mozilla.org/show_bug.cgi?id=450468
These two NSS bugs seem to be related to this particular crash.  It fails similarly with native Firefox and nspluginwrapper.  So this might be a bug not in Flash.

This seems to be caused by multiple calls to NSS_Init, NSS_Shutdown, partly triggered by the flash plugin (init), partly triggered by libcurl, which always calls NSS_Shutdown unconditionally during cleanup.

Rob C: 

Does libcurl detect, whether NSS has already been initialized?
If it's already initialized, does it avoid the call to NSS_Init* ?

That would be good.

In addition, it would be helpful if the same bool were used to avoid calling NSS_Shutdown.

> In addition, it would be helpful if the same bool were used to avoid calling
> NSS_Shutdown.

/* Global cleanup */
void Curl_nss_cleanup(void)
{
  NSS_Shutdown();
  initialized = 0;
}

If a call to NSS_IsInitialized() is added prior to these two lines, is there a way to lock to prevent the tiny potential of another thread racing between?

libcurl is actually not smart enough to detect that NSS is already initialized. NSS is detecting this itself in nss_Init():

I think this is the right fix (untested). It will call NSS_Shutdown() only if libcurl did the initialization:

--- curl-7.18.2.orig/lib/nss.c  2008-05-26 11:02:49.000000000 -0400
+++ curl-7.18.2/lib/nss.c       2008-09-02 21:32:27.000000000 -0400
@@ -730,7 +730,8 @@
 /* Global cleanup */
 void Curl_nss_cleanup(void)
 {
-  NSS_Shutdown();
+  if (initialized)
+    NSS_Shutdown();
   initialized = 0;
 }
 
@@ -805,7 +806,7 @@
   curlerr = CURLE_SSL_CONNECT_ERROR;
 
   /* FIXME. NSS doesn't support multiple databases open at the same time. */
-  if(!initialized) {
+  if(!initialized && !NSS_IsInitialized()) {
     initialized = 1;
 
     certDir = getenv("SSL_DIR"); /* Look in $SSL_DIR */

This is exactly what I was hinting at.  Doing an if conditional prior to NSS_Shutdown() in libcurl does not guarantee that initialized could change between testing it and NSS_Shutdown().  Isn't there still the potential for racy behavior here?

It seems that NSS internally must do locking, so NSS_Init and NSS_Shutdown cannot happen simultaneously in multiple threads?

Hmm, looking at libcurl-*/lib/ssluse.c it appears that the old OpenSSL equivalent could be racy as well?  (This is important for RHEL5 support.)

Probably only in a misbehaving libcurl app. curl_global_cleanup() which ends up calling NSS_Shutdown() is only supposed to be called when an an application exits, not between SSL requests.

NSS_Shutdown() isn't equivalent to the OpenSSL SSL_shutdown() function. 

SSL_shutdown is a per-request function that closes down an SSL connection. 

NSS_Shutdown() unloads NSS.

I think the bigger threat is multiple calls to NSS_Initialize() in a threaded application.

I'll take a crack at creating a lock in Curl_nss_init() that we can use.

Created attachment 315638 [details]
Add locking around NSS_Initialize()

curl_global_init() and curl_global_cleanup() are both documented as not thread-safe so these new conditionals in Curl_nss_init() and Curl_nss_cleanup() are ok.

Note that there is a somewhat subtle side-effect here.

If NSS is initialized outside of libcurl then it won't be re-initialized here even if if the global variable initialized is not set because we also check NSS_IsInitialized().

NSS_Shutdown() will not be called if the global initialized is not set. So if NSS is initialized outside of libcurl it needs to be shut down there as well.

> -  NSS_Shutdown();
> +  if (initialized)
> +    NSS_Shutdown();

This isn't protected by the lock.  Isn't there a chance of it being pre-empted between the if conditional and NSS_Shutdown()?

Created attachment 315685 [details]
Also lock during cleanup to make it thread-safe

Make the cleanup function thread-safe too for safety purposes

curl-7.18.2-5.fc9 has been submitted as an update for Fedora 9.
http://admin.fedoraproject.org/updates/curl-7.18.2-5.fc9

curl-7.18.2-5.fc8 has been submitted as an update for Fedora 8.
http://admin.fedoraproject.org/updates/curl-7.18.2-5.fc8

Can I just remind you that the upstream project appreciate getting patches...

Hold your horses, I'm working on it :-) I need to make sure it applies cleanly to the source tip before submitting it.

curl-7.18.2-5.fc8 has been pushed to the Fedora 8 stable repository.  If problems still persist, please make note of it in this bug report.

curl-7.18.2-5.fc9 has been pushed to the Fedora 9 stable repository.  If problems still persist, please make note of it in this bug report.