987925 - Small leak under PK11_ChangePW

Status: RESOLVED WORKSFORME

(NSS :: Libraries, defect, P2)

Description

[Andrew McCreight [:mccr8]]

Attachment: leak stacks

I found this by running Firefox's Mochitest 5 under LeakSanitizer.

Comment 1

[Wan-Teh Chang]

Comment on attachment 8396633 [details]
leak stacks

>Direct leak of 48 byte(s) in 2 object(s) allocated from:
>    #0 0x440f25 in malloc (/home/amccreight/ff-dbg-asan/dist/bin/firefox+0x440f25)
>    #1 0x7f8552e5959e in PORT_Alloc_Util /home/amccreight/mc/security/nss/lib/util/secport.c:86
>    #2 0x7f8552e5505f in sec_asn1e_allocate_item /home/amccreight/mc/security/nss/lib/util/secasn1e.c:1506
>    #3 0x7f8552e54ee3 in SEC_ASN1EncodeItem_Util /home/amccreight/mc/security/nss/lib/util/secasn1e.c:1537
>    #4 0x7f852114f6f0
>    #5 0x7f852114f45f
>    #6 0x7f8521151aa2
>    #7 0x7f852110a107
>    #8 0x7f854f514f37 in PK11_ChangePW /home/amccreight/mc/security/nss/lib/pk11wrap/pk11auth.c:498

Andrew:

Thanks a lot for the bug report. If you can get the function
names of stack frames #4 - #7, that will help me track this
down.

Comment 2

[Andrew McCreight [:mccr8]]

Yeah, I don't know why those frames are missing.  Maybe with a newer version of Clang I'll get more useful information.

Comment 3

[:Cykesiopka]

FWIW, the top few frames from Bug 1164364 comment 0 and from Attachment 8396633 [details] here look nearly the same. Maybe this was fixed by Bug 1164364?

Comment 4

[Andrew McCreight [:mccr8]]

Yeah, it could be.  To verify it, we'll have to wait until the NSS in Firefox is updated, then check the log for a LSan Mochitest-5 run.  There should be no mention of sec_asn1e_allocate_item or PORT_Strdup_Util under the various "Suppressions used" lists in the log.  Then we can remove those suppressions from lsan_suppressions.txt.

Comment 5

[Wan-Teh Chang]

PK11_ChangePW calls the following functions:

PK11_GETTAB(slot)->C_SetPIN > NSC_SetPIN > sftkdb_ChangePassword

So I agree this is very likely the memory leak in sftkdb_ChangePassword
that was fixed in bug 1164364.

Comment 6

[Wan-Teh Chang]

(In reply to Andrew McCreight [:mccr8] from comment #4)
> To verify it, we'll have to wait until the NSS in
> Firefox is updated, then check the log for a LSan Mochitest-5 run.  There
> should be no mention of sec_asn1e_allocate_item or PORT_Strdup_Util under
> the various "Suppressions used" lists in the log.  Then we can remove those
> suppressions from lsan_suppressions.txt.

Andrew: the NSS in Firefox has been updated. Could you please check if we
can remove those suppressions from lsan_suppressions.txt now? Thanks.

Comment 7

[Andrew McCreight [:mccr8]]

The two suppressions listed for this bug are sec_asn1e_allocate_item and PORT_Strdup_Util. I don't see sec_asn1e_allocate_item in the log, but I do see 48 bytes for PORT_Strdup_Util. However, that's a pretty generic signature so I'm not sure if it is the same leak. It is also different than what I listed here in comment 0, so it is hard to say if that's the same thing or not.

I'll do a try push with both removed and see what happens:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=33861e8c0c4a

Sorry for the delay.

Comment 8

[Andrew McCreight [:mccr8]]

This is what the leak looks like on try:

Direct leak of 48 byte(s) in 2 object(s) allocated from:
      #0 0x472131 in malloc /builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:74
      #1 0x7f70919b7f54 in PORT_Alloc_Util /builds/slave/try-l64-asan-00000000000000000/build/src/security/nss/lib/util/secport.c:86
      #2 0x7f70919b7f54 in PORT_Strdup_Util /builds/slave/try-l64-asan-00000000000000000/build/src/security/nss/lib/util/secport.c:149

Not very useful! But it does look different that the report in comment 0.

Comment 9

[Wan-Teh Chang]

Andrew: thank you for the update. Is the stack in comment 8 the
complete stack, or is PK11_ChangePW in a higher frame of that
stack?

Comment 10

[Andrew McCreight [:mccr8]]

That's the complete stack, unfortunately. The LSan we run on try doesn't give very good stacks. I'll try to run M5 locally and see if it produces more useful information. (I suspect the stack in comment 0 is from a local run.)

Comment 11

[Wan-Teh Chang]

Andrew: I just ran valgrind on the NSS command line tool "modutil".
I performed a change-password operation and verified that PK11_ChangePW
was called (see comment 1). But valgrind did not report any leak
in PK11_ChangePW or PORT_Strdup_Util.

Comment 12

[Andrew McCreight [:mccr8]]

When I run it locally, I do get a leak that involves PK11_ChangePW:

==17312==ERROR: LeakSanitizer: detected memory leaks
Direct leak of 48 byte(s) in 2 object(s) allocated from:
    #0 0x468b09 in __interceptor_malloc /home/johns/AUR/llvm-svn/src/llvm-branches_release_34/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:74
    #1 0x7f0165b36fb4 in PORT_Alloc_Util /home/amccreight/mc/security/nss/lib/util/secport.c:86
    #2 0x7f0165b37128 in PORT_Strdup_Util /home/amccreight/mc/security/nss/lib/util/secport.c:149
    #3 0x7f013ae68bb9 (+0x4bb9)
    #4 0x7f013ae69596 (+0x5596)
    #5 0x7f013ae79279 (+0x15279)
    #6 0x7f013af2a186 (+0xc6186)
    #7 0x7f013af2a8d2 (+0xc68d2)
    #8 0x7f013aee1c3c (+0x7dc3c)
    #9 0x7f016224c77f in PK11_ChangePW /home/amccreight/mc/security/nss/lib/pk11wrap/pk11auth.c:498
    #10 0x7f01532c20bf in nsPK11Token::ChangePassword(char16_t const*, char16_t const*) /home/amccreight/mc/security/manager/ssl/nsPK11TokenDB.cpp:373
    #11 0x7f014d1b5799 in NS_InvokeByIndex /home/amccreight/mc/xpcom/reflect/xptcall/md/unix/xptcinvoke_x86_64_unix.cpp:174
    #12 0x7f014e3839d5 in CallMethodHelper::Invoke() /home/amccreight/mc/js/xpconnect/src/XPCWrappedNative.cpp:2080
    #13 0x7f014e3839d5 in CallMethodHelper::Call() /home/amccreight/mc/js/xpconnect/src/XPCWrappedNative.cpp:1417
    #14 0x7f014e3839d5 in XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) /home/amccreight/mc/js/xpconnect/src/XPCWrappedNative.cpp:1384

I'll try to figure out which test is calling ChangePassword from JS.

Comment 13

[Andrew McCreight [:mccr8]]

The leak appears to be caused by running the test toolkit/components/passwordmgr/test/test_master_password.html

Comment 14

[Ryan VanderMeulen [:RyanVM]]

This leak appears to be gone now. The suppression was removed in bug 1311584.