Horton

Thread

Commits

  1. Fix low-probability leaks of PGresult objects in the backend.

  2. postgres_fdw: Allow cancellation of transaction control commands.

  1. Assorted leaks of PGresults

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-06-15T00:36:21Z

    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

  2. Re: Assorted leaks of PGresults

    Michael Paquier <michael.paquier@gmail.com> — 2017-06-15T03:01:01Z

    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