Bug 472489

I compiled a version of curl with nss but without our two nss patches and found that that version works.  Looking into which of our two nss patches is causing the issues now.

This is the patch causing the issue: curl-7.18.2-nss-thread-safety.patch

And here's the offending hunk in the patch::

@@ -808,7 +823,8 @@
     return CURLE_OK;
 
   /* FIXME. NSS doesn't support multiple databases open at the same time. */
-  if(!initialized) {
+  PR_Lock(nss_initlock);
+  if(!initialized && !NSS_IsInitialized()) {
     initialized = 1;

If I change that hunk to this the test case works::

@@ -808,7 +823,8 @@
     return CURLE_OK;
 
   /* FIXME. NSS doesn't support multiple databases open at the same time. */
-  if(!initialized) {
+  PR_Lock(nss_initlock);
+  if(!initialized) {
     initialized = 1;
 

At this point I really have reached the end of where I can effectively diagnose this.  Someone who's familiar with nss should look at whether the right thing to do is to remove the NSS_IsInitialized() check as I've done or to move some of the code from within the !initialized block out.  I can experiment more but I'll be largely in the dark about whether the fix is "correct", just whether or not it works.

This seems to be exactly the problem mentioned here:
  https://bugzilla.redhat.com/show_bug.cgi?id=459297#c9

Created attachment 324333 [details]
Check whether we initialized NSS separately

Here's the patch I'm using now.  It checks whether libcurl initialized nss and shuts down nss if so.  If not, it leaves it up to the program to shutdown nss separately.  It runs all of libcurl's initialization except for NSS_Initialize().  This should address all the comments I saw in bug #459297.

My previous comments about needing to have my work reviewed still stands, though :-)

It replaces the current curl-7.18.2-nss-thread-safety.patch

Hi Kai,

Not sure if you or rcrit would be the person to look at this revised patch.  The previous patch fails to properly initialize libcurl when NSS has already been initialized by some other code.  The revised patch attempts to be more fine grained but I'm not an NSS guru so a review would be appreciated.

Thanks doing so much of the research. I'm looking into this now.

Ok, I've figured out what the problem is but the solution isn't so obvious.

The real fix is the call to NSS_SetDomesticPolicy().

When NSS is initialized it disables all ciphers.

rpm is apparently not enabling any ciphers either directly or by calling NSS_Set*Policy().

So when it gets into libcurl and we detect that NSS is initialized we don't call NSS_SetDomesticPolicy(). This means that when NSS starts creating a client socket it finds that it has no ciphers enabled and gives up.

What we need is an AreAnyCiphersEnabled() call. If this returns FALSE then we can call NSS_SetDomesticPolicy(). Unfortunately no such call currently exists AFAIK.

We shouldn't simply call NSS_SetDomesticPolicy() because then any settings done prior to getting into libcurl will be silently dropped.

Created attachment 325609 [details]
See if any ciphers are enabled

New patch that adds a test to see if any ciphers have been enabled and if not, call NSS_SetDomesticPolicy().

Verified the new patch works here as a replacement for curl-7.18.2-nss-thread-safety.patch.

It seems like initializing globals in a library is prone to clashes, though (what if rpm did initialize the Policy but only enabled a single cipher, for instance?).  Does libnss not have a session or context abstraction?  So we could create an nss context and then initialize that context separately from the context that rpm creates?

Patch from comment #9 is now applied in rawhide.

You can only call NSS_Init* once in a program. Subsequent calls are ignored.

Calling NSS_Init* initializes some default values such as what ciphers are available (none by default).

One can override this on both the default level and the per-socket level. So globally you can enable cipher X but disable it in a given connection if you want.

(In reply to comment #11)
> Patch from comment #9 is now applied in rawhide.

Thanks jnovy.  If we could get this applied to Fedora 9 & 10 as well, it will fix some bugs we see when using koji's python bindings and pycurl together.

Jnovy, do you plan on pushing this fix to Fedora 8-10 ?

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

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

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

curl-7.18.2-7.fc8 has been pushed to the Fedora 8 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing-newkey update curl'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F8/FEDORA-2008-11405

curl-7.18.2-7.fc9 has been pushed to the Fedora 9 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing-newkey update curl'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F9/FEDORA-2008-11375

curl-7.18.2-9.fc10 has been pushed to the Fedora 10 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update curl'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F10/FEDORA-2008-11340

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

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

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