Assorted leaks of PGresults
Coverity complained that libpqwalreceiver.c's libpqrcv_PQexec()
potentially leaks a PGresult. It's right: if PQconsumeInput() fails after
we've already collected at least one PGresult, the function just returns
NULL without remembering to free last_result. That's easy enough to fix,
just insert "PQclear(lastResult)" before the return. I do not think this
is a significant leak, because the walreceiver process would just exit
anyway after the failure. But we ought to fix it in case somebody copies
the logic into someplace less forgiving.
In fact ... I looked through other callers of PQconsumeInput() to see if
we had the same wrong coding pattern elsewhere, and we do, in two places
in postgres_fdw/connection.c. One of those is new as of last week, in
commit ae9bfc5d6. What's worse, in those cases we have code inside the
loop that might throw elog(ERROR), resulting in a permanently leaked
PGresult --- and since the backend process would keep going, this could
possibly be repeated and build up to a leak that amounted to something
significant. We need to have PG_TRY blocks in both these places that
ensure that any held PGresult gets freed before we exit the functions.
Further review showed an additional leak introduced by ae9bfc5d6, to wit
that pgfdw_exec_cleanup_query() just forgets to clear the PGresult
returned by pgfdw_get_cleanup_result() in its normal non-error path.
In short, I think we need the attached patch in HEAD. It needs to be
back-patched too, though the code seems to be a bit different in the
back branches.
regards, tom lane
Attachments:
fix-some-PGresult-leaks.patchtext/x-diff; charset=us-ascii; name=fix-some-PGresult-leaks.patchDownload+158-142
On Thu, Jun 15, 2017 at 9:36 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Coverity complained that libpqwalreceiver.c's libpqrcv_PQexec()
potentially leaks a PGresult. It's right: if PQconsumeInput() fails after
we've already collected at least one PGresult, the function just returns
NULL without remembering to free last_result. That's easy enough to fix,
just insert "PQclear(lastResult)" before the return. I do not think this
is a significant leak, because the walreceiver process would just exit
anyway after the failure. But we ought to fix it in case somebody copies
the logic into someplace less forgiving.
Indeed.
In fact ... I looked through other callers of PQconsumeInput() to see if
we had the same wrong coding pattern elsewhere, and we do, in two places
in postgres_fdw/connection.c. One of those is new as of last week, in
commit ae9bfc5d6.
It may be actually the reason why coverity complained about the one in
libpqwalreceiver.c. I have noticed a couple of times in the past that
similar code patterns are more likely to be checked again if one of
them is added or changed. Impossible to say if that's true, but I
suspect that there is some underground intelligence to improve failure
detection, or at least re-check them.
What's worse, in those cases we have code inside the
loop that might throw elog(ERROR), resulting in a permanently leaked
PGresult --- and since the backend process would keep going, this could
possibly be repeated and build up to a leak that amounted to something
significant. We need to have PG_TRY blocks in both these places that
ensure that any held PGresult gets freed before we exit the functions.Further review showed an additional leak introduced by ae9bfc5d6, to wit
that pgfdw_exec_cleanup_query() just forgets to clear the PGresult
returned by pgfdw_get_cleanup_result() in its normal non-error path.In short, I think we need the attached patch in HEAD. It needs to be
back-patched too, though the code seems to be a bit different in the
back branches.
I had a look at this patch, checking the surroundings and I am not
noticing any other leaks in postgres_fdw or other modules.
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers