mirror/postgresql
2
0
Fork 0
mirror of https://git.postgresql.org/git/postgresql.git synced 2024-12-27 08:39:28 +08:00
Code Issues Packages Projects Releases Wiki Activity

Fix snapshot reference leak if lo_export fails.

Browse Source 
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
This commit is contained in:
Heikki Linnakangas 2021-11-03 10:28:52 +02:00
parent ef6f047d2c
commit 6b1b405ebf
4 changed files with 104 additions and 91 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

142
src/backend/libpq/be-fsstubs.c
View File

@ -42,6 +42,7 @@
#include <sys/stat.h> #include <sys/stat.h>
#include <unistd.h> #include <unistd.h>
#include "access/xact.h"
#include "libpq/be-fsstubs.h" #include "libpq/be-fsstubs.h"
#include "libpq/libpq-fs.h" #include "libpq/libpq-fs.h"
#include "miscadmin.h" #include "miscadmin.h"
@ -50,6 +51,7 @@
#include "utils/acl.h" #include "utils/acl.h"
#include "utils/builtins.h" #include "utils/builtins.h"
#include "utils/memutils.h" #include "utils/memutils.h"
#include "utils/snapmgr.h"
/* define this to enable debug logging */ /* define this to enable debug logging */
/* #define FSDB 1 */ /* #define FSDB 1 */
@ -67,19 +69,11 @@
static LargeObjectDesc **cookies = NULL; static LargeObjectDesc **cookies = NULL;
static int cookies_size = 0; static int cookies_size = 0;
static bool lo_cleanup_needed = false;
static MemoryContext fscxt = NULL; static MemoryContext fscxt = NULL;
#define CreateFSContext() \ static int newLOfd(void);
do { \ static void closeLOfd(int fd);
if (fscxt == NULL) \
fscxt = AllocSetContextCreate(TopMemoryContext, \
"Filesystem", \
ALLOCSET_DEFAULT_SIZES); \
} while (0)
static int newLOfd(LargeObjectDesc *lobjCookie);
static void deleteLOfd(int fd);
static Oid lo_import_internal(text *filename, Oid lobjOid); 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); elog(DEBUG4, "lo_open(%u,%d)", lobjId, mode);
#endif #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 = inv_open(lobjId, mode, fscxt);
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); PG_RETURN_INT32(fd);
} }
@ -122,9 +131,7 @@ be_lo_close(PG_FUNCTION_ARGS)
elog(DEBUG4, "lo_close(%d)", fd); elog(DEBUG4, "lo_close(%d)", fd);
#endif #endif
inv_close(cookies[fd]); closeLOfd(fd);
deleteLOfd(fd);
PG_RETURN_INT32(0); PG_RETURN_INT32(0);
} }
@ -238,12 +245,7 @@ be_lo_creat(PG_FUNCTION_ARGS)
{ {
Oid lobjId; Oid lobjId;
/* lo_cleanup_needed = true;
* 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();
lobjId = inv_create(InvalidOid); lobjId = inv_create(InvalidOid);
PG_RETURN_OID(lobjId); PG_RETURN_OID(lobjId);
@ -254,12 +256,7 @@ be_lo_create(PG_FUNCTION_ARGS)
{ {
Oid lobjId = PG_GETARG_OID(0); Oid lobjId = PG_GETARG_OID(0);
/* lo_cleanup_needed = true;
* 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();
lobjId = inv_create(lobjId); lobjId = inv_create(lobjId);
PG_RETURN_OID(lobjId); PG_RETURN_OID(lobjId);
@ -330,16 +327,13 @@ be_lo_unlink(PG_FUNCTION_ARGS)
for (i = 0; i < cookies_size; i++) for (i = 0; i < cookies_size; i++)
{ {
if (cookies[i] != NULL && cookies[i]->id == lobjId) if (cookies[i] != NULL && cookies[i]->id == lobjId)
{ closeLOfd(i);
inv_close(cookies[i]);
deleteLOfd(i);
}
} }
} }
/* /*
* inv_drop does not create a need for end-of-transaction cleanup and * 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)); PG_RETURN_INT32(inv_drop(lobjId));
} }
@ -419,8 +413,6 @@ lo_import_internal(text *filename, Oid lobjOid)
LargeObjectDesc *lobj; LargeObjectDesc *lobj;
Oid oid; Oid oid;
CreateFSContext();
/* /*
* open the file to be read in * open the file to be read in
*/ */
@ -435,12 +427,13 @@ lo_import_internal(text *filename, Oid lobjOid)
/* /*
* create an inversion object * create an inversion object
*/ */
lo_cleanup_needed = true;
oid = inv_create(lobjOid); oid = inv_create(lobjOid);
/* /*
* read in from the filesystem and write to the inversion object * 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) while ((nbytes = read(fd, buf, BUFSIZE)) > 0)
{ {
@ -482,12 +475,11 @@ be_lo_export(PG_FUNCTION_ARGS)
LargeObjectDesc *lobj; LargeObjectDesc *lobj;
mode_t oumask; mode_t oumask;
CreateFSContext();
/* /*
* open the inversion object (no need to test for failure) * 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 * open the file to be written to
@ -592,20 +584,22 @@ AtEOXact_LargeObject(bool isCommit)
{ {
int i; int i;
if (fscxt == NULL) if (!lo_cleanup_needed)
return; /* no LO operations in this xact */ return; /* no LO operations in this xact */
/* /*
* Close LO fds and clear cookies array so that LO fds are no longer good. * 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.
*/ */
if (isCommit)
{
for (i = 0; i < cookies_size; i++) for (i = 0; i < cookies_size; i++)
{ {
if (cookies[i] != NULL) if (cookies[i] != NULL)
{ closeLOfd(i);
if (isCommit)
inv_close(cookies[i]);
deleteLOfd(i);
} }
} }
@ -614,11 +608,14 @@ AtEOXact_LargeObject(bool isCommit)
cookies_size = 0; cookies_size = 0;
/* Release the LO memory context to prevent permanent memory leaks. */ /* Release the LO memory context to prevent permanent memory leaks. */
if (fscxt)
MemoryContextDelete(fscxt); MemoryContextDelete(fscxt);
fscxt = NULL; fscxt = NULL;
/* Give inv_api.c a chance to clean up, too */ /* Give inv_api.c a chance to clean up, too */
close_lo_relation(isCommit); close_lo_relation(isCommit);
lo_cleanup_needed = false;
} }
/* /*
@ -646,14 +643,7 @@ AtEOSubXact_LargeObject(bool isCommit, SubTransactionId mySubid,
if (isCommit) if (isCommit)
lo->subid = parentSubid; lo->subid = parentSubid;
else else
{ closeLOfd(i);
/*
* 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);
}
} }
} }
} }
@ -663,20 +653,23 @@ AtEOSubXact_LargeObject(bool isCommit, SubTransactionId mySubid,
*****************************************************************************/ *****************************************************************************/
static int static int
newLOfd(LargeObjectDesc *lobjCookie) newLOfd(void)
{ {
int i, int i,
newsize; newsize;
lo_cleanup_needed = true;
if (fscxt == NULL)
fscxt = AllocSetContextCreate(TopMemoryContext,
"Filesystem",
ALLOCSET_DEFAULT_SIZES);
/* Try to find a free slot */ /* Try to find a free slot */
for (i = 0; i < cookies_size; i++) for (i = 0; i < cookies_size; i++)
{ {
if (cookies[i] == NULL) if (cookies[i] == NULL)
{
cookies[i] = lobjCookie;
return i; return i;
} }
}
/* No free slot, so make the array bigger */ /* No free slot, so make the array bigger */
if (cookies_size <= 0) if (cookies_size <= 0)
@ -700,15 +693,25 @@ newLOfd(LargeObjectDesc *lobjCookie)
cookies_size = newsize; cookies_size = newsize;
} }
Assert(cookies[i] == NULL);
cookies[i] = lobjCookie;
return i; return i;
} }
static void 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; 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; int total_read PG_USED_FOR_ASSERTS_ONLY;
bytea *result = NULL; bytea *result = NULL;
/* lo_cleanup_needed = true;
* We don't actually need to store into fscxt, but create it anyway to loDesc = inv_open(loOid, INV_READ, CurrentMemoryContext);
* ensure that AtEOXact_LargeObject knows there is state to clean up
*/
CreateFSContext();
loDesc = inv_open(loOid, INV_READ, fscxt);
/* /*
* Compute number of bytes we'll actually read, accommodating nbytes == -1 * Compute number of bytes we'll actually read, accommodating nbytes == -1
@ -817,10 +815,9 @@ be_lo_from_bytea(PG_FUNCTION_ARGS)
LargeObjectDesc *loDesc; LargeObjectDesc *loDesc;
int written PG_USED_FOR_ASSERTS_ONLY; int written PG_USED_FOR_ASSERTS_ONLY;
CreateFSContext(); lo_cleanup_needed = true;
loOid = inv_create(loOid); 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)); written = inv_write(loDesc, VARDATA_ANY(str), VARSIZE_ANY_EXHDR(str));
Assert(written == VARSIZE_ANY_EXHDR(str)); Assert(written == VARSIZE_ANY_EXHDR(str));
inv_close(loDesc); inv_close(loDesc);
@ -840,9 +837,8 @@ be_lo_put(PG_FUNCTION_ARGS)
LargeObjectDesc *loDesc; LargeObjectDesc *loDesc;
int written PG_USED_FOR_ASSERTS_ONLY; int written PG_USED_FOR_ASSERTS_ONLY;
CreateFSContext(); lo_cleanup_needed = true;
loDesc = inv_open(loOid, INV_WRITE, CurrentMemoryContext);
loDesc = inv_open(loOid, INV_WRITE, fscxt);
/* Permission check */ /* Permission check */
if (!lo_compat_privileges && if (!lo_compat_privileges &&

27
src/backend/storage/large_object/inv_api.c
View File

@ -244,10 +244,12 @@ inv_create(Oid lobjId)
/* /*
* inv_open -- access an existing large object. * inv_open -- access an existing large object.
* *
* Returns: * Returns a large object descriptor, appropriately filled in.
* Large object descriptor, appropriately filled in. The descriptor * The descriptor and subsidiary data are allocated in the specified
* and subsidiary data are allocated in the specified memory context, * memory context, which must be suitably long-lived for the caller's
* which must be suitably long-lived for the caller's purposes. * 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 * LargeObjectDesc *
inv_open(Oid lobjId, int flags, MemoryContext mcxt) inv_open(Oid lobjId, int flags, MemoryContext mcxt)
@ -314,19 +316,16 @@ inv_open(Oid lobjId, int flags, MemoryContext mcxt)
retval = (LargeObjectDesc *) MemoryContextAlloc(mcxt, retval = (LargeObjectDesc *) MemoryContextAlloc(mcxt,
sizeof(LargeObjectDesc)); sizeof(LargeObjectDesc));
retval->id = lobjId; retval->id = lobjId;
retval->subid = GetCurrentSubTransactionId();
retval->offset = 0; retval->offset = 0;
retval->flags = descflags; 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 * The snapshot (if any) is just the currently active snapshot. The
* must stay alive until the LO is closed rather than until the current * caller will replace it with a longer-lived copy if needed.
* portal shuts down. Do this last to avoid uselessly leaking the
* snapshot if an error is thrown above.
*/ */
if (snapshot)
snapshot = RegisterSnapshotOnOwner(snapshot,
TopTransactionResourceOwner);
retval->snapshot = snapshot; retval->snapshot = snapshot;
return retval; return retval;
@ -340,10 +339,6 @@ void
inv_close(LargeObjectDesc *obj_desc) inv_close(LargeObjectDesc *obj_desc)
{ {
Assert(PointerIsValid(obj_desc)); Assert(PointerIsValid(obj_desc));
UnregisterSnapshotFromOwner(obj_desc->snapshot,
TopTransactionResourceOwner);
pfree(obj_desc); pfree(obj_desc);
} }

11
src/test/regress/input/largeobject.source
View File

@ -124,6 +124,17 @@ BEGIN;
SELECT lo_open(loid, x'40000'::int) from lotest_stash_values; SELECT lo_open(loid, x'40000'::int) from lotest_stash_values;
ABORT; 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. -- Test truncation.
BEGIN; BEGIN;
UPDATE lotest_stash_values SET fd=lo_open(loid, CAST(x'20000' | x'40000' AS integer)); UPDATE lotest_stash_values SET fd=lo_open(loid, CAST(x'20000' | x'40000' AS integer));

11
src/test/regress/output/largeobject.source
View File

@ -161,6 +161,17 @@ SELECT lo_open(loid, x'40000'::int) from lotest_stash_values;
(1 row) (1 row)
ABORT; 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. -- Test truncation.
BEGIN; BEGIN;
UPDATE lotest_stash_values SET fd=lo_open(loid, CAST(x'20000' | x'40000' AS integer)); UPDATE lotest_stash_values SET fd=lo_open(loid, CAST(x'20000' | x'40000' AS integer));