RE: ldap_sasl_interactive_bind_s leaks? (ITS#2423)

[igor@ipass.net]

On Mon, 14 Apr 2003, Howard Chu wrote:

> > -----Original Message-----
> > From: Igor Brezac [mailto:igor@ipass.net]
>
> > On Mon, 14 Apr 2003, Howard Chu wrote:
> > > > -----Original Message-----
> > > > From: Igor Brezac [mailto:igor@ipass.net]
> > >
> > > > It turns out cyrus DIGEST-MD5 is not leaking.  Openldap API
> > > > does not call
> > > > sasl_done() to clear cyrus-sasl buffers.  Your ldapsearch.c
> > > > patch includes
> > > > sasl_done(), but I think this is a wrong solution: ldapsearch
> > > > needs to be
> > > > explicitely linked with -lsasl2 and if cyrus sasl is not
> > > > configured with
> > > > openldap, the compile will fail.
> > >
> > > You're right, the patch is bad, it should be conditional #ifdef
> > > HAVE_CYRUS_SASL.
> >
> > I still think sasl_done() belongs in LDAP API somewhere.
> > Otherwise this
> > fix needs to be duplicated in a bunch of places.
>
> True. The correct fix requires introducing a new LDAP API call to shutdown
> libldap. This change will also require patching everywhere.
> > > >
> > > > I think sasl_done() needs to be called during ldap_unbind() and
> > > > ldap_int_sasl_init() needs to be called every time
> > ldap_init(ialize)()
> > > > runs rather than just once.  Please see attached patch.
> > My patch also
> > > > fixes threadsafe issue in ldap_int_sasl_init().
> > >
> > > This solution isn't any better. My interpretation of the
> > SASL docs is that
> > > sasl_done() only needs to be called once, at the end of the
> > particular
> >
> > This is probably true until cyrus-sasl bug 1963 is developed.
> > sasl_done() clears digest-md5 reauth buffer.  This is what causes the
> > leak, the buffer is never cleared.
>
> Yes. Of course this statement contradicts what you said earlier; DIGEST-MD5

True.  I did some tests with Cyrus team to determine where the leak is
coming from and the suggestion was to use sasl_done() to clear the buffer.

> is the actual cause of this memory leak. The correct fix is to talk to the
> Cyrus team, analyze the actual purpose of the reauth buffer, and probably
> introduce an option to turn it on or off, with default "off." The point of
> the reauth buffer as I understand it is to allow fast rebinds when a client
> makes multiple connections with the same user credentials. As such, the
> reauth buffer must live beyond the sasl_dispose() on a particular SASL
> session. If your application makes multiple connections with different user
> credentials, then the reauth buffer provides no benefit.
> >
> > > application. The LDAP API doesn't provide a similar
> > ldap_done() function to
> > > cleanup its library, though it certainly needs one. The big
> > problem with your
> > > patch is if any client uses two (or more) LDAP sessions at
> > once with SASL,
> > > calling ldap_unbind on any one of them will tear down the
> > SASL library for
> > > all of them. That's certainly not correct.
> >
> > True, but the current code leaks memory a fair amount.
>
> That fact is clearly acknowledged, but it's also clear that the problem
> arises from the DIGEST-MD5 SASL mech, not our use of the Cyrus API. This
> needs to be fixed by the SASL team.
>
> > At the same time,
> > if an app uses SASL API as well as LDAP API and if the app calls
> > sasl_done(), openldap session will break with and without the patch.
>
> Yes. Another Cyrus SASL bug.
>
> > This problem will be solved once the cyrus-sasl bug is fixed:
> > http://bugzilla.andrew.cmu.edu/show_bug.cgi?id=1963
>
> Yes.
>
> > Patch like the one I proposed still needs to be applied to openldap.
>
> No. Your patch masks one problem with another. The DIGEST-MD5 code needs to
> be fixed.
>

http://asg.web.cmu.edu/archive/message.php?mailbox=archive.cyrus-devel&msg=169

-- 
Igor