382817 - Memory leak when using SASL I/O

Status: RESOLVED FIXED

(Directory Graveyard :: LDAP C SDK, defect)

Description

[nkinder]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.0.10) Gecko/20070510 Fedora/1.5.0.10-6.fc6 Firefox/1.5.0.10
Build Identifier: 

When using the LDAP C SDK to bind via a SASL mechanism that uses a SASL I/O layer (such as DIGEST-MD5), we don't release memory when doing an ldap_unbind.  This isn't a big deal for clients such as ldapsearch since the process exits after a single connection, but it does cause some major leak issues for long running applications that do a large number of bind/unbinds using SASL.

Here is a leak report from valgrind:

==4159== 131,424 (352 direct, 131,072 indirect) bytes in 4 blocks are definitely lost in loss record 16 of 23
==4159==    at 0x400473F: calloc (vg_replace_malloc.c:279)
==4159==    by 0x4042CA2: ldap_x_calloc (open.c:851)
==4159==    by 0x405FB23: new_SASLIOSocketArg (saslio.c:84)
==4159==    by 0x40606EC: nsldapi_sasl_install (saslio.c:542)
==4159==    by 0x40529DE: nsldapi_sasl_do_bind (saslbind.c:547)
==4159==    by 0x4053513: ldap_sasl_interactive_bind_ext_s (saslbind.c:808)
==4159==    by 0x804B8EE: connectToServer (ldapfct.c:841)
==4159==    by 0x804FEDC: doBindOnly (ldapfct.c:3316)
==4159==    by 0x805A1EF: threadMain (threadMain.c:1141)
==4159==    by 0x42FCA3DA: start_thread (in /lib/libpthread-2.5.so)
==4159==    by 0x42F2426D: clone (in /lib/libc-2.5.so)

The problem is that new_SASLIOSocketArg() allocates memory for a SASLIOSocketArg, but we never call the destroy_SASLIOSocketArg() function to clean it up when we are done with the connection.  The SASL I/O code uses the I/O function callback features of the LDAPSDK, but we never register a callback to the proper sasl cleanup function ( nsldapi_sasl_close() ).

Reproducible: Always

Comment 1

[nkinder]

Here's my analysis of this issue:

There are ldap and lber extended IO functions:

   struct lber_x_ext_io_fns
   struct ldap_x_ext_io_fns

Before nsldapi_sasl_install() does any work, the io fns look like this:

   sb_io_fns = {lbiof_read = 0,
   lbiof_write = 0}, sb_ext_io_fns = {lbextiofn_size = 0, lbextiofn_read = 0,
   lbextiofn_write = 0, lbextiofn_socket_arg = 0x0, lbextiofn_writev = 0}

After calling new_SASLIOSocketArg(), the io fns look the same.  We just have this
additional SASLIOSocketArg (sockarg) now, which has libldap function pointers of
it's own:

   (gdb) p *sockarg
   $16 = {sess_io_fns = {lextiof_size = 0, lextiof_connect = 0, lextiof_close = 0,
   lextiof_read = 0, lextiof_write = 0, lextiof_poll = 0,
   lextiof_newhandle = 0, lextiof_disposehandle = 0, lextiof_session_arg = 0x0,
   lextiof_writev = 0}, sock_io_fns = {lbextiofn_size = 0, lbextiofn_read = 0,
   lbextiofn_write = 0, lbextiofn_socket_arg = 0x0, lbextiofn_writev = 0},
   sasl_ctx = 0x8d05250, sb_sasl_ibuf = 0x8d08fa0 "", sb_sasl_iptr = 0x0,
   sb_sasl_bfsz = 65536, sb_sasl_ilen = 0, ld = 0x8cff5c8, sb = 0x8cff7b0}

The existing libldap ext io fns (LDAP_X_EXTIO_FN_PTRS) are then copied into the
sess_io_fns of the SASLIOSocketArg. In our case, there are no ldap ext io fns, so
they remain NULL.  We then copy the liblber ext io fns (LBER_X_EXTIO_FN_PTRS) into
the sock_io_fns of the SASLIOSocketArg.  In out case, there are no lber ext io fns,
so they remain NULL.  At this stage, the SASLIOSocketArg still remains unmodified.

We then check to see if we copied any libldap ext io fns into the SASLIOSocketArg.
If we did, we then override the libldap ext io fns in the main sockbuf with our
sasl I/O fns (read, write, poll, close, and a sockarg pointer).  Since we didn't
copy any libldap ext io fns over, we don't override anything.

We then set the liblber ext io fns to point our sasl I/O functions as well as setting
a pointer to out SASLIOSocketArg.   The only function pointers that we set in liblber
read and write.  There is no close function pointer here.  At this point, the sockbuf
looks like this:

   sb_io_fns = {lbiof_read = 0,
   lbiof_write = 0}, sb_ext_io_fns = {lbextiofn_size = 20,
   lbextiofn_read = 0x43fbef <nsldapi_sasl_read>,
   lbextiofn_write = 0x43fffd <nsldapi_sasl_write>,
   lbextiofn_socket_arg = 0x8d06528, lbextiofn_writev = 0}

This leaves us with no way to clean up the SASLIOSocketArg since we don't have a
callback to nsldapi_sasl_close().

I have run some tests with a modified LDAPSDK that seems to fix my issue, but it needs some cleanup done to it.  My changes basically always register the nsldapi_sasl_close() function as the libldap extended I/O close function, regardless of any pre-existing ext io fns.  I then had to modify nsldapi_sasl_close() to use the system close() function if there wasn't an original ext io close function set.

Before these changes, I'd hit 3G of memory in a matter of a few minutes with a looping bind/unbind DIGEST-MD5 test.  I can now run much longer than that with minor growth (which looks to be caused by leaks in SASL itself). 

Comment 2

[nkinder]

Attachment: CVS Diffs

Here is my proposed fix.  The desciption of this fix is in my previous comment to this bug.

In addition to the leak of the SASL I/O socket arg, there was another small (2 byte) leak that I have addressed in error.c.  The necessary code to free the memory was already there, but it was commented out.  The commented code was put in as part of the merge between the Mozilla and Sun code bases.  Simply uncommenting the code would trigger some errors about illegal frees in valgrind.  From testing, I found that we needed to check and free (and set the pointers to NULL) before potentially passing the pointers into LDAP_SET_LDERRNO.

Comment 3

[Rich Megginson]

(In reply to comment #2)
> Created an attachment (id=267154) [details]
> CVS Diffs
> 
> Here is my proposed fix.  The desciption of this fix is in my previous comment
> to this bug.
> 
> In addition to the leak of the SASL I/O socket arg, there was another small (2
> byte) leak that I have addressed in error.c.  The necessary code to free the
> memory was already there, but it was commented out.  The commented code was put
> in as part of the merge between the Mozilla and Sun code bases.  Simply
> uncommenting the code would trigger some errors about illegal frees in
> valgrind.  From testing, I found that we needed to check and free (and set the
> pointers to NULL) before potentially passing the pointers into
> LDAP_SET_LDERRNO.

Ok.

Comment 4

[Mark Smith [:mcs]]

Are you sure there is a leak inside ldap_parse_result()?  LDAP_SET_LDERRNO() is supposed to consume the m and e parameters (the next call to LDAP_SET_LDERRNO() will free any old values).

Comment 5

[nkinder]

(In reply to comment #4)
> Are you sure there is a leak inside ldap_parse_result()?  LDAP_SET_LDERRNO() is
> supposed to consume the m and e parameters (the next call to LDAP_SET_LDERRNO()
> will free any old values).
> 

I see.  It was being reported as a leak, but that must have been from the last call to LDAP_SET_LDERRNO().  I will remove that part of my changes.

How does the other code around the SASL I/O socket arg look?

Comment 6

[Mark Smith [:mcs]]

Comment on attachment 267154 [details] [diff] [review]
CVS Diffs

r=mcs (without the ldap_parse_result() changes)

There should be a way for I/O functions to free the error information when the LDAP handle is being freed... is there a way?

Comment 7

[nkinder]

Thanks for the reviews Mark & Rich!  I've checked in the changes to saslio.c.

Checking in saslio.c;
/cvsroot/mozilla/directory/c-sdk/ldap/libraries/libldap/saslio.c,v  <--  saslio.c
new revision: 5.3; previous revision: 5.2
done

As for the other leak, there should be a way to clean up the error info when freeing the LDAP handle.  I'll need to look into this, but I'll do that in another bug.