Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Commit11a399f

Browse files
committed
Fix snapshot reference leak if lo_export fails.
If lo_export() fails to open the target file or to write to it, it leaksthe created LargeObjectDesc and its snapshot in the top-transactioncontext and resource owner. That's pretty harmless, it's a small leakafter all, but it gives the user a "Snapshot reference leak" warning.Fix by using a short-lived memory context and no resource owner fortransient LargeObjectDescs that are opened and closed within one functioncall. The leak is easiest to reproduce with lo_export() on a directorythat doesn't exist, but in principle the other lo_* functions could alsofail.Backpatch to all supported versions.Reported-by: Andrew BReviewed-by: Alvaro HerreraDiscussion:https://www.postgresql.org/message-id/32bf767a-2d65-71c4-f170-122f416bab7e@iki.fi
1 parentda782bc commit11a399f

File tree

4 files changed

+104
-91
lines changed

4 files changed

+104
-91
lines changed

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

Lines changed: 71 additions & 75 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
/* define this to enable debug logging */
5557
/* #define FSDB 1 */
@@ -67,19 +69,11 @@
6769
staticLargeObjectDesc**cookies=NULL;
6870
staticintcookies_size=0;
6971

72+
staticboollo_cleanup_needed= false;
7073
staticMemoryContextfscxt=NULL;
7174

72-
#defineCreateFSContext() \
73-
do { \
74-
if (fscxt == NULL) \
75-
fscxt = AllocSetContextCreate(TopMemoryContext, \
76-
"Filesystem", \
77-
ALLOCSET_DEFAULT_SIZES); \
78-
} while (0)
79-
80-
81-
staticintnewLOfd(LargeObjectDesc*lobjCookie);
82-
staticvoiddeleteLOfd(intfd);
75+
staticintnewLOfd(void);
76+
staticvoidcloseLOfd(intfd);
8377
staticOidlo_import_internal(text*filename,OidlobjOid);
8478

8579

@@ -99,11 +93,26 @@ be_lo_open(PG_FUNCTION_ARGS)
9993
elog(DEBUG4,"lo_open(%u,%d)",lobjId,mode);
10094
#endif
10195

102-
CreateFSContext();
96+
/*
97+
* Allocate a large object descriptor first. This will also create
98+
* 'fscxt' if this is the first LO opened in this transaction.
99+
*/
100+
fd=newLOfd();
103101

104102
lobjDesc=inv_open(lobjId,mode,fscxt);
103+
lobjDesc->subid=GetCurrentSubTransactionId();
104+
105+
/*
106+
* We must register the snapshot in TopTransaction's resowner so that it
107+
* stays alive until the LO is closed rather than until the current portal
108+
* shuts down.
109+
*/
110+
if (lobjDesc->snapshot)
111+
lobjDesc->snapshot=RegisterSnapshotOnOwner(lobjDesc->snapshot,
112+
TopTransactionResourceOwner);
105113

106-
fd=newLOfd(lobjDesc);
114+
Assert(cookies[fd]==NULL);
115+
cookies[fd]=lobjDesc;
107116

108117
PG_RETURN_INT32(fd);
109118
}
@@ -122,9 +131,7 @@ be_lo_close(PG_FUNCTION_ARGS)
122131
elog(DEBUG4,"lo_close(%d)",fd);
123132
#endif
124133

125-
inv_close(cookies[fd]);
126-
127-
deleteLOfd(fd);
134+
closeLOfd(fd);
128135

129136
PG_RETURN_INT32(0);
130137
}
@@ -238,12 +245,7 @@ be_lo_creat(PG_FUNCTION_ARGS)
238245
{
239246
OidlobjId;
240247

241-
/*
242-
* We don't actually need to store into fscxt, but create it anyway to
243-
* ensure that AtEOXact_LargeObject knows there is state to clean up
244-
*/
245-
CreateFSContext();
246-
248+
lo_cleanup_needed= true;
247249
lobjId=inv_create(InvalidOid);
248250

249251
PG_RETURN_OID(lobjId);
@@ -254,12 +256,7 @@ be_lo_create(PG_FUNCTION_ARGS)
254256
{
255257
OidlobjId=PG_GETARG_OID(0);
256258

257-
/*
258-
* We don't actually need to store into fscxt, but create it anyway to
259-
* ensure that AtEOXact_LargeObject knows there is state to clean up
260-
*/
261-
CreateFSContext();
262-
259+
lo_cleanup_needed= true;
263260
lobjId=inv_create(lobjId);
264261

265262
PG_RETURN_OID(lobjId);
@@ -330,16 +327,13 @@ be_lo_unlink(PG_FUNCTION_ARGS)
330327
for (i=0;i<cookies_size;i++)
331328
{
332329
if (cookies[i]!=NULL&&cookies[i]->id==lobjId)
333-
{
334-
inv_close(cookies[i]);
335-
deleteLOfd(i);
336-
}
330+
closeLOfd(i);
337331
}
338332
}
339333

340334
/*
341335
* inv_drop does not create a need for end-of-transaction cleanup and
342-
* hence we don't need tohave created fscxt.
336+
* hence we don't need toset lo_cleanup_needed.
343337
*/
344338
PG_RETURN_INT32(inv_drop(lobjId));
345339
}
@@ -419,8 +413,6 @@ lo_import_internal(text *filename, Oid lobjOid)
419413
LargeObjectDesc*lobj;
420414
Oidoid;
421415

422-
CreateFSContext();
423-
424416
/*
425417
* open the file to be read in
426418
*/
@@ -435,12 +427,13 @@ lo_import_internal(text *filename, Oid lobjOid)
435427
/*
436428
* create an inversion object
437429
*/
430+
lo_cleanup_needed= true;
438431
oid=inv_create(lobjOid);
439432

440433
/*
441434
* read in from the filesystem and write to the inversion object
442435
*/
443-
lobj=inv_open(oid,INV_WRITE,fscxt);
436+
lobj=inv_open(oid,INV_WRITE,CurrentMemoryContext);
444437

445438
while ((nbytes=read(fd,buf,BUFSIZE))>0)
446439
{
@@ -482,12 +475,11 @@ be_lo_export(PG_FUNCTION_ARGS)
482475
LargeObjectDesc*lobj;
483476
mode_toumask;
484477

485-
CreateFSContext();
486-
487478
/*
488479
* open the inversion object (no need to test for failure)
489480
*/
490-
lobj=inv_open(lobjId,INV_READ,fscxt);
481+
lo_cleanup_needed= true;
482+
lobj=inv_open(lobjId,INV_READ,CurrentMemoryContext);
491483

492484
/*
493485
* open the file to be written to
@@ -594,20 +586,22 @@ AtEOXact_LargeObject(bool isCommit)
594586
{
595587
inti;
596588

597-
if (fscxt==NULL)
589+
if (!lo_cleanup_needed)
598590
return;/* no LO operations in this xact */
599591

600592
/*
601593
* Close LO fds and clear cookies array so that LO fds are no longer good.
602-
* On abort we skip the close step.
594+
* The memory context and resource owner holding them are going away at
595+
* the end-of-transaction anyway, but on commit, we need to close them to
596+
* avoid warnings about leaked resources at commit. On abort we can skip
597+
* this step.
603598
*/
604-
for (i=0;i<cookies_size;i++)
599+
if (isCommit)
605600
{
606-
if (cookies[i]!=NULL)
601+
for (i=0;i<cookies_size;i++)
607602
{
608-
if (isCommit)
609-
inv_close(cookies[i]);
610-
deleteLOfd(i);
603+
if (cookies[i]!=NULL)
604+
closeLOfd(i);
611605
}
612606
}
613607

@@ -616,11 +610,14 @@ AtEOXact_LargeObject(bool isCommit)
616610
cookies_size=0;
617611

618612
/* Release the LO memory context to prevent permanent memory leaks. */
619-
MemoryContextDelete(fscxt);
613+
if (fscxt)
614+
MemoryContextDelete(fscxt);
620615
fscxt=NULL;
621616

622617
/* Give inv_api.c a chance to clean up, too */
623618
close_lo_relation(isCommit);
619+
620+
lo_cleanup_needed= false;
624621
}
625622

626623
/*
@@ -648,14 +645,7 @@ AtEOSubXact_LargeObject(bool isCommit, SubTransactionId mySubid,
648645
if (isCommit)
649646
lo->subid=parentSubid;
650647
else
651-
{
652-
/*
653-
* Make sure we do not call inv_close twice if it errors out
654-
* for some reason. Better a leak than a crash.
655-
*/
656-
deleteLOfd(i);
657-
inv_close(lo);
658-
}
648+
closeLOfd(i);
659649
}
660650
}
661651
}
@@ -665,19 +655,22 @@ AtEOSubXact_LargeObject(bool isCommit, SubTransactionId mySubid,
665655
*****************************************************************************/
666656

667657
staticint
668-
newLOfd(LargeObjectDesc*lobjCookie)
658+
newLOfd(void)
669659
{
670660
inti,
671661
newsize;
672662

663+
lo_cleanup_needed= true;
664+
if (fscxt==NULL)
665+
fscxt=AllocSetContextCreate(TopMemoryContext,
666+
"Filesystem",
667+
ALLOCSET_DEFAULT_SIZES);
668+
673669
/* Try to find a free slot */
674670
for (i=0;i<cookies_size;i++)
675671
{
676672
if (cookies[i]==NULL)
677-
{
678-
cookies[i]=lobjCookie;
679673
returni;
680-
}
681674
}
682675

683676
/* No free slot, so make the array bigger */
@@ -702,15 +695,25 @@ newLOfd(LargeObjectDesc *lobjCookie)
702695
cookies_size=newsize;
703696
}
704697

705-
Assert(cookies[i]==NULL);
706-
cookies[i]=lobjCookie;
707698
returni;
708699
}
709700

710701
staticvoid
711-
deleteLOfd(intfd)
702+
closeLOfd(intfd)
712703
{
704+
LargeObjectDesc*lobj;
705+
706+
/*
707+
* Make sure we do not try to free twice if this errors out for some
708+
* reason. Better a leak than a crash.
709+
*/
710+
lobj=cookies[fd];
713711
cookies[fd]=NULL;
712+
713+
if (lobj->snapshot)
714+
UnregisterSnapshotFromOwner(lobj->snapshot,
715+
TopTransactionResourceOwner);
716+
inv_close(lobj);
714717
}
715718

716719
/*****************************************************************************
@@ -729,13 +732,8 @@ lo_get_fragment_internal(Oid loOid, int64 offset, int32 nbytes)
729732
inttotal_readPG_USED_FOR_ASSERTS_ONLY;
730733
bytea*result=NULL;
731734

732-
/*
733-
* We don't actually need to store into fscxt, but create it anyway to
734-
* ensure that AtEOXact_LargeObject knows there is state to clean up
735-
*/
736-
CreateFSContext();
737-
738-
loDesc=inv_open(loOid,INV_READ,fscxt);
735+
lo_cleanup_needed= true;
736+
loDesc=inv_open(loOid,INV_READ,CurrentMemoryContext);
739737

740738
/*
741739
* Compute number of bytes we'll actually read, accommodating nbytes == -1
@@ -819,10 +817,9 @@ be_lo_from_bytea(PG_FUNCTION_ARGS)
819817
LargeObjectDesc*loDesc;
820818
intwrittenPG_USED_FOR_ASSERTS_ONLY;
821819

822-
CreateFSContext();
823-
820+
lo_cleanup_needed= true;
824821
loOid=inv_create(loOid);
825-
loDesc=inv_open(loOid,INV_WRITE,fscxt);
822+
loDesc=inv_open(loOid,INV_WRITE,CurrentMemoryContext);
826823
written=inv_write(loDesc,VARDATA_ANY(str),VARSIZE_ANY_EXHDR(str));
827824
Assert(written==VARSIZE_ANY_EXHDR(str));
828825
inv_close(loDesc);
@@ -842,9 +839,8 @@ be_lo_put(PG_FUNCTION_ARGS)
842839
LargeObjectDesc*loDesc;
843840
intwrittenPG_USED_FOR_ASSERTS_ONLY;
844841

845-
CreateFSContext();
846-
847-
loDesc=inv_open(loOid,INV_WRITE,fscxt);
842+
lo_cleanup_needed= true;
843+
loDesc=inv_open(loOid,INV_WRITE,CurrentMemoryContext);
848844

849845
/* Permission check */
850846
if (!lo_compat_privileges&&

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

Lines changed: 11 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -242,10 +242,12 @@ inv_create(Oid lobjId)
242242
/*
243243
*inv_open -- access an existing large object.
244244
*
245-
*Returns:
246-
* Large object descriptor, appropriately filled in. The descriptor
247-
* and subsidiary data are allocated in the specified memory context,
248-
* which must be suitably long-lived for the caller's purposes.
245+
* Returns a large object descriptor, appropriately filled in.
246+
* The descriptor and subsidiary data are allocated in the specified
247+
* memory context, which must be suitably long-lived for the caller's
248+
* purposes. If the returned descriptor has a snapshot associated
249+
* with it, the caller must ensure that it also lives long enough,
250+
* e.g. by calling RegisterSnapshotOnOwner
249251
*/
250252
LargeObjectDesc*
251253
inv_open(OidlobjId,intflags,MemoryContextmcxt)
@@ -312,19 +314,16 @@ inv_open(Oid lobjId, int flags, MemoryContext mcxt)
312314
retval= (LargeObjectDesc*)MemoryContextAlloc(mcxt,
313315
sizeof(LargeObjectDesc));
314316
retval->id=lobjId;
315-
retval->subid=GetCurrentSubTransactionId();
316317
retval->offset=0;
317318
retval->flags=descflags;
318319

320+
/* caller sets if needed, not used by the functions in this file */
321+
retval->subid=InvalidSubTransactionId;
322+
319323
/*
320-
* We must register the snapshot in TopTransaction's resowner, because it
321-
* must stay alive until the LO is closed rather than until the current
322-
* portal shuts down. Do this last to avoid uselessly leaking the
323-
* snapshot if an error is thrown above.
324+
* The snapshot (if any) is just the currently active snapshot. The
325+
* caller will replace it with a longer-lived copy if needed.
324326
*/
325-
if (snapshot)
326-
snapshot=RegisterSnapshotOnOwner(snapshot,
327-
TopTransactionResourceOwner);
328327
retval->snapshot=snapshot;
329328

330329
returnretval;
@@ -338,10 +337,6 @@ void
338337
inv_close(LargeObjectDesc*obj_desc)
339338
{
340339
Assert(PointerIsValid(obj_desc));
341-
342-
UnregisterSnapshotFromOwner(obj_desc->snapshot,
343-
TopTransactionResourceOwner);
344-
345340
pfree(obj_desc);
346341
}
347342

‎src/test/regress/input/largeobject.source

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -124,6 +124,17 @@ BEGIN;
124124
SELECT lo_open(loid, x'40000'::int) from lotest_stash_values;
125125
ABORT;
126126

127+
DO $$
128+
DECLARE
129+
loid oid;
130+
BEGIN
131+
SELECT tbl.loid INTO loid FROM lotest_stash_values tbl;
132+
PERFORM lo_export(loid, '@abs_builddir@/results/invalid/path');
133+
EXCEPTION
134+
WHEN UNDEFINED_FILE THEN RAISE NOTICE 'could not open file, as expected';
135+
END;
136+
$$;
137+
127138
-- Test truncation.
128139
BEGIN;
129140
UPDATE lotest_stash_values SET fd=lo_open(loid, CAST(x'20000' | x'40000' AS integer));

0 commit comments

Comments
 (0)
[8]ページ先頭
©2009-2025 Movatter.jp