Fix snapshot reference leak if lo_export fails.

If lo_export() fails to open the target file or to write to it, it leaks
the created LargeObjectDesc and its snapshot in the top-transaction
context and resource owner. That's pretty harmless, it's a small leak
after all, but it gives the user a "Snapshot reference leak" warning.

Fix by using a short-lived memory context and no resource owner for
transient LargeObjectDescs that are opened and closed within one function
call. The leak is easiest to reproduce with lo_export() on a directory
that doesn't exist, but in principle the other lo_* functions could also
fail.

Backpatch to all supported versions.

Reported-by: Andrew B
Reviewed-by: Alvaro Herrera
Discussion: https://www.postgresql.org/message-id/32bf767a-2d65-71c4-f170-122f416bab7e@iki.fi

diff --git a/src/backend/libpq/be-fsstubs.c b/src/backend/libpq/be-fsstubs.c
index 6073540d8d9ad8f3bfb056d263e5d220641f949c..39e59f1c791428cf914cdee1fced9ec93aa49f22 100644 (file)
--- a/src/backend/libpq/be-fsstubs.c
+++ b/src/backend/libpq/be-fsstubs.c
@@ -42,6 +42,7 @@
 #include <sys/stat.h>
 #include <unistd.h>
 
+#include "access/xact.h"
 #include "libpq/be-fsstubs.h"
 #include "libpq/libpq-fs.h"
 #include "miscadmin.h"
@@ -50,6 +51,7 @@
 #include "utils/acl.h"
 #include "utils/builtins.h"
 #include "utils/memutils.h"
+#include "utils/snapmgr.h"
 
 /* define this to enable debug logging */
 /* #define FSDB 1 */
@@ -67,19 +69,11 @@
 static LargeObjectDesc **cookies = NULL;
 static int cookies_size = 0;
 
+static bool lo_cleanup_needed = false;
 static MemoryContext fscxt = NULL;
 
-#define CreateFSContext() \
-   do { \
-       if (fscxt == NULL) \
-           fscxt = AllocSetContextCreate(TopMemoryContext, \
-                                         "Filesystem", \
-                                         ALLOCSET_DEFAULT_SIZES); \
-   } while (0)
-
-
-static int newLOfd(LargeObjectDesc *lobjCookie);
-static void deleteLOfd(int fd);
+static int newLOfd(void);
+static void closeLOfd(int fd);
 static Oid lo_import_internal(text *filename, Oid lobjOid);
 
 
@@ -99,11 +93,26 @@ be_lo_open(PG_FUNCTION_ARGS)
    elog(DEBUG4, "lo_open(%u,%d)", lobjId, mode);
 #endif
 
-   CreateFSContext();
+   /*
+    * Allocate a large object descriptor first.  This will also create
+    * 'fscxt' if this is the first LO opened in this transaction.
+    */
+   fd = newLOfd();
 
    lobjDesc = inv_open(lobjId, mode, fscxt);
+   lobjDesc->subid = GetCurrentSubTransactionId();
+
+   /*
+    * We must register the snapshot in TopTransaction's resowner so that it
+    * stays alive until the LO is closed rather than until the current portal
+    * shuts down.
+    */
+   if (lobjDesc->snapshot)
+       lobjDesc->snapshot = RegisterSnapshotOnOwner(lobjDesc->snapshot,
+                                                    TopTransactionResourceOwner);
 
-   fd = newLOfd(lobjDesc);
+   Assert(cookies[fd] == NULL);
+   cookies[fd] = lobjDesc;
 
    PG_RETURN_INT32(fd);
 }
@@ -122,9 +131,7 @@ be_lo_close(PG_FUNCTION_ARGS)
    elog(DEBUG4, "lo_close(%d)", fd);
 #endif
 
-   inv_close(cookies[fd]);
-
-   deleteLOfd(fd);
+   closeLOfd(fd);
 
    PG_RETURN_INT32(0);
 }
@@ -238,12 +245,7 @@ be_lo_creat(PG_FUNCTION_ARGS)
 {
    Oid         lobjId;
 
-   /*
-    * We don't actually need to store into fscxt, but create it anyway to
-    * ensure that AtEOXact_LargeObject knows there is state to clean up
-    */
-   CreateFSContext();
-
+   lo_cleanup_needed = true;
    lobjId = inv_create(InvalidOid);
 
    PG_RETURN_OID(lobjId);
@@ -254,12 +256,7 @@ be_lo_create(PG_FUNCTION_ARGS)
 {
    Oid         lobjId = PG_GETARG_OID(0);
 
-   /*
-    * We don't actually need to store into fscxt, but create it anyway to
-    * ensure that AtEOXact_LargeObject knows there is state to clean up
-    */
-   CreateFSContext();
-
+   lo_cleanup_needed = true;
    lobjId = inv_create(lobjId);
 
    PG_RETURN_OID(lobjId);
@@ -330,16 +327,13 @@ be_lo_unlink(PG_FUNCTION_ARGS)
        for (i = 0; i < cookies_size; i++)
        {
            if (cookies[i] != NULL && cookies[i]->id == lobjId)
-           {
-               inv_close(cookies[i]);
-               deleteLOfd(i);
-           }
+               closeLOfd(i);
        }
    }
 
    /*
     * inv_drop does not create a need for end-of-transaction cleanup and
-    * hence we don't need to have created fscxt.
+    * hence we don't need to set lo_cleanup_needed.
     */
    PG_RETURN_INT32(inv_drop(lobjId));
 }
@@ -419,8 +413,6 @@ lo_import_internal(text *filename, Oid lobjOid)
    LargeObjectDesc *lobj;
    Oid         oid;
 
-   CreateFSContext();
-
    /*
     * open the file to be read in
     */
@@ -435,12 +427,13 @@ lo_import_internal(text *filename, Oid lobjOid)
    /*
     * create an inversion object
     */
+   lo_cleanup_needed = true;
    oid = inv_create(lobjOid);
 
    /*
     * read in from the filesystem and write to the inversion object
     */
-   lobj = inv_open(oid, INV_WRITE, fscxt);
+   lobj = inv_open(oid, INV_WRITE, CurrentMemoryContext);
 
    while ((nbytes = read(fd, buf, BUFSIZE)) > 0)
    {
@@ -482,12 +475,11 @@ be_lo_export(PG_FUNCTION_ARGS)
    LargeObjectDesc *lobj;
    mode_t      oumask;
 
-   CreateFSContext();
-
    /*
     * open the inversion object (no need to test for failure)
     */
-   lobj = inv_open(lobjId, INV_READ, fscxt);
+   lo_cleanup_needed = true;
+   lobj = inv_open(lobjId, INV_READ, CurrentMemoryContext);
 
    /*
     * open the file to be written to
@@ -592,20 +584,22 @@ AtEOXact_LargeObject(bool isCommit)
 {
    int         i;
 
-   if (fscxt == NULL)
+   if (!lo_cleanup_needed)
        return;                 /* no LO operations in this xact */
 
    /*
     * Close LO fds and clear cookies array so that LO fds are no longer good.
-    * On abort we skip the close step.
+    * The memory context and resource owner holding them are going away at
+    * the end-of-transaction anyway, but on commit, we need to close them to
+    * avoid warnings about leaked resources at commit.  On abort we can skip
+    * this step.
     */
-   for (i = 0; i < cookies_size; i++)
+   if (isCommit)
    {
-       if (cookies[i] != NULL)
+       for (i = 0; i < cookies_size; i++)
        {
-           if (isCommit)
-               inv_close(cookies[i]);
-           deleteLOfd(i);
+           if (cookies[i] != NULL)
+               closeLOfd(i);
        }
    }
 
@@ -614,11 +608,14 @@ AtEOXact_LargeObject(bool isCommit)
    cookies_size = 0;
 
    /* Release the LO memory context to prevent permanent memory leaks. */
-   MemoryContextDelete(fscxt);
+   if (fscxt)
+       MemoryContextDelete(fscxt);
    fscxt = NULL;
 
    /* Give inv_api.c a chance to clean up, too */
    close_lo_relation(isCommit);
+
+   lo_cleanup_needed = false;
 }
 
 /*
@@ -646,14 +643,7 @@ AtEOSubXact_LargeObject(bool isCommit, SubTransactionId mySubid,
            if (isCommit)
                lo->subid = parentSubid;
            else
-           {
-               /*
-                * Make sure we do not call inv_close twice if it errors out
-                * for some reason.  Better a leak than a crash.
-                */
-               deleteLOfd(i);
-               inv_close(lo);
-           }
+               closeLOfd(i);
        }
    }
 }
@@ -663,19 +653,22 @@ AtEOSubXact_LargeObject(bool isCommit, SubTransactionId mySubid,
  *****************************************************************************/
 
 static int
-newLOfd(LargeObjectDesc *lobjCookie)
+newLOfd(void)
 {
    int         i,
                newsize;
 
+   lo_cleanup_needed = true;
+   if (fscxt == NULL)
+       fscxt = AllocSetContextCreate(TopMemoryContext,
+                                     "Filesystem",
+                                     ALLOCSET_DEFAULT_SIZES);
+
    /* Try to find a free slot */
    for (i = 0; i < cookies_size; i++)
    {
        if (cookies[i] == NULL)
-       {
-           cookies[i] = lobjCookie;
            return i;
-       }
    }
 
    /* No free slot, so make the array bigger */
@@ -700,15 +693,25 @@ newLOfd(LargeObjectDesc *lobjCookie)
        cookies_size = newsize;
    }
 
-   Assert(cookies[i] == NULL);
-   cookies[i] = lobjCookie;
    return i;
 }
 
 static void
-deleteLOfd(int fd)
+closeLOfd(int fd)
 {
+   LargeObjectDesc *lobj;
+
+   /*
+    * Make sure we do not try to free twice if this errors out for some
+    * reason.  Better a leak than a crash.
+    */
+   lobj = cookies[fd];
    cookies[fd] = NULL;
+
+   if (lobj->snapshot)
+       UnregisterSnapshotFromOwner(lobj->snapshot,
+                                   TopTransactionResourceOwner);
+   inv_close(lobj);
 }
 
 /*****************************************************************************
@@ -727,13 +730,8 @@ lo_get_fragment_internal(Oid loOid, int64 offset, int32 nbytes)
    int         total_read PG_USED_FOR_ASSERTS_ONLY;
    bytea      *result = NULL;
 
-   /*
-    * We don't actually need to store into fscxt, but create it anyway to
-    * ensure that AtEOXact_LargeObject knows there is state to clean up
-    */
-   CreateFSContext();
-
-   loDesc = inv_open(loOid, INV_READ, fscxt);
+   lo_cleanup_needed = true;
+   loDesc = inv_open(loOid, INV_READ, CurrentMemoryContext);
 
    /*
     * Compute number of bytes we'll actually read, accommodating nbytes == -1
@@ -817,10 +815,9 @@ be_lo_from_bytea(PG_FUNCTION_ARGS)
    LargeObjectDesc *loDesc;
    int         written PG_USED_FOR_ASSERTS_ONLY;
 
-   CreateFSContext();
-
+   lo_cleanup_needed = true;
    loOid = inv_create(loOid);
-   loDesc = inv_open(loOid, INV_WRITE, fscxt);
+   loDesc = inv_open(loOid, INV_WRITE, CurrentMemoryContext);
    written = inv_write(loDesc, VARDATA_ANY(str), VARSIZE_ANY_EXHDR(str));
    Assert(written == VARSIZE_ANY_EXHDR(str));
    inv_close(loDesc);
@@ -840,9 +837,8 @@ be_lo_put(PG_FUNCTION_ARGS)
    LargeObjectDesc *loDesc;
    int         written PG_USED_FOR_ASSERTS_ONLY;
 
-   CreateFSContext();
-
-   loDesc = inv_open(loOid, INV_WRITE, fscxt);
+   lo_cleanup_needed = true;
+   loDesc = inv_open(loOid, INV_WRITE, CurrentMemoryContext);
 
    /* Permission check */
    if (!lo_compat_privileges &&

diff --git a/src/backend/storage/large_object/inv_api.c b/src/backend/storage/large_object/inv_api.c
index 20130e47b76c18eb4b0636e020b033b228f5aae2..7917977ac7287da708d6efa318d7c22cd81aca35 100644 (file)
--- a/src/backend/storage/large_object/inv_api.c
+++ b/src/backend/storage/large_object/inv_api.c
@@ -244,10 +244,12 @@ inv_create(Oid lobjId)
 /*
  * inv_open -- access an existing large object.
  *
- *     Returns:
- *       Large object descriptor, appropriately filled in.  The descriptor
- *       and subsidiary data are allocated in the specified memory context,
- *       which must be suitably long-lived for the caller's purposes.
+ * Returns a large object descriptor, appropriately filled in.
+ * The descriptor and subsidiary data are allocated in the specified
+ * memory context, which must be suitably long-lived for the caller's
+ * purposes.  If the returned descriptor has a snapshot associated
+ * with it, the caller must ensure that it also lives long enough,
+ * e.g. by calling RegisterSnapshotOnOwner
  */
 LargeObjectDesc *
 inv_open(Oid lobjId, int flags, MemoryContext mcxt)
@@ -314,19 +316,16 @@ inv_open(Oid lobjId, int flags, MemoryContext mcxt)
    retval = (LargeObjectDesc *) MemoryContextAlloc(mcxt,
                                                    sizeof(LargeObjectDesc));
    retval->id = lobjId;
-   retval->subid = GetCurrentSubTransactionId();
    retval->offset = 0;
    retval->flags = descflags;
 
+   /* caller sets if needed, not used by the functions in this file */
+   retval->subid = InvalidSubTransactionId;
+
    /*
-    * We must register the snapshot in TopTransaction's resowner, because it
-    * must stay alive until the LO is closed rather than until the current
-    * portal shuts down.  Do this last to avoid uselessly leaking the
-    * snapshot if an error is thrown above.
+    * The snapshot (if any) is just the currently active snapshot.  The
+    * caller will replace it with a longer-lived copy if needed.
     */
-   if (snapshot)
-       snapshot = RegisterSnapshotOnOwner(snapshot,
-                                          TopTransactionResourceOwner);
    retval->snapshot = snapshot;
 
    return retval;
@@ -340,10 +339,6 @@ void
 inv_close(LargeObjectDesc *obj_desc)
 {
    Assert(PointerIsValid(obj_desc));
-
-   UnregisterSnapshotFromOwner(obj_desc->snapshot,
-                               TopTransactionResourceOwner);
-
    pfree(obj_desc);
 }
 

diff --git a/src/test/regress/input/largeobject.source b/src/test/regress/input/largeobject.source
index ff42697d11238801f54ff05bcf69d1a0da8db0e9..b1e7ae99096e5b3cca47ae0631eeea0ea9365ef7 100644 (file)
--- a/src/test/regress/input/largeobject.source
+++ b/src/test/regress/input/largeobject.source
@@ -124,6 +124,17 @@ BEGIN;
 SELECT lo_open(loid, x'40000'::int) from lotest_stash_values;
 ABORT;
 
+DO $$
+DECLARE
+  loid oid;
+BEGIN
+  SELECT tbl.loid INTO loid FROM lotest_stash_values tbl;
+  PERFORM lo_export(loid, '@abs_builddir@/results/invalid/path');
+EXCEPTION
+  WHEN UNDEFINED_FILE THEN RAISE NOTICE 'could not open file, as expected';
+END;
+$$;
+
 -- Test truncation.
 BEGIN;
 UPDATE lotest_stash_values SET fd=lo_open(loid, CAST(x'20000' | x'40000' AS integer));

diff --git a/src/test/regress/output/largeobject.source b/src/test/regress/output/largeobject.source
index 91090f0fde4fbefaf77939259d2a6e396948c952..91d33b4d0c7afce7222ee1af2f2e4f2fa054f6a2 100644 (file)
--- a/src/test/regress/output/largeobject.source
+++ b/src/test/regress/output/largeobject.source
@@ -161,6 +161,17 @@ SELECT lo_open(loid, x'40000'::int) from lotest_stash_values;
 (1 row)
 
 ABORT;
+DO $$
+DECLARE
+  loid oid;
+BEGIN
+  SELECT tbl.loid INTO loid FROM lotest_stash_values tbl;
+  PERFORM lo_export(loid, '@abs_builddir@/results/invalid/path');
+EXCEPTION
+  WHEN UNDEFINED_FILE THEN RAISE NOTICE 'could not open file, as expected';
+END;
+$$;
+NOTICE:  could not open file, as expected
 -- Test truncation.
 BEGIN;
 UPDATE lotest_stash_values SET fd=lo_open(loid, CAST(x'20000' | x'40000' AS integer));