RE: ldap_sasl_interactive_bind_s leaks? (ITS#2423)

[hyc@highlandsun.com]

> -----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
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.

  -- Howard Chu
  Chief Architect, Symas Corp.       Director, Highland Sun
  http://www.symas.com               http://highlandsun.com/hyc
  Symas: Premier OpenSource Development and Support