987263 - Small leak in NSS_GetError during shutdown

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Andrew McCreight [:mccr8]]

Attachment: leak stack

NSS_Shutdown calls NSS_GetError, which results in a small leak that is detected by LeakSanitizer.  See the attached file for the full stack.

Note that Chrome also has a suppression on file for this:
# Another leak due to not shutting down NSS properly. http://crbug.com/124445
leak:error_get_my_stack

Comment 1

[Wan-Teh Chang]

Comment on attachment 8395805 [details]
leak stack

Thank you for the bug report. The source code in nss/lib/nss/nssinit.c
relevant to this leak call stack is:

1069 SECStatus
1070 nss_Shutdown(void)
1071 {
1072     SECStatus shutdownRV = SECSuccess;
1073     SECStatus rv;
1074     PRStatus status;
1075     NSSInitContext *temp;
1076 
1077     rv = nss_ShutdownShutdownList();
1078     if (rv != SECSuccess) {
1079         shutdownRV = SECFailure;
1080     }
1081     cert_DestroyLocks();
1082     ShutdownCRLCache();
1083     OCSP_ShutdownGlobal();
1084     PKIX_Shutdown(plContext);
1085     SECOID_Shutdown();
1086     status = STAN_Shutdown();
         ...
1094     /*
1095      * A thread's error stack is automatically destroyed when the thread
1096      * terminates, except for the primordial thread, whose error stack is
1097      * destroyed by PR_Cleanup.  Since NSS is usually shut down by the
1098      * primordial thread and many NSS-based apps don't call PR_Cleanup,
1099      * we destroy the calling thread's error stack here.
1100      */
1101     nss_DestroyErrorStack();
1102     nssArena_Shutdown();
1103     if (status == PR_FAILURE) {
1104         if (NSS_GetError() == NSS_ERROR_BUSY) {
1105             PORT_SetError(SEC_ERROR_BUSY);
1106         }
1107         shutdownRV = SECFailure;
1108     }
         ...
1120     return shutdownRV;

This means STAN_Shutdown() failed, which would cause
nss_Shutdown to fail as well. This means this is a
leak on the error path of NSS_Shutdown. It would be
nice to know why NSS_Shutdown failed.

Regarding the leak, it is an interesting realization
that a FOO_Shutdown function probably can't report the
error code of its failure using any function from the
FOO subsystem. Fortunately, here we seem to have an
easy solution: the error stack used by error_get_my_stack()
is destroyed by the nss_DestroyErrorStack() call on
line 1101, so we just need to call NSS_GetError() earlier
and save the returned error code in a local variable.

Comment 2

[Wan-Teh Chang]

Attachment: Patch: nss_Shutdown should call nss_DestroyErrorStack after any NSS_GetError calls

Andrew: could you review and test this patch? Thanks.

This patch does not deal with the failure of NSS_Shutdown.
It only fixes the memory leak when the nks.

This patch does not deal with the failure of NSS_Shutdown.
It only fixes the memory leak when STAN_Shutdown call in
nss_Shutdown fails.

Comment 3

[Andrew McCreight [:mccr8]]

Comment on attachment 8395857 [details] [diff] [review]
Patch: nss_Shutdown should call nss_DestroyErrorStack after any NSS_GetError calls

This fixes the leak for me, thanks!

Comment 4

[Kai Engert (:KaiE:)]

Comment on attachment 8395857 [details] [diff] [review]
Patch: nss_Shutdown should call nss_DestroyErrorStack after any NSS_GetError calls

r=kaie

Comment 5

[Wan-Teh Chang]

Comment on attachment 8395857 [details] [diff] [review]
Patch: nss_Shutdown should call nss_DestroyErrorStack after any NSS_GetError calls

Patch checked in: https://hg.mozilla.org/projects/nss/rev/b22c4a5f8a53