From 29dfffae0a6db6cb880ae873169f5512ddab703d Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Sat, 11 Jan 2025 11:45:56 -0500 Subject: [PATCH] Repair memory leaks in plpython. PLy_spi_execute_plan (PLyPlan.execute) and PLy_cursor_plan (plpy.cursor) use PLy_output_convert to convert Python values into Datums that can be passed to the query-to-execute. But they failed to pay much attention to its warning that it can leave "cruft generated along the way" behind. Repeated use of these methods can result in a substantial memory leak for the duration of the calling plpython function. To fix, make a temporary memory context to invoke PLy_output_convert in. This also lets us get rid of the rather fragile code that was here for retail pfree's of the converted Datums. Indeed, we don't need the PLyPlanObject.values field anymore at all, though I left it in place in the back branches in the name of ABI stability. Mat Arye and Tom Lane, per report from Mat Arye. Back-patch to all supported branches. Discussion: https://postgr.es/m/CADsUR0DvVgnZYWwnmKRK65MZg7YLUSTDLV61qdnrwtrAJgU6xw@mail.gmail.com --- src/pl/plpython/plpy_cursorobject.c | 52 ++++++++++++------------- src/pl/plpython/plpy_planobject.c | 1 - src/pl/plpython/plpy_planobject.h | 1 - src/pl/plpython/plpy_spi.c | 59 ++++++++++++----------------- 4 files changed, 49 insertions(+), 64 deletions(-) diff --git a/src/pl/plpython/plpy_cursorobject.c b/src/pl/plpython/plpy_cursorobject.c index 6108384c9a..bb3fa8a390 100644 --- a/src/pl/plpython/plpy_cursorobject.c +++ b/src/pl/plpython/plpy_cursorobject.c @@ -140,7 +140,6 @@ PLy_cursor_plan(PyObject *ob, PyObject *args) { PLyCursorObject *cursor; volatile int nargs; - int i; PLyPlanObject *plan; PLyExecutionContext *exec_ctx = PLy_current_execution_context(); volatile MemoryContext oldcontext; @@ -199,13 +198,30 @@ PLy_cursor_plan(PyObject *ob, PyObject *args) PG_TRY(); { Portal portal; + MemoryContext tmpcontext; + Datum *volatile values; char *volatile nulls; volatile int j; + /* + * Converted arguments and associated cruft will be in this context, + * which is local to our subtransaction. + */ + tmpcontext = AllocSetContextCreate(CurTransactionContext, + "PL/Python temporary context", + ALLOCSET_SMALL_SIZES); + MemoryContextSwitchTo(tmpcontext); + if (nargs > 0) - nulls = palloc(nargs * sizeof(char)); + { + values = (Datum *) palloc(nargs * sizeof(Datum)); + nulls = (char *) palloc(nargs * sizeof(char)); + } else + { + values = NULL; nulls = NULL; + } for (j = 0; j < nargs; j++) { @@ -217,7 +233,7 @@ PLy_cursor_plan(PyObject *ob, PyObject *args) { bool isnull; - plan->values[j] = PLy_output_convert(arg, elem, &isnull); + values[j] = PLy_output_convert(arg, elem, &isnull); nulls[j] = isnull ? 'n' : ' '; } PG_FINALLY(2); @@ -227,7 +243,9 @@ PLy_cursor_plan(PyObject *ob, PyObject *args) PG_END_TRY(2); } - portal = SPI_cursor_open(NULL, plan->plan, plan->values, nulls, + MemoryContextSwitchTo(oldcontext); + + portal = SPI_cursor_open(NULL, plan->plan, values, nulls, exec_ctx->curr_proc->fn_readonly); if (portal == NULL) elog(ERROR, "SPI_cursor_open() failed: %s", @@ -237,40 +255,18 @@ PLy_cursor_plan(PyObject *ob, PyObject *args) PinPortal(portal); + MemoryContextDelete(tmpcontext); PLy_spi_subtransaction_commit(oldcontext, oldowner); } PG_CATCH(); { - int k; - - /* cleanup plan->values array */ - for (k = 0; k < nargs; k++) - { - if (!plan->args[k].typbyval && - (plan->values[k] != PointerGetDatum(NULL))) - { - pfree(DatumGetPointer(plan->values[k])); - plan->values[k] = PointerGetDatum(NULL); - } - } - Py_DECREF(cursor); - + /* Subtransaction abort will remove the tmpcontext */ PLy_spi_subtransaction_abort(oldcontext, oldowner); return NULL; } PG_END_TRY(); - for (i = 0; i < nargs; i++) - { - if (!plan->args[i].typbyval && - (plan->values[i] != PointerGetDatum(NULL))) - { - pfree(DatumGetPointer(plan->values[i])); - plan->values[i] = PointerGetDatum(NULL); - } - } - Assert(cursor->portalname != NULL); return (PyObject *) cursor; } diff --git a/src/pl/plpython/plpy_planobject.c b/src/pl/plpython/plpy_planobject.c index bbef889329..9427674d2f 100644 --- a/src/pl/plpython/plpy_planobject.c +++ b/src/pl/plpython/plpy_planobject.c @@ -54,7 +54,6 @@ PLy_plan_new(void) ob->plan = NULL; ob->nargs = 0; ob->types = NULL; - ob->values = NULL; ob->args = NULL; ob->mcxt = NULL; diff --git a/src/pl/plpython/plpy_planobject.h b/src/pl/plpython/plpy_planobject.h index 729effb163..a6b34fae19 100644 --- a/src/pl/plpython/plpy_planobject.h +++ b/src/pl/plpython/plpy_planobject.h @@ -15,7 +15,6 @@ typedef struct PLyPlanObject SPIPlanPtr plan; int nargs; Oid *types; - Datum *values; PLyObToDatum *args; MemoryContext mcxt; } PLyPlanObject; diff --git a/src/pl/plpython/plpy_spi.c b/src/pl/plpython/plpy_spi.c index bcbd07b70a..77fbfd6c86 100644 --- a/src/pl/plpython/plpy_spi.c +++ b/src/pl/plpython/plpy_spi.c @@ -66,7 +66,6 @@ PLy_spi_prepare(PyObject *self, PyObject *args) plan->nargs = nargs; plan->types = nargs ? palloc0(sizeof(Oid) * nargs) : NULL; - plan->values = nargs ? palloc0(sizeof(Datum) * nargs) : NULL; plan->args = nargs ? palloc0(sizeof(PLyObToDatum) * nargs) : NULL; MemoryContextSwitchTo(oldcontext); @@ -172,8 +171,7 @@ PyObject * PLy_spi_execute_plan(PyObject *ob, PyObject *list, long limit) { volatile int nargs; - int i, - rv; + int rv; PLyPlanObject *plan; volatile MemoryContext oldcontext; volatile ResourceOwner oldowner; @@ -219,13 +217,30 @@ PLy_spi_execute_plan(PyObject *ob, PyObject *list, long limit) PG_TRY(); { PLyExecutionContext *exec_ctx = PLy_current_execution_context(); + MemoryContext tmpcontext; + Datum *volatile values; char *volatile nulls; volatile int j; + /* + * Converted arguments and associated cruft will be in this context, + * which is local to our subtransaction. + */ + tmpcontext = AllocSetContextCreate(CurTransactionContext, + "PL/Python temporary context", + ALLOCSET_SMALL_SIZES); + MemoryContextSwitchTo(tmpcontext); + if (nargs > 0) - nulls = palloc(nargs * sizeof(char)); + { + values = (Datum *) palloc(nargs * sizeof(Datum)); + nulls = (char *) palloc(nargs * sizeof(char)); + } else + { + values = NULL; nulls = NULL; + } for (j = 0; j < nargs; j++) { @@ -237,7 +252,7 @@ PLy_spi_execute_plan(PyObject *ob, PyObject *list, long limit) { bool isnull; - plan->values[j] = PLy_output_convert(arg, elem, &isnull); + values[j] = PLy_output_convert(arg, elem, &isnull); nulls[j] = isnull ? 'n' : ' '; } PG_FINALLY(2); @@ -247,47 +262,23 @@ PLy_spi_execute_plan(PyObject *ob, PyObject *list, long limit) PG_END_TRY(2); } - rv = SPI_execute_plan(plan->plan, plan->values, nulls, + MemoryContextSwitchTo(oldcontext); + + rv = SPI_execute_plan(plan->plan, values, nulls, exec_ctx->curr_proc->fn_readonly, limit); ret = PLy_spi_execute_fetch_result(SPI_tuptable, SPI_processed, rv); - if (nargs > 0) - pfree(nulls); - + MemoryContextDelete(tmpcontext); PLy_spi_subtransaction_commit(oldcontext, oldowner); } PG_CATCH(); { - int k; - - /* - * cleanup plan->values array - */ - for (k = 0; k < nargs; k++) - { - if (!plan->args[k].typbyval && - (plan->values[k] != PointerGetDatum(NULL))) - { - pfree(DatumGetPointer(plan->values[k])); - plan->values[k] = PointerGetDatum(NULL); - } - } - + /* Subtransaction abort will remove the tmpcontext */ PLy_spi_subtransaction_abort(oldcontext, oldowner); return NULL; } PG_END_TRY(); - for (i = 0; i < nargs; i++) - { - if (!plan->args[i].typbyval && - (plan->values[i] != PointerGetDatum(NULL))) - { - pfree(DatumGetPointer(plan->values[i])); - plan->values[i] = PointerGetDatum(NULL); - } - } - if (rv < 0) { PLy_exception_set(PLy_exc_spi_error, -- 2.39.5