8000 Prevent leakage of SPI tuple tables during subtransaction abort. · postgrespro/postgres_cluster@3d13623 · GitHub
[go: up one dir, main page]
Skip to content

Commit 3d13623

Browse files
tglsfdctglsfdc
committed
Prevent leakage of SPI tuple tables during subtransaction abort.
plpgsql often just remembers SPI-result tuple tables in local variables, and has no mechanism for freeing them if an ereport(ERROR) causes an escape out of the execution function whose local variable it is. In the original coding, that wasn't a problem because the tuple table would be cleaned up when the function's SPI context went away during transaction abort. However, once plpgsql grew the ability to trap exceptions, repeated trapping of errors within a function could result in significant intra-function-call memory leakage, as illustrated in bug #8279 from Chad Wagner. We could fix this locally in plpgsql with a bunch of PG_TRY/PG_CATCH coding, but that would be tedious, probably slow, and prone to bugs of omission; moreover it would do nothing for similar risks elsewhere. What seems like a better plan is to make SPI itself responsible for freeing tuple tables at subtransaction abort. This patch attacks the problem that way, keeping a list of live tuple tables within each SPI function context. Currently, such freeing is automatic for tuple tables made within the failed subtransaction. We might later add a SPI call to mark a tuple table as not to be freed this way, allowing callers to opt out; but until someone exhibits a clear use-case for such behavior, it doesn't seem worth bothering. A very useful side-effect of this change is that SPI_freetuptable() can now defend itself against bad calls, such as duplicate free requests; this should make things more robust in many places. (In particular, this reduces the risks involved if a third-party extension contains now-redundant SPI_freetuptable() calls in error cleanup code.) Even though the leakage problem is of long standing, it seems imprudent to back-patch this into stable branches, since it does represent an API semantics change for SPI users. We'll patch this in 9.3, but live with the leakage in older branches.
1 parent fd27b99 commit 3d13623

File tree

7 files changed

+121
-17
lines changed
  • include/executor
  • pl

    • 7 files changed

    +121
    -17
    lines changed

    ‎doc/src/sgml/spi.sgml

    Lines changed: 13 additions & 3 deletions
    Original file line numberDiff line numberDiff line change
    @@ -3934,8 +3934,8 @@ void SPI_freetuptable(SPITupleTable * <parameter>tuptable</parameter>)
    39343934
    <para>
    39353935
    <function>SPI_freetuptable</function> frees a row set created by a
    39363936
    prior SPI command execution function, such as
    3937-
    <function>SPI_execute</>. Therefore, this function is usually called
    3938-
    with the global variable <varname>SPI_tupletable</varname> as
    3937+
    <function>SPI_execute</>. Therefore, this function is often called
    3938+
    with the global variable <varname>SPI_tuptable</varname> as
    39393939
    argument.
    39403940
    </para>
    39413941

    @@ -3944,6 +3944,16 @@ void SPI_freetuptable(SPITupleTable * <parameter>tuptable</parameter>)
    39443944
    multiple commands and does not want to keep the results of earlier
    39453945
    commands around until it ends. Note that any unfreed row sets will
    39463946
    be freed anyway at <function>SPI_finish</>.
    3947+
    Also, if a subtransaction is started and then aborted within execution
    3948+
    of a SPI procedure, SPI automatically frees any row sets created while
    3949+
    the subtransaction was running.
    3950+
    </para>
    3951+
    3952+
    <para>
    3953+
    Beginning in <productname>PostgreSQL</> 9.3,
    3954+
    <function>SPI_freetuptable</function> contains guard logic to protect
    3955+
    against duplicate deletion requests for the same row set. In previous
    3956+
    releases, duplicate deletions would lead to crashes.
    39473957
    </para>
    39483958
    </refsect1>
    39493959

    @@ -3955,7 +3965,7 @@ void SPI_freetuptable(SPITupleTable * <parameter>tuptable</parameter>)
    39553965
    <term><literal>SPITupleTable * <parameter>tuptable</parameter></literal></term>
    39563966
    <listitem>
    39573967
    <para>
    3958-
    pointer to row set to free
    3968+
    pointer to row set to free, or NULL to do nothing
    39593969
    </para>
    39603970
    </listitem>
    39613971
    </varlistentry>

    ‎src/backend/executor/spi.c

    Lines changed: 95 additions & 7 deletions
    Original file line numberDiff line numberDiff line change
    @@ -126,6 +126,7 @@ SPI_connect(void)
    126126
    _SPI_current->processed = 0;
    127127
    _SPI_current->lastoid = InvalidOid;
    128128
    _SPI_current->tuptable = NULL;
    129+
    slist_init(&_SPI_current->tuptables);
    129130
    _SPI_current->procCxt = NULL; /* in case we fail to create 'em */
    130131
    _SPI_current->execCxt = NULL;
    131132
    _SPI_current->connectSubid = GetCurrentSubTransactionId();
    @@ -166,7 +167,7 @@ SPI_finish(void)
    166167
    /* Restore memory context as it was before procedure call */
    167168
    MemoryContextSwitchTo(_SPI_current->savedcxt);
    168169

    169-
    /* Release memory used in procedure call */
    170+
    /* Release memory used in procedure call (including tuptables) */
    170171
    MemoryContextDelete(_SPI_current->execCxt);
    171172
    _SPI_current->execCxt = NULL;
    172173
    MemoryContextDelete(_SPI_current->procCxt);
    @@ -282,11 +283,35 @@ AtEOSubXact_SPI(bool isCommit, SubTransactionId mySubid)
    282283
    */
    283284
    if (_SPI_current && !isCommit)
    284285
    {
    286+
    slist_mutable_iter siter;
    287+
    285288
    /* free Executor memory the same as _SPI_end_call would do */
    286289
    MemoryContextResetAndDeleteChildren(_SPI_current->execCxt);
    287-
    /* throw away any partially created tuple-table */
    288-
    SPI_freetuptable(_SPI_current->tuptable);
    289-
    _SPI_current->tuptable = NULL;
    290+
    291+
    /* throw away any tuple tables created within current subxact */
    292+
    slist_foreach_modify(siter, &_SPI_current->tuptables)
    293+
    {
    294+
    SPITupleTable *tuptable;
    295+
    296+
    tuptable = slist_container(SPITupleTable, next, siter.cur);
    297+
    if (tuptable->subid >= mySubid)
    298+
    {
    299+
    /*
    300+
    * If we used SPI_freetuptable() here, its internal search of
    301+
    * the tuptables list would make this operation O(N^2).
    302+
    * Instead, just free the tuptable manually. This should
    303+
    * match what SPI_freetuptable() does.
    304+
    */
    305+
    slist_delete_current(&siter);
    306+
    if (tuptable == _SPI_current->tuptable)
    307+
    _SPI_current->tuptable = NULL;
    308+
    if (tuptable == SPI_tuptable)
    309+
    SPI_tuptable = NULL;
    310+
    MemoryContextDelete(tuptable->tuptabcxt);
    311+
    }
    312+
    }
    313+
    /* in particular we should have gotten rid of any in-progress table */
    314+
    Assert(_SPI_current->tuptable == NULL);
    290315
    }
    291316
    }
    292317

    @@ -1021,8 +1046,59 @@ SPI_freetuple(HeapTuple tuple)
    10211046
    void
    10221047
    SPI_freetuptable(SPITupleTable *tuptable)
    10231048
    {
    1024-
    if (tuptable != NULL)
    1025-
    MemoryContextDelete(tuptable->tuptabcxt);
    1049+
    bool found = false;
    1050+
    1051+
    /* ignore call if NULL pointer */
    1052+
    if (tuptable == NULL)
    1053+
    return;
    1054+
    1055+
    /*
    1056+
    * Since this function might be called during error recovery, it seems
    1057+
    * best not to insist that the caller be actively connected. We just
    1058+
    * search the topmost SPI context, connected or not.
    1059+
    */
    1060+
    if (_SPI_connected >= 0)
    1061+
    {
    1062+
    slist_mutable_iter siter;
    1063+
    1064+
    if (_SPI_current != &(_SPI_stack[_SPI_connected]))
    1065+
    elog(ERROR, "SPI stack corrupted");
    1066+
    1067+
    /* find tuptable in active list, then remove it */
    1068+
    slist_foreach_modify(siter, &_SPI_current->tuptables)
    1069+
    {
    1070+
    SPITupleTable *tt;
    1071+
    1072+
    tt = slist_container(SPITupleTable, next, siter.cur);
    1073+
    if (tt == tuptable)
    1074+
    {
    1075+
    slist_delete_current(&siter);
    1076+
    found = true;
    1077+
    break;
    1078+
    }
    1079+
    }
    1080+
    }
    1081+
    1082+
    /*
    1083+
    * Refuse the deletion if we didn't find it in the topmost SPI context.
    1084+
    * This is primarily a guard against double deletion, but might prevent
    1085+
    * other errors as well. Since the worst consequence of not deleting a
    1086+
    * tuptable would be a transient memory leak, this is just a WARNING.
    1087+
    */
    1088+
    if (!found)
    1089+
    {
    1090+
    elog(WARNING, "attempt to delete invalid SPITupleTable %p", tuptable);
    1091+
    return;
    1092+
    }
    1093+
    1094+
    /* for safety, reset global variables that might point at tuptable */
    1095+
    if (tuptable == _SPI_current->tuptable)
    1096+
    _SPI_current->tuptable = NULL;
    1097+
    if (tuptable == SPI_tuptable)
    1098+
    SPI_tuptable = NULL;
    1099+
    1100+
    /* release all memory belonging to tuptable */
    1101+
    MemoryContextDelete(tuptable->tuptabcxt);
    10261102
    }
    10271103

    10281104

    @@ -1656,6 +1732,8 @@ spi_dest_startup(DestReceiver *self, int operation, TupleDesc typeinfo)
    16561732
    if (_SPI_current->tuptable != NULL)
    16571733
    elog(ERROR, "improper call to spi_dest_startup");
    16581734

    1735+
    /* We create the tuple table context as a child of procCxt */
    1736+
    16591737
    oldcxt = _SPI_procmem(); /* switch to procedure memory context */
    16601738

    16611739
    tuptabcxt = AllocSetContextCreate(CurrentMemoryContext,
    @@ -1666,8 +1744,18 @@ spi_dest_startup(DestReceiver *self, int operation, TupleDesc typeinfo)
    16661744
    MemoryContextSwitchTo(tuptabcxt);
    16671745

    16681746
    _SPI_current->tuptable = tuptable = (SPITupleTable *)
    1669-
    palloc(sizeof(SPITupleTable));
    1747+
    palloc0(sizeof(SPITupleTable));
    16701748
    tuptable->tuptabcxt = tuptabcxt;
    1749+
    tuptable->subid = GetCurrentSubTransactionId();
    1750+
    1751+
    /*
    1752+
    * The tuptable is now valid enough to be freed by AtEOSubXact_SPI, so put
    1753+
    * it onto the SPI context's tuptables list. This will ensure it's not
    1754+
    * leaked even in the unlikely event the following few lines fail.
    1755+
    */
    1756+
    slist_push_head(&_SPI_current->tuptables, &tuptable->next);
    1757+
    1758+
    /* set up initial allocations */
    16711759
    tuptable->alloced = tuptable->free = 128;
    16721760
    tuptable->vals = (HeapTuple *) palloc(tuptable->alloced * sizeof(HeapTuple));
    16731761
    tuptable->tupdesc = CreateTupleDescCopy(typeinfo);

    ‎src/include/executor/spi.h

    Lines changed: 3 additions & 0 deletions
    Original file line numberDiff line numberDiff line change
    @@ -13,6 +13,7 @@
    1313
    #ifndef SPI_H
    1414
    #define SPI_H
    1515

    16+
    #include "lib/ilist.h"
    1617
    #include "nodes/parsenodes.h"
    1718
    #include "utils/portal.h"
    1819

    @@ -24,6 +25,8 @@ typedef struct SPITupleTable
    2425
    uint32 free; /* # of free vals */
    2526
    TupleDesc tupdesc; /* tuple descriptor */
    2627
    HeapTuple *vals; /* tuples */
    28+
    slist_node next; /* link for internal bookkeeping */
    29+
    SubTransactionId subid; /* subxact in which tuptable was created */
    2730
    } SPITupleTable;
    2831

    2932
    /* Plans are opaque structs for standard users of SPI */

    ‎src/include/executor/spi_priv.h

    Lines changed: 3 additions & 1 deletion
    Original file line numberDiff line numberDiff line change
    @@ -23,8 +23,10 @@ typedef struct
    2323
    /* current results */
    2424
    uint32 processed; /* by Executor */
    2525
    Oid lastoid;
    26-
    SPITupleTable *tuptable;
    26+
    SPITupleTable *tuptable; /* tuptable currently being built */
    2727

    28+
    /* resources of this execution context */
    29+
    slist_head tuptables; /* list of all live SPITupleTables */
    2830
    MemoryContext procCxt; /* procedure context */
    2931
    MemoryContext execCxt; /* executor context */
    3032
    MemoryContext savedcxt; /* context of SPI_connect's caller */

    ‎src/pl/plpgsql/src/pl_exec.c

    Lines changed: 7 additions & 1 deletion
    Original file line numberDiff line numberDiff line change
    @@ -1202,7 +1202,13 @@ exec_stmt_block(PLpgSQL_execstate *estate, PLpgSQL_stmt_block *block)
    12021202
    */
    12031203
    SPI_restore_connection();
    12041204

    1205-
    /* Must clean up the econtext too */
    1205+
    /*
    1206+
    * Must clean up the econtext too. However, any tuple table made
    1207+
    * in the subxact will have been thrown away by SPI during subxact
    1208+
    * abort, so we don't need to (and mustn't try to) free the
    1209+
    * eval_tuptable.
    1210+
    */
    1211+
    estate->eval_tuptable = NULL;
    12061212
    exec_eval_cleanup(estate);
    12071213

    12081214
    /* Look for a matching exception handler */

    ‎src/pl/plpython/plpy_cursorobject.c

    Lines changed: 0 additions & 4 deletions
    Original file line numberDiff line numberDiff line change
    @@ -377,8 +377,6 @@ PLy_cursor_iternext(PyObject *self)
    377377
    }
    378378
    PG_CATCH();
    379379
    {
    380-
    SPI_freetuptable(SPI_tuptable);
    381-
    382380
    PLy_spi_subtransaction_abort(oldcontext, oldowner);
    383381
    return NULL;
    384382
    }
    @@ -461,8 +459,6 @@ PLy_cursor_fetch(PyObject *self, PyObject *args)
    461459
    }
    462460
    PG_CATCH();
    463461
    {
    464-
    SPI_freetuptable(SPI_tuptable);
    465-
    466462
    PLy_spi_subtransaction_abort(oldcontext, oldowner);
    467463
    return NULL;
    468464
    }

    ‎src/pl/plpython/plpy_spi.c

    Lines changed: 0 additions & 1 deletion
    Original file line numberDiff line numberDiff line change
    @@ -439,7 +439,6 @@ PLy_spi_execute_fetch_result(SPITupleTable *tuptable, int rows, int status)
    439439
    {
    440440
    MemoryContextSwitchTo(oldcontext);
    441441
    PLy_typeinfo_dealloc(&args);
    442-
    SPI_freetuptable(tuptable);
    443442
    Py_DECREF(result);
    444443
    PG_RE_THROW();
    445444
    }

    0 commit comments

    Comments
     (0)
    0