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
