freeing LDAPMessage in CheckLDAPAuth
Hi,
In CheckLDAPAuth(), around line 2606:
if (r != LDAP_SUCCESS)
{
ereport(LOG,
(errmsg("could not search LDAP for filter \"%s\" on
server \"%s\": %s",
It seems that the call to ldap_msgfree() is missing in the above case.
According to
https://www.openldap.org/software//man.cgi?query=ldap_search_s&sektion=3&apropos=0&manpath=OpenLDAP+2.4-Release
:
Note that *res* parameter of *ldap*_*search*_*ext*_*s()*
and *ldap*_*search*_*s()*
should be freed with *ldap*_*msgfree()* regardless of return
value of these
functions.
Please see the attached patch which frees the search_message in the above case.
Thanks
Attachments:
ldap-msg-free.patchapplication/octet-stream; name=ldap-msg-free.patchDownload
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index b2b0b83a97..c9ba2c6e6d 100644
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -2612,6 +2612,7 @@ CheckLDAPAuth(Port *port)
ldap_unbind(ldap);
pfree(passwd);
pfree(filter);
+ ldap_msgfree(search_message);
return STATUS_ERROR;
}
On Sat, Sep 03, 2022 at 05:00:30PM -0700, Zhihong Yu wrote:
Note that *res* parameter of *ldap*_*search*_*ext*_*s()*
and *ldap*_*search*_*s()*
should be freed with *ldap*_*msgfree()* regardless of return
value of these
functions.Please see the attached patch which frees the search_message in the above case.
Yep, nice catch, I am reading the same thing as you do. I can see
that we already do that after a failing ldap_search_st() call in
fe-connect.c for libpq. Hence, similarly, we'd better call
ldap_msgfree() on search_message when it is not NULL after a search
failure, no? The patch you are proposing does not do that.
--
Michael
Michael Paquier <michael@paquier.xyz> writes:
On Sat, Sep 03, 2022 at 05:00:30PM -0700, Zhihong Yu wrote:
Please see the attached patch which frees the search_message in the above case.
Yep, nice catch, I am reading the same thing as you do. I can see
that we already do that after a failing ldap_search_st() call in
fe-connect.c for libpq. Hence, similarly, we'd better call
ldap_msgfree() on search_message when it is not NULL after a search
failure, no? The patch you are proposing does not do that.
I can't get too excited about this. All of the error exit paths in
backend authentication code will lead immediately to process exit, so
the possibility of some memory being leaked really has no consequences
worth worrying about. If we *were* worried about it, sprinkling a few
more ldap_msgfree() calls into the existing code would hardly make it
more bulletproof. There's lots of psprintf() and other
Postgres-universe calls in that code that could potentially fail and
force an elog exit without reaching ldap_msgfree. So if you wanted to
make this completely clean you'd need to resort to doing the freeing
in PG_CATCH blocks ... and I don't see any value in hacking it to that
extent.
What might be worth inspecting is the code paths in frontend libpq
that call ldap_msgfree(), because on the client side we don't get to
assume that an error will lead to immediate process exit. If we've
missed any cleanups over there, that *would* be worth fixing.
regards, tom lane
On Sun, Sep 04, 2022 at 01:52:10AM -0400, Tom Lane wrote:
I can't get too excited about this. All of the error exit paths in
backend authentication code will lead immediately to process exit, so
the possibility of some memory being leaked really has no consequences
worth worrying about. If we *were* worried about it, sprinkling a few
more ldap_msgfree() calls into the existing code would hardly make it
more bulletproof.
Even if this is not critical in the backend for this authentication
path, I'd like to think that it is still a good practice for future
code so as anything code-pasted around would get the call. So I see
no reason to not put smth on HEAD at least.
There's lots of psprintf() and other
Postgres-universe calls in that code that could potentially fail and
force an elog exit without reaching ldap_msgfree. So if you wanted to
make this completely clean you'd need to resort to doing the freeing
in PG_CATCH blocks ... and I don't see any value in hacking it to that
extent.
Agreed. I cannot get excited about going down to that in this case.
What might be worth inspecting is the code paths in frontend libpq
that call ldap_msgfree(), because on the client side we don't get to
assume that an error will lead to immediate process exit. If we've
missed any cleanups over there, that *would* be worth fixing.
FWIW, I have looked at the frontend while writing my previous message
and did not notice anything.
--
Michael
On Sun, Sep 4, 2022 at 12:25 AM Michael Paquier <michael@paquier.xyz> wrote:
On Sun, Sep 04, 2022 at 01:52:10AM -0400, Tom Lane wrote:
I can't get too excited about this. All of the error exit paths in
backend authentication code will lead immediately to process exit, so
the possibility of some memory being leaked really has no consequences
worth worrying about. If we *were* worried about it, sprinkling a few
more ldap_msgfree() calls into the existing code would hardly make it
more bulletproof.Even if this is not critical in the backend for this authentication
path, I'd like to think that it is still a good practice for future
code so as anything code-pasted around would get the call. So I see
no reason to not put smth on HEAD at least.
Hi,
Here is updated patch as you suggested in your previous email.
Thanks
Attachments:
v2-ldap-msg-free.patchapplication/octet-stream; name=v2-ldap-msg-free.patchDownload
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index b2b0b83a97..5ad2f4ce11 100644
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -2610,6 +2610,8 @@ CheckLDAPAuth(Port *port)
filter, server_name, ldap_err2string(r)),
errdetail_for_ldap(ldap)));
ldap_unbind(ldap);
+ if (search_message != NULL)
+ ldap_msgfree(search_message);
pfree(passwd);
pfree(filter);
return STATUS_ERROR;
On Sun, Sep 4, 2022 at 3:58 AM Zhihong Yu <zyu@yugabyte.com> wrote:
On Sun, Sep 4, 2022 at 12:25 AM Michael Paquier <michael@paquier.xyz>
wrote:On Sun, Sep 04, 2022 at 01:52:10AM -0400, Tom Lane wrote:
I can't get too excited about this. All of the error exit paths in
backend authentication code will lead immediately to process exit, so
the possibility of some memory being leaked really has no consequences
worth worrying about. If we *were* worried about it, sprinkling a few
more ldap_msgfree() calls into the existing code would hardly make it
more bulletproof.Even if this is not critical in the backend for this authentication
path, I'd like to think that it is still a good practice for future
code so as anything code-pasted around would get the call. So I see
no reason to not put smth on HEAD at least.Hi,
Here is updated patch as you suggested in your previous email.Thanks
Hi,
Please take a look at patch v3.
Thanks
Attachments:
v3-ldap-msg-free.patchapplication/octet-stream; name=v3-ldap-msg-free.patchDownload
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index b2b0b83a97..4d4fe7a366 100644
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -2538,7 +2538,7 @@ CheckLDAPAuth(Port *port)
* trying to log in as.
*/
char *filter;
- LDAPMessage *search_message;
+ LDAPMessage *search_message = NULL;
LDAPMessage *entry;
char *attributes[] = {LDAP_NO_ATTRS, NULL};
char *dn;
@@ -2610,6 +2610,8 @@ CheckLDAPAuth(Port *port)
filter, server_name, ldap_err2string(r)),
errdetail_for_ldap(ldap)));
ldap_unbind(ldap);
+ if (search_message != NULL)
+ ldap_msgfree(search_message);
pfree(passwd);
pfree(filter);
return STATUS_ERROR;
On Sun, Sep 04, 2022 at 06:52:37AM -0700, Zhihong Yu wrote:
Please take a look at patch v3.
Fine as far as it goes. I would have put the initialization of
search_message closer to ldap_search_s() for consistency with libpq.
That's what we do with ldap_search_st().
--
Michael
On Sun, Sep 4, 2022 at 10:37 PM Michael Paquier <michael@paquier.xyz> wrote:
On Sun, Sep 04, 2022 at 06:52:37AM -0700, Zhihong Yu wrote:
Please take a look at patch v3.
Fine as far as it goes. I would have put the initialization of
search_message closer to ldap_search_s() for consistency with libpq.
That's what we do with ldap_search_st().
--
Michael
Hi,
Here is patch v4.
Attachments:
v4-ldap-msg-free.patchapplication/octet-stream; name=v4-ldap-msg-free.patchDownload
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index b2b0b83a97..945a0d370c 100644
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -2566,7 +2566,8 @@ CheckLDAPAuth(Port *port)
return STATUS_ERROR;
}
}
-
+
+ search_message = NULL;
/*
* Bind with a pre-defined username/password (if available) for
* searching. If none is specified, this turns into an anonymous bind.
@@ -2610,6 +2611,8 @@ CheckLDAPAuth(Port *port)
filter, server_name, ldap_err2string(r)),
errdetail_for_ldap(ldap)));
ldap_unbind(ldap);
+ if (search_message != NULL)
+ ldap_msgfree(search_message);
pfree(passwd);
pfree(filter);
return STATUS_ERROR;
On Mon, Sep 05, 2022 at 02:50:09AM -0700, Zhihong Yu wrote:
Here is patch v4.
FWIW, I am fine with what you are basically doing with v4, so I'd like
to apply that on HEAD on the basis of consistency with libpq. As Tom
said, this authentication path will fail, but I'd like to think that
this is a good practice anyway. I'll wait a few days first, in case
others have comments.
--
Michael