open-dsi
/
postgres
mirror of https://github.com/postgres/postgres
1
0
Fork 0
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
pull/72/head
Heikki Linnakangas 4 years ago
parent ada667b454
commit 07070c0082
4 changed files with 104 additions and 91 deletions
Show all changes Ignore whitespace when comparing lines Ignore changes in amount of whitespace Ignore changes in whitespace at EOL
Show Stats Download Patch File Download Diff File
  1. 146
      src/backend/libpq/be-fsstubs.c
  2. 27
      src/backend/storage/large_object/inv_api.c
  3. 11
      src/test/regress/input/largeobject.source
  4. 11
      src/test/regress/output/largeobject.source

146
src/backend/libpq/be-fsstubs.c
Unescape 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();
/*
* 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); 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.
*/ */
for (i = 0; i < cookies_size; i++) if (isCommit)
{ {
if (cookies[i] != NULL) for (i = 0; i < cookies_size; i++)
{ {
if (isCommit) if (cookies[i] != NULL)
inv_close(cookies[i]); closeLOfd(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. */
MemoryContextDelete(fscxt); if (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,19 +653,22 @@ 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 */
@ -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
Unescape 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
Unescape 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
Unescape 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));

Write Preview
Loading…
Cancel
Save