Fix minor memory leak in connection string validation
Introduced in commit c3afe8cf5a.
Someone issuing repeated "CREATE SUBSCRIPTION" commands where the
connection has no password and must_have_password is true will leak
malloc'd memory in the error path. Minor issue in practice, because I
suspect that a user privileged enough to create a subscription could
cause bigger problems.
It makes me wonder if we should use the resowner mechanism to track
pointers to malloc'd memory. Then we could use a standard pattern for
these kinds of cases, and it would also catch more remote issues, like
if a pstrdup() fails in an error path (which can happen a few lines up
if the parse fails).
Patch attached; intended for 16 and 17.
Regards,
Jeff Davis
Attachments:
0001-Fix-memory-leak-in-connection-string-validation.patchtext/x-patch; charset=UTF-8; name=0001-Fix-memory-leak-in-connection-string-validation.patchDownload+5-1
On Fri, Jan 12, 2024 at 03:06:26PM -0800, Jeff Davis wrote:
It makes me wonder if we should use the resowner mechanism to track
pointers to malloc'd memory. Then we could use a standard pattern for
these kinds of cases, and it would also catch more remote issues, like
if a pstrdup() fails in an error path (which can happen a few lines up
if the parse fails).
That seems worth exploring.
if (!uses_password) + { + /* malloc'd, so we must free it explicitly */ + PQconninfoFree(opts); + ereport(ERROR, (errcode(ERRCODE_S_R_E_PROHIBITED_SQL_STATEMENT_ATTEMPTED), errmsg("password is required"), errdetail("Non-superusers must provide a password in the connection string."))); + } }PQconninfoFree(opts);
Another option could be to surround this with PG_TRY/PG_FINALLY, but your
patch seems sufficient, too.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Nathan Bossart <nathandbossart@gmail.com> writes:
On Fri, Jan 12, 2024 at 03:06:26PM -0800, Jeff Davis wrote:
It makes me wonder if we should use the resowner mechanism to track
pointers to malloc'd memory. Then we could use a standard pattern for
these kinds of cases, and it would also catch more remote issues, like
if a pstrdup() fails in an error path (which can happen a few lines up
if the parse fails).
That seems worth exploring.
I'm pretty dubious about adding overhead for that, mainly because
most of the direct callers of malloc in a backend are going to be
code that's not under our control. Modifying the callers that we
do control is not going to give a full solution, and could well be
outright misleading.
Another option could be to surround this with PG_TRY/PG_FINALLY, but your
patch seems sufficient, too.
Yeah, seems fine for now. If that function grows any more complexity
then we could think about using PG_TRY.
regards, tom lane