PKIX_PL_Initialize must not call NSS_Init

RESOLVED FIXED in 3.12

Status

NSS
Libraries
P1
normal
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: Alexei Volkov, Assigned: Alexei Volkov)

Tracking

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: PKIX)

Attachments

(1 attachment)

(Assignee)

Description

10 years ago
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.
(Assignee)

Updated

10 years ago
Whiteboard: PKIX
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.
(Assignee)

Comment 2

10 years ago
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.
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.  
(Assignee)

Comment 4

10 years ago
Created attachment 281279 [details] [diff] [review]
patch v1. Remove nss init/shutdown code from libpkix
Assignee: nobody → alexei.volkov.bugs
Status: NEW → ASSIGNED
Attachment #281279 - Flags: review?(nelson)
(Assignee)

Updated

10 years ago
Blocks: 395265
(Assignee)

Updated

10 years ago
Priority: -- → P1
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();
(Assignee)

Comment 6

10 years ago
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
(Assignee)

Updated

10 years ago
Blocks: 396598
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();
Attachment #281279 - Flags: review?(nelson) → review+
(Assignee)

Comment 8

10 years ago
Patch v1 is integrated into trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.