Skip to content

Commit 7d8f595

Browse files
tglsfdctglsfdc
committed
Create infrastructure to reliably prevent leakage of PGresults.
Commit 232d8ca fixed a case where postgres_fdw could lose track of a PGresult object, resulting in a process-lifespan memory leak. But I have little faith that there aren't other potential PGresult leakages, now or in future, in the backend modules that use libpq. Therefore, this patch proposes infrastructure that makes all PGresults returned from libpq act as though they are palloc'd in the CurrentMemoryContext (with the option to relocate them to another context later). This should greatly reduce the risk of careless leaks, and it also permits removal of a bunch of code that attempted to prevent such leaks via PG_TRY blocks. This patch adds infrastructure that wraps each PGresult in a "libpqsrv_PGresult" that provides a memory context reset callback to PQclear the PGresult. Code using this abstraction is inherently memory-safe to the same extent as we are accustomed to in most backend code. Furthermore, we add some macros that automatically redirect calls of the libpq functions concerned with PGresults to use this infrastructure, so that almost no source-code changes are needed to wheel this infrastructure into place in all the backend code that uses libpq. Perhaps in future we could create similar infrastructure for PGconn objects, but there seems less need for that. This patch just creates the infrastructure and makes relevant code use it, including reverting 232d8ca in favor of this mechanism. A good deal of follow-on simplification is possible now that we don't have to be so cautious about freeing PGresults, but I'll put that in a separate patch. Author: Tom Lane <tgl@sss.pgh.pa.us> Reviewed-by: Matheus Alcantara <matheusssilv97@gmail.com> Discussion: https://postgr.es/m/2976982.1748049023@sss.pgh.pa.us
1 parent 5457ea4 commit 7d8f595

File tree

7 files changed

+322
-44
lines changed

7 files changed

+322
-44
lines changed

‎contrib/postgres_fdw/postgres_fdw.c

Lines changed: 9 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -240,7 +240,6 @@ typedef struct PgFdwDirectModifyState
240240
PGresult *result; /* result for query */
241241
int num_tuples; /* # of result tuples */
242242
int next_tuple; /* index of next one to return */
243-
MemoryContextCallback result_cb; /* ensures result will get freed */
244243
Relation resultRel; /* relcache entry for the target relation */
245244
AttrNumber *attnoMap; /* array of attnums of input user columns */
246245
AttrNumber ctidAttno; /* attnum of input ctid column */
@@ -2671,17 +2670,6 @@ postgresBeginDirectModify(ForeignScanState *node, int eflags)
26712670
dmstate = (PgFdwDirectModifyState *) palloc0(sizeof(PgFdwDirectModifyState));
26722671
node->fdw_state = dmstate;
26732672

2674-
/*
2675-
* We use a memory context callback to ensure that the dmstate's PGresult
2676-
* (if any) will be released, even if the query fails somewhere that's
2677-
* outside our control. The callback is always armed for the duration of
2678-
* the query; this relies on PQclear(NULL) being a no-op.
2679-
*/
2680-
dmstate->result_cb.func = (MemoryContextCallbackFunction) PQclear;
2681-
dmstate->result_cb.arg = NULL;
2682-
MemoryContextRegisterResetCallback(CurrentMemoryContext,
2683-
&dmstate->result_cb);
2684-
26852673
/*
26862674
* Identify which user to do the remote access as. This should match what
26872675
* ExecCheckPermissions() does.
@@ -2829,13 +2817,7 @@ postgresEndDirectModify(ForeignScanState *node)
28292817
return;
28302818

28312819
/* Release PGresult */
2832-
if (dmstate->result)
2833-
{
2834-
PQclear(dmstate->result);
2835-
dmstate->result = NULL;
2836-
/* ... and don't forget to disable the callback */
2837-
dmstate->result_cb.arg = NULL;
2838-
}
2820+
PQclear(dmstate->result);
28392821

28402822
/* Release remote connection */
28412823
ReleaseConnection(dmstate->conn);
@@ -4615,20 +4597,20 @@ execute_dml_stmt(ForeignScanState *node)
46154597

46164598
/*
46174599
* Get the result, and check for success.
4618-
*
4619-
* We use a memory context callback to ensure that the PGresult will be
4620-
* released, even if the query fails somewhere that's outside our control.
4621-
* The callback is already registered, just need to fill in its arg.
46224600
*/
4623-
Assert(dmstate->result == NULL);
46244601
dmstate->result = pgfdw_get_result(dmstate->conn);
4625-
dmstate->result_cb.arg = dmstate->result;
4626-
46274602
if (PQresultStatus(dmstate->result) !=
46284603
(dmstate->has_returning ? PGRES_TUPLES_OK : PGRES_COMMAND_OK))
4629-
pgfdw_report_error(ERROR, dmstate->result, dmstate->conn, false,
4604+
pgfdw_report_error(ERROR, dmstate->result, dmstate->conn, true,
46304605
dmstate->query);
46314606

4607+
/*
4608+
* The result potentially needs to survive across multiple executor row
4609+
* cycles, so move it to the context where the dmstate is.
4610+
*/
4611+
dmstate->result = libpqsrv_PGresultSetParent(dmstate->result,
4612+
GetMemoryChunkContext(dmstate));
4613+
46324614
/* Get the number of rows affected. */
46334615
if (dmstate->has_returning)
46344616
dmstate->num_tuples = PQntuples(dmstate->result);

‎contrib/postgres_fdw/postgres_fdw.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@
1515

1616
#include "foreign/foreign.h"
1717
#include "lib/stringinfo.h"
18-
#include "libpq-fe.h"
18+
#include "libpq/libpq-be-fe.h"
1919
#include "nodes/execnodes.h"
2020
#include "nodes/pathnodes.h"
2121
#include "utils/relcache.h"

‎src/backend/utils/mmgr/mcxt.c

Lines changed: 36 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -560,9 +560,7 @@ MemoryContextDeleteChildren(MemoryContext context)
560560
* the specified context, since that means it will automatically be freed
561561
* when no longer needed.
562562
*
563-
* There is no API for deregistering a callback once registered. If you
564-
* want it to not do anything anymore, adjust the state pointed to by its
565-
* "arg" to indicate that.
563+
* Note that callers can assume this cannot fail.
566564
*/
567565
void
568566
MemoryContextRegisterResetCallback(MemoryContext context,
@@ -577,6 +575,41 @@ MemoryContextRegisterResetCallback(MemoryContext context,
577575
context->isReset = false;
578576
}
579577

578+
/*
579+
* MemoryContextUnregisterResetCallback
580+
* Undo the effects of MemoryContextRegisterResetCallback.
581+
*
582+
* This can be used if a callback's effects are no longer required
583+
* at some point before the context has been reset/deleted. It is the
584+
* caller's responsibility to pfree the callback struct (if needed).
585+
*
586+
* An assertion failure occurs if the callback was not registered.
587+
* We could alternatively define that case as a no-op, but that seems too
588+
* likely to mask programming errors such as passing the wrong context.
589+
*/
590+
void
591+
MemoryContextUnregisterResetCallback(MemoryContext context,
592+
MemoryContextCallback *cb)
593+
{
594+
MemoryContextCallback *prev,
595+
*cur;
596+
597+
Assert(MemoryContextIsValid(context));
598+
599+
for (prev = NULL, cur = context->reset_cbs; cur != NULL;
600+
prev = cur, cur = cur->next)
601+
{
602+
if (cur != cb)
603+
continue;
604+
if (prev)
605+
prev->next = cur->next;
606+
else
607+
context->reset_cbs = cur->next;
608+
return;
609+
}
610+
Assert(false);
611+
}
612+
580613
/*
581614
* MemoryContextCallResetCallbacks
582615
* Internal function to call all registered callbacks for context.

‎src/include/libpq/libpq-be-fe-helpers.h

Lines changed: 14 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -30,17 +30,7 @@
3030
#ifndef LIBPQ_BE_FE_HELPERS_H
3131
#define LIBPQ_BE_FE_HELPERS_H
3232

33-
/*
34-
* Despite the name, BUILDING_DLL is set only when building code directly part
35-
* of the backend. Which also is where libpq isn't allowed to be
36-
* used. Obviously this doesn't protect against libpq-fe.h getting included
37-
* otherwise, but perhaps still protects against a few mistakes...
38-
*/
39-
#ifdef BUILDING_DLL
40-
#error "libpq may not be used code directly built into the backend"
41-
#endif
42-
43-
#include "libpq-fe.h"
33+
#include "libpq/libpq-be-fe.h"
4434
#include "miscadmin.h"
4535
#include "storage/fd.h"
4636
#include "storage/latch.h"
@@ -462,13 +452,21 @@ exit: ;
462452
* This function is intended to be set via PQsetNoticeReceiver() so that
463453
* NOTICE, WARNING, and similar messages from the connection are reported via
464454
* ereport(), instead of being printed to stderr.
455+
*
456+
* Because this will be called from libpq with a "real" (not wrapped)
457+
* PGresult, we need to temporarily ignore libpq-be-fe.h's wrapper macros
458+
* for PGresult and also PQresultErrorMessage, and put back the wrappers
459+
* afterwards. That's not pretty, but there seems no better alternative.
465460
*/
461+
#undef PGresult
462+
#undef PQresultErrorMessage
463+
466464
static inline void
467465
libpqsrv_notice_receiver(void *arg, const PGresult *res)
468466
{
469-
char *message;
467+
const char *message;
470468
int len;
471-
char *prefix = (char *) arg;
469+
const char *prefix = (const char *) arg;
472470

473471
/*
474472
* Trim the trailing newline from the message text returned from
@@ -484,4 +482,7 @@ libpqsrv_notice_receiver(void *arg, const PGresult *res)
484482
errmsg_internal("%s: %.*s", prefix, len, message));
485483
}
486484

485+
#define PGresult libpqsrv_PGresult
486+
#define PQresultErrorMessage libpqsrv_PQresultErrorMessage
487+
487488
#endif /* LIBPQ_BE_FE_HELPERS_H */

0 commit comments

Comments
 (0)