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

We had three occurrences of essentially the same coding pattern
wherein we tried to retrieve a query result from a libpq connection
without blocking.  In the case where PQconsumeInput failed (typically
indicating a lost connection), all three loops simply gave up and
returned, forgetting to clear any previously-collected PGresult
object.  Since those are malloc'd not palloc'd, the oversight results
in a process-lifespan memory leak.

One instance, in libpqwalreceiver, is of little significance because
the walreceiver process would just quit anyway if its connection fails.
But we might as well fix it.

The other two instances, in postgres_fdw, are somewhat more worrisome
because at least in principle the scenario could be repeated, allowing
the amount of memory leaked to build up to something worth worrying
about.  Moreover, in these cases the loops contain CHECK_FOR_INTERRUPTS
calls, as well as other calls that could potentially elog(ERROR),
providing another way to exit without having cleared the PGresult.
Here we need to add PG_TRY logic similar to what exists in quite a
few other places in postgres_fdw.

Coverity noted the libpqwalreceiver bug; I found the other two cases
by checking all calls of PQconsumeInput.

Back-patch to all supported versions as appropriate (9.2 lacks
postgres_fdw, so this is really quite unexciting for that branch).

Discussion: https://postgr.es/m/22620.1497486981@sss.pgh.pa.us

diff --git a/contrib/postgres_fdw/connection.c b/contrib/postgres_fdw/connection.c
index 1b691fb05e0cf757900fbb0d946cd6af76cb6728..9818d2750f88645323e75ce545f284776d21e943 100644 (file)
--- a/contrib/postgres_fdw/connection.c
+++ b/contrib/postgres_fdw/connection.c
@@ -485,7 +485,7 @@ pgfdw_exec_query(PGconn *conn, const char *query)
  *
  * This function offers quick responsiveness by checking for any interruptions.
  *
- * This function emulates the PQexec()'s behavior of returning the last result
+ * This function emulates PQexec()'s behavior of returning the last result
  * when there are many.
  *
  * Caller is responsible for the error handling on the result.
@@ -493,40 +493,50 @@ pgfdw_exec_query(PGconn *conn, const char *query)
 PGresult *
 pgfdw_get_result(PGconn *conn, const char *query)
 {
-       PGresult   *last_res = NULL;
+       PGresult   *volatile last_res = NULL;
 
-       for (;;)
+       /* In what follows, do not leak any PGresults on an error. */
+       PG_TRY();
        {
-               PGresult   *res;
-
-               while (PQisBusy(conn))
+               for (;;)
                {
-                       int                     wc;
+                       PGresult   *res;
 
-                       /* Sleep until there's something to do */
-                       wc = WaitLatchOrSocket(MyLatch,
-                                                                  WL_LATCH_SET | WL_SOCKET_READABLE,
-                                                                  PQsocket(conn),
-                                                                  -1L, PG_WAIT_EXTENSION);
-                       ResetLatch(MyLatch);
+                       while (PQisBusy(conn))
+                       {
+                               int                     wc;
 
-                       CHECK_FOR_INTERRUPTS();
+                               /* Sleep until there's something to do */
+                               wc = WaitLatchOrSocket(MyLatch,
+                                                                          WL_LATCH_SET | WL_SOCKET_READABLE,
+                                                                          PQsocket(conn),
+                                                                          -1L, PG_WAIT_EXTENSION);
+                               ResetLatch(MyLatch);
 
-                       /* Data available in socket */
-                       if (wc & WL_SOCKET_READABLE)
-                       {
-                               if (!PQconsumeInput(conn))
-                                       pgfdw_report_error(ERROR, NULL, conn, false, query);
+                               CHECK_FOR_INTERRUPTS();
+
+                               /* Data available in socket? */
+                               if (wc & WL_SOCKET_READABLE)
+                               {
+                                       if (!PQconsumeInput(conn))
+                                               pgfdw_report_error(ERROR, NULL, conn, false, query);
+                               }
                        }
-               }
 
-               res = PQgetResult(conn);
-               if (res == NULL)
-                       break;                          /* query is complete */
+                       res = PQgetResult(conn);
+                       if (res == NULL)
+                               break;                  /* query is complete */
 
+                       PQclear(last_res);
+                       last_res = res;
+               }
+       }
+       PG_CATCH();
+       {
                PQclear(last_res);
-               last_res = res;
+               PG_RE_THROW();
        }
+       PG_END_TRY();
 
        return last_res;
 }
@@ -1006,6 +1016,7 @@ pgfdw_exec_cleanup_query(PGconn *conn, const char *query, bool ignore_errors)
                pgfdw_report_error(WARNING, result, conn, true, query);
                return ignore_errors;
        }
+       PQclear(result);
 
        return true;
 }
@@ -1028,56 +1039,75 @@ pgfdw_exec_cleanup_query(PGconn *conn, const char *query, bool ignore_errors)
 static bool
 pgfdw_get_cleanup_result(PGconn *conn, TimestampTz endtime, PGresult **result)
 {
-       PGresult   *last_res = NULL;
+       volatile bool timed_out = false;
+       PGresult   *volatile last_res = NULL;
 
-       for (;;)
+       /* In what follows, do not leak any PGresults on an error. */
+       PG_TRY();
        {
-               PGresult   *res;
-
-               while (PQisBusy(conn))
+               for (;;)
                {
-                       int                     wc;
-                       TimestampTz now = GetCurrentTimestamp();
-                       long            secs;
-                       int                     microsecs;
-                       long            cur_timeout;
-
-                       /* If timeout has expired, give up, else get sleep time. */
-                       if (now >= endtime)
-                               return true;
-                       TimestampDifference(now, endtime, &secs, &microsecs);
-
-                       /* To protect against clock skew, limit sleep to one minute. */
-                       cur_timeout = Min(60000, secs * USECS_PER_SEC + microsecs);
-
-                       /* Sleep until there's something to do */
-                       wc = WaitLatchOrSocket(MyLatch,
+                       PGresult   *res;
+
+                       while (PQisBusy(conn))
+                       {
+                               int                     wc;
+                               TimestampTz now = GetCurrentTimestamp();
+                               long            secs;
+                               int                     microsecs;
+                               long            cur_timeout;
+
+                               /* If timeout has expired, give up, else get sleep time. */
+                               if (now >= endtime)
+                               {
+                                       timed_out = true;
+                                       goto exit;
+                               }
+                               TimestampDifference(now, endtime, &secs, &microsecs);
+
+                               /* To protect against clock skew, limit sleep to one minute. */
+                               cur_timeout = Min(60000, secs * USECS_PER_SEC + microsecs);
+
+                               /* Sleep until there's something to do */
+                               wc = WaitLatchOrSocket(MyLatch,
                                                          WL_LATCH_SET | WL_SOCKET_READABLE | WL_TIMEOUT,
-                                                                  PQsocket(conn),
-                                                                  cur_timeout, PG_WAIT_EXTENSION);
-                       ResetLatch(MyLatch);
+                                                                          PQsocket(conn),
+                                                                          cur_timeout, PG_WAIT_EXTENSION);
+                               ResetLatch(MyLatch);
 
-                       CHECK_FOR_INTERRUPTS();
+                               CHECK_FOR_INTERRUPTS();
 
-                       /* Data available in socket */
-                       if (wc & WL_SOCKET_READABLE)
-                       {
-                               if (!PQconsumeInput(conn))
+                               /* Data available in socket? */
+                               if (wc & WL_SOCKET_READABLE)
                                {
-                                       *result = NULL;
-                                       return false;
+                                       if (!PQconsumeInput(conn))
+                                       {
+                                               /* connection trouble; treat the same as a timeout */
+                                               timed_out = true;
+                                               goto exit;
+                                       }
                                }
                        }
-               }
 
-               res = PQgetResult(conn);
-               if (res == NULL)
-                       break;                          /* query is complete */
+                       res = PQgetResult(conn);
+                       if (res == NULL)
+                               break;                  /* query is complete */
 
+                       PQclear(last_res);
+                       last_res = res;
+               }
+exit:  ;
+       }
+       PG_CATCH();
+       {
                PQclear(last_res);
-               last_res = res;
+               PG_RE_THROW();
        }
+       PG_END_TRY();
 
-       *result = last_res;
-       return false;
+       if (timed_out)
+               PQclear(last_res);
+       else
+               *result = last_res;
+       return timed_out;
 }

diff --git a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
index 7509b4fe60a60ec808a025b644f2cdff334556b0..f6fa0e4c16aaabcf3620244128cf831ca707f831 100644 (file)
--- a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
+++ b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
@@ -591,13 +591,19 @@ libpqrcv_PQexec(PGconn *streamConn, const char *query)
                                ResetLatch(MyLatch);
                                CHECK_FOR_INTERRUPTS();
                        }
+
+                       /* Consume whatever data is available from the socket */
                        if (PQconsumeInput(streamConn) == 0)
-                               return NULL;    /* trouble */
+                       {
+                               /* trouble; drop whatever we had and return NULL */
+                               PQclear(lastResult);
+                               return NULL;
+                       }
                }
 
                /*
-                * Emulate the PQexec()'s behavior of returning the last result when
-                * there are many. We are fine with returning just last error message.
+                * Emulate PQexec()'s behavior of returning the last result when there
+                * are many.  We are fine with returning just last error message.
                 */
                result = PQgetResult(streamConn);
                if (result == NULL)