Re: [PATCHES] ldap: fix resource leak
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 ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] ldap: fix resource leak
Neil Conway [EMAIL PROTECTED] 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 ---(end of broadcast)--- TIP 5: don't forget to increase your free space map settings
Re: [PATCHES] ldap: fix resource leak
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 ---(end of broadcast)--- TIP 6: explain analyze is your friend
Re: [PATCHES] ldap: fix resource leak
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 ---(end of broadcast)--- TIP 7: You can help support the PostgreSQL project by donating at http://www.postgresql.org/about/donate
Re: [PATCHES] ldap: fix resource leak
Neil Conway [EMAIL PROTECTED] 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 ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
