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
This commit is contained in:
Heikki Linnakangas
parent
99197701e5
commit
8f32afee01
|
|
@ -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 &&
|
||||
|
|
|
|||
|
|
@ -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);
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -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));
|
||||
|
|
|
|||
|
|
@ -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));
|
||||
|
|
|
|||
Loading…
Reference in New Issue