Beginning on October 25th, 2016, Persona will no longer be an option for authentication on BMO. For more details see Persona Deprecated.
Last Comment Bug 399021 - doublefree in nsldapi_free_request
: doublefree in nsldapi_free_request
Product: Directory
Classification: Components
Component: LDAP C SDK (show other bugs)
: other
: All All
: -- normal (vote)
: ---
Assigned To: Mark Smith (:mcs)
Depends on:
  Show dependency treegraph
Reported: 2007-10-08 09:32 PDT by
Modified: 2007-10-08 15:17 PDT (History)
1 user (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---

one liner fix (338 bytes, patch)
2007-10-08 09:42 PDT,
no flags Details | Diff | Splinter Review
context diff. (965 bytes, patch)
2007-10-08 09:51 PDT,
no flags Details | Diff | Splinter Review
context diff. (965 bytes, patch)
2007-10-08 09:51 PDT,
mcs: review+
Details | Diff | Splinter Review

Description 2007-10-08 09:32:13 PDT
User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv: Gecko/20070914 Firefox/
Build Identifier: 

this is basically a nasty doublefree in nsldapi_free_request when chasing
referrals. if chase fails due to connection error and we call ldap_unbind
to clean up nsldapi_free_request will free child request twice, once as
part of freeing request list one by one and the second time when freeing
child requests for each parent request.

normally this doublefree is not a problem, its harmless and since standard
libc free ignores doublefree in most implementations it happens without a
trace however some allocators such as mtmalloc, umem, watchmalloc and alike
dont take doublefree lightly and would either call abort() or generate some
term signal explicitly as doublefree is not allowed in those implementations
by default. this obviously does cause problems.

below is libumem audit and transaction output that illustrates the problem:

Status:         ready and active
Concurrency:    4
Logs:           transaction=64k 
Message buffer:
free(8098bc8): double-free or invalid buffer
stack trace:'umem_err_recoverable+0x39'process_free+0xe1'free+0x17'?? (0xbff47bf4)'ber_free+0x74'?? (0xbff293a2)'?? (0xbff292ef)'?? (0xbff3d6ae)'ldap_unbind+0x88

            ADDR          BUFADDR        TIMESTAMP           THREAD
                            CACHE          LASTLOG         CONTENTS
         8071bb8          8098bc0     1da416de429a                1
                          8086010          807076c                0

         807076c          8098bc0     1da416c2f4fe                1
                          8086010                0                0

the stack trace out of message buffer above maps to something like this in dbx:

  [1] _libc_kill(0x0, 0x6, 0x0, 0xffbfdc88, 0x0, 0xff30eaf4), at 0xff29fc08 
  [2] umem_do_abort(0x1, 0xa, 0xa000000, 0x7efefeff, 0x81010100, 0xff00), at 0xff303dcc 
  [3] umem_err_recoverable(0xff31064c, 0x41008, 0xff310630, 0xffbfdca0, 0x2c388, 0x534), at 0xff304074 
  [4] free(0x41008, 0x246e4, 0x21b3c, 0xff29792c, 0x3dc20, 0x4c), at 0xff303938 
=>[5] nslberi_free(ptr = 0x41008), line 1650 in "io.c"
  [6] ber_free(ber = 0x41008, freebuf = 1), line 327 in "io.c"
  [7] nsldapi_free_request(ld = 0x37bc8, lr = 0x3dc28, free_conn = 0), line 839 in "request.c"
  [8] nsldapi_free_request(ld = 0x37bc8, lr = 0x3dcc8, free_conn = 0), line 821 in "request.c"
  [9] ldap_ld_free(ld = 0x37bc8, serverctrls = (nil), clientctrls = (nil), close = 1), line 87 in "unbind.c"
  [10] ldap_unbind(ld = 0x37bc8), line 45 in "unbind.c"
  [11] do_modify(opt = 0xffbfe338), line 80 in "modify.c"
  [12] main(argc = 5, argv = 0xffbfe3b4), line 95 in "modify.c"

Reproducible: Always

Steps to Reproduce:
Comment 1 2007-10-08 09:42:17 PDT
Created attachment 284008 [details] [diff] [review]
one liner fix

this one liner should be safe to get rid of the doublefree without causing any 
leaks when freeing requests but i might be missing something so please review.
Comment 2 2007-10-08 09:51:32 PDT
Created attachment 284009 [details] [diff] [review]
context diff.

same patch but with some context which i stupidly missed the first time.
Comment 3 2007-10-08 09:51:38 PDT
Created attachment 284010 [details] [diff] [review]
context diff.

same patch but with some context which i stupidly missed the first time.
Comment 4 Rich Megginson 2007-10-08 09:53:35 PDT
(In reply to comment #3)
> Created an attachment (id=284010) [details]
> context diff.
> same patch but with some context which i stupidly missed the first time.

Looks good.
Comment 5 Mark Smith (:mcs) 2007-10-08 10:04:29 PDT
Comment on attachment 284010 [details] [diff] [review]
context diff.

OK.  I think this is a safe fix.  Thanks for tracking this down; I am surprised we have not found it before.
Comment 6 2007-10-08 10:19:45 PDT
thanx Rich and Mark! the credit for tracking this down goes to Pierre Rogier of
our DS sustaining team. we have seen this as early as 2004 i think with our Msg
server because they link with mtmalloc by default however it is rather rare in
occurrence because it requires specific DS setup to reproduce. Pierre been able
to put such DS setup in place and write the testcase for this.
Comment 7 2007-10-08 15:17:04 PDT
Checking in mozilla/directory/c-sdk/ldap/libraries/libldap/request.c;
/cvsroot/mozilla/directory/c-sdk/ldap/libraries/libldap/request.c,v  <--  request.c
new revision: 5.10; previous revision: 5.9

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