1279105 - AddressSanitizer: 72 byte(s) leaked in 1 allocation(s) #1 0x7f9eaa9a19a1 in error_get_my_stack

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[Mike Hommey [:glandium]]

This happens when switching to clang 3.8 for ASAN builds (instead of 3.5 currently): 
https://treeherder.mozilla.org/logviewer.html#?job_id=22157690&repo=try#L2940

 23:35:20     INFO -  PROCESS | 17882 | =================================================================
 23:35:20     INFO -  PROCESS | 17882 | ==17900==ERROR: LeakSanitizer: detected memory leaks
 23:35:20     INFO -  PROCESS | 17882 | Direct leak of 72 byte(s) in 1 object(s) allocated from:
 23:35:20     INFO -  PROCESS | 17882 |     #0 0x4b12a4 in calloc /builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:66:3
 23:35:20     INFO -  PROCESS | 17882 |     #1 0x7f9eaa9a19a1 in error_get_my_stack /builds/slave/try-l64-asan-00000000000000000/build/src/security/nss/lib/base/error.c:109:17
 23:35:20     INFO -  PROCESS | 17882 |     #2 0x7f9eaa9a1cf8 in nss_ClearErrorStack /builds/slave/try-l64-asan-00000000000000000/build/src/security/nss/lib/base/error.c:245:23
 23:35:20     INFO -  PROCESS | 17882 |     #3 0x7f9eaa9a04a8 in NSSArena_Create /builds/slave/try-l64-asan-00000000000000000/build/src/security/nss/lib/base/arena.c:329:5
 23:35:20     INFO -  PROCESS | 17882 |     #4 0x7f9eaa98196e in NSSTrustDomain_Create /builds/slave/try-l64-asan-00000000000000000/build/src/security/nss/lib/pki/trustdomain.c:34:13
 23:35:20     INFO -  PROCESS | 17882 |     #5 0x7f9eaa98d0a9 in STAN_LoadDefaultNSS3TrustDomain /builds/slave/try-l64-asan-00000000000000000/build/src/security/nss/lib/pki/pki3hack.c:118:10
 23:35:20     INFO -  PROCESS | 17882 |     #6 0x7f9eaa8b2423 in nss_Init /builds/slave/try-l64-asan-00000000000000000/build/src/security/nss/lib/nss/nssinit.c:655:6
 23:35:20     INFO -  PROCESS | 17882 |     #7 0x7f9eaa8b348f in NSS_Initialize /builds/slave/try-l64-asan-00000000000000000/build/src/security/nss/lib/nss/nssinit.c:812:12
 23:35:20     INFO -  PROCESS | 17882 |     #8 0x4e6d9b in NSSInitCryptoContext /builds/slave/try-l64-asan-00000000000000000/build/src/modules/libmar/sign/mar_sign.c:37:22
 23:35:20     INFO -  PROCESS | 17882 |     #9 0x4e6d9b in mar_repackage_and_sign /builds/slave/try-l64-asan-00000000000000000/build/src/modules/libmar/sign/mar_sign.c:853
 23:35:20     INFO -  PROCESS | 17882 |     #10 0x4deb08 in main /builds/slave/try-l64-asan-00000000000000000/build/src/modules/libmar/tool/mar.c:412:12
 23:35:20     INFO -  PROCESS | 17882 |     #11 0x7f9ea8acd76c in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2176c)
 23:35:20     INFO -  PROCESS | 17882 | SUMMARY: AddressSanitizer: 72 byte(s) leaked in 1 allocation(s).
23:35:20 WARNING - TEST-UNEXPECTED-FAIL | modules/libmar/tests/unit/test_sign_verify.js | signMAR - [signMAR : 44] 1 == 0

This may or may not be mar's fault, I haven't dug yet.

Comment 1

[Franziskus Kiefer [:franziskus]]

This looks like a benign leak to me.

In error.c:109 (error_get_my_stack) we return |new_stack = PR_Calloc(1, new_bytes);| which we return to nss_ClearErrorStack

    error_stack *es = error_get_my_stack();
    if ((error_stack *)NULL == es) {
        /* Oh, well. */
        return;
    }

    es->header.count = 0;
    es->stack[0] = 0;
    return;

We have to free |es| here.
But then I'm not sure if this function actually does anything useful. error_get_my_stack copies the stack before returning it (nsslibc_memcpy(new_stack, rv, rv->header.space)). So if I'm not missing anything, nss_ClearErrorStack has no observable effect.

Comment 2

[Mike Hommey [:glandium]]

The important part is in error_get_my_stack, which uses PR_SetThreadPrivate.

It would seem that nss_DestroyErrorStack is never called (or NSS_Shutdown, for that matter, but shouldn't there be more leaks detected if that were the case?).

Comment 3

[Mike Hommey [:glandium]]

modules/libmar/sign/mar_sign.c never calls NSS_Shutdown...

Comment 4

[Mike Hommey [:glandium]]

Attachment: Bug 1279105 - Properly shutdown NSS after NSSInitCryptoContext was called.

Review commit: https://reviewboard.mozilla.org/r/58788/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58788/

Comment 5

[Robert Strong (they/them - no direct email)]

Comment on attachment 8761690 [details]
Bug 1279105 - Properly shutdown NSS after NSSInitCryptoContext was called.

https://reviewboard.mozilla.org/r/58788/#review55740

I'm fine with this but I'd like to have Brian Smith (:briansmith) also look at this so clearing review.

Comment 6

[Robert Strong (they/them - no direct email)]

Comment on attachment 8761690 [details]
Bug 1279105 - Properly shutdown NSS after NSSInitCryptoContext was called.

Brian, could you verify that calling NSS_Shutdown is ok for the app update use case? Thanks.

Comment 7

[Robert Strong (they/them - no direct email)]

Comment on attachment 8761690 [details]
Bug 1279105 - Properly shutdown NSS after NSSInitCryptoContext was called.

Talked with Brian Smith and he doesn't recall any reason for this. I'll review this in the near future

Comment 8

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

https://reviewboard.mozilla.org/r/58788/#review56246

::: modules/libmar/sign/mar_sign.c:1156
(Diff revision 1)
>      }
>  
>      SECITEM_FreeItem(&secItems[k], PR_FALSE);
>    }
>  
> +  (void)NSS_Shutdown();

Pardon the unsolicited comment, but checking the return value from NSS_Shutdown can indicate when NSS resources (e.g. CERTCertificates) haven't been released. I'm not sure that's a particular concern in this code, but it can help find bugs.

Comment 9

[Robert Strong (they/them - no direct email)]

Comment on attachment 8761690 [details]
Bug 1279105 - Properly shutdown NSS after NSSInitCryptoContext was called.

https://reviewboard.mozilla.org/r/58788/#review56256

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dda36986f0ce
Properly shutdown NSS after NSSInitCryptoContext was called. r=rstrong

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/dda36986f0ce