]> git.ipfire.org Git - thirdparty/postgresql.git/commitdiff
Fix snapshot reference leak if lo_export fails.
authorHeikki Linnakangas <heikki.linnakangas@iki.fi>
Wed, 3 Nov 2021 09:09:08 +0000 (11:09 +0200)
committerHeikki Linnakangas <heikki.linnakangas@iki.fi>
Wed, 3 Nov 2021 09:15:02 +0000 (11:15 +0200)
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

src/backend/libpq/be-fsstubs.c
src/backend/storage/large_object/inv_api.c
src/test/regress/input/largeobject.source
src/test/regress/output/largeobject.source

index 0112428e7d0c42e00b7eae2b4335fdc618f8628c..0c49aa7fe6f84b202a9533aa5b83d6b6f13b8955 100644 (file)
@@ -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"
 
 /*
  * compatibility flag for permission checks
@@ -72,19 +74,11 @@ bool                lo_compat_privileges;
 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);
 
 
@@ -104,10 +98,13 @@ 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);
-
        if (lobjDesc == NULL)
        {                                                       /* lookup failed */
 #if FSDB
@@ -115,8 +112,19 @@ lo_open(PG_FUNCTION_ARGS)
 #endif
                PG_RETURN_INT32(-1);
        }
+       lobjDesc->subid = GetCurrentSubTransactionId();
 
-       fd = newLOfd(lobjDesc);
+       /*
+        * 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);
+
+       Assert(cookies[fd] == NULL);
+       cookies[fd] = lobjDesc;
 
        PG_RETURN_INT32(fd);
 }
@@ -135,9 +143,7 @@ lo_close(PG_FUNCTION_ARGS)
        elog(DEBUG4, "lo_close(%d)", fd);
 #endif
 
-       inv_close(cookies[fd]);
-
-       deleteLOfd(fd);
+       closeLOfd(fd);
 
        PG_RETURN_INT32(0);
 }
@@ -271,12 +277,7 @@ 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);
@@ -287,12 +288,7 @@ 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);
@@ -359,16 +355,13 @@ 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));
 }
@@ -456,8 +449,6 @@ lo_import_internal(text *filename, Oid lobjOid)
                                 errhint("Anyone can use the client-side lo_import() provided by libpq.")));
 #endif
 
-       CreateFSContext();
-
        /*
         * open the file to be read in
         */
@@ -472,12 +463,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)
        {
@@ -522,12 +514,11 @@ lo_export(PG_FUNCTION_ARGS)
                                 errhint("Anyone can use the client-side lo_export() provided by libpq.")));
 #endif
 
-       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
@@ -643,20 +634,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);
                }
        }
 
@@ -665,11 +658,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;
 }
 
 /*
@@ -697,14 +693,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);
                }
        }
 }
@@ -714,19 +703,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 */
@@ -751,15 +743,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);
 }
 
 /*****************************************************************************
@@ -778,13 +780,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);
 
        /* Permission check */
        if (!lo_compat_privileges &&
@@ -879,10 +876,9 @@ 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);
@@ -902,9 +898,8 @@ 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 &&
index ec6738c8b8aa7758ce7d7f2aab7ae797b30b1d71..40f3341c3bacd47474266cca1e214e3890637731 100644 (file)
@@ -256,10 +256,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)
@@ -290,22 +292,20 @@ inv_open(Oid lobjId, int flags, MemoryContext mcxt)
                                (errcode(ERRCODE_UNDEFINED_OBJECT),
                                 errmsg("large object %u does not exist", lobjId)));
 
-       /*
-        * 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 after checking that the LO exists, to avoid
-        * leaking the snapshot if an error is thrown.
-        */
-       if (snapshot)
-               snapshot = RegisterSnapshotOnOwner(snapshot,
-                                                                                  TopTransactionResourceOwner);
-
-       /* All set, create a descriptor */
+       /* OK to create a descriptor */
        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;
+
+       /*
+        * The snapshot (if any) is just the currently active snapshot.  The
+        * caller will replace it with a longer-lived copy if needed.
+        */
        retval->snapshot = snapshot;
        retval->flags = descflags;
 
@@ -320,10 +320,6 @@ void
 inv_close(LargeObjectDesc *obj_desc)
 {
        Assert(PointerIsValid(obj_desc));
-
-       UnregisterSnapshotFromOwner(obj_desc->snapshot,
-                                                               TopTransactionResourceOwner);
-
        pfree(obj_desc);
 }
 
index 974f36b21f307ed8ccedb7e4fb27f69594dbb0db..aa03a51ddf61130d861f322845fe1f317cbb9a67 100644 (file)
@@ -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));
index 1b7b3daee2df03f2fcb116fbcaa6cad8fe22e5d1..94470625eec28a64402247a530ee29a2a38e1d75 100644 (file)
@@ -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));