453495 - Add new ref counted NSS_Shutdown-like function

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Nelson Bolyard (seldom reads bugmail)]

Now that NSS is being used in libraries that are not dedicated to any
particular application program, it is possible to get two libraries
or a library and a program who all want to initialize and shut down NSS
in the same process address space.  The immediate problem is that 
the first call to NSS_Shutdown shuts down NSS for all code in the process.

The NSS_Init* functions can be changed to do reference counting without
any change to the API, no binary incompatibility, no new functions needed.

NSS_Shutdown can still do as it does today and shutdown NSS unconditionally
without regard to the reference count value.  It can zero the reference 
count.

But we need a new flavor of NSS_Shutdown with a new name that will decrement
the ref count and shut down NSS if it is zero.  This new function could 
just decrement the ref count, and call NSS_Shutdown if it is zero.

I can't think of a good name.  None of the following seems very appealing.
NSS_ShutdownRefCounted
NSS_ShutdownConditional
NSS_ShutdownMaybe
NSS_ByeBye           :)

Comment 1

[Julien Pierre]

Nelson,

I think the initialization is at least as much a problem as the shutdown phase.
And binary compatibility does come into issue.

Right now, NSS_Init functions can be called an indefinite number of times (at least once). If NSS is already initialized, subsequent calls simply succeed silently, without actually doing anything.

No extra NSS shutdown calls are required - only a single one.

So, even if we add a new shutdown function call, there is no assurance that there would be a matching number of shutdown calls to the init calls.

So, I think if we want to have a refcounted shutdown function, the init function should be refcounted as well.

Also, there is the issue of whether the multiple library users, or one library and one executable, etc. all want to use the same NSS cert/key/trust databases, and/or tokens. That may or may not be the case.

IMO, we should have at least a design meeting about how we want to deal with these issues before we start implementation .

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Julien, In my comment 0, I proposed that NSS_Init would do ref counting, and
that doing so would not break any compatibility or necessitate any new Init 
function.  Please re-read it in that light.

The proposal says that Existing users of the existing functions can continue
to get the same binary compatible behavior as they got before, and that 
callers of the new shutdown function can get ref counted behavior.

Comment 3

[Julien Pierre]

Nelson,

Thanks, I didn't read it carefully enough. Still, I'm not sure how useful the proposal really is.

It only helps if all components/libraries use the new refcounted shutdown API. Then it keeps NSS initialized until the last one shuts down.

But if any single component uses the old NSS_Shutdown(), it will shut down early, and break the other components that still intend to continue using NSS.

So in essence this means that all NSS users in a single process - all programs and all libraries - must be rewritten to never use the existing NSS_Shutdown for this to help. That seems of limited benefit.

If we want to make things work more consistently, I think it would make sense to do a major release that removes NSS_Shutdown and replaces it with the new call.

Also, I continue to think this issue of sharing NSS between multiple components is much more than just keep NSS initialized or not. I don't think we can really treat it separately from the issue of which NSS database/modules are used by the executable and libraries.

The fix doesn't necessarily have to be implemented at the same time, but I feel that we should at least know where we stand on that issue before we proceed implementing a fix for this problem, as it may well affect initialization and shutdown APIs too.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

I believe that a new major release of NSS will not happen in our lifetimes.

To even seriously propose a new major release at this time would probably 
convince the LSB that they have made (or are making) a wrong decision in 
adopting NSS.  NSS's big attraction to them was its policy of backwards 
binary compatibility.

Comment 5

[Julien Pierre]

Well, we are at NSS 3.x, not 1.x . Binary compatibility got broken before, and it could get broken again if there is a good enough reason. We have been at 3.x for almost 8 years already.

Just because the LSB is just picking up NSS 3.x at the present time doesn't mean we should rule out ever doing another major release again if there are problems for which this is the only solution. This certainly looks like one because initialization and shutdown is part of every NSS application and we can't just change those requirements without breaking binary compatibility. But on the other hand, if we don't change them, then we really don't achieve what we want either - make sure multiple NSS consumers work well together. The whole problem about multiple consumers is that they don't know about each other, or if there is even another consumer at all, and they must be able to operate the same way regardless . IMO that means not using some other components' cert/trust database just because another library happened to be loaded.

We are talking about trying to do something NSS was not at all designed for. Do we really want a band-aid fix right now ? Or do we want to at least spend some time brainstorming about the whole problem before we jump in and implement something that will help very few cases and fail to implement other requirements of multiple consumers ?

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Another idea that Bob suggested recently is to create new forms of 
NSS_Initialize and NSS_Shutdown, where init would return a handle, and 
shutdown would take that handle as input.

Comment 7

[Wan-Teh Chang]

In other APIs, such a session or context handle would need
to be passed to the creation functions of various objects,
and closing the session/context handle would automatically
destroy all the objects created in that session/context.
Unless we really want to go that far, a reference-counted
flavor of init and shutdown would be equivalent.

Comment 8

[Julien Pierre]

Another possibility that I have been thinking about is to factor out all NSS global variables, and key them all off to some thread-private data.

Then, any library or application code could choose to create or switch to its own private NSS context. There could be 3 new public APIs such as :

NSSContext* NSS_NewContext(void);
NSS_SetContext(NSSContext*);
NSSContext* NSS_GetContext(void);

A library that needs a new separate context would call NSS_NewContext() to create its own, making sure it doesn't interfere with any existing use of NSS.
Library code could look like this :

public_ldap_function()
{
   NSSContext* oldctx = NSS_GetContext()
   NSS_SetContext(ldap_nss_ctx);
   ...
   /* do stuff with NSS here */
   ...
   NSS_SetContext(oldctx);
   return;
}

Application code could remain completely unchanged. A global NSSContext would be created and used if there isn't one already on the thread where NSS gets initialized.

I believe this would solve the problem we have in that it doesn't require any cooperation from applications. Subsystem libraries still need to be changed of course.

There are probably some corner cases with callbacks from one library to anoth or to the application that may be tricky.

Of course the trickiest part would be to hunt down every global in every one of the shared libs we have.

Comment 10

[Robert Relyea]

A proposal for how to fix this is presented at 
https://wiki.mozilla.org/NSS_Library_Init

Comment 12

[Robert Relyea]

Attachment: Multi-init preview: test program (multinit)

This program is the test program I'm using for to test the multi-init code.

What still needs to be done:
   write a test case for importing a cert.
   add code to output a status string that all.sh can test to see if the desired result occured.

Comment 13

[Robert Relyea]

Attachment: Preview: changes to the sysinit code.

This patch pre-reqs bug 5055533. since that patch is not in, this patch includes all the code in 5055533. It's included for completeness. Once 5055533 is approved and checked in, I'll include a new version of this patch which will only have the differences, and as a result will be much smaller.

code differences between this an the original sysinit patch:
1) code has been added to prevent opening the same database multiple times (it's permitted to open a database a second time if the first time is R/O and the second time is R/W and the database is not a dbm database).
2) map the slot and token names from their database type to their generic slot type.

Comment 14

[Robert Relyea]

Attachment: Preview: actual init changes.

Here are the actual init changes.

Comment 15

[Robert Relyea]

Comment on attachment 400874 [details] [diff] [review]
Preview: actual init changes.

This is just to mark that these are available for preview. I will continue working on them, but if you have some pre-comments, I'd be happy to incorporate them.

If this reminder is 'annoying' simply cancel the review.

This is not the state I intend to check the code in. It's only a preview.

bob

Comment 16

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 400874 [details] [diff] [review]
Preview: actual init changes.

I'm cancelling this review request, since this patch includes most of the patch for another bug that has since been revised and reviewed, and (IINM) committed.  So, Please regenerate this patch against the trunk.  Thanks.

Comment 17

[Robert Relyea]

Attachment: Changes to PK11 wrap to support multi init.

1) add code to remember which databases have already been open and prevent those databases from being opened a second time. (if sql is in use, one second open is allowed if the initial open is read only and the subsequent open is r/w).

2) Add an NSS= flag to the module called internalKeySlot, which will cause the 'main database slot' of this module to be the new default internalKeySlot.

3) When parsing the spec, map the appropriate 'main database slot' Slot and Token description paremters to the generic slot and token description parameters (example dbTokenDescription or FIPSTokenDescription to tokenDescription).

4) Work around a softoken bug. Softoken currently reports readonly databases as if they were R/W tokens. In addition Softoken will quietly open a RO session on such a database if asked to open a R/W session (instead of failing). Code was added to work around these to bugs and report RO softoken slots as ReadOnly.

Comment 18

[Robert Relyea]

Attachment: Add reference counted init functions.

Add the new functions NSS_InitContext() and NSS_ShutdownContext().

The following was added that wasn't in the original design:
NSS_InitContext takes an additional structure: NSSInitString.
This structure contains the strings normally set by PK11_ConfigurePKCS11(). In this case a library that uses NSS_InitContext() can pass it's own strings without interference from PK11_ConfigurePKCS11(). Such a library will no longer have to call PK11_ConfigurePKCS11 and interfere with an application's settings.

Making it a parameter was easier and more robust than trying to invent a new semantic for PK11_ConfigurePKCS11 that would still be backward compatible.

Also fixed:

1) nss_init now uses the common double escape function defined in pk11wrap.
2) automatic root cert code has special code to handle an explicit sql:, but not any of our other reserved keywords (of which only dbm: is likely). Added code to handle the other keywords.
3) Different from the previously attached patch: InitContexts are now remembered in a linked list. Rather than simply validating a magic number, the contexts are validated by looking them up in the list.

bob

Comment 19

[Robert Relyea]

Attachment: New Test program

new test program for multinit.

Comment 20

[Robert Relyea]

Attachment: Adding test case to all.sh

This consists of:
1) all.sh changes to add multinit
2) new multinit.sh which uses our existing alicedir, bobdir, and davedir.
3) multinit.txt to define new test cases and list expected output.
4) changes to tools.sh so it doesn't modify alicedir by adding a new certificate.

Comment 21

[Robert Relyea]

I've updated https://wiki.mozilla.org/NSS_Library_Init to include the new parameter in this patch.

Comment 22

[Wan-Teh Chang]

Comment on attachment 406596 [details] [diff] [review]
Add reference counted init functions.

>+/*
>+ * strings used to internationalize softoken
>+ */
>+struct NSSInitStringsStr {
>+   int	  len;  	/* allow this structure to grow in the future */
>+   PRBool passwordRequired;
>+   int    minimumPasswordLength;
>+   char * manufactureID;           /* variable names for strings match the */
>+   char * libraryDescription;      /*   parameter name in softoken */
>+   char * cryptoTokenDescription;
>+   char * dbTokenDescription;
>+   char * FIPSTokenDescription;
>+   char * cryptoSlotDescription;
>+   char * dbSlotDescription;
>+   char * FIPSSlotDescription;
>+};

Nit: We should establish a convention for extensible structures.
libSSL has two such structures whose first field is named 'length':
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/ssl/sslt.h&rev=1.12&mark=129,148#128

I suggest that the first field of NSSInitStrings also be named 'length'.
It probably should use an unsigned type like the two structures in
libSSL.  The same type should be used for minimumPasswordLength.

NSSInitStrings is not accurate because two fields, passwordRequired
and minimumPasswordLength, are not strings, and have nothing to do
with internationalization.  Something like "NSSInitParams" or
"NSSInitAttrs" might be more accurate.

Comment 23

[Robert Relyea]

Thanks for the input Wan-Teh..

Nelson, if you are ok with wtc's comments, assume that I will integrate them into the next version of my patch after your review.

bob

Comment 24

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 406597 [details] [diff] [review]
New Test program

It's really not possible to determine if this patch is "correct" or 
not because there's no specification that defines the correct 
behavior.  So, I'll just make cursory review comments.

There really needs to be a specification for the syntax of the 
long output strings produced by this program.  I'm pretty sure 
that some of the "label" letters are ambiguous, used for more 
than one purpose.  I'd also suggest introducing some sort of 
delimiters into the output strings.  Maybe semicolons.  
I'm pretty sure that if a string ever produced an unexpected result, 
no-one but Bob could figure out what was wrong with it. 

>Index: cmd/multinit/manifest.mn

>+# The Original Code is the Netscape security libraries.
>+#
>+# The Initial Developer of the Original Code is
>+# Netscape Communications Corporation.
>+# Portions created by the Initial Developer are Copyright (C) 1994-2000
>+# the Initial Developer. All Rights Reserved.

I don't think Netscape developed this file, and I think it didn't exist 
in 1994.  It is now 2009.  

>+DEFINES = -DNSPR20

We should have stopped propagating that define in our manifests
10 years ago or more when we dropped support for NSPR 1.


>Index: cmd/multinit/multinit.c

>+ * print out long help. unlike short_help, this does not exit

That comment is misleading. short_help does not exit, either.  

>+struct pushData {
>+   char * data;		/* lowest address of the stack */
>+   char * top;		/* pointer to the next element on the stack */
>+   int  len;		/* length of the stack */
>+};
>+
>+/* our actual stack. If data is NULL, then all push ops except init are noops */

Actual stack?  As opposed to what other kind?

>+static struct pushData stack= { NULL, NULL, 0 };

I spent a LOT of review time trying to understand this "stack". 
Eventually I figured out that it's not a stack at all.  
There's no POP.  It's just a buffer.  
All the "pushType" functions (e.g. pushLabel, pushBoolean) merely
append strings to the buffer, and finally the buffer is written 
and flushed.  It would have saved me a LOT of review time if the 
names had been more conventional.  I suggest doing some global search 
and replace such as:
  :%s/pushFinal/printAndFlush/g
  :%s/push/append/g
  :%s/stack/buffer/g


>+/* push a trust flag */
>+static void
>+pushFlags(unsigned int flag)


Put commas into the printed trust flags to separate SSL trust flags 
from SMIME flags from object signing flags.

As it is, the string produced for a cert that is only trusted for SSL is 
identical to the string that is produced for a cert that is only trusted 
for SMIME.  An error would go undetected because of that ambiguity.  

>+/*
>+ * need to implement yet... try to add a new certificate
>+ */
>+void
>+do_add_cert(const char *progName, int log)
>+{
>+}

Should that be an r-? 
Should the function call abort() until it does something more useful?  


>+   if (log) {
>+	fprintf(stderr, "*Execuing nss command \"%s\" for %s*\n", 
                               ^t

>+static int main_initialized=0;
>+static int lib1_initialized=0;
>+static int lib2_initialized=0;

Nit: statics are always implicitly initialized to zero.

>+static NSSInitContext *lib1_context = 0;
>+static NSSInitContext *lib2_context = 0;

Comment 25

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 406598 [details] [diff] [review]
Adding test case to all.sh

The explanatory text in 
>+++ tests/multinit/multinit.txt	
helps to understand the syntax of the commands (input) to the test 
program, which is documented nowhere else.  But it doesn't help much
with the output. 

Here are examples of strings that demonstrate the need for delimiters.

>+all 1C<Bob>uuuC<Dave>pppC<Eve>pppC<NSS Test CA>CTCCMS<NSS Generic Crypto 

Since 'C' is a valid trust flag letter, the string "pppC" seen above could be 
the trust string "p,p,pC" or it could be "p,p,p" with C meaning something else.
A delimiter to mark the end of the trust string would be good.

CA>CTCCZC<Alice>uuuC<Bob>pupupuC<Dave>pupupuC<Eve>pppC<NSS Test CA>CTCCNIE0xffffe09a

For the life of me, I cannot decode CTCCZC or CTCCNIE. 
Maybe delimiters would help.  I think 'C' is seriously ambiguous.

Comment 26

[Robert Relyea]

>>+++ tests/multinit/multinit.txt	
>helps to understand the syntax of the commands (input) to the test 
> program, which is documented nowhere else.

multinit --help will give you a nice long description of *All* the options. I would hardly say that it was "nowhere else" (particularly compared to other tools in cmd). For a test only tool, it's actually relatively well documented.

> For the life of me, I cannot decode CTCCZC or CTCCNIE. 

The strings were meant to be compact for test case comparison, not human readability. For everything outputted in a string, there's an actual full, human readable version outputted as well (to standard error). This output winds up in the log. When there is a failure, both the expected and actual strings are printed out. Seeing where the strings differ plus looking at the actual output should be sufficient to determine what went wrong.

The ambiguous output (C for cert versus C for trusted CA) was an unfortunately result of trying to reuse existing library calls go collect the trust information.Since the purpose was simply to get a computer checkable synopsis of the outputted result, I deemed that sufficient. Again it's a step up from other tests we have, which simply compare success codes, and seldom check that the actual expected result occurred. 


The rest of your comments sound reasonable. I'll attach updated patches once you complete the rest of the review.

Thanks,

bob

Comment 27

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 406593 [details] [diff] [review]
Changes to PK11 wrap to support multi init.

My only comments about this patch are about improving the comments in
the patch.

I didn't understand this comment at all.  I think it can be explained
much better. 

>+    /* the matching stuff knows about softoken specs, only try to match
>+     * to detect multiple opens for softoken databases */


> #define SECMOD_FLAG_SKIP_FIRST    0x02
> #define SECMOD_FLAG_DEFAULT_MODDB 0x04
> 
>+
>+#define SECMOD_FLAG_IS_INTERNAL       0x01 /* must be set if any of the other
>+                                            * flags are set */
>+#define SECMOD_FLAG_INTERNAL_KEY_SLOT 0x02

There's an existing set of #defines named SECMOD_FLAG_xxxx  defining names 
for low order bits.  This adds some new names, also of the form SECMOD_FLAG_xxxx
for the same bits.  The comments need to explain that these flags are in 
two different words.  I'd suggest using separate prefixes, but I won't 
push it.

Comment 28

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 406596 [details] [diff] [review]
Add reference counted init functions.


>+/*
>+ * strings used to internationalize softoken
>+ */
>+struct NSSInitStringsStr {
>+   int	  len;  	/* allow this structure to grow in the future */
>+   PRBool passwordRequired;
>+   int    minimumPasswordLength;
>+   char * manufactureID;           /* variable names for strings match the */
>+   char * libraryDescription;      /*   parameter name in softoken */
>+   char * cryptoTokenDescription;
>+   char * dbTokenDescription;
>+   char * FIPSTokenDescription;
>+   char * cryptoSlotDescription;
>+   char * dbSlotDescription;
>+   char * FIPSSlotDescription;
>+};

IMO, the code above ought to explain everything the caller needs to know to 
use those strings properly, such as 
- the strings are UTF8
- the length limits for each string
- the fact that the caller can free his copy when the function returns



>-void
>-PK11_ConfigurePKCS11(const char *man, const char *libdes, const char *tokdes,
>+char *
>+nss_mkConfigString(const char *man, const char *libdes, const char *tokdes,

That new function name doesn't follow the NSS naming convention.  
Since it's a function, the first character after the underscore should be 
a capital letter. 


> 	const char *ptokdes, const char *slotdes, const char *pslotdes, 
>-	const char *fslotdes, const char *fpslotdes, int minPwd, int pwRequired)
>+	const char *fslotdes, const char *fpslotdes, int minPwd)

Please use global search and replace to rename all those *des variables to *desc.
Thanks.



>+    /* if we are trying to init with a traditional NSS_Init call, maintain
>+     * the traditional idempotent behavior. */
>+    if (!initContextPtr && nss_IsInitted) {

nss_IsInitted is a variable, yet its name follows the NSS naming convention for 
a function, since it bears a prefix, an underscore, and a capital letter after 
the underscore.  Since it's a static, please use global search and replace to 
remove the underscore.


>+    if (!isReallyInitted) {
>+	/* New option bits must not change the size of CERTCertificate. */
>+	PORT_Assert(sizeof(dummyCert.options) == sizeof(void *));
>+
>+	if (SECSuccess != cert_InitLocks()) {
>+            return SECFailure;
>+	}
>+
>+	if (SECSuccess != InitCRLCache()) {
>+            return SECFailure;
>+	}
>+    
>+	if (SECSuccess != OCSP_InitGlobal()) {
>+            return SECFailure;
>+	}

If InitCRLCache fails, or OCSP_InitGlobal fails, this function returns
leaving the previously successfully initialized stuff still initialized.
In other words, it leaves NSS partly initialized.  I think this function
should be all or nothing.  It should either leave NSS fully initialized, 
or else completely uninitialized, but not somewhere half way in between.

I realize this is an existing problem, not a new failure of this patch, 
so I won't give r- for this, but it should get fixed. 

Also, now that we are explicitly allowing multiple callers of init, 
which may have separate outcomes (e.g. different slots), it is possible 
that these callers will race.  This code takes no precautions against 
those races.  At the VERY minimum, we must explicitly declare that the 
callers are responsible to avoid races.  But I think that's lame.  We 
really need to deal with the races ourselves.  Probably a simple lock
in the common init function would suffice. 

>@@ -912,13 +1056,86 @@ NSS_Shutdown(void)
> 	shutdownRV = SECFailure;
>     }
>     nss_IsInitted = PR_FALSE;
>+    temp = nss_InitContextList;
>+    nss_InitContextList = NULL;
>+    /* free the old list. This is necessary when we are called from
>+     * NSS_Shutdown(). */
>+    while (temp) {
>+	NSSInitContext *next = temp->next;
>+	temp->magic = 0;
>+	PORT_Free(temp);
>+	temp = next;
>+    }
>     return shutdownRV;
> }

This is one of those odd loops that goes around the loop potentially 
many times, but only goes through the body of the loop once, at the 
very end and then stops.  I'd prefer it to be structured so that that 
one-time-only code comes after (below) the loop, not inside it.  
But that's a personal preference. 

The only aspect of this review for this patch that I think might really 
deserve r- is the lack of race avoidance for multiple initializers, 
which we now explicitly allow.  But I think there probably needs to be 
some group agreement on that, so I won't give it r-.  The other issues 
are cosmetic, and can be fixed without another review.

If another review is desired for this bug, please ask another reviewer 
to review it.

Comment 29

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 406597 [details] [diff] [review]
New Test program

I had a lot of review comments about this patch, but they were all cosmetic, so I won't require another review before committing it.

Comment 30

[Robert Relyea]

Checking in cmd/manifest.mn;
/cvsroot/mozilla/security/nss/cmd/manifest.mn,v  <--  manifest.mn
new revision: 1.28; previous revision: 1.27
done
RCS file: /cvsroot/mozilla/security/nss/cmd/multinit/Makefile,v
done
Checking in cmd/multinit/Makefile;
/cvsroot/mozilla/security/nss/cmd/multinit/Makefile,v  <--  Makefile
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/cmd/multinit/manifest.mn,v
done
Checking in cmd/multinit/manifest.mn;
/cvsroot/mozilla/security/nss/cmd/multinit/manifest.mn,v  <--  manifest.mn
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/cmd/multinit/multinit.c,v
done
Checking in cmd/multinit/multinit.c;
/cvsroot/mozilla/security/nss/cmd/multinit/multinit.c,v  <--  multinit.c
initial revision: 1.1
done
Checking in lib/nss/nss.def;
/cvsroot/mozilla/security/nss/lib/nss/nss.def,v  <--  nss.def
new revision: 1.203; previous revision: 1.202
done
Checking in lib/nss/nss.h;
/cvsroot/mozilla/security/nss/lib/nss/nss.h,v  <--  nss.h
new revision: 1.72; previous revision: 1.71
done
Checking in lib/nss/nssinit.c;
/cvsroot/mozilla/security/nss/lib/nss/nssinit.c,v  <--  nssinit.c
new revision: 1.101; previous revision: 1.100
done
Checking in lib/pk11wrap/pk11load.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11load.c,v  <--  pk11load.c
new revision: 1.27; previous revision: 1.26
done
Checking in lib/pk11wrap/pk11pars.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11pars.c,v  <--  pk11pars.c
new revision: 1.25; previous revision: 1.24
done
Checking in lib/pk11wrap/pk11priv.h;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11priv.h,v  <--  pk11priv.h
new revision: 1.13; previous revision: 1.12
done
Checking in lib/pk11wrap/pk11slot.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11slot.c,v  <--  pk11slot.c
new revision: 1.100; previous revision: 1.99
done
Checking in lib/pk11wrap/secmodi.h;
/cvsroot/mozilla/security/nss/lib/pk11wrap/secmodi.h,v  <--  secmodi.h
new revision: 1.33; previous revision: 1.32
done
Checking in tests/all.sh;
/cvsroot/mozilla/security/nss/tests/all.sh,v  <--  all.sh
new revision: 1.55; previous revision: 1.54
done
RCS file: /cvsroot/mozilla/security/nss/tests/multinit/multinit.sh,v
done
Checking in tests/multinit/multinit.sh;
/cvsroot/mozilla/security/nss/tests/multinit/multinit.sh,v  <--  multinit.sh
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/tests/multinit/multinit.txt,v
done
Checking in tests/multinit/multinit.txt;
/cvsroot/mozilla/security/nss/tests/multinit/multinit.txt,v  <--  multinit.txt
initial revision: 1.1
done
Checking in tests/tools/tools.sh;
/cvsroot/mozilla/security/nss/tests/tools/tools.sh,v  <--  tools.sh
new revision: 1.22; previous revision: 1.21
done

checked in...

Before closing this bug, there still needs to be 3 issues resolved.
1) attaching the updated patches that addressed the review comments.
2) Bug to document the problem with Partial init failures.
3) Bug to document the problem of race conditions in Init.

Once those are done, this bug can be closed.

Comment 31

[Slavomir Katuscak]

Bob, this patch causes some failures of Tinderboxes. I have seen that after you checked in main patch, you did 2 minor fixes in .c files, and finally one more fix that just removed multinit tests from all.sh.

That works for many platforms, however not for all.

On machine dopushups - Linux memory leak tinderbox - tools are crashing.

bash-3.00$ certutil -N -d .
bash-3.00$ gdb certutil
(gdb) run -N -d .
Starting program: /export/tinderlight/data/crashbuild/mozilla/dist/Linux2.6_x86_glibc_PTH_DBG.OBJ/bin/certutil -N -d .
[Thread debugging using libthread_db enabled]
[New Thread -1208158528 (LWP 5153)]

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread -1208158528 (LWP 5153)]
0x0060e2bb in strlen () from /lib/tls/libc.so.6
(gdb) bt
#0  0x0060e2bb in strlen () from /lib/tls/libc.so.6
#1  0x00cb0eb2 in cvt_s (ss=0xbfe56030, str=0x22 <Address 0x22 out of bounds>, width=0, prec=-1, flags=0)
    at ../../../../pr/src/io/prprf.c:396
#2  0x00cb1edc in dosprintf (ss=0xbfe56030, fmt=0x33a69f "\"", ap=0xbfe560cc "^�#") at ../../../../pr/src/io/prprf.c:980
#3  0x00cb22bd in PR_vsmprintf (
    fmt=0x33a5a8 "name=\"%s\" parameters=\"configdir='%s' certPrefix='%s' keyPrefix='%s' secmod='%s' flags=%s updatedir='%s' updateCertPrefix='%s' updateKeyPrefix='%s' updateid='%s' updateTokenDescription='%s' %s\" NSS=\"fl"..., ap=0xbfe56094 "\222�3")
    at ../../../../pr/src/io/prprf.c:1127
#4  0x00cb223f in PR_smprintf (
    fmt=0x33a5a8 "name=\"%s\" parameters=\"configdir='%s' certPrefix='%s' keyPrefix='%s' secmod='%s' flags=%s updatedir='%s' updateCertPrefix='%s' updateKeyPrefix='%s' updateid='%s' updateTokenDescription='%s' %s\" NSS=\"fl"...)
    at ../../../../pr/src/io/prprf.c:1105
#5  0x0023e484 in nss_InitModules (configdir=0x8070300 ".", certPrefix=0x8061f6e "", keyPrefix=0x8061f6e "", 
    secmodName=0x80652b2 "secmod.db", updateDir=0x33a46e "", updCertPrefix=0x33a46e "", updKeyPrefix=0x33a46e "", 
    updateID=0x33a46e "", updateName=0x33a46e "", configName=0x0, configStrings=0x0, pwRequired=0, readOnly=0, noCertDB=0, 
    noModDB=0, forceOpen=0, optimizeSpace=0, isContextInit=0) at nssinit.c:439
#6  0x0023e7ae in nss_Init (configdir=0x8070300 ".", certPrefix=0x8061f6e "", keyPrefix=0x8061f6e "", 
    secmodName=0x80652b2 "secmod.db", updateDir=0x33a46e "", updCertPrefix=0x33a46e "", updKeyPrefix=0x33a46e "", 
    updateID=0x33a46e "", updateName=0x33a46e "", initContextPtr=0x0, initParams=0x0, readOnly=0, noCertDB=0, noModDB=0, 
    forceOpen=0, noRootInit=0, optimizeSpace=0, noSingleThreadedModules=0, allowAlreadyInitializedModules=0, 
    dontFinalizeModules=0) at nssinit.c:622
#7  0x0023eb4e in NSS_Initialize (configdir=0x8070300 ".", certPrefix=0x8061f6e "", keyPrefix=0x8061f6e "", 
    secmodName=0x80652b2 "secmod.db", flags=0) at nssinit.c:781
#8  0x08054f6a in certutil_main (argc=4, argv=0xbfe565d4, initialize=1) at certutil.c:2370
#9  0x0805665c in main (argc=4, argv=0xbfe565d4) at certutil.c:2982

You can see that failure occurs already in nssinit. 

As this bug causes crashes, I'm raising severity to critical.

Comment 32

[Robert Relyea]

Thanks slavo, I've been trying to reproduce the problem on my local machine, but have not been able to. This stack traceback is infinitely helpful.

bob

Comment 33

[Nelson Bolyard (seldom reads bugmail)]

Is this bug resolved now?

Comment 34

[Wan-Teh Chang]

The new functions NSS_InitContext and NSS_ShutdownContext
were added in NSS 3.12.5.

Bob noted at the end of comment 30 some remaining work.
But this bug should be marked fixed now.

Comment 35

[Robert Relyea]

Two new bugs were created. Those bugs were made dependant on this one so that there is a link between them (for history purposes).

this bug can now be closed.