395427 - PKIX_PL_Initialize must not call NSS_Init

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Alexei Volkov]

Nelson has strong opinion about this: 

"Code inside libPKIX that initializes NSS is a bug that IMO MUST be removed
for NSS 3.12 RTM.   That makes it a P1 for 3.12 RTM.  I don't know if a 
separate bug needs to be filed about that, or if it can be fixed in this bug."

If the change is made to remove NSS initialization from PKIX_PL_Initialize function, then all 65 libpkix unit test programs must be modified to call NSS_Init . 

Also see bug 395265.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Alexei, you supposedly consolidated all the test programs into one.
Is that work not done?  Do we still have 65 test executables?  
Or do we have only one?  If we have only one, then there is only one
program that needs to be fixed.

Comment 2

[Alexei Volkov]

There is only one executable called pkixutil. But is it nothing more then just a wrapper that calls renamed main functions of all unit test programs. I presume we are going to change the number of arguments in PKIX_Initialize function, e.t.c. remove the first argument that tells if libpkix need to initialize underlying crypto library. In this case wrapper modification would not be enough.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

I don't think there is any need to change the arguments to any existing 
PKIX funcitons.  Just don't initialize NSS in PKIX_PL_Initialize, ever,
and don't shut down NSS in the PKIX shutdown code, either.  Just ignore
the arguments that say to do that.  

Just put a call to NSS_Init in the main function that calls those other 
renamed main functions.  

Comment 4

[Alexei Volkov]

Attachment: patch v1. Remove nss init/shutdown code from libpkix

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 281279 [details] [diff] [review]
patch v1. Remove nss init/shutdown code from libpkix

Alexei, Why this ifdef?
Shouldn't NSS_Shutdown always shutdown PKIX?

>@@ -804,21 +804,23 @@ NSS_Shutdown(void)
>     rv = nss_ShutdownShutdownList();
>     if (rv != SECSuccess) {
> 	shutdownRV = SECFailure;
>     }
>     ShutdownCRLCache();
>     OCSP_ShutdownCache();
>+#ifdef BUILD_LIBPKIX_TESTS
>     PKIX_Shutdown(plContext);
>+#endif /* BUILD_LIBPKIX_TESTS */
>     SECOID_Shutdown();
>     status = STAN_Shutdown();

Comment 6

[Alexei Volkov]

It should. But as you recommended, I ifdef it for memory leak checking tests.
See https://bugzilla.mozilla.org/show_bug.cgi?id=395265#c10

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 281279 [details] [diff] [review]
patch v1. Remove nss init/shutdown code from libpkix

r=nelson

A comment is required explaining why this is ifdef'ed and explaining that 
the ifdef will be removed when the memory leak detection bug is fixed.

>     OCSP_ShutdownCache();
>+#ifdef BUILD_LIBPKIX_TESTS
>     PKIX_Shutdown(plContext);
>+#endif /* BUILD_LIBPKIX_TESTS */
>     SECOID_Shutdown();

Comment 8

[Alexei Volkov]

Patch v1 is integrated into trunk.