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

Commit8f32afe

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 parent9919770 commit8f32afe

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 @@ boollo_compat_privileges;
7274
staticLargeObjectDesc**cookies=NULL;
7375
staticintcookies_size=0;
7476

77+
staticboollo_cleanup_needed= false;
7578
staticMemoryContextfscxt=NULL;
7679

77-
#defineCreateFSContext() \
78-
do { \
79-
if (fscxt == NULL) \
80-
fscxt = AllocSetContextCreate(TopMemoryContext, \
81-
"Filesystem", \
82-
ALLOCSET_DEFAULT_SIZES); \
83-
} while (0)
84-
85-
86-
staticintnewLOfd(LargeObjectDesc*lobjCookie);
87-
staticvoiddeleteLOfd(intfd);
80+
staticintnewLOfd(void);
81+
staticvoidcloseLOfd(intfd);
8882
staticOidlo_import_internal(text*filename,OidlobjOid);
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
#ifFSDB
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
OidlobjId;
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
OidlobjId=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 tohave created fscxt.
364+
* hence we don't need toset 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
inti;
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
staticint
717-
newLOfd(LargeObjectDesc*lobjCookie)
706+
newLOfd(void)
718707
{
719708
inti,
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
returni;
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
returni;
757747
}
758748

759749
staticvoid
760-
deleteLOfd(intfd)
750+
closeLOfd(intfd)
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
inttotal_readPG_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
intwrittenPG_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
intwrittenPG_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(OidlobjId,intflags,MemoryContextmcxt)
@@ -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)
[8]ページ先頭
©2009-2025 Movatter.jp