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

Commit07070c0

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 parentada667b commit07070c0

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
@@ -592,20 +584,22 @@ AtEOXact_LargeObject(bool isCommit)
592584
{
593585
inti;
594586

595-
if (fscxt==NULL)
587+
if (!lo_cleanup_needed)
596588
return;/* no LO operations in this xact */
597589

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

@@ -614,11 +608,14 @@ AtEOXact_LargeObject(bool isCommit)
614608
cookies_size=0;
615609

616610
/* Release the LO memory context to prevent permanent memory leaks. */
617-
MemoryContextDelete(fscxt);
611+
if (fscxt)
612+
MemoryContextDelete(fscxt);
618613
fscxt=NULL;
619614

620615
/* Give inv_api.c a chance to clean up, too */
621616
close_lo_relation(isCommit);
617+
618+
lo_cleanup_needed= false;
622619
}
623620

624621
/*
@@ -646,14 +643,7 @@ AtEOSubXact_LargeObject(bool isCommit, SubTransactionId mySubid,
646643
if (isCommit)
647644
lo->subid=parentSubid;
648645
else
649-
{
650-
/*
651-
* Make sure we do not call inv_close twice if it errors out
652-
* for some reason. Better a leak than a crash.
653-
*/
654-
deleteLOfd(i);
655-
inv_close(lo);
656-
}
646+
closeLOfd(i);
657647
}
658648
}
659649
}
@@ -663,19 +653,22 @@ AtEOSubXact_LargeObject(bool isCommit, SubTransactionId mySubid,
663653
*****************************************************************************/
664654

665655
staticint
666-
newLOfd(LargeObjectDesc*lobjCookie)
656+
newLOfd(void)
667657
{
668658
inti,
669659
newsize;
670660

661+
lo_cleanup_needed= true;
662+
if (fscxt==NULL)
663+
fscxt=AllocSetContextCreate(TopMemoryContext,
664+
"Filesystem",
665+
ALLOCSET_DEFAULT_SIZES);
666+
671667
/* Try to find a free slot */
672668
for (i=0;i<cookies_size;i++)
673669
{
674670
if (cookies[i]==NULL)
675-
{
676-
cookies[i]=lobjCookie;
677671
returni;
678-
}
679672
}
680673

681674
/* No free slot, so make the array bigger */
@@ -700,15 +693,25 @@ newLOfd(LargeObjectDesc *lobjCookie)
700693
cookies_size=newsize;
701694
}
702695

703-
Assert(cookies[i]==NULL);
704-
cookies[i]=lobjCookie;
705696
returni;
706697
}
707698

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

714717
/*****************************************************************************
@@ -727,13 +730,8 @@ lo_get_fragment_internal(Oid loOid, int64 offset, int32 nbytes)
727730
inttotal_readPG_USED_FOR_ASSERTS_ONLY;
728731
bytea*result=NULL;
729732

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

738736
/*
739737
* Compute number of bytes we'll actually read, accommodating nbytes == -1
@@ -817,10 +815,9 @@ be_lo_from_bytea(PG_FUNCTION_ARGS)
817815
LargeObjectDesc*loDesc;
818816
intwrittenPG_USED_FOR_ASSERTS_ONLY;
819817

820-
CreateFSContext();
821-
818+
lo_cleanup_needed= true;
822819
loOid=inv_create(loOid);
823-
loDesc=inv_open(loOid,INV_WRITE,fscxt);
820+
loDesc=inv_open(loOid,INV_WRITE,CurrentMemoryContext);
824821
written=inv_write(loDesc,VARDATA_ANY(str),VARSIZE_ANY_EXHDR(str));
825822
Assert(written==VARSIZE_ANY_EXHDR(str));
826823
inv_close(loDesc);
@@ -840,9 +837,8 @@ be_lo_put(PG_FUNCTION_ARGS)
840837
LargeObjectDesc*loDesc;
841838
intwrittenPG_USED_FOR_ASSERTS_ONLY;
842839

843-
CreateFSContext();
844-
845-
loDesc=inv_open(loOid,INV_WRITE,fscxt);
840+
lo_cleanup_needed= true;
841+
loDesc=inv_open(loOid,INV_WRITE,CurrentMemoryContext);
846842

847843
/* Permission check */
848844
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
@@ -244,10 +244,12 @@ inv_create(Oid lobjId)
244244
/*
245245
*inv_open -- access an existing large object.
246246
*
247-
*Returns:
248-
* Large object descriptor, appropriately filled in. The descriptor
249-
* and subsidiary data are allocated in the specified memory context,
250-
* which must be suitably long-lived for the caller's purposes.
247+
* Returns a large object descriptor, appropriately filled in.
248+
* The descriptor and subsidiary data are allocated in the specified
249+
* memory context, which must be suitably long-lived for the caller's
250+
* purposes. If the returned descriptor has a snapshot associated
251+
* with it, the caller must ensure that it also lives long enough,
252+
* e.g. by calling RegisterSnapshotOnOwner
251253
*/
252254
LargeObjectDesc*
253255
inv_open(OidlobjId,intflags,MemoryContextmcxt)
@@ -314,19 +316,16 @@ inv_open(Oid lobjId, int flags, MemoryContext mcxt)
314316
retval= (LargeObjectDesc*)MemoryContextAlloc(mcxt,
315317
sizeof(LargeObjectDesc));
316318
retval->id=lobjId;
317-
retval->subid=GetCurrentSubTransactionId();
318319
retval->offset=0;
319320
retval->flags=descflags;
320321

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

332331
returnretval;
@@ -340,10 +339,6 @@ void
340339
inv_close(LargeObjectDesc*obj_desc)
341340
{
342341
Assert(PointerIsValid(obj_desc));
343-
344-
UnregisterSnapshotFromOwner(obj_desc->snapshot,
345-
TopTransactionResourceOwner);
346-
347342
pfree(obj_desc);
348343
}
349344

‎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