679524 - Crash in NSS when using NSS_InitContext

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Elio Maldonado]

Attachment: stack trace

As reported by Rich Meggison

This is with the latest NSS on RHEL 6.1
Name        : nss                          Relocations: (not relocatable)
Version     : 3.12.9                       Vendor: Red Hat, Inc.
Release     : 9.el6                        Build Date: Thu 21 Apr 2011 03:10:06 PM MDT

The stack trace is attached.  The code has two threads calling NSS_InitContext at the same time.  The crash occurs in SECOID_Init.  I don't believe this barrier is sufficient:
    if (oidhash) {
    return SECSuccess; /* already initialized */
    }

The crash occurs here:

    for ( i = 0; i < SEC_OID_TOTAL; i++ ) {
        oid = &oids[i];

        PORT_Assert ( oid->offset == i );

 ->       entry = PL_HashTableAdd( oidhash, &oid->oid, (void *)oid );

because there are two different threads accessing oidhash at the same time.

It's not easy to reproduce because it is a timing thing, but I suppose I could try to write a small test program.

Comment 1

[Robert Relyea]

Attachment: Allow only one thread to init and shutdown at a time.

Traditionally NSS was initialized once for an application. This code still expects the application wide initialization to happen once. Now, however, various libraries can now initialize NSS on the fly. Often these libraries will initialize NSS use it, then shut it down (because the library doesn't have any forward carrying state). It's also possible that these libraries may be working on different threads, thus making it possible for NSS_Init and NSS_Shutdown to be called on different threads at the same time.

this code protects the various NSS initialization routines from multiple calls. The locks NSS uses to protect this are now leaked because it's not safe to free them at shutdown (as they may still be in use).

bob

Comment 2

[Elio Maldonado]

Comment on attachment 553640 [details] [diff] [review]
Allow only one thread to init and shutdown at a time.

r+ from me.

Comment 3

[Robert Relyea]

cvs commit: Examining .
Checking in nssinit.c;
/cvsroot/mozilla/security/nss/lib/nss/nssinit.c,v  <--  nssinit.c
new revision: 1.109; previous revision: 1.108
done

wtc, you can still review this if you want. If you have any comments that need addressing just reopen the bug, otherwise you can just clear the review flag.

bob

Comment 4

[Wan-Teh Chang]

Attachment: Add a missing return statement to NSS_Shutdown

This fixes compilation error on Windows.  I think it
should also fix some test failures on other platforms.

Patch checked in on the NSS trunk (NSS 3.13).

Checking in nssinit.c;
/cvsroot/mozilla/security/nss/lib/nss/nssinit.c,v  <--  nssinit.c
new revision: 1.110; previous revision: 1.109
done

Comment 5

[Wan-Teh Chang]

Bob: I didn't get your comment 3 because I was not
on the cc list.  Sorry about the late review.  I
will review your patch later.

Comment 6

[Wan-Teh Chang]

Comment on attachment 553640 [details] [diff] [review]
Allow only one thread to init and shutdown at a time.

This patch causes all the NSS "memleak" tinderboxes to fail
because it introduces two memory leaks:

NEW STACK: vfychain/main/NSS_Init/nss_Init/PR_CallOnce/nss_doLockInit/PR_NewCondVar/PR_Malloc/malloc
NEW STACK: vfychain/main/NSS_Init/nss_Init/PR_CallOnce/nss_doLockInit/PR_NewLock/PR_Calloc/calloc

Other examples:
NEW STACK: selfserv/main/NSS_Initialize/nss_Init/PR_CallOnce/nss_doLockInit/PR_NewCondVar/PR_Malloc/malloc
NEW STACK: selfserv/main/NSS_Initialize/nss_Init/PR_CallOnce/nss_doLockInit/PR_NewLock/PR_Calloc/calloc

NEW STACK: strsclnt/main/NSS_Initialize/nss_Init/PR_CallOnce/nss_doLockInit/PR_NewCondVar/PR_Malloc/malloc
NEW STACK: strsclnt/main/NSS_Initialize/nss_Init/PR_CallOnce/nss_doLockInit/PR_NewLock/PR_Calloc/calloc

NEW STACK: ocspclnt/main/NSS_Init/nss_Init/PR_CallOnce/nss_doLockInit/PR_NewCondVar/PR_Malloc/malloc
NEW STACK: ocspclnt/main/NSS_Init/nss_Init/PR_CallOnce/nss_doLockInit/PR_NewLock/PR_Calloc/calloc

We need to fix the memory leaks, or arrange for the "memleak"
tinderboxes to ignore these two memory leak call stacks.

Comment 7

[Robert Relyea]

They should be on the ignore list. The introduction of a new one-time leak with this patch is already known.

The stack should be */nss_init/PR_CallOnce/nss_doLockInit/*

wtc, do you remember where the exclude stack files are?

bob

Comment 8

[Wan-Teh Chang]

Bob: I don't remember.  I always asked Slavo to exclude memory
leak call stacks for me.  If the exclude stack files are in the
NSS CVS tree, they should be under mozilla/security/nss/tests.
I hope they are not just on the "memleak" tinderboxes' file
systems.

Comment 9

[Wan-Teh Chang]

Bob: I found the file of ignored leak stacks:
http://mxr.mozilla.org/security/find?text=&string=leak

Could you add the stack to mozilla/security/nss/tests/memleak/ignored?
Thanks.

Comment 10

[Robert Relyea]

dhcp-227.sjc.redhat.com(57) cvs commit
cvs commit: Examining .
Checking in ignored;
/cvsroot/mozilla/security/nss/tests/memleak/ignored,v  <--  ignored
new revision: 1.81; previous revision: 1.80
done

Comment 11

[Wan-Teh Chang]

Bob: the leak stack that you checked in has nss_init.
I think "init" needs to be capitalized: nss_Init.

Comment 12

[Wan-Teh Chang]

The CVS commit message referenced the wrong bug number 679424.
(The bug number is the tests/memleak/ignored file is correct.)

Comment 13

[Wan-Teh Chang]

Fixed typo nss_init => nss_Init.

Checking in ignored;
/cvsroot/mozilla/security/nss/tests/memleak/ignored,v  <--  ignored
new revision: 1.82; previous revision: 1.81
done

Comment 14

[Robert Relyea]

Comment on attachment 556977 [details] [diff] [review]
Add a missing return statement to NSS_Shutdown

r+ rrelyea

Comment 16

[Robert Relyea]

Comment on attachment 553640 [details] [diff] [review]
Allow only one thread to init and shutdown at a time.

clearing review request