391771 - pk11_config_name and pk11_config_strings leaked on shutdown

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This bug is filed based on the NSS_3_12_ALPHA1B tag, which is what is currently used by the trunk of Firefox, etc.

The pk11_config_name and pk11_config_strings variables are leaked on shutdown.  Adding the following code in security/nss/lib/nss/nssinit.c fixes the leak, although I'm not sure if it's the correct time to free them:

+    if (pk11_config_name != NULL) {
+        PORT_Free(pk11_config_name);
+        pk11_config_name = NULL;
+    }
+    if (pk11_config_strings != NULL) {
+       PR_smprintf_free(pk11_config_strings);
+       pk11_config_strings = NULL;
+    }

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

David, one wonders WHERE in nssinit.c you added the above lines of code.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

NSS_Shutdown

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

The fix for this bug should not be committed while it remains blocked by 
bug 391815, and bug 391815 remains unresolved.

Comment 4

[Julien Pierre]

pk11_config_name is allocated in PK11_ConfigurePKCS11, which is a legacy function that is not called in any NSS program. Only PSM must be calling it. This is why our testing didn't pick up this memory leak.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

lxr will show you all the callers of the function(s) that allocate these.
If the callers are only in PSM, then this is a PSM bug.

Comment 6

[Julien Pierre]

PK11_ConfigurePKCS11 is a function implemented and exported from NSS, and the pointer variables are declared static in nssinit.c, so there would be no way to free the memory from the PSM side. This needs to be done on the NSS side, probably at shutdown time.

Comment 7

[Julien Pierre]

Attachment: Fix string leaks at shutdown time

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 288751 [details] [diff] [review]
Fix string leaks at shutdown time

Comments:
1. In one of the error paths of function PK11_ConfigurePKCS11, it frees
the string named pk11_config_strings, but does not NULL the pointer.
With this new patch, that would result in a double-free (hence the r-).
The solution is to NULL that pointer when it is freed in PK11_ConfigurePKCS11.

2. While we're in there, let's cleanup PK11_ConfigurePKCS11 so that all the
error paths free and NULL both pk11_config_name and pk11_config_strings.

3. I think the right place to free those strings is not in NSS_Shutdown but
rather is in nss_Init. If nss_Init fails, the caller will not call NSS_Shutdown, 
and so they will be leaked.  nss_Init is the only place in NSS that uses the 
values of these strings.  It copies them into a larger string.  See
http://lxr.mozilla.org/security/source/security/nss/lib/nss/nssinit.c#478
It appears to me that it should simply free them immediately after it uses
them, at label loser:, and should NULL the pointers there.

Comment 9

[Julien Pierre]

Comment on attachment 288751 [details] [diff] [review]
Fix string leaks at shutdown time

elson,

Re: comment 8,

1. where pk11_config_strings is freed, it immediately gets reassigned to another non-NULL valid pointer :

    newStrings = PR_smprintf("%s minPS=%d", strings, minPwd);
    PR_smprintf_free(strings);
    strings = newStrings;
   if (strings == NULL) return;

    if (pk11_config_strings != NULL) {
	PR_smprintf_free(pk11_config_strings);
    }
    pk11_config_strings = strings;

So, the code I added at shutdown time cannot cause a double-free of pk11_config_strings.

2. I'm not sure if that's desirable. If those strings had a previously valid value, and PK11_ConfigurePKCS11 fails, wouldn't we want to retain those previous values ?

3. I see your point. However, freeing those strings in nss_Init would also mean that subsequent nss_Init calls would revert to the default names. In a product like the browser, which we know is calling this function, and may be doing it multiple times during profile switch, I think that's not the desired behavior. I also know of a server product that also makes multiple NSS initialization attempts, but only calls PK11_ConfigurePKCS11 once.

If we knew for sure that the strings that are passed in to PK11_ConfigurePKCS11 are const, we wouldn't have to copy them at all - only hold references to them, and then we could move all the string processing/allocation to nss_Initialize. Unfortunately, there is no way to guarantee that. The arguments to PK11_ConfigurePKCS11 are const char*, but that only allows constant strings to be passed in, it doesn't require them to be . So that is not an option either.

I think we may have to leave that part of the leak unplugged and decide that if the application never gets to initialize NSS successfully, and thus never gets to call NSS_Shutdown, then this leak will remain.

I think this PK11_ConfigurePKCS11 API is not very well designed. These string arguments should be passed in to NSS_Initialize somehow, but they are not. This seems to be a kludge to pass them in without having to change the NSS initialization function prototype.

We can fix the remaining part of the leak by adding a corresponding PK11_ConfigurePKCS11_FreeStrings(void), to be called specifically this failure case ... Or an NSS_Failed_Shutdown(void). But that wouldn't be very elegant, and requires application participation to plug it.

If we had some kind of global NSPR pool to allocate from, maybe we could use it in PK11_ConfigurePKCS11, copy the strings in there, and then rely on PR_Cleanup to free them rather than NSS_Shutdown. But that's not very good design either - freeing at a different layer than we are allocating.

I am not going to try to implement those other solutions unless there is agreement. In the meantime, please reconsider this patch, in light of my comments.

Comment 10

[Julien Pierre]

Comment on attachment 288751 [details] [diff] [review]
Fix string leaks at shutdown time

Hmm, actually this patch would work for the server product, which calls PK11_ConfigurePKCS11 once, and then does multiple NSS_Init attempts and no reinitialization.
But it wouldn't work for the browser with profile switching - the token names would be lost after switching. The only fix is to add an explicit API to free these strings. I propose PK11_UnconfigurePKCS11 , and will attach a patch for that.

Comment 11

[Julien Pierre]

Attachment: Free the strings in new PK11_UnconfigurePKCS11 call

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 288903 [details] [diff] [review]
Free the strings in new PK11_UnconfigurePKCS11 call

ulien,  (:-)
You're right, I was mistaken about the potential for double free.

Comment 13

[Julien Pierre]

Attachment: Also expose PK11_UnconfigurePKCS11 in nss.h

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 288923 [details] [diff] [review]
Also expose PK11_UnconfigurePKCS11 in nss.h

I should have caught this in my review. :-/

>  * set the PKCS #11 strings for the internal token.
>+ * These remain good for all subsequent NSS initializations until
>+ * PK11_UnconfigurePKCS11 is called.

I would add: or until PK11_ConfigurePKCS11 is called again.

Comment 15

[Robert Relyea]

> If we knew for sure that the strings that are passed in to PK11_ConfigurePKCS11
> are const, we wouldn't have to copy them at all - only hold references to them,
> and then we could move all the string processing/allocation to nss_Initialize.
> Unfortunately, there is no way to guarantee that. The arguments to
> PK11_ConfigurePKCS11 are const char*, but that only allows constant strings to
> be passed in, it doesn't require them to be . So that is not an option either.

const char * is not the same as char const *. I am quite sure the callers are not passing char const * values in. (The values are likely read from some bundle, and freed afterwards, to do otherwise would be a burden on the application, so there is no way around PK11_ConfigurePKCS11() owning the resulting memory and having to free it later).

> I propose PK11_UnconfigurePKCS11 , and will attach a patch for that.

This is the best idea of those presented. Some applications (including mozilla, have a hierarchial initialization of NSS, where it first tries to initialize it R/W, then R/O, then without a database. We don't want to loose the strings because the first of those failed.

Comment 16

[Robert Relyea]

Comment on attachment 288923 [details] [diff] [review]
Also expose PK11_UnconfigurePKCS11 in nss.h

r+

Comment 17

[Julien Pierre]

I checked in this patch to the trunk .
Checking in nssinit.c;
/cvsroot/mozilla/security/nss/lib/nss/nssinit.c,v  <--  nssinit.c
new revision: 1.86; previous revision: 1.85
done
Checking in nss.def;
/cvsroot/mozilla/security/nss/lib/nss/nss.def,v  <--  nss.def
new revision: 1.183; previous revision: 1.182
done

And NSS_3_11_BRANCH :
Checking in nss.def;
/cvsroot/mozilla/security/nss/lib/nss/nss.def,v  <--  nss.def
new revision: 1.158.2.8; previous revision: 1.158.2.7
done
Checking in nssinit.c;
/cvsroot/mozilla/security/nss/lib/nss/nssinit.c,v  <--  nssinit.c
new revision: 1.69.2.9; previous revision: 1.69.2.8
done

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #15)
> const char * is not the same as char const *. I am quite sure the callers are
> not passing char const * values in. (The values are likely read from some

Actually "const char*" and "char const*" are the same.  What's different is "char * const", but that (at least in C++, where such things matter more) isn't part of the type of a function, just like a "const int" isn't part of the type of a function (instead, it just says the argument can't be modified inside the function, so its effects are entirely local).

Comment 19

[Slavomir Katuscak]

Patches causes build failures on Linux on branch:

/usr/bin/ld:Linux2.6_x86_64_glibc_PTH_64_DBG.OBJ/nss.def:896: syntax error in VERSION script
collect2: ld returned 1 exit status
gmake[2]: *** [Linux2.6_x86_64_glibc_PTH_64_DBG.OBJ/libnss3.so] Error 1
gmake[2]: Leaving directory `/export/tinderbox/Linux_2.6.9-42.ELsmp/mozilla/security/nss/lib/nss'
gmake[1]: *** [libs] Error 2
gmake[1]: Leaving directory `/export/tinderbox/Linux_2.6.9-42.ELsmp/mozilla/security/nss/lib'
gmake: *** [libs] Error 2

On line 896 of nss.def is last brace }; before NSS 3.11.9 linr added by patch.

Comment 20

[Julien Pierre]

Sorry about that. I checked in a fix to unbreak the Linux build to NSS_3_11_BRANCH .

Checking in nss.def;
/cvsroot/mozilla/security/nss/lib/nss/nss.def,v  <--  nss.def
new revision: 1.158.2.9; previous revision: 1.158.2.8
done

Comment 21

[Slavomir Katuscak]

Julien, seems that there are also problems on HPUX caused by those patches (in branch only).

rm -f HP-UXB.11.11_OPT.OBJ/libnss3.sl
ld -b +h libnss3.sl -c HP-UXB.11.11_OPT.OBJ/nss.def -o HP-UXB.11.11_OPT.OBJ/libnss3.sl HP-UXB.11.11_OPT.OBJ/nssinit.o HP-UXB.11.11_OPT.OBJ/nssver.o ../certhigh/HP-UXB.11.11_OPT.OBJ/certhtml.o ../certhigh/HP-UXB.11.11_OPT.OBJ/certreq.o ../certhigh/HP-UXB.11.11_OPT.OBJ/crlv2.o ../certhigh/HP-UXB.11.11_OPT.OBJ/ocsp.o ../certhigh/HP-UXB.11.11_OPT.OBJ/certhigh.o ../certhigh/HP-UXB.11.11_OPT.OBJ/certvfy.o ../certhigh/HP-UXB.11.11_OPT.OBJ/xcrldist.o ../cryptohi/HP-UXB.11.11_OPT.OBJ/sechash.o ../cryptohi/HP-UXB.11.11_OPT.OBJ/seckey.o ../cryptohi/HP-UXB.11.11_OPT.OBJ/secsign.o ../cryptohi/HP-UXB.11.11_OPT.OBJ/secvfy.o ../cryptohi/HP-UXB.11.11_OPT.OBJ/dsautil.o ../pk11wrap/HP-UXB.11.11_OPT.OBJ/dev3hack.o ../pk11wrap/HP-UXB.11.11_OPT.OBJ/pk11akey.o ../pk11wrap/HP-UXB.11.11_OPT.OBJ/pk11auth.o ../pk11wrap/HP-UXB.11.11_OPT.OBJ/pk11cert.o ../pk11wrap/HP-UXB.11.11_OPT.OBJ/pk11cxt.o ../pk11wrap/HP-UXB.11.11_OPT.OBJ/pk11err.o ../pk11wrap/HP-UXB.11.11_OPT.OBJ/pk11kea.o ../pk11wrap/HP-UXB.11.11_OPT.OBJ/pk11list.o ../pk11wrap/HP-UXB.11.11_OPT.OBJ/pk11load.o ../pk11wrap/HP-UXB.11.11_OPT.OBJ/pk11mech.o ../pk11wrap/HP-UXB.11.11_OPT.OBJ/pk11nobj.o ../pk11wrap/HP-UXB.11.11_OPT.OBJ/pk11obj.o ../pk11wrap/HP-UXB.11.11_OPT.OBJ/pk11pars.o ../pk11wrap/HP-UXB.11.11_OPT.OBJ/pk11pbe.o ../pk11wrap/HP-UXB.11.11_OPT.OBJ/pk11pk12.o ../pk11wrap/HP-UXB.11.11_OPT.OBJ/pk11pqg.o ../pk11wrap/HP-UXB.11.11_OPT.OBJ/pk11sdr.o ../pk11wrap/HP-UXB.11.11_OPT.OBJ/pk11skey.o ../pk11wrap/HP-UXB.11.11_OPT.OBJ/pk11slot.o ../pk11wrap/HP-UXB.11.11_OPT.OBJ/pk11util.o ../certdb/HP-UXB.11.11_OPT.OBJ/alg1485.o ../certdb/HP-UXB.11.11_OPT.OBJ/certdb.o ../certdb/HP-UXB.11.11_OPT.OBJ/certv3.o ../certdb/HP-UXB.11.11_OPT.OBJ/certxutl.o ../certdb/HP-UXB.11.11_OPT.OBJ/crl.o ../certdb/HP-UXB.11.11_OPT.OBJ/genname.o ../certdb/HP-UXB.11.11_OPT.OBJ/stanpcertdb.o ../certdb/HP-UXB.11.11_OPT.OBJ/polcyxtn.o ../certdb/HP-UXB.11.11_OPT.OBJ/secname.o ../certdb/HP-UXB.11.11_OPT.OBJ/xauthkid.o ../certdb/HP-UXB.11.11_OPT.OBJ/xbsconst.o ../certdb/HP-UXB.11.11_OPT.OBJ/xconst.o ../util/HP-UXB.11.11_OPT.OBJ/quickder.o ../util/HP-UXB.11.11_OPT.OBJ/secdig.o ../util/HP-UXB.11.11_OPT.OBJ/derdec.o ../util/HP-UXB.11.11_OPT.OBJ/derenc.o ../util/HP-UXB.11.11_OPT.OBJ/dersubr.o ../util/HP-UXB.11.11_OPT.OBJ/dertime.o ../util/HP-UXB.11.11_OPT.OBJ/nssb64d.o ../util/HP-UXB.11.11_OPT.OBJ/nssb64e.o ../util/HP-UXB.11.11_OPT.OBJ/nssrwlk.o ../util/HP-UXB.11.11_OPT.OBJ/nssilock.o ../util/HP-UXB.11.11_OPT.OBJ/nsslocks.o ../util/HP-UXB.11.11_OPT.OBJ/portreg.o ../util/HP-UXB.11.11_OPT.OBJ/pqgutil.o ../util/HP-UXB.11.11_OPT.OBJ/secalgid.o ../util/HP-UXB.11.11_OPT.OBJ/secasn1d.o ../util/HP-UXB.11.11_OPT.OBJ/secasn1e.o ../util/HP-UXB.11.11_OPT.OBJ/secasn1u.o ../util/HP-UXB.11.11_OPT.OBJ/secitem.o ../util/HP-UXB.11.11_OPT.OBJ/secoid.o ../util/HP-UXB.11.11_OPT.OBJ/sectime.o ../util/HP-UXB.11.11_OPT.OBJ/secport.o ../util/HP-UXB.11.11_OPT.OBJ/secinit.o ../util/HP-UXB.11.11_OPT.OBJ/utf8.o ../pki/HP-UXB.11.11_OPT.OBJ/asymmkey.o ../pki/HP-UXB.11.11_OPT.OBJ/certificate.o ../pki/HP-UXB.11.11_OPT.OBJ/cryptocontext.o ../pki/HP-UXB.11.11_OPT.OBJ/symmkey.o ../pki/HP-UXB.11.11_OPT.OBJ/trustdomain.o ../pki/HP-UXB.11.11_OPT.OBJ/tdcache.o ../pki/HP-UXB.11.11_OPT.OBJ/certdecode.o ../pki/HP-UXB.11.11_OPT.OBJ/pkistore.o ../pki/HP-UXB.11.11_OPT.OBJ/pkibase.o ../pki/HP-UXB.11.11_OPT.OBJ/pki3hack.o ../dev/HP-UXB.11.11_OPT.OBJ/devslot.o ../dev/HP-UXB.11.11_OPT.OBJ/devtoken.o ../dev/HP-UXB.11.11_OPT.OBJ/devutil.o ../dev/HP-UXB.11.11_OPT.OBJ/ckhelper.o ../base/HP-UXB.11.11_OPT.OBJ/arena.o ../base/HP-UXB.11.11_OPT.OBJ/error.o ../base/HP-UXB.11.11_OPT.OBJ/errorval.o ../base/HP-UXB.11.11_OPT.OBJ/hashops.o ../base/HP-UXB.11.11_OPT.OBJ/libc.o ../base/HP-UXB.11.11_OPT.OBJ/tracker.o ../base/HP-UXB.11.11_OPT.OBJ/item.o ../base/HP-UXB.11.11_OPT.OBJ/utf8.o ../base/HP-UXB.11.11_OPT.OBJ/list.o ../base/HP-UXB.11.11_OPT.OBJ/hash.o ../base/HP-UXB.11.11_OPT.OBJ/whatnspr.o   -L../../../../dist/HP-UXB.11.11_OPT.OBJ/lib -lsoftokn3 -L../../../../dist/HP-UXB.11.11_OPT.OBJ/lib -lplc4 -lplds4 -lnspr4  -lpthread -lm -lrt
ld: Can't open HP-UXB.11.11_OPT.OBJ/libnss3.sl
ld: No such file or directory
gmake-3.80[3]: *** [HP-UXB.11.11_OPT.OBJ/libnss3.sl] Error 1
gmake-3.80[3]: Leaving directory `/share/builds/mccrel3/security/securityjes5/builds/20071209.1/wozzeck_Solaris8/mozilla/security/nss/lib/nss'
gmake-3.80[2]: *** [libs] Error 2
gmake-3.80[2]: Leaving directory `/share/builds/mccrel3/security/securityjes5/builds/20071209.1/wozzeck_Solaris8/mozilla/security/nss/lib'
gmake-3.80[1]: *** [libs] Error 2
gmake-3.80[1]: Leaving directory `/share/builds/mccrel3/security/securityjes5/builds/20071209.1/wozzeck_Solaris8/mozilla/security/nss'
gmake-3.80: *** [nss] Error 2

Comment 22

[Christophe Ravel]

Fix build on HP-UX: remove the extra blank line at the end of nss.def on NSS_3_11_BRANCH.

Checking in nss.def;
/cvsroot/mozilla/security/nss/lib/nss/nss.def,v  <--  nss.def
new revision: 1.158.2.10; previous revision: 1.158.2.9
done

Comment 23

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Presumably for the leak I reported to be fixed, something would need to call PK11_UnconfigurePKCS11?  Is the idea that the caller would be in PSM, not NSS?  If so, I presume I should file a separate bug on that?  (Or is there a caller that I'm somehow not finding?)

Comment 24

[Nelson Bolyard (seldom reads bugmail)]

(In reply to comment #23)
> Presumably for the leak I reported to be fixed, something would need to call
> PK11_UnconfigurePKCS11?  

Yes.

> Is the idea that the caller would be in PSM, not NSS? 

Yes.

> If so, I presume I should file a separate bug on that?  

Yes.  Probably.

> (Or is there a caller that I'm somehow not finding?)

I haven't looked.

Comment 25

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

OK, filed bug 437690.