Thread: ldap: fix resource leak

Attached is a patch that fixes a minor error in CheckLDAPAuth() in 8.2:
when an LDAP handle is obtained via ldap_init(), it needs to be released
via ldap_unbind(). The code did this, but only if an error did not
occur.

I fixed this by adding the appropriate ldap_unbind() calls in error
control paths. An alternative would be to have a single place do the
error handling, and jump to that via goto. Anyone have any strong
feelings about which style is preferable here? I also didn't bother
checking the return value of ldap_unbind().

I also made a minor stylistic fix (use a "bool" for a boolean variable,
not an int).

Barring any objections, I'll apply this to HEAD tomorrow.

-Neil

Neil Conway <neilc@samurai.com> writes:
> I fixed this by adding the appropriate ldap_unbind() calls in error
> control paths. An alternative would be to have a single place do the
> error handling, and jump to that via goto.

Perhaps use a PG_TRY construct?

            regards, tom lane

On Sat, 2006-11-04 at 23:34 -0500, Tom Lane wrote:
> Perhaps use a PG_TRY construct?

At least for the existing code, this doesn't work well: the function
exits early via ereport(LOG) and then "return STATUS_ERROR;", so AFAICS
there isn't an easy way to simplify the existing error handling logic
via PG_TRY.

Note that this is related to a more general problem: if *any* backend
function allocates a resource that needs to be manually cleaned up, it
probably ought to be using a PG_TRY block. Otherwise, the resource will
be leaked on elog(ERROR). I wouldn't be surprised if various parts of
the backend neglected to get this right. However, in this particular
case, I didn't bother doing this, since it didn't seem likely that
anything we're going to invoke will report errors via elog. One could
make an argument for doing, for the sake of correctness/paranoia...

-Neil

Neil Conway <neilc@samurai.com> writes:
> On Sat, 2006-11-04 at 23:34 -0500, Tom Lane wrote:
>> Perhaps use a PG_TRY construct?

> At least for the existing code, this doesn't work well: the function
> exits early via ereport(LOG) and then "return STATUS_ERROR;", so AFAICS
> there isn't an easy way to simplify the existing error handling logic
> via PG_TRY.

OK, no biggie.

> Note that this is related to a more general problem: if *any* backend
> function allocates a resource that needs to be manually cleaned up, it
> probably ought to be using a PG_TRY block. Otherwise, the resource will
> be leaked on elog(ERROR). I wouldn't be surprised if various parts of
> the backend neglected to get this right.

For the most part we've tried to see to it that manual cleanup isn't
required, although I agree that ldap_unbind doesn't seem worth having a
tracking mechanism for.

> However, in this particular
> case, I didn't bother doing this, since it didn't seem likely that
> anything we're going to invoke will report errors via elog. One could
> make an argument for doing, for the sake of correctness/paranoia...

In theory one could put
    START_CRIT_SECTION();
    END_CRIT_SECTION();
around the code, as a form of "Assert(no elog here)".  Not sure that
this is actually a net win though, as a PANIC might well be considered a
worse problem than a one-time leak of some LDAP state.

            regards, tom lane

I wrote:
> ...  Not sure that
> this is actually a net win though, as a PANIC might well be considered a
> worse problem than a one-time leak of some LDAP state.

Come to think of it: either elog(ERROR) or a failure return from
CheckLDAPAuth is going to lead directly to backend exit, so the
whole thing is pretty much a cosmetic issue anyway.  I don't object
to adding the ldap_unbind calls, but I'd not recommend adding a large
pile of code.

            regards, tom lane

On Sun, 2006-11-05 at 19:28 -0500, Tom Lane wrote:
> Come to think of it: either elog(ERROR) or a failure return from
> CheckLDAPAuth is going to lead directly to backend exit, so the
> whole thing is pretty much a cosmetic issue anyway.

Thanks for the feedback. Patch applied, with an additional fix (noticed
some minor infelicities in the usage of snprintf()).

-Neil