Skip to content

Commit 8f32afe

Browse files
hlinnakahlinnaka
committed
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
1 parent 9919770 commit 8f32afe

File tree

4 files changed

+109
-96
lines changed

4 files changed

+109
-96
lines changed

‎src/backend/libpq/be-fsstubs.c

Lines changed: 71 additions & 76 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@
4242
#include <sys/stat.h>
4343
#include <unistd.h>
4444

45+
#include "access/xact.h"
4546
#include "libpq/be-fsstubs.h"
4647
#include "libpq/libpq-fs.h"
4748
#include "miscadmin.h"
@@ -50,6 +51,7 @@
5051
#include "utils/acl.h"
5152
#include "utils/builtins.h"
5253
#include "utils/memutils.h"
54+
#include "utils/snapmgr.h"
5355

5456
/*
5557
* compatibility flag for permission checks
@@ -72,19 +74,11 @@ bool lo_compat_privileges;
7274
static LargeObjectDesc **cookies = NULL;
7375
static int cookies_size = 0;
7476

77+
static bool lo_cleanup_needed = false;
7578
static MemoryContext fscxt = NULL;
7679

77-
#define CreateFSContext() \
78-
do { \
79-
if (fscxt == NULL) \
80-
fscxt = AllocSetContextCreate(TopMemoryContext, \
81-
"Filesystem", \
82-
ALLOCSET_DEFAULT_SIZES); \
83-
} while (0)
84-
85-
86-
static int newLOfd(LargeObjectDesc *lobjCookie);
87-
static void deleteLOfd(int fd);
80+
static int newLOfd(void);
81+
static void closeLOfd(int fd);
8882
static Oid lo_import_internal(text *filename, Oid lobjOid);
8983

9084

@@ -104,19 +98,33 @@ lo_open(PG_FUNCTION_ARGS)
10498
elog(DEBUG4, "lo_open(%u,%d)", lobjId, mode);
10599
#endif
106100

107-
CreateFSContext();
101+
/*
102+
* Allocate a large object descriptor first. This will also create
103+
* 'fscxt' if this is the first LO opened in this transaction.
104+
*/
105+
fd = newLOfd();
108106

109107
lobjDesc = inv_open(lobjId, mode, fscxt);
110-
111108
if (lobjDesc == NULL)
112109
{ /* lookup failed */
113110
#if FSDB
114111
elog(DEBUG4, "could not open large object %u", lobjId);
115112
#endif
116113
PG_RETURN_INT32(-1);
117114
}
115+
lobjDesc->subid = GetCurrentSubTransactionId();
118116

119-
fd = newLOfd(lobjDesc);
117+
/*
118+
* We must register the snapshot in TopTransaction's resowner so that it
119+
* stays alive until the LO is closed rather than until the current portal
120+
* shuts down.
121+
*/
122+
if (lobjDesc->snapshot)
123+
lobjDesc->snapshot = RegisterSnapshotOnOwner(lobjDesc->snapshot,
124+
TopTransactionResourceOwner);
125+
126+
Assert(cookies[fd] == NULL);
127+
cookies[fd] = lobjDesc;
120128

121129
PG_RETURN_INT32(fd);
122130
}
@@ -135,9 +143,7 @@ lo_close(PG_FUNCTION_ARGS)
135143
elog(DEBUG4, "lo_close(%d)", fd);
136144
#endif
137145

138-
inv_close(cookies[fd]);
139-
140-
deleteLOfd(fd);
146+
closeLOfd(fd);
141147

142148
PG_RETURN_INT32(0);
143149
}
@@ -271,12 +277,7 @@ lo_creat(PG_FUNCTION_ARGS)
271277
{
272278
Oid lobjId;
273279

274-
/*
275-
* We don't actually need to store into fscxt, but create it anyway to
276-
* ensure that AtEOXact_LargeObject knows there is state to clean up
277-
*/
278-
CreateFSContext();
279-
280+
lo_cleanup_needed = true;
280281
lobjId = inv_create(InvalidOid);
281282

282283
PG_RETURN_OID(lobjId);
@@ -287,12 +288,7 @@ lo_create(PG_FUNCTION_ARGS)
287288
{
288289
Oid lobjId = PG_GETARG_OID(0);
289290

290-
/*
291-
* We don't actually need to store into fscxt, but create it anyway to
292-
* ensure that AtEOXact_LargeObject knows there is state to clean up
293-
*/
294-
CreateFSContext();
295-
291+
lo_cleanup_needed = true;
296292
lobjId = inv_create(lobjId);
297293

298294
PG_RETURN_OID(lobjId);
@@ -359,16 +355,13 @@ lo_unlink(PG_FUNCTION_ARGS)
359355
for (i = 0; i < cookies_size; i++)
360356
{
361357
if (cookies[i] != NULL && cookies[i]->id == lobjId)
362-
{
363-
inv_close(cookies[i]);
364-
deleteLOfd(i);
365-
}
358+
closeLOfd(i);
366359
}
367360
}
368361

369362
/*
370363
* inv_drop does not create a need for end-of-transaction cleanup and
371-
* hence we don't need to have created fscxt.
364+
* hence we don't need to set lo_cleanup_needed.
372365
*/
373366
PG_RETURN_INT32(inv_drop(lobjId));
374367
}
@@ -456,8 +449,6 @@ lo_import_internal(text *filename, Oid lobjOid)
456449
errhint("Anyone can use the client-side lo_import() provided by libpq.")));
457450
#endif
458451

459-
CreateFSContext();
460-
461452
/*
462453
* open the file to be read in
463454
*/
@@ -472,12 +463,13 @@ lo_import_internal(text *filename, Oid lobjOid)
472463
/*
473464
* create an inversion object
474465
*/
466+
lo_cleanup_needed = true;
475467
oid = inv_create(lobjOid);
476468

477469
/*
478470
* read in from the filesystem and write to the inversion object
479471
*/
480-
lobj = inv_open(oid, INV_WRITE, fscxt);
472+
lobj = inv_open(oid, INV_WRITE, CurrentMemoryContext);
481473

482474
while ((nbytes = read(fd, buf, BUFSIZE)) > 0)
483475
{
@@ -522,12 +514,11 @@ lo_export(PG_FUNCTION_ARGS)
522514
errhint("Anyone can use the client-side lo_export() provided by libpq.")));
523515
#endif
524516

525-
CreateFSContext();
526-
527517
/*
528518
* open the inversion object (no need to test for failure)
529519
*/
530-
lobj = inv_open(lobjId, INV_READ, fscxt);
520+
lo_cleanup_needed = true;
521+
lobj = inv_open(lobjId, INV_READ, CurrentMemoryContext);
531522

532523
/*
533524
* open the file to be written to
@@ -643,20 +634,22 @@ AtEOXact_LargeObject(bool isCommit)
643634
{
644635
int i;
645636

646-
if (fscxt == NULL)
637+
if (!lo_cleanup_needed)
647638
return; /* no LO operations in this xact */
648639

649640
/*
650641
* Close LO fds and clear cookies array so that LO fds are no longer good.
651-
* On abort we skip the close step.
642+
* The memory context and resource owner holding them are going away at
643+
* the end-of-transaction anyway, but on commit, we need to close them to
644+
* avoid warnings about leaked resources at commit. On abort we can skip
645+
* this step.
652646
*/
653-
for (i = 0; i < cookies_size; i++)
647+
if (isCommit)
654648
{
655-
if (cookies[i] != NULL)
649+
for (i = 0; i < cookies_size; i++)
656650
{
657-
if (isCommit)
658-
inv_close(cookies[i]);
659-
deleteLOfd(i);
651+
if (cookies[i] != NULL)
652+
closeLOfd(i);
660653
}
661654
}
662655

@@ -665,11 +658,14 @@ AtEOXact_LargeObject(bool isCommit)
665658
cookies_size = 0;
666659

667660
/* Release the LO memory context to prevent permanent memory leaks. */
668-
MemoryContextDelete(fscxt);
661+
if (fscxt)
662+
MemoryContextDelete(fscxt);
669663
fscxt = NULL;
670664

671665
/* Give inv_api.c a chance to clean up, too */
672666
close_lo_relation(isCommit);
667+
668+
lo_cleanup_needed = false;
673669
}
674670

675671
/*
@@ -697,14 +693,7 @@ AtEOSubXact_LargeObject(bool isCommit, SubTransactionId mySubid,
697693
if (isCommit)
698694
lo->subid = parentSubid;
699695
else
700-
{
701-
/*
702-
* Make sure we do not call inv_close twice if it errors out
703-
* for some reason. Better a leak than a crash.
704-
*/
705-
deleteLOfd(i);
706-
inv_close(lo);
707-
}
696+
closeLOfd(i);
708697
}
709698
}
710699
}
@@ -714,19 +703,22 @@ AtEOSubXact_LargeObject(bool isCommit, SubTransactionId mySubid,
714703
*****************************************************************************/
715704

716705
static int
717-
newLOfd(LargeObjectDesc *lobjCookie)
706+
newLOfd(void)
718707
{
719708
int i,
720709
newsize;
721710

711+
lo_cleanup_needed = true;
712+
if (fscxt == NULL)
713+
fscxt = AllocSetContextCreate(TopMemoryContext,
714+
"Filesystem",
715+
ALLOCSET_DEFAULT_SIZES);
716+
722717
/* Try to find a free slot */
723718
for (i = 0; i < cookies_size; i++)
724719
{
725720
if (cookies[i] == NULL)
726-
{
727-
cookies[i] = lobjCookie;
728721
return i;
729-
}
730722
}
731723

732724
/* No free slot, so make the array bigger */
@@ -751,15 +743,25 @@ newLOfd(LargeObjectDesc *lobjCookie)
751743
cookies_size = newsize;
752744
}
753745

754-
Assert(cookies[i] == NULL);
755-
cookies[i] = lobjCookie;
756746
return i;
757747
}
758748

759749
static void
760-
deleteLOfd(int fd)
750+
closeLOfd(int fd)
761751
{
752+
LargeObjectDesc *lobj;
753+
754+
/*
755+
* Make sure we do not try to free twice if this errors out for some
756+
* reason. Better a leak than a crash.
757+
*/
758+
lobj = cookies[fd];
762759
cookies[fd] = NULL;
760+
761+
if (lobj->snapshot)
762+
UnregisterSnapshotFromOwner(lobj->snapshot,
763+
TopTransactionResourceOwner);
764+
inv_close(lobj);
763765
}
764766

765767
/*****************************************************************************
@@ -778,13 +780,8 @@ lo_get_fragment_internal(Oid loOid, int64 offset, int32 nbytes)
778780
int total_read PG_USED_FOR_ASSERTS_ONLY;
779781
bytea *result = NULL;
780782

781-
/*
782-
* We don't actually need to store into fscxt, but create it anyway to
783-
* ensure that AtEOXact_LargeObject knows there is state to clean up
784-
*/
785-
CreateFSContext();
786-
787-
loDesc = inv_open(loOid, INV_READ, fscxt);
783+
lo_cleanup_needed = true;
784+
loDesc = inv_open(loOid, INV_READ, CurrentMemoryContext);
788785

789786
/* Permission check */
790787
if (!lo_compat_privileges &&
@@ -879,10 +876,9 @@ lo_from_bytea(PG_FUNCTION_ARGS)
879876
LargeObjectDesc *loDesc;
880877
int written PG_USED_FOR_ASSERTS_ONLY;
881878

882-
CreateFSContext();
883-
879+
lo_cleanup_needed = true;
884880
loOid = inv_create(loOid);
885-
loDesc = inv_open(loOid, INV_WRITE, fscxt);
881+
loDesc = inv_open(loOid, INV_WRITE, CurrentMemoryContext);
886882
written = inv_write(loDesc, VARDATA_ANY(str), VARSIZE_ANY_EXHDR(str));
887883
Assert(written == VARSIZE_ANY_EXHDR(str));
888884
inv_close(loDesc);
@@ -902,9 +898,8 @@ lo_put(PG_FUNCTION_ARGS)
902898
LargeObjectDesc *loDesc;
903899
int written PG_USED_FOR_ASSERTS_ONLY;
904900

905-
CreateFSContext();
906-
907-
loDesc = inv_open(loOid, INV_WRITE, fscxt);
901+
lo_cleanup_needed = true;
902+
loDesc = inv_open(loOid, INV_WRITE, CurrentMemoryContext);
908903

909904
/* Permission check */
910905
if (!lo_compat_privileges &&

‎src/backend/storage/large_object/inv_api.c

Lines changed: 16 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -256,10 +256,12 @@ inv_create(Oid lobjId)
256256
/*
257257
* inv_open -- access an existing large object.
258258
*
259-
* Returns:
260-
* Large object descriptor, appropriately filled in. The descriptor
261-
* and subsidiary data are allocated in the specified memory context,
262-
* which must be suitably long-lived for the caller's purposes.
259+
* Returns a large object descriptor, appropriately filled in.
260+
* The descriptor and subsidiary data are allocated in the specified
261+
* memory context, which must be suitably long-lived for the caller's
262+
* purposes. If the returned descriptor has a snapshot associated
263+
* with it, the caller must ensure that it also lives long enough,
264+
* e.g. by calling RegisterSnapshotOnOwner
263265
*/
264266
LargeObjectDesc *
265267
inv_open(Oid lobjId, int flags, MemoryContext mcxt)
@@ -290,22 +292,20 @@ inv_open(Oid lobjId, int flags, MemoryContext mcxt)
290292
(errcode(ERRCODE_UNDEFINED_OBJECT),
291293
errmsg("large object %u does not exist", lobjId)));
292294

293-
/*
294-
* We must register the snapshot in TopTransaction's resowner, because it
295-
* must stay alive until the LO is closed rather than until the current
296-
* portal shuts down. Do this after checking that the LO exists, to avoid
297-
* leaking the snapshot if an error is thrown.
298-
*/
299-
if (snapshot)
300-
snapshot = RegisterSnapshotOnOwner(snapshot,
301-
TopTransactionResourceOwner);
302-
303-
/* All set, create a descriptor */
295+
/* OK to create a descriptor */
304296
retval = (LargeObjectDesc *) MemoryContextAlloc(mcxt,
305297
sizeof(LargeObjectDesc));
306298
retval->id = lobjId;
307-
retval->subid = GetCurrentSubTransactionId();
308299
retval->offset = 0;
300+
retval->flags = descflags;
301+
302+
/* caller sets if needed, not used by the functions in this file */
303+
retval->subid = InvalidSubTransactionId;
304+
305+
/*
306+
* The snapshot (if any) is just the currently active snapshot. The
307+
* caller will replace it with a longer-lived copy if needed.
308+
*/
309309
retval->snapshot = snapshot;
310310
retval->flags = descflags;
311311

@@ -320,10 +320,6 @@ void
320320
inv_close(LargeObjectDesc *obj_desc)
321321
{
322322
Assert(PointerIsValid(obj_desc));
323-
324-
UnregisterSnapshotFromOwner(obj_desc->snapshot,
325-
TopTransactionResourceOwner);
326-
327323
pfree(obj_desc);
328324
}
329325

0 commit comments

Comments
 (0)