Last Comment Bug 395427 - PKIX_PL_Initialize must not call NSS_Init
: PKIX_PL_Initialize must not call NSS_Init
Status: RESOLVED FIXED
PKIX
:
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: 3.12
: All All
: P1 normal (vote)
: 3.12
Assigned To: Alexei Volkov
:
Mentors:
Depends on:
Blocks: 395265 396598
  Show dependency treegraph
 
Reported: 2007-09-07 13:09 PDT by Alexei Volkov
Modified: 2007-09-19 12:30 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch v1. Remove nss init/shutdown code from libpkix (11.87 KB, patch)
2007-09-17 22:49 PDT, Alexei Volkov
nelson: review+
Details | Diff | Splinter Review

Description Alexei Volkov 2007-09-07 13:09:34 PDT
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) 2007-09-07 13:45:03 PDT
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 2007-09-07 14:59:24 PDT
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) 2007-09-07 15:11:05 PDT
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 2007-09-17 22:49:48 PDT
Created attachment 281279 [details] [diff] [review]
patch v1. Remove nss init/shutdown code from libpkix
Comment 5 Nelson Bolyard (seldom reads bugmail) 2007-09-17 23:23:42 PDT
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 2007-09-18 13:58:54 PDT
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) 2007-09-18 16:14:31 PDT
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 2007-09-19 12:30:19 PDT
Patch v1 is integrated into trunk.

Note You need to log in before you can comment on or make changes to this bug.